Sat, Jul 02, 2022 at 09:14:59AM +0100, Sean Young wrote:
Hi,
On Tue, Jun 28, 2022 at 09:27:26AM +0300, Marko Mäkelä wrote:
Mon, Jun 27, 2022 at 11:53:52AM +0100, Sean Young wrote:
> Hi Marko,
>
> On Sun, Jun 26, 2022 at 03:33:47PM +0300, Marko Mäkelä wrote:
> > I finally took the time to get a deeper understanding of the infrared remote
> > control subsystem. I think that I now understand the translation into
> > key-down, key-up, and key-repeat events. For the RC5 protocol, rc_repeat()
> > will not be called by ir-rc5-decoder.c but instead, ir_do_keydown() will
> > handle the repeat. For lirc_scancode_event() it will never set the
> > LIRC_SCANCODE_FLAG_REPEAT bit, even if !new_event and the protocol does
> > support the toggle bit. That might qualify as a bug.
>
> You are right, this was missed. Patches welcome.
Attached (for 5.19.0-rc3, on top of the two commits of this patch series).
I thought that it would be the least amount of trouble to slightly change
the interpretation of the "toggle" parameter of
rc_keydown(). My intention was to use the values 1 and 2 when the toggle
flag is present. Any nonzero values would work.
I don't understand why this is needed.
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.
A patch needs to be tested. Just rebuild the entire kernel and boot
from that.
Yes, I will do that for revising my patch.
- rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), toggle);
+ rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key),
+ 1 + toggle);
You can't change the toggle value because you want a repeat flag. This makes
no sense.
[snip]
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -782,18 +782,19 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol,
{
bool new_event = (!dev->keypressed ||
dev->last_protocol != protocol ||
- dev->last_scancode != scancode ||
- dev->last_toggle != toggle);
+ dev->last_scancode != scancode);
+ bool repeat_event = !new_event && toggle && dev->last_toggle == toggle;
Why this change?
struct lirc_scancode sc = {
.scancode = scancode, .rc_proto = protocol,
- .flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0,
+ .flags = (toggle > 1 ? LIRC_SCANCODE_FLAG_TOGGLE : 0) |
+ (repeat_event ? LIRC_SCANCODE_FLAG_REPEAT : 0),
Why not simply (!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0) and be done with it?
Drivers that invoke rc_repeat() do not want rc_keydown() to ever set
LIRC_SCANCODE_FLAG_REPEAT. The patch slightly changed the meaning of
toggle: it *must* be 0 if and only if the protocol does not implement a
toggle bit. If it does, the values must alternate between 1 and some
greater-than-1 value.
A cleaner alternative could be to retain the interface of rc_keydown()
as is, and add a new function, say, rc_keydown_or_repeat(), which would
generate key-repeat events from the toggle bit.
Best regards,
Marko
