Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver

["Marko Mäkelä" <marko.makela@iki.fi>]

[-- Attachment #1: Type: text/plain, Size: 4931 bytes --]

Hi Sean,

Mon, Jul 04, 2022 at 11:00:38AM +0100, Sean Young wrote:
>Hi Marko,
>
>On Mon, Jul 04, 2022 at 12:20:01PM +0300, Marko Mäkelä wrote:
>> Mon, Jul 04, 2022 at 08:21:39AM +0100, Sean Young wrote:
>> > On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote:
>> > > For protocols that do not use a toggle bit, the last parameter of
>> > > rc_keydown() will usually be toggle=0, and explicit calls to rc_repeat()
>> > > will be issued when needed. For those protocols, I thought that we would not
>> > > want rc_keydown() to set any LIRC_SCANCODE_FLAG_REPEAT flag under any
>> > > circumstances.
>> >
>> > Toggle and repeat are distinct concepts.
>> >
>> > rc_repeat() is for protocols which have a special repeat message, which
>> > carry no information other that "repeat the last message". However,
>> > all protocols repeat. Whether they use a special repeat message or not.
>> >
>> > It's right that if a protocol repeats a message, LIRC_SCANCODE_FLAG_REPEAT
>> > is set.
>>
>> Is it right to set the flag when a message is being repeated due to user
>> effort (repeatedly pressing and releasing a button, instead of holding the
>> button down)?
>
>The problem here is that the nec repeat is used by some remotes, but 
>not others. Some nec remotes repeat the entire code every time. Our 
>generic nec decoder cannot distinguish between the two. So, our nec 
>decoder interprets both a nec repeat and a repeated code as "button 
>being held down".

I see. My experience of remote control protocols is mostly limited to 
RC5.

>> As far as I understand, the change that you suggested would set the
>> LIRC_SCANCODE_FLAG_REPEAT if I repeatedly press a button on the NEC protocol
>> remote control, but not on an RC5 remote control.
>
>RC5 too.

I think that it is depends on the implementation of the firmware that 
runs on the remote control unit. If the button is released and quickly 
pressed again so that no keyboard matrix scan was scheduled to run in 
between, then the firmware could report it as a repeat event.  
Alternatively, every second IR message could be lost, so that the 
receiver would observe the same toggle bit value for every IR message 
that it receives.

>>I tested the attached patch (which was created on 5.19-rc5, which 
>>failed to boot on my system for unrelated reasons) on Linux 5.17, on 
>>top of your fixes to rtl28xxu and rc-core.
>
>You'll need to fix this.

The 5.19-rc5 boot failure could have been related to LUKS setup on that 
machine, because a kernel panic message was displayed before I was being 
prompted for an encryption key. The modules would not have been loaded 
at that point, so I do not think that it is related to my modifications.

When compiled for the v5.17 kernel release tag on another computer, the 
patch that implements rc_keydown_or_repeat() worked for me.

It does not look like there are many changes in drivers/media/rc between 
5.17 and 5.19.

>>If the user wants to quickly switch to channel 111 by quickly pressing 
>>the button three times, it should not be misreported as an 
>>auto-repeated event, but reported as 3 LIRC events without the 
>>"repeat" flag, and as 3 pairs of keydown and keyup events.
>
>Ideally yes, if we can distinguish between the two.

Okay, I understand that we indeed cannot always do that.

>See https://github.com/seanyoung/cir/

This could open up many possibilities. Would the decoded events also be 
available via some low-level interface to user-space programs, in 
addition to the input event driver?

>>On the other hand, there should be no reason for an application to not 
>>honor repeat events for a volume button. That is of course up to the 
>>application to decide, not the kernel.
>
>Well, that's not the way things work. Keys have autorepeats which are 
>generated kernel-side. I think libinput wants to change this to user 
>space but certainly not application side.

It is how https://tvdr.de works. I have been using it via two 
interfaces: a built-in LIRC interface, and vdr-plugin-remote for the 
Linux input device driver. In http://git.tvdr.de/vdr.git you can find 
that in some cases, normal key-presses are being distinguished from 
repeat events (git grep -w k_Repeat). This is the reason why I brought 
up the missing LIRC_SCANCODE_FLAG_REPEAT in the RC5 protocol decoder: 
VDR with the LIRC interface would observe that the user is repeatedly 
pressing a button even when the button is being held down.

>> If you agree that this patch is on the right track, an interface for the new
>> function rc_keydown_or_repeat() may have to be exposed to the BPF interface
>> as well.
>
>I'm not sure why that is needed.

I am attaching a minimal version of the patch, just one hunk, like you 
suggested earlier. I did not observe any bad effects with either remote 
control unit I tested it with: RC5 and NEC, again, on the rtl28xxu and 
with your two commits, on the v5.17 tag.

Best regards,

	Marko

[-- Attachment #2: 0001-media-rc-Always-report-LIRC-repeat-flag.patch --]
[-- Type: text/x-diff, Size: 1064 bytes --]

From 7bdda9eccd704076297a0d0802c8638b4562c703 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <marko.makela@mariadb.com>
Date: Mon, 4 Jul 2022 21:56:08 +0300
Subject: [PATCH] media: rc: Always report LIRC repeat flag

The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown().
Previously it was only set by rc_repeat(), but not all protocol
decoders invoke that function.
---
 drivers/media/rc/rc-main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index f27f6b383816..d875d84ea221 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -786,7 +786,8 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol,
 			  dev->last_toggle   != toggle);
 	struct lirc_scancode sc = {
 		.scancode = scancode, .rc_proto = protocol,
-		.flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0,
+		.flags = (toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0) |
+		(!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0),
 		.keycode = keycode
 	};
 
-- 
2.36.1