| Message ID | 704b3d7e5a7a95cbd5e4dfc25a41454e919aed95.1644683294.git.sean@mess.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix rtl28xxu nec/rc5 receiver | expand |
Hi Sean, 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. Sat, Feb 12, 2022 at 04:32:19PM +0000, Sean Young wrote: >This device presents an IR buffer, which can be read and cleared. >Clearing the buffer is racey with receiving IR, so wait until the IR >message is finished before clearing it. > >This should minimize the chance of the buffer clear happening while >IR is being received, although we cannot avoid this completely. I just realized that this limitation of the interface may be causing exactly what I was observing when I was testing this. If a constant stream of data is being received because a button is being held down, a buffer overflow or wrap-around glitch is inevitable, maybe expect if the wrap-around occurs exactly at the 128-byte boundary. How about the following improvement? If IR_RX_BC is a simple cursor to the 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending refresh_tab[] but simply remember where the previous call left off. We could always read the 128 bytes at IR_RX_BUF, and process everything between the previous position reported by IR_RX_BC and the current position reported by IR_RX_BC, and treat buf[] as a ring buffer. Last time I tested it, the patch was a significant improvement. I think that "perfect" is the enemy of "good enough", and the patch should be included in the kernel. Best regards, Marko
Sun, Jun 26, 2022 at 03:33:48PM +0300, Marko Mäkelä wrote: >How about the following improvement? If IR_RX_BC is a simple cursor to >the 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending >refresh_tab[] but simply remember where the previous call left off. We >could always read the 128 bytes at IR_RX_BUF, and process everything >between the previous position reported by IR_RX_BC and the current >position reported by IR_RX_BC, and treat buf[] as a ring buffer. I experimented with this on the 5.19.0-rc3 kernel. With the attached patch applied on top of this patch series, "ir-keytables -t" reported only one RC5 encoded key-down event. I had to unplug and plug in the adapter in order to receive another RC5 event. The refresh command seems to be necessary for the device to store and forward further IR data. >Last time I tested it, the patch was a significant improvement. I think >that "perfect" is the enemy of "good enough", and the patch should be >included in the kernel. The remaining problem definitely is a limitation of the interface. There is little that can be done to work around it. Marko
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. > Sat, Feb 12, 2022 at 04:32:19PM +0000, Sean Young wrote: > > This device presents an IR buffer, which can be read and cleared. > > Clearing the buffer is racey with receiving IR, so wait until the IR > > message is finished before clearing it. > > > > This should minimize the chance of the buffer clear happening while > > IR is being received, although we cannot avoid this completely. > > I just realized that this limitation of the interface may be causing exactly > what I was observing when I was testing this. If a constant stream of data > is being received because a button is being held down, a buffer overflow or > wrap-around glitch is inevitable, maybe expect if the wrap-around occurs > exactly at the 128-byte boundary. > > How about the following improvement? If IR_RX_BC is a simple cursor to the > 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending > refresh_tab[] but simply remember where the previous call left off. We could > always read the 128 bytes at IR_RX_BUF, and process everything between the > previous position reported by IR_RX_BC and the current position reported by > IR_RX_BC, and treat buf[] as a ring buffer. This is a great idea. Very original. > Last time I tested it, the patch was a significant improvement. I think that > "perfect" is the enemy of "good enough", and the patch should be included in > the kernel. The idea sounds really good. I'll review/test the patch and get back to you. Thanks, Sean
Hi Sean, 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 am not that familiar with updating the modules, and I suspect that instead of actually testing this code, I was testing the "ring buffer" patch that I posted yesterday. I could not use the rmmod/insmod approach for testing this change, because the rc_core module was in use by the display driver. So, I can only say that the patch compiled for me. A few FIXME places are indicated where I am not sure that a correct (nonzero) toggle value would be computed. An alternative approach would be to use the value toggle=1 for the case when the toggle bit is set, and toggle>1 for the case when it is not set. Basically, change things like 1+!!x to 1+!x in the callers, and change the condition toggle > 1 to toggle == 1 in rc-main.c. In that way, any old driver that would use the toggle values 0 and 1 would still generate LIRC_SCANCODE_FLAG_TOGGLE. But then, the repeat_event logic would only work half the time (when toggle!=0). Marko
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. > I am not that familiar with updating the modules, and I suspect that instead > of actually testing this code, I was testing the "ring buffer" patch that I > posted yesterday. I could not use the rmmod/insmod approach for testing this > change, because the rc_core module was in use by the display driver. So, I > can only say that the patch compiled for me. A few FIXME places are > indicated where I am not sure that a correct (nonzero) toggle value would be > computed. A patch needs to be tested. Just rebuild the entire kernel and boot from that. > An alternative approach would be to use the value toggle=1 for the case when > the toggle bit is set, and toggle>1 for the case when it is not set. > Basically, change things like 1+!!x to 1+!x in the callers, and change the > condition toggle > 1 to toggle == 1 in rc-main.c. In that way, any old > driver that would use the toggle values 0 and 1 would still generate > LIRC_SCANCODE_FLAG_TOGGLE. But then, the repeat_event logic would only work > half the time (when toggle!=0). > > Marko > >From 29a5c2a00653f49c854109b2f2c8f99b4431430f Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <marko.makela@iki.fi> > Date: Tue, 28 Jun 2022 07:59:17 +0300 > Subject: [PATCH] rc_keydown(): Report LIRC_SCANCODE_FLAG_REPEAT based on > toggle bit > > Drivers that will not call rc_repeat() will let rc_keydown() > create repeat events. For the LIRC interface, the repeat flag > would only be set by rc_repeat(), never by rc_keydown(). > > We change the meaning of the toggle parameter: Drivers that > invoke rc_repeat() by themselves should always pass toggle=0, > while protocols that include a toggle bit should pass toggle>0, > with the value 1 meaning that the toggle bit is clear. > > This is largely untested code. See FIXME comments. > Also, an interface change for bpf_rc_keydown() might have to be > documented. > --- > drivers/media/cec/platform/seco/seco-cec.c | 3 +- > drivers/media/i2c/ir-kbd-i2c.c | 4 +- > drivers/media/pci/bt8xx/bttv-input.c | 2 +- > drivers/media/pci/ttpci/budget-ci.c | 4 +- > drivers/media/rc/bpf-lirc.c | 2 +- > drivers/media/rc/img-ir/img-ir-rc5.c | 2 +- > drivers/media/rc/img-ir/img-ir-rc6.c | 2 +- > drivers/media/rc/imon.c | 2 +- > drivers/media/rc/ir-jvc-decoder.c | 3 +- > drivers/media/rc/ir-rc5-decoder.c | 2 +- > drivers/media/rc/ir-rc6-decoder.c | 4 +- > drivers/media/rc/ir-rcmm-decoder.c | 2 +- > drivers/media/rc/rc-main.c | 9 ++-- > drivers/media/usb/dvb-usb-v2/az6007.c | 2 +- > drivers/media/usb/dvb-usb-v2/dvbsky.c | 2 +- > drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 48 ++++----------------- > drivers/media/usb/dvb-usb-v2/rtl28xxu.h | 1 + > drivers/media/usb/dvb-usb/dib0700_core.c | 2 +- > drivers/media/usb/dvb-usb/dib0700_devices.c | 2 +- > drivers/media/usb/dvb-usb/ttusb2.c | 3 +- > drivers/media/usb/em28xx/em28xx-input.c | 4 +- > drivers/staging/media/av7110/av7110_ir.c | 2 +- > 22 files changed, 40 insertions(+), 67 deletions(-) > > diff --git a/drivers/media/cec/platform/seco/seco-cec.c b/drivers/media/cec/platform/seco/seco-cec.c > index a056412883b9..6aa889add090 100644 > --- a/drivers/media/cec/platform/seco/seco-cec.c > +++ b/drivers/media/cec/platform/seco/seco-cec.c > @@ -416,7 +416,8 @@ static int secocec_ir_rx(struct secocec_data *priv) > addr = (val & SECOCEC_IR_ADDRESS_MASK) >> SECOCEC_IR_ADDRESS_SHL; > toggle = (val & SECOCEC_IR_TOGGLE_MASK) >> SECOCEC_IR_TOGGLE_SHL; > > - 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? > .keycode = keycode > }; > > if (dev->allowed_protocols != RC_PROTO_BIT_CEC) > lirc_scancode_event(dev, &sc); > > - if (new_event && dev->keypressed) > + if (!repeat_event && dev->keypressed) > ir_do_keyup(dev, false); > > if (scancode <= U32_MAX) > diff --git a/drivers/media/usb/dvb-usb-v2/az6007.c b/drivers/media/usb/dvb-usb-v2/az6007.c > index 62ee09f28a0b..cc218c0d71a8 100644 > --- a/drivers/media/usb/dvb-usb-v2/az6007.c > +++ b/drivers/media/usb/dvb-usb-v2/az6007.c > @@ -224,7 +224,7 @@ static int az6007_rc_query(struct dvb_usb_device *d) > proto = RC_PROTO_NEC32; > } > > - rc_keydown(d->rc_dev, proto, code, st->data[5]); > + rc_keydown(d->rc_dev, proto, code, 1 + st->data[5]); /* FIXME */ I still have no idea what you are trying to achieve with this. Sean
On Mon, Jun 27, 2022 at 08:00:45AM +0300, Marko Mäkelä wrote: > Sun, Jun 26, 2022 at 03:33:48PM +0300, Marko Mäkelä wrote: > > How about the following improvement? If IR_RX_BC is a simple cursor to > > the 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending > > refresh_tab[] but simply remember where the previous call left off. We > > could always read the 128 bytes at IR_RX_BUF, and process everything > > between the previous position reported by IR_RX_BC and the current > > position reported by IR_RX_BC, and treat buf[] as a ring buffer. > > I experimented with this on the 5.19.0-rc3 kernel. With the attached patch > applied on top of this patch series, "ir-keytables -t" reported only one RC5 > encoded key-down event. I had to unplug and plug in the adapter in order to > receive another RC5 event. The refresh command seems to be necessary for the > device to store and forward further IR data. > > > Last time I tested it, the patch was a significant improvement. I think > > that "perfect" is the enemy of "good enough", and the patch should be > > included in the kernel. > > The remaining problem definitely is a limitation of the interface. There is > little that can be done to work around it. This particular device is really badly designed. Unless we come up with a better solution to work around its limitations, it's just broken. Sean
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
On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote: > 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. 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. Sean
Hi Sean, 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)? If it is, is it consistent to avoid setting the flag when a protocol uses a toggle bit (say, RC5)? In that case, the toggle bit would change its value each time the button is pressed, and your suggested change to rc_keydown() would not set the repeat flag. 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. 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. By the way, the patch that I sent earlier accidentally included my futile attempt at avoiding buffer overrun on the rtl28xxu. That is probably why my previous test failed. With the NEC protocol of the bundled remote of the Astrometa DVB-T2 adapter, the "repeat" flag was occasionally set when I repeated keypresses too quickly. I think that this was because the key matrix scanning algorithm on the remote control did not get a reading of "no key pressed" between two key presses. When I used an RC5 based remote (that of Hauppauge Nova-T PCI 90002), I only saw the "repeat" flag in the output of "ir-keytable -t" when holding a button down. If I repeated keypresses manually, the toggle bit was changing between them. In other words, for these two remote controls, the patch works exactly like I intended. One might think that it is not necessary to make difference between long button presses (which should generate repeat events) and short button presses that are quickly repeated by the user. I can think of a user-space application that would intentionally ignore repeat events for some buttons where it would make little sense. For example, when the number button 1 is pressed for a long time, the application might choose not to repeat the keypress, but "demand" multiple separate button presses by the user, if the channel should really be switched to 11, 111, or 1111. The intention of ignoring "repeat" events would be to avoid "punishing" users who are pressing a button longer, possibly compensating for unreliable IR signal reception. 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. 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. 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. Marko
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". rc5 is a much nicer protocol and explicitly uses a toggle bit to specify the button has been released/pressed. Some protocols use more than one bit for toggle, in case a toggle was lost due to packet loss. > If it is, is it consistent to avoid setting the flag when a > protocol uses a toggle bit (say, RC5)? No, RC5 repeats the same message if a button is being held down with the same toggle value. We should get a LIRC_SCANCODE_FLAG_REPEAT in this case. > In that case, the toggle bit would > change its value each time the button is pressed, and your suggested change > to rc_keydown() would not set the repeat flag. If we can distinguish between press/release vs hold then it is not a repeat. If it is being held down. then it is a repeat. > 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 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. > One might think that it is not necessary to make difference between long > button presses (which should generate repeat events) and short button > presses that are quickly repeated by the user. I can think of a user-space > application that would intentionally ignore repeat events for some buttons > where it would make little sense. For example, when the number button 1 is > pressed for a long time, the application might choose not to repeat the > keypress, but "demand" multiple separate button presses by the user, if the > channel should really be switched to 11, 111, or 1111. The intention of > ignoring "repeat" events would be to avoid "punishing" users who are > pressing a button longer, possibly compensating for unreliable IR signal > reception. The input layer create autorepeat key events for keys that are being held down. > 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. FWIW I'm (slowly) working on new tooling that allows you specify the IR protocol in IRP format. This would allow you say for NEC: {38.4k,564}<1,-1|1,-3>(16,-8,D:8,S:8,F:8,~F:8,1,^108m)* [D:0..255,S:0..255=255-D,F:0..255] For remotes that repeat the entire code each time, and {38.4k,564}<1,-1|1,-3>(16,-8,D:8,S:8,F:8,~F:8,1,^108m,(16,-4,1,^108m)*) [D:0..255,S:0..255=255-D,F:0..255]]] For remotes that send nec repeats. This would be compiled down BPF. I'm still working on the decoder and I haven't started on the BPF compilation side yet (the encoder is fully working). See https://github.com/seanyoung/cir/ > 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. > 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. Sean
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
Hi Marko, On Mon, Jul 04, 2022 at 10:04:38PM +0300, Marko Mäkelä wrote: > Mon, Jul 04, 2022 at 11:00:38AM +0100, Sean Young wrote: > > 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: > > > 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. Your patch needs a `Signed-off-by` and it should not be attached, it should be inline in your email. https://www.kernel.org/doc/html/latest/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text > > 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? The plan was for it to run once, generate an eBPF program, attach that an exit. The eBPF program sends the decoded stuff to the lirc chardev in this struct: https://www.kernel.org/doc/html/latest/userspace-api/media/rc/lirc-dev-intro.html#data-types-used-by-lirc-mode-scancode This is the struct you're amending with LIRC_SCANCODE_FLAG_REPEAT. Will that be sufficient for your needs? Sean
Hi Sean, Tue, Jul 05, 2022 at 08:25:48AM +0100, Sean Young wrote: >On Mon, Jul 04, 2022 at 10:04:38PM +0300, Marko Mäkelä wrote: >> Mon, Jul 04, 2022 at 11:00:38AM +0100, Sean Young wrote: >> > 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: >> > > 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. > >Your patch needs a `Signed-off-by` and it should not be attached, it should >be inline in your email. Thank you for your patience. I hope that I got it right. It would be my very first patch submission to the Linux kernel. I did not see it appear on this list archive yet. You are Cc'd. >> > 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? > >The plan was for it to run once, generate an eBPF program, attach that an >exit. The eBPF program sends the decoded stuff to the lirc chardev in >this struct: > >https://www.kernel.org/doc/html/latest/userspace-api/media/rc/lirc-dev-intro.html#data-types-used-by-lirc-mode-scancode > >This is the struct you're amending with LIRC_SCANCODE_FLAG_REPEAT. > >Will that be sufficient for your needs? I think that it should cover the most common types of remote control units. I can name an example of a complex IR remote control, which I think would be challenging to repurpose for controlling anything else than the original type of device. But, I would think that something Bluetooth or WLAN based on a touchscreen device will replace IR in such applications. The remote control of my air conditioner presents all settings on a local LCD. On every change, maybe after a short timeout of inactivity, it will send a long IR message with all the settings. The 32 bits of keycode or 64 bits of scancode would not be sufficient for that. Marko
Hi Marko, On Tue, Jul 05, 2022 at 11:48:50AM +0300, Marko Mäkelä wrote: > Tue, Jul 05, 2022 at 08:25:48AM +0100, Sean Young wrote: > > On Mon, Jul 04, 2022 at 10:04:38PM +0300, Marko Mäkelä wrote: > > > Mon, Jul 04, 2022 at 11:00:38AM +0100, Sean Young wrote: > > > > 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: > > > > > 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. > > > > Your patch needs a `Signed-off-by` and it should not be attached, it should > > be inline in your email. > > Thank you for your patience. I hope that I got it right. It would be my very > first patch submission to the Linux kernel. I did not see it appear on this > list archive yet. You are Cc'd. Thanks, I'll have a look. > > > > 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? > > > > The plan was for it to run once, generate an eBPF program, attach that an > > exit. The eBPF program sends the decoded stuff to the lirc chardev in > > this struct: > > > > https://www.kernel.org/doc/html/latest/userspace-api/media/rc/lirc-dev-intro.html#data-types-used-by-lirc-mode-scancode > > > > This is the struct you're amending with LIRC_SCANCODE_FLAG_REPEAT. > > > > Will that be sufficient for your needs? > > I think that it should cover the most common types of remote control units. That is the hope. > I can name an example of a complex IR remote control, which I think would be > challenging to repurpose for controlling anything else than the original > type of device. But, I would think that something Bluetooth or WLAN based on > a touchscreen device will replace IR in such applications. The remote > control of my air conditioner presents all settings on a local LCD. On every > change, maybe after a short timeout of inactivity, it will send a long IR > message with all the settings. The 32 bits of keycode or 64 bits of scancode > would not be sufficient for that. There certainly can be an eBPF decoder for this type of IR, but I'm not sure what the use case is, because it's not key information. Maybe there is something else in eBPF which is more suitable. BTW I'm sure it's possible to control an air conditioner with the cir tool above, using the IRP language. Would be great to see something like that. Transmission is already working, so just requires reverse engineering the protocol and writing some IRP for it. :) Sean
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c index 60e5153fcb24c..a83b1107fc7fd 100644 --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c @@ -1720,6 +1720,7 @@ static int rtl2832u_rc_query(struct dvb_usb_device *d) {IR_RX_BUF_CTRL, 0x80, 0xff}, {IR_RX_CTRL, 0x80, 0xff}, }; + u32 idle_length; /* init remote controller */ if (!dev->rc_active) { @@ -1770,6 +1771,28 @@ static int rtl2832u_rc_query(struct dvb_usb_device *d) if (ret) goto err; + dev_dbg(&d->intf->dev, "IR_RX_BUF=%*ph\n", len, buf); + + /* if the receiver is not idle yet, do not process */ + idle_length = 0; + if (len > 2) { + if (!(buf[len - 1] & 0x80)) + idle_length += buf[len - 1]; + if (!(buf[len - 2] & 0x80)) + idle_length += buf[len - 2]; + } + + if (idle_length < 0xbf) { + /* + * If the IR does not end with a space equal to the idle + * length, then the IR is not complete yet and more is to + * arrive shortly. If we process it and flush the buffer now, + * we end up missing IR. + */ + dev_dbg(&d->intf->dev, "ignoring idle=%x\n", idle_length); + return 0; + } + /* let hw receive new code */ for (i = 0; i < ARRAY_SIZE(refresh_tab); i++) { ret = rtl28xxu_wr_reg_mask(d, refresh_tab[i].reg, @@ -1807,10 +1830,10 @@ static int rtl2832u_get_rc_config(struct dvb_usb_device *d, rc->allowed_protos = RC_PROTO_BIT_ALL_IR_DECODER; rc->driver_type = RC_DRIVER_IR_RAW; rc->query = rtl2832u_rc_query; - rc->interval = 200; + rc->interval = 100; /* we program idle len to 0xc0, set timeout to one less */ rc->rawir_timeout = 0xbf * 51; - rc->keyup_delay = MS_TO_US(210); + rc->keyup_delay = MS_TO_US(110); return 0; }
This device presents an IR buffer, which can be read and cleared. Clearing the buffer is racey with receiving IR, so wait until the IR message is finished before clearing it. This should minimize the chance of the buffer clear happening while IR is being received, although we cannot avoid this completely. Signed-off-by: Sean Young <sean@mess.org> --- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 27 +++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)