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
>From 7bdda9eccd704076297a0d0802c8638b4562c703 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <marko.makela@xxxxxxxxxxx>
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
