| Message ID | cover.1604589023.git.michael@fossekall.de |
|---|---|
| Headers | show |
| Series | media: rc: gpio-ir-recv: add timeout property | expand |
On Mon, Nov 09, 2020 at 04:23:09PM +0100, Michael Klein wrote: > The default recorder timeout of 125ms is too high for some BPF protocol > decoders when a remote sends repeat codes at high rates. This makes the > timeout configurable via the devicetree. To be honest, 125ms is too much by any measurement. The longest space in any protocol I'm aware of is 40ms in the sharp ir protocol. I think changing IR_DEFAUL_TIMEOUT to something like 50ms would make sense. Also, when an BPF protocol is loaded, user-space can set the timeout with the LIRC_SET_REC_TIMEOUT ioctl which can depend on the protocol (set to longest space + 10ms error margin). This would mean that the bare minimum timeout can be set, which means decoding is as responsive as can be. I'm not sure that device tree is really the place for this. Thanks, Sean > > Changes in v2: > fix checkpatch.pl warnings > > Michael Klein (2): > media: rc: gpio-ir-recv: add recorder timeout property > media: bindings: media: gpio-ir-receiver: add linux,timeout-us > property > > .../devicetree/bindings/media/gpio-ir-receiver.txt | 3 +++ > Documentation/devicetree/bindings/media/rc.yaml | 6 ++++++ > drivers/media/rc/gpio-ir-recv.c | 3 ++- > 3 files changed, 11 insertions(+), 1 deletion(-) > > -- > 2.28.0
On Tue, Nov 10, 2020 at 01:48:05PM +0100, Michael Klein wrote: > On Tue, Nov 10, 2020 at 10:17:27AM +0000, Sean Young wrote: > > On Mon, Nov 09, 2020 at 04:23:09PM +0100, Michael Klein wrote: > > > The default recorder timeout of 125ms is too high for some BPF protocol > > > decoders when a remote sends repeat codes at high rates. This makes the > > > timeout configurable via the devicetree. > > > > To be honest, 125ms is too much by any measurement. The longest space > > in any protocol I'm aware of is 40ms in the sharp ir protocol. I think > > changing IR_DEFAUL_TIMEOUT to something like 50ms would make sense. > > Seconded. I'm happy to prepare a patch if changing the default value is > acceptable. Actually I don't understand why the high timeout is an issue. It means that between ir messages you don't get a LIRC_TIMEOUT, just a LIRC_SPACE. Why is this a problem? I'm not opposed to such a patch, but we would need to know if it really solves the problem you are having and it would need to sit in linux-next for some time. > > Also, when an BPF protocol is loaded, user-space can set the timeout > > with the LIRC_SET_REC_TIMEOUT ioctl which can depend on the protocol > > (set to longest space + 10ms error margin). > > Right, although this is a bit cumbersome with current user-space tools. The > BPF is loaded with ir-keytable, while the recorder timeout needs to be set > with it-ctl. In the Debian world, those tools are even in different > packages. ir-keytable can use the LIRC_SET_REC_TIMEOUT ioctl to adjust the timeout. It has opened the lirc device already to load the bpf program. ir-keytable would need to calculate the minimum timeout needed for all enabled protocols (bpf and non-bpf). Then it can simply do the ioctl. > > This would mean that the > > bare minimum timeout can be set, which means decoding is as responsive > > as can be. > > > > I'm not sure that device tree is really the place for this. > > Not arguing about this, but IMHO no less than for rc-map-name. So this seems > to be at least consistent. Well, I guess it can be argued. However, it can also be argued that it is not the best solution for this problem. Sean
On Tue, Nov 10, 2020 at 01:19:18PM +0000, Sean Young wrote: >On Tue, Nov 10, 2020 at 01:48:05PM +0100, Michael Klein wrote: >> On Tue, Nov 10, 2020 at 10:17:27AM +0000, Sean Young wrote: >> > On Mon, Nov 09, 2020 at 04:23:09PM +0100, Michael Klein wrote: >> > > The default recorder timeout of 125ms is too high for some BPF protocol >> > > decoders when a remote sends repeat codes at high rates. This makes the >> > > timeout configurable via the devicetree. >> > >> > To be honest, 125ms is too much by any measurement. The longest space >> > in any protocol I'm aware of is 40ms in the sharp ir protocol. I think >> > changing IR_DEFAUL_TIMEOUT to something like 50ms would make sense. >> >> Seconded. I'm happy to prepare a patch if changing the default value is >> acceptable. > >Actually I don't understand why the high timeout is an issue. It means that >between ir messages you don't get a LIRC_TIMEOUT, just a LIRC_SPACE. Why is >this a problem? Never mind; this turned out do be a problem of the BPF protocol decoder, which relied on LIRC_TIMEOUT to terminate each IR message. After overhaul it is quite a bit simpler now and works fine with the long timeout. Thank you for your insights. Michael
The default recorder timeout of 125ms is too high for some BPF protocol decoders when a remote sends repeat codes at high rates. This makes the timeout configurable via the devicetree. Changes in v2: fix checkpatch.pl warnings Michael Klein (2): media: rc: gpio-ir-recv: add recorder timeout property media: bindings: media: gpio-ir-receiver: add linux,timeout-us property .../devicetree/bindings/media/gpio-ir-receiver.txt | 3 +++ Documentation/devicetree/bindings/media/rc.yaml | 6 ++++++ drivers/media/rc/gpio-ir-recv.c | 3 ++- 3 files changed, 11 insertions(+), 1 deletion(-)