[PATCH] media: rc: ir-rc6-decoder: enable toggle bit for Kathrein RCU-676 remote

[Matthias Reichl]

The Kathrein RCU-676 remote uses the 32-bit rc6 protocol and toggles
bit 15 (0x8000) on repeated button presses, like MCE remotes.

Add it's customer code 0x8046 to the 32-bit rc6 toggle
handling code to get proper scancodes and toggle reports.

Signed-off-by: Matthias Reichl 
---
Here's the link to the bugreport and discussion:
https://forum.libreelec.tv/thread/13086-get-kathrein-rcu-676-remote-to-work-with-le9/

 drivers/media/rc/ir-rc6-decoder.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/ir-rc6-decoder.c 
b/drivers/media/rc/ir-rc6-decoder.c
index 68487ce9f79b..d96aed1343e4 100644
--- a/drivers/media/rc/ir-rc6-decoder.c
+++ b/drivers/media/rc/ir-rc6-decoder.c
@@ -40,6 +40,7 @@
 #define RC6_6A_MCE_TOGGLE_MASK 0x8000  /* for the body bits */
 #define RC6_6A_LCC_MASK0x /* RC6-6A-32 long customer 
code mask */
 #define RC6_6A_MCE_CC  0x800f /* MCE customer code */
+#define RC6_6A_KATHREIN_CC 0x8046 /* Kathrein RCU-676 customer code */
 #ifndef CHAR_BIT
 #define CHAR_BIT 8 /* Normally in  */
 #endif
@@ -242,13 +243,17 @@ static int ir_rc6_decode(struct rc_dev *dev, struct 
ir_raw_event ev)
toggle = 0;
break;
case 32:
-   if ((scancode & RC6_6A_LCC_MASK) == 
RC6_6A_MCE_CC) {
+   switch (scancode & RC6_6A_LCC_MASK) {
+   case RC6_6A_MCE_CC:
+   case RC6_6A_KATHREIN_CC:
protocol = RC_PROTO_RC6_MCE;
toggle = !!(scancode & 
RC6_6A_MCE_TOGGLE_MASK);
scancode &= ~RC6_6A_MCE_TOGGLE_MASK;
-   } else {
+   break;
+   default:
protocol = RC_PROTO_RC6_6A_32;
toggle = 0;
+   break;
}
break;
default:
-- 
2.18.0

Re: [PATCH v3 0/5] Add BPF decoders to ir-keytable

[Matthias Reichl]

Hi Sean,

On Sat, Jul 28, 2018 at 10:29:31AM +0100, Sean Young wrote:
> Hi Hias,
> 
> On Sat, Jul 21, 2018 at 08:13:27PM +0200, Matthias Reichl wrote:
> > Hi Sean,
> > 
> > thanks a lot, this is a really nice new feature!
> 
> Thank you for testing it and finding all those issues, it has become much
> better from your testing.
> 
> > On Fri, Jul 13, 2018 at 03:30:06PM +0100, Sean Young wrote:
> > > Once kernel v4.18 is released with IR BPF decoding, this can be merged
> > > to v4l-utils.
> > > 
> > > The idea is that IR decoders can be written in C, compiled to BPF 
> > > relocatable
> > > object file. Any global variables can overriden, so we can supports lots
> > > of variants of similiar protocols (just like in the lircd.conf file).
> > > 
> > > The existing rc_keymap file format can't be used for variables, so I've
> > > converted the format to toml. An alternative would be to use the existing
> > > lircd.conf file format, but it's a very awkward file to parse in C and it
> > > contains many features which are irrelevant to us.
> > > 
> > > We use libelf to load the bpf relocatable object file.
> > > 
> > > After loading our example grundig keymap with bpf decoder, the output of
> > > ir-keytable is:
> > > 
> > > Found /sys/class/rc/rc0/ (/dev/input/event8) with:
> > > Name: Winbond CIR
> > > Driver: winbond-cir, table: rc-rc6-mce
> > > LIRC device: /dev/lirc0
> > > Attached BPF protocols: grundig
> > > Supported kernel protocols: lirc rc-5 rc-5-sz jvc sony nec sanyo 
> > > mce_kbd rc-6 sharp xmp imon
> > > Enabled protocols: lirc
> > > bus: 25, vendor/product: 10ad:00f1, version: 0x0004
> > > Repeat delay = 500 ms, repeat period = 125 ms
> > > 
> > > Alternatively, you simply specify the path to the object file on the 
> > > command
> > > line:
> > > 
> > > $ ir-keytable -e header_pulse=9000,header_space=4500 -p ./pulse_distance.o
> > > 
> > > Derek, please note that you can now convert the dish lircd.conf to toml
> > > and load the keymap; it should just work. It would be great to have your
> > > feedback, thank you.
> > 
> > I did a few tests with one of my RC-5 remotes, this lircd.conf file
> > https://github.com/PiSupply/JustBoom/blob/master/LIRC/lircd.conf
> > and kernel 4.18-rc5 on RPi2, with the 32bit ARM kernel and
> > gpio-ir-recv, and on LePotato / aarch64 with meson-ir.
> > 
> > lircd2toml.py did a really good job on converting it, the only
> > thing missing was the toggle_bit.
> 
> Right, there was a bug in lirc2html.py. I've added a fix to my bpf branch:
> 
> https://git.linuxtv.org/syoung/v4l-utils.git/log/?h=bpf

Thanks a lot, lircd2toml.py now also seems to detect that the lircd.conf
uses the rc-5 protocol and uses the rc-5 decoder - that's really nice!

> > When testing the converted toml (with "toggle_bit = 11" added
> > and the obvious volume keycode fixes) I noticed a couple of issues:
> > 
> > Buttons seem to be "stuck". The scancode is decoded, key_down
> > event is generated, but after release the key_down events repeat
> > indefinitely - with the built-in rc-5 decoder this works fine.
> > 
> > root@upstream:/home/hias/ir-test# ir-keytable -c -w justboom.toml -t
> > Old keytable cleared
> > Wrote 12 keycode(s) to driver
> > Protocols changed to
> > Loaded BPF protocol manchester
> > Testing events. Please, press CTRL-C to abort.
> > 29.065820: lirc protocol(66): scancode = 0x141b
> > 29.065890: event type EV_MSC(0x04): scancode = 0x141b
> > 29.065890: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
> > 29.065890: event type EV_SYN(0x00).
> > 29.570059: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
> > 29.570059: event type EV_SYN(0x00).
> > 29.710062: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
> > 29.710062: event type EV_SYN(0x00).
> > 29.850057: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
> > 29.850057: event type EV_SYN(0x00).
> > 29.990057: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
> > 29.990057: event type EV_SYN(0x00).
> > 30.130055: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
> > 30.130055: event type EV_SYN(0x00).
> > ...
> 
> Thanks, I had not seen this yet either. There is a fix here:
> 
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133813.html

Thanks, the patch seems to fix it, I posted a small nit in the t

Re: [PATCH] media: rc: read out of bounds if bpf reports high protocol number

[Matthias Reichl]

Hi Sean,

On Sat, Jul 28, 2018 at 10:11:15AM +0100, Sean Young wrote:
> The repeat period is read from a static array. If a keydown event is
> reported from bpf with a high protocol number, we read out of bounds. This
> is unlikely to end up with a reasonable repeat period at the best of times,
> in which case no timely key up event is generated.
> 
> Signed-off-by: Sean Young 
> ---
>  drivers/media/rc/rc-main.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 2e222d9ee01f..a24850be1f4f 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -679,6 +679,14 @@ static void ir_timer_repeat(struct timer_list *t)
>   spin_unlock_irqrestore(>keylock, flags);
>  }
>  
> +unsigned int repeat_period(int protocol)
> +{
> + if (protocol >= ARRAY_SIZE(protocols))
> + return 100;

100 seems a bit arbitrarily chosen to me. Wouldn't it be better to
(re-)use eg protocols[RC_PROTO_UNKNOWN].repeat_period here?

so long,

Hias

> +
> + return protocols[protocol].repeat_period;
> +}
> +
>  /**
>   * rc_repeat() - signals that a key is still pressed
>   * @dev: the struct rc_dev descriptor of the device
> @@ -691,7 +699,7 @@ void rc_repeat(struct rc_dev *dev)
>  {
>   unsigned long flags;
>   unsigned int timeout = nsecs_to_jiffies(dev->timeout) +
> - msecs_to_jiffies(protocols[dev->last_protocol].repeat_period);
> + msecs_to_jiffies(repeat_period(dev->last_protocol));
>   struct lirc_scancode sc = {
>   .scancode = dev->last_scancode, .rc_proto = dev->last_protocol,
>   .keycode = dev->keypressed ? dev->last_keycode : KEY_RESERVED,
> @@ -803,7 +811,7 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto 
> protocol, u32 scancode,
>  
>   if (dev->keypressed) {
>   dev->keyup_jiffies = jiffies + nsecs_to_jiffies(dev->timeout) +
> - msecs_to_jiffies(protocols[protocol].repeat_period);
> + msecs_to_jiffies(repeat_period(protocol));
>   mod_timer(>timer_keyup, dev->keyup_jiffies);
>   }
>   spin_unlock_irqrestore(>keylock, flags);
> -- 
> 2.17.1
> 

Re: Logspam with "two consecutive events of type space" on gpio-ir-recv and meson-ir

[Matthias Reichl]

Hi Sean,

On Wed, Jul 25, 2018 at 09:21:00PM +0100, Sean Young wrote:
> Hi Hias,
> 
> On Sat, Jul 21, 2018 at 09:04:21PM +0200, Matthias Reichl wrote:
> > Hi Sean,
> > 
> > I noticed that on 4.18-rc5 I get dmesg logspam with
> > "rc rc0: two consecutive events of type space" on gpio-ir-recv
> > and meson-ir - mceusb seems to be fine (haven't tested with
> > other IR receivers yet).
> 
> This does not have a proper fix yet, however we have a workaround
> here:
> 
> https://git.linuxtv.org/media_tree.git/commit/?h=fixes=0ca54b29054151b7a52cbb8904732280afe5a302

Ah, thanks a lot for the pointer, must have missed the discussion
and patch on the list.

The workaround looks fine to me and should be good enough for now.

so long,

Hias

Logspam with "two consecutive events of type space" on gpio-ir-recv and meson-ir

[Matthias Reichl]

Hi Sean,

I noticed that on 4.18-rc5 I get dmesg logspam with
"rc rc0: two consecutive events of type space" on gpio-ir-recv
and meson-ir - mceusb seems to be fine (haven't tested with
other IR receivers yet).

With the default, short IR timeout I get these messages on each
IR message, which is rather spammy on longer button presses:

[ 1988.053215] rc rc0: two consecutive events of type space
[ 1988.173189] rc rc0: two consecutive events of type space
[ 1988.283188] rc rc0: two consecutive events of type space
[ 1988.403185] rc rc0: two consecutive events of type space
[ 1988.513193] rc rc0: two consecutive events of type space
[ 1988.623190] rc rc0: two consecutive events of type space
[ 1988.743190] rc rc0: two consecutive events of type space
[ 1988.853193] rc rc0: two consecutive events of type space
[ 1988.973193] rc rc0: two consecutive events of type space
[ 1989.083193] rc rc0: two consecutive events of type space
[ 1989.193196] rc rc0: two consecutive events of type space
[ 1989.313216] rc rc0: two consecutive events of type space
[ 1989.423197] rc rc0: two consecutive events of type space
...

With a longer timeout (eg 125ms and testing with a RC-5 remote) I get
these messages once per button press.

Eg on 2 shorter button presses:
# ir-keytable -t
Testing events. Please, press CTRL-C to abort.
2045.990064: lirc protocol(rc5): scancode = 0x101b
2045.990123: event type EV_MSC(0x04): scancode = 0x101b
2045.990123: event type EV_SYN(0x00).
2046.100077: lirc protocol(rc5): scancode = 0x101b
2046.100126: event type EV_MSC(0x04): scancode = 0x101b
2046.100126: event type EV_SYN(0x00).
2046.230075: lirc protocol(rc5): scancode = 0x101b
2046.230118: event type EV_MSC(0x04): scancode = 0x101b
2046.230118: event type EV_SYN(0x00).
2050.970078: lirc protocol(rc5): scancode = 0x101b toggle=1
2050.970137: event type EV_MSC(0x04): scancode = 0x101b
2050.970137: event type EV_SYN(0x00).
2051.080071: lirc protocol(rc5): scancode = 0x101b toggle=1
2051.080119: event type EV_MSC(0x04): scancode = 0x101b
2051.080119: event type EV_SYN(0x00).
2051.210056: lirc protocol(rc5): scancode = 0x101b toggle=1
2051.210099: event type EV_MSC(0x04): scancode = 0x101b
2051.210099: event type EV_SYN(0x00).

I get this in dmesg:
[ 2045.933635] rc rc0: two consecutive events of type space
[ 2050.923689] rc rc0: two consecutive events of type space

So it looks like that might be a timeout-related issue with
these 2 drivers.

so long,

Hias

Re: [PATCH v3 0/5] Add BPF decoders to ir-keytable

[Matthias Reichl]

Hi Sean,

thanks a lot, this is a really nice new feature!

On Fri, Jul 13, 2018 at 03:30:06PM +0100, Sean Young wrote:
> Once kernel v4.18 is released with IR BPF decoding, this can be merged
> to v4l-utils.
> 
> The idea is that IR decoders can be written in C, compiled to BPF relocatable
> object file. Any global variables can overriden, so we can supports lots
> of variants of similiar protocols (just like in the lircd.conf file).
> 
> The existing rc_keymap file format can't be used for variables, so I've
> converted the format to toml. An alternative would be to use the existing
> lircd.conf file format, but it's a very awkward file to parse in C and it
> contains many features which are irrelevant to us.
> 
> We use libelf to load the bpf relocatable object file.
> 
> After loading our example grundig keymap with bpf decoder, the output of
> ir-keytable is:
> 
> Found /sys/class/rc/rc0/ (/dev/input/event8) with:
> Name: Winbond CIR
> Driver: winbond-cir, table: rc-rc6-mce
> LIRC device: /dev/lirc0
> Attached BPF protocols: grundig
> Supported kernel protocols: lirc rc-5 rc-5-sz jvc sony nec sanyo 
> mce_kbd rc-6 sharp xmp imon
> Enabled protocols: lirc
> bus: 25, vendor/product: 10ad:00f1, version: 0x0004
> Repeat delay = 500 ms, repeat period = 125 ms
> 
> Alternatively, you simply specify the path to the object file on the command
> line:
> 
> $ ir-keytable -e header_pulse=9000,header_space=4500 -p ./pulse_distance.o
> 
> Derek, please note that you can now convert the dish lircd.conf to toml
> and load the keymap; it should just work. It would be great to have your
> feedback, thank you.

I did a few tests with one of my RC-5 remotes, this lircd.conf file
https://github.com/PiSupply/JustBoom/blob/master/LIRC/lircd.conf
and kernel 4.18-rc5 on RPi2, with the 32bit ARM kernel and
gpio-ir-recv, and on LePotato / aarch64 with meson-ir.

lircd2toml.py did a really good job on converting it, the only
thing missing was the toggle_bit.

When testing the converted toml (with "toggle_bit = 11" added
and the obvious volume keycode fixes) I noticed a couple of issues:

Buttons seem to be "stuck". The scancode is decoded, key_down
event is generated, but after release the key_down events repeat
indefinitely - with the built-in rc-5 decoder this works fine.

root@upstream:/home/hias/ir-test# ir-keytable -c -w justboom.toml -t
Old keytable cleared
Wrote 12 keycode(s) to driver
Protocols changed to
Loaded BPF protocol manchester
Testing events. Please, press CTRL-C to abort.
29.065820: lirc protocol(66): scancode = 0x141b
29.065890: event type EV_MSC(0x04): scancode = 0x141b
29.065890: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
29.065890: event type EV_SYN(0x00).
29.570059: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
29.570059: event type EV_SYN(0x00).
29.710062: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
29.710062: event type EV_SYN(0x00).
29.850057: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
29.850057: event type EV_SYN(0x00).
29.990057: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
29.990057: event type EV_SYN(0x00).
30.130055: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
30.130055: event type EV_SYN(0x00).
...

Even scancodes, eg KEY_UP / scancode 0x141a, aren't decoded at
all, only odd scancodes work. My guess is the manchester decoder
could have a problem when the last bit is zero and the message
doesn't end with a pulse, but a (rather long) timeout.

(Re-)loading a bpf decoder only works 8 times. The 9th attempt
gives an error message.

# for i in `seq 1 9` ; do ir-keytable -p manchester ; done
Protocols changed to
Loaded BPF protocol manchester
Protocols changed to
Loaded BPF protocol manchester
Protocols changed to
Loaded BPF protocol manchester
Protocols changed to
Loaded BPF protocol manchester
Protocols changed to
Loaded BPF protocol manchester
Protocols changed to
Loaded BPF protocol manchester
Protocols changed to
Loaded BPF protocol manchester
Protocols changed to
Loaded BPF protocol manchester
Protocols changed to
failed to create a map: 1 Operation not permitted
Loaded BPF protocol manchester

so long,

Hias

Re: [RFC PATCH v1 0/4] Add BPF decoders to ir-keytable

[Matthias Reichl]

Hi Sean,

On Sat, Jun 02, 2018 at 01:37:54PM +0100, Sean Young wrote:
> This is not ready for merging yet, however while I finish this work I would
> like some feedback and ideas.
> 
> The idea is that IR decoders can be written in C, compiled to BPF relocatable
> object file. Any global variables can overriden, so we can supports lots
> of variants of similiar protocols (just like in the lircd.conf file).
> 
> The existing rc_keymap file format can't be used for variables, so I've
> converted the format to toml. An alternative would be to use the existing
> lircd.conf file format, but it's a very awkward file to parse in C and it
> contains many features which are irrelevant to us.
> 
> We use libelf to load the bpf relocatable object file.
> 
> After loading our example grundig keymap with bpf decoder, the output of
> ir-keytable is:
> 
> Found /sys/class/rc/rc0/ (/dev/input/event8) with:
>   Name: Winbond CIR
>   Driver: winbond-cir, table: rc-rc6-mce
>   lirc device: /dev/lirc0
>   Attached bpf protocols: grundig
>   Supported protocols: lirc rc-5 rc-5-sz jvc sony nec sanyo mce_kbd rc-6 
> sharp xmp imon 
>   Enabled protocols: lirc
>   bus: 25, vendor/product: 10ad:00f1, version: 0x0004
>   Repeat delay = 500 ms, repeat period = 125 ms

I did a few tests with this patch and noticed some issues:

clang didn't find asm/types.h, I'm running Debian Stretch here
and asm isn't in /usr/include but in /usr/include/HOST-TRIPLET
which gcc uses by default, but clang doesn't seem to know about.

With a quick workaround (adding /usr/include/x86_64-linux-gnu/
on x86_64 and /usr/include/aarch64-linux-gnu/) the grundig decoder
built fine both on x86_64 and aarch64 and grundig.o files were
identical. I noticed some pointer arithmetic warnings though:

hias@lepotato:~/install/v4l-utils/git/utils/keytable$ make
Making all in bpf_protocols
make[1]: Entering directory 
'/home/hias/install/v4l-utils/git/utils/keytable/bpf_protocols'
clang -I../../../include -nostdinc -isystem 
/usr/lib/llvm-3.8/bin/../lib/clang/3.8.1/include -I/usr/include 
-I/usr/include/aarch64-linux-gnu -target bpf -O2 -emit-llvm -c grundig.c -o - | 
llc -march=bpf -filetype=obj -o grundig.o
make[1]: Leaving directory 
'/home/hias/install/v4l-utils/git/utils/keytable/bpf_protocols'
make[1]: Entering directory '/home/hias/install/v4l-utils/git/utils/keytable'
  CC   keytable.o
  CC   ir-encode.o
  CC   toml.o
  CC   bpf.o
  CC   bpf_load.o
bpf_load.c: In function ‘parse_relo_and_apply’:
bpf_load.c:192:41: warning: pointer of type ‘void *’ used in arithmetic 
[-Wpointer-arith]
 int32_t *p = (bpf_file->data->d_buf + sym.st_value);
 ^
bpf_load.c: In function ‘load_elf_maps_section’:
bpf_load.c:308:54: warning: pointer of type ‘void *’ used in arithmetic 
[-Wpointer-arith]
   def = (struct bpf_load_map_def *)(data_maps->d_buf + offset);
  ^
  CCLD ir-keytable
make[1]: Leaving directory '/home/hias/install/v4l-utils/git/utils/keytable'

When trying to load the grundig decoder (tested on aarch64 with
a kernel compiled from yesterday's head of bpf-next, commit
bd3a08aaa9a3) I get an "invalid mem access 'map_value_or_null'"
error:

hias@lepotato:~/install/v4l-utils/git/utils/keytable$ sudo ./ir-keytable -c -w 
rc_keymaps_bpf/RP75_LCD.toml
bpf protocols removed
Old keytable cleared
Wrote 40 keycode(s) to driver
header_space (null)
header_pulse 1244
leader_pulse (null)
bpf_load_program() err=13
0: (bf) r6 = r1
1: (b7) r8 = 0
2: (63) *(u32 *)(r10 -4) = r8
3: (bf) r2 = r10
4: (07) r2 += -4
5: (18) r1 = 0x80006960fa00
7: (85) call bpf_map_lookup_elem#1
8: (bf) r7 = r10
9: (07) r7 += -24
10: (1d) if r0 == r8 goto pc+1
 R0=map_value_or_null(id=1,off=0,ks=4,vs=16,imm=0) R6=ctx(id=0,off=0,imm=0) 
R7=fp-24,call_-1 R8=inv0 R10=fp0,call_-1
11: (bf) r7 = r0
12: (7b) *(u64 *)(r10 -16) = r8
13: (7b) *(u64 *)(r10 -24) = r8
14: (61) r1 = *(u32 *)(r6 +0)
15: (18) r3 = 0xff00
17: (bf) r2 = r1
18: (5f) r2 &= r3
19: (15) if r2 == 0x200 goto pc+131
 R0=map_value_or_null(id=1,off=0,ks=4,vs=16,imm=0) 
R1=inv(id=0,umax_value=4294967295,var_off=(0x0; 0x)) 
R2=inv(id=0,umax_value=4278190080,var_off=(0x0; 0xff00)) R3=inv4278190080 
R6=ctx(id=0,off=0,imm=0) R7=map_value_or_null(id=1,off=0,ks=4,vs=16,imm=0) 
R8=inv0 R10=fp0,call_-1 fp-16=0 fp-24=0
20: (57) r1 &= 16777215
21: (61) r3 = *(u32 *)(r7 +4)
R7 invalid mem access 'map_value_or_null'

Not quite sure who's to blame here, clang/llvm, the grundig code or
the loader. Do you have any ideas or hints?

so long,

Hias

Re: [PATCH v4 0/3] IR decoding using BPF

[Matthias Reichl]

Hi Sean,

On Fri, May 18, 2018 at 03:07:27PM +0100, Sean Young wrote:
> The kernel IR decoders (drivers/media/rc/ir-*-decoder.c) support the most
> widely used IR protocols, but there are many protocols which are not
> supported[1]. For example, the lirc-remotes[2] repo has over 2700 remotes,
> many of which are not supported by rc-core. There is a "long tail" of
> unsupported IR protocols, for which lircd is need to decode the IR .
> 
> IR encoding is done in such a way that some simple circuit can decode it;
> therefore, bpf is ideal.
> 
> In order to support all these protocols, here we have bpf based IR decoding.
> The idea is that user-space can define a decoder in bpf, attach it to
> the rc device through the lirc chardev.
> 
> Separate work is underway to extend ir-keytable to have an extensive library
> of bpf-based decoders, and a much expanded library of rc keymaps.
> 
> Another future application would be to compile IRP[3] to a IR BPF program, and
> so support virtually every remote without having to write a decoder for each.
> It might also be possible to support non-button devices such as analog
> directional pads or air conditioning remote controls and decode the target
> temperature in bpf, and pass that to an input device.

Thanks a lot, this looks like a very interesting feature to me!

Unfortunately I don't have time to test it ATM, but please keep
me posted - also on ir-keytable progress - I'm rather excited
to give it a try.

so long & thanks,

Hias

> 
> Thanks,
> 
> Sean Young
> 
> [1] http://www.hifi-remote.com/wiki/index.php?title=DecodeIR
> [2] https://sourceforge.net/p/lirc-remotes/code/ci/master/tree/remotes/
> [3] http://www.hifi-remote.com/wiki/index.php?title=IRP_Notation
> 
> Changes since v3:
>  - Implemented review comments from Quentin Monnet and Y Song (thanks!)
>  - More helpful and better formatted bpf helper documentation
>  - Changed back to bpf_prog_array rather than open-coded implementation
>  - scancodes can be 64 bit
>  - bpf gets passed values in microseconds, not nanoseconds.
>microseconds is more than than enough (IR receivers support carriers upto
>70kHz, at which point a single period is already 14 microseconds). Also,
>this makes it much more consistent with lirc mode2.
>  - Since it looks much more like lirc mode2, rename the program type to
>BPF_PROG_TYPE_LIRC_MODE2.
>  - Rebased on bpf-next
> 
> Changes since v2:
>  - Fixed locking issues
>  - Improved self-test to cover more cases
>  - Rebased on bpf-next again
> 
> Changes since v1:
>  - Code review comments from Y Song  and
>Randy Dunlap 
>  - Re-wrote sample bpf to be selftest
>  - Renamed RAWIR_DECODER -> RAWIR_EVENT (Kconfig, context, bpf prog type)
>  - Rebase on bpf-next
>  - Introduced bpf_rawir_event context structure with simpler access checking
> 
> Sean Young (3):
>   bpf: bpf_prog_array_copy() should return -ENOENT if exclude_prog not
> found
>   media: rc: introduce BPF_PROG_LIRC_MODE2
>   bpf: add selftest for lirc_mode2 type program
> 
>  drivers/media/rc/Kconfig  |  13 +
>  drivers/media/rc/Makefile |   1 +
>  drivers/media/rc/bpf-lirc.c   | 308 ++
>  drivers/media/rc/lirc_dev.c   |  30 ++
>  drivers/media/rc/rc-core-priv.h   |  22 ++
>  drivers/media/rc/rc-ir-raw.c  |  12 +-
>  include/linux/bpf_rcdev.h |  30 ++
>  include/linux/bpf_types.h |   3 +
>  include/uapi/linux/bpf.h  |  53 ++-
>  kernel/bpf/core.c |  11 +-
>  kernel/bpf/syscall.c  |   7 +
>  kernel/trace/bpf_trace.c  |   2 +
>  tools/bpf/bpftool/prog.c  |   1 +
>  tools/include/uapi/linux/bpf.h|  53 ++-
>  tools/include/uapi/linux/lirc.h   | 217 
>  tools/lib/bpf/libbpf.c|   1 +
>  tools/testing/selftests/bpf/Makefile  |   8 +-
>  tools/testing/selftests/bpf/bpf_helpers.h |   6 +
>  .../testing/selftests/bpf/test_lirc_mode2.sh  |  28 ++
>  .../selftests/bpf/test_lirc_mode2_kern.c  |  23 ++
>  .../selftests/bpf/test_lirc_mode2_user.c  | 154 +
>  21 files changed, 974 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/media/rc/bpf-lirc.c
>  create mode 100644 include/linux/bpf_rcdev.h
>  create mode 100644 tools/include/uapi/linux/lirc.h
>  create mode 100755 tools/testing/selftests/bpf/test_lirc_mode2.sh
>  create mode 100644 tools/testing/selftests/bpf/test_lirc_mode2_kern.c
>  create mode 100644 tools/testing/selftests/bpf/test_lirc_mode2_user.c
> 
> -- 
> 2.17.0
> 

Re: [PATCH v2 7/7] media: rc: mceusb: allow the timeout to be configurable

[Matthias Reichl]

Hi Sean,

On Thu, May 10, 2018 at 01:01:34PM +0200, Matthias Reichl wrote:
> On Wed, May 09, 2018 at 10:44:42PM +0100, Sean Young wrote:
> > On Mon, May 07, 2018 at 05:54:55PM +0200, Matthias Reichl wrote:
> > > The original reporter gave up before I could get enough info
> > > to understand what's going on, but now another user with an identical
> > > receiver and the same problems showed up and I could get debug logs.
> > > 
> > > FYI: I've uploaded the full dmesg here if you need more info
> > > or I snipped off too much:
> > > http://www.horus.com/~hias/tmp/mceusb-settimeout-issue.txt
> > > 
> > > Here's the info about the IR receiver:
> > > [2.208684] usb 1-1.3: New USB device found, idVendor=1784, 
> > > idProduct=0011
> > > [2.208699] usb 1-1.3: New USB device strings: Mfr=1, Product=2, 
> > > SerialNumber=3
> > > [2.208708] usb 1-1.3: Product: eHome Infrared Transceiver
> > > [2.208716] usb 1-1.3: Manufacturer: Topseed Technology Corp.
> > > [2.208724] usb 1-1.3: SerialNumber: EID0137AG-8-104054
> >
> > I think that is correct. For this mceusb device, we should add a quirk
> > which disables the setting of a timeout.
> > 
> > I do wonder if this could work in a slightly different way. Since the
> > device sends out space information before the timeout (every ~6ms), rather
> > than relying on setting a timeout on the device, we could rely on a 
> > software timeout handler. It would not be able to set timeouts > 100ms
> > but we're mostly not interested in that anyway.
> 
> Ah, this is a very good idea!
> 
> I had thought about a quirk to use that device in the old, fixed 100ms,
> timeout mode but if we can do <= 100ms in software it's a far
> better and elegant solution.
> 
> The patch looks fine to me, I'll add it to our builds and see if
> I can get some feedback on it.

The user with the Topseed remote reported back, your patch fixed
the issue - thanks a lot!

so long,

Hias

> > Here is a patch which hopefully solves it.
> > 
> > Thank you!
> > 
> > Sean
> > 
> > >>From 075e1567851ebe3e9d36b30f436c9468fd8772a8 Mon Sep 17 00:00:00 2001
> > From: Sean Young <s...@mess.org>
> > Date: Wed, 9 May 2018 11:11:28 +0100
> > Subject: [PATCH] media: mceusb: MCE_CMD_SETIRTIMEOUT cause strange behaviour
> >  on device
> > 
> > If the IR timeout is set on vid 1784 pid 0011, the device starts
> > behaving strangely.
> > 
> > Reported-by: Matthias Reichl <h...@horus.com>
> > Signed-off-by: Sean Young <s...@mess.org>
> > ---
> >  drivers/media/rc/mceusb.c | 22 +++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> > index 5c0bf61fae26..1619b748469b 100644
> > --- a/drivers/media/rc/mceusb.c
> > +++ b/drivers/media/rc/mceusb.c
> > @@ -181,6 +181,7 @@ enum mceusb_model_type {
> > MCE_GEN2 = 0,   /* Most boards */
> > MCE_GEN1,
> > MCE_GEN3,
> > +   MCE_GEN3_BROKEN_IRTIMEOUT,
> > MCE_GEN2_TX_INV,
> > MCE_GEN2_TX_INV_RX_GOOD,
> > POLARIS_EVK,
> > @@ -199,6 +200,7 @@ struct mceusb_model {
> > u32 mce_gen3:1;
> > u32 tx_mask_normal:1;
> > u32 no_tx:1;
> > +   u32 broken_irtimeout:1;
> > /*
> >  * 2nd IR receiver (short-range, wideband) for learning mode:
> >  * 0, absent 2nd receiver (rx2)
> > @@ -242,6 +244,12 @@ static const struct mceusb_model mceusb_model[] = {
> > .tx_mask_normal = 1,
> > .rx2 = 2,
> > },
> > +   [MCE_GEN3_BROKEN_IRTIMEOUT] = {
> > +   .mce_gen3 = 1,
> > +   .tx_mask_normal = 1,
> > +   .rx2 = 2,
> > +   .broken_irtimeout = 1
> > +   },
> > [POLARIS_EVK] = {
> > /*
> >  * In fact, the EVK is shipped without
> > @@ -352,7 +360,7 @@ static const struct usb_device_id mceusb_dev_table[] = {
> >   .driver_info = MCE_GEN2_TX_INV },
> > /* Topseed eHome Infrared Transceiver */
> > { USB_DEVICE(VENDOR_TOPSEED, 0x0011),
> > - .driver_info = MCE_GEN3 },
> > + .driver_info = MCE_GEN3_BROKEN_IRTIMEOUT },
> > /* Ricavision internal Infrared Transceiver */
> > { USB_DEVICE(VENDOR_RICAVISION, 0x0010) },
> > /* Itron ione Libra Q-11 */
> > @@ -1441,8 +1449,16 @@ static struct rc_dev *mceusb_init_rc_dev(struct 
> > mceusb_dev *ir)
> > rc->allowed_pr

Re: [PATCH 1/3] media: mceusb: filter out bogus timing irdata of duration 0

[Matthias Reichl]

Hi Sean,

On Fri, May 11, 2018 at 09:36:48AM +0100, Sean Young wrote:
> A mceusb device has been observed producing invalid irdata. Proactively
> guard against this.

Thanks a lot, the patch series looks good to me! Good catch on the
missing break BTW.

We've included this patch in LibreELEC testbuilds, so far we got
not complaints.

so long,

Hias

> 
> Suggested-by: Matthias Reichl <h...@horus.com>
> Signed-off-by: Sean Young <s...@mess.org>
> ---
>  drivers/media/rc/mceusb.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> index 1619b748469b..1ca49491abc8 100644
> --- a/drivers/media/rc/mceusb.c
> +++ b/drivers/media/rc/mceusb.c
> @@ -1177,6 +1177,11 @@ static void mceusb_process_ir_data(struct mceusb_dev 
> *ir, int buf_len)
>   init_ir_raw_event();
>   rawir.pulse = ((ir->buf_in[i] & MCE_PULSE_BIT) != 0);
>   rawir.duration = (ir->buf_in[i] & MCE_PULSE_MASK);
> + if (unlikely(!rawir.duration)) {
> + dev_warn(ir->dev, "nonsensical irdata %02x with 
> duration 0",
> +  ir->buf_in[i]);
> + break;
> + }
>   if (rawir.pulse) {
>   ir->pulse_tunit += rawir.duration;
>   ir->pulse_count++;
> -- 
> 2.14.3
> 

[PATCH] media: rc: ite-cir: lower timeout and extend allowed timeout range

[Matthias Reichl]

The minimum possible timeout of ite-cir is 8 samples, which is
typically about 70us. The driver however changes the FIFO trigger
level from the hardware's default of 1 byte to 17 bytes, so the minimum
usable timeout value is 17 * 8 samples, which is typically about 1.2ms.

Tests showed that using timeouts down to 1.2ms actually work fine.

The current default timeout of 200ms is much longer than necessary and
the maximum timeout of 1s seems to have been chosen a bit arbitrarily.

So change the minimum timeout to the driver's limit of 17 * 8 samples
and bring timeout and maximum timeout in line with the settings
of many other receivers.

Signed-off-by: Matthias Reichl <h...@horus.com>
---
 drivers/media/rc/ite-cir.c | 8 +---
 drivers/media/rc/ite-cir.h | 7 ---
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
index 65e104c7ddfc..de77d22c30a7 100644
--- a/drivers/media/rc/ite-cir.c
+++ b/drivers/media/rc/ite-cir.c
@@ -1561,9 +1561,11 @@ static int ite_probe(struct pnp_dev *pdev, const struct 
pnp_device_id
rdev->close = ite_close;
rdev->s_idle = ite_s_idle;
rdev->s_rx_carrier_range = ite_set_rx_carrier_range;
-   rdev->min_timeout = ITE_MIN_IDLE_TIMEOUT;
-   rdev->max_timeout = ITE_MAX_IDLE_TIMEOUT;
-   rdev->timeout = ITE_IDLE_TIMEOUT;
+   /* FIFO threshold is 17 bytes, so 17 * 8 samples minimum */
+   rdev->min_timeout = 17 * 8 * ITE_BAUDRATE_DIVISOR *
+   itdev->params.sample_period;
+   rdev->timeout = IR_DEFAULT_TIMEOUT;
+   rdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
rdev->rx_resolution = ITE_BAUDRATE_DIVISOR *
itdev->params.sample_period;
rdev->tx_resolution = ITE_BAUDRATE_DIVISOR *
diff --git a/drivers/media/rc/ite-cir.h b/drivers/media/rc/ite-cir.h
index 0e8ebc880d1f..9cb24ac01350 100644
--- a/drivers/media/rc/ite-cir.h
+++ b/drivers/media/rc/ite-cir.h
@@ -154,13 +154,6 @@ struct ite_dev {
 /* default carrier freq for when demodulator is off (Hz) */
 #define ITE_DEFAULT_CARRIER_FREQ   38000
 
-/* default idling timeout in ns (0.2 seconds) */
-#define ITE_IDLE_TIMEOUT   2UL
-
-/* limit timeout values */
-#define ITE_MIN_IDLE_TIMEOUT   1UL
-#define ITE_MAX_IDLE_TIMEOUT   10UL
-
 /* convert bits to us */
 #define ITE_BITS_TO_NS(bits, sample_period) \
 ((u32) ((bits) * ITE_BAUDRATE_DIVISOR * sample_period))
-- 
2.11.0

Re: [PATCH v2 7/7] media: rc: mceusb: allow the timeout to be configurable

[Matthias Reichl]

Hi Sean!

On Wed, May 09, 2018 at 10:44:42PM +0100, Sean Young wrote:
> Hi Hias,
> 
> On Mon, May 07, 2018 at 05:54:55PM +0200, Matthias Reichl wrote:
> > Hi Sean!
> > 
> > [ I trimmed the Cc list, as this is mceusb specific ]
> > 
> > On Sat, Apr 21, 2018 at 07:41:21PM +0200, Matthias Reichl wrote:
> > > On Sat, Apr 21, 2018 at 03:18:52PM +0200, Matthias Reichl wrote:
> > > > Another bug report came in, button press results in multiple
> > > > key down/up events
> > > > https://forum.kodi.tv/showthread.php?tid=298461=2727837#pid2727837
> > > > (and following posts).
> > 
> > The original reporter gave up before I could get enough info
> > to understand what's going on, but now another user with an identical
> > receiver and the same problems showed up and I could get debug logs.
> > 
> > FYI: I've uploaded the full dmesg here if you need more info
> > or I snipped off too much:
> > http://www.horus.com/~hias/tmp/mceusb-settimeout-issue.txt
> > 
> > Here's the info about the IR receiver:
> > [2.208684] usb 1-1.3: New USB device found, idVendor=1784, 
> > idProduct=0011
> > [2.208699] usb 1-1.3: New USB device strings: Mfr=1, Product=2, 
> > SerialNumber=3
> > [2.208708] usb 1-1.3: Product: eHome Infrared Transceiver
> > [2.208716] usb 1-1.3: Manufacturer: Topseed Technology Corp.
> > [2.208724] usb 1-1.3: SerialNumber: EID0137AG-8-104054
> > 
> > With timeout configuration in the mceusb driver disabled everything
> > works fine. But with timeout configuration enabled spurious "keyup"
> > events show up during a button press and sometimes also a spurious
> > "ghost" keypress several seconds after the original button press.
> > 
> > Here's the ir-keytable -t output to illustrate the behaviour:
> > 
> > 80.385585: event type EV_MSC(0x04): scancode = 0x800f0422
> > 80.385585: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
> > 80.385585: event type EV_SYN(0x00).
> > 80.492469: event type EV_MSC(0x04): scancode = 0x800f0422
> > 80.492469: event type EV_SYN(0x00).
> > 80.633371: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
> > 80.633371: event type EV_SYN(0x00).
> > 80.642478: event type EV_MSC(0x04): scancode = 0x800f0422
> > 80.642478: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
> > 80.642478: event type EV_SYN(0x00).
> > 80.783375: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
> > 80.783375: event type EV_SYN(0x00).
> > 84.318011: event type EV_MSC(0x04): scancode = 0x800f0422
> > 84.318011: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
> > 84.318011: event type EV_SYN(0x00).
> > 84.460049: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
> > 84.460049: event type EV_SYN(0x00).
> > 
> > >From the kernel log the first 2 scancodes are perfectly fine,
> > we get the timeout space in chunks, followed by the "End of raw IR data"
> > message and the scancode is properly decoded. Then about 45ms later
> > the pulses of the following IR message come in.
> > 
> > Snippet from end of second scancode:
> > 
> > [   80.505896] mceusb 1-1.3:1.0: rx data: 81 7f (length=2)
> 
> I've been staring at these logs for days now. :)

You are not the only one :-)

> So here, 0x81 means one
> byte of IR data, which is 0x7f. For each byte of IR data, if the high 
> bit is set (so value >= 0x80) it is a pulse, else space. The remaining
> 7 bits denote the length of the pulse or space, times 50us. So here
> we have 127 * 50 = 6350us space.
> 
> > [   80.505902] mceusb 1-1.3:1.0: Raw IR data, 1 pulse/space samples
> > [   80.505907] mceusb 1-1.3:1.0: Storing space with duration 635
> > [   80.505911] mceusb 1-1.3:1.0: processed IR data
> > [   80.506894] mceusb 1-1.3:1.0: rx data: 81 05 (length=2)
> > [   80.506899] mceusb 1-1.3:1.0: Raw IR data, 1 pulse/space samples
> > [   80.506904] mceusb 1-1.3:1.0: Storing space with duration 25
> 
> 5 * 50 us = 250us space
> 
> > [   80.506908] rc rc0: enter idle mode
> > [   80.506913] rc rc0: sample: (25650us space)
> > [   80.506918] mceusb 1-1.3:1.0: rx data: 80 (length=1)
> 
> So the 80 here is the byte which marks the timeout.

Yes. More specifically, it's the "IR Data End" message / command,
as mentioned on page 130 of the "Green Button" pdf.

> > [   80.506922] mceusb 1-1.3:1.0: End of raw IR data
> > [   80.506925] mceusb 1-1.3:1.0: processed IR data
> > [   80.506943] rc rc0: RC6 decode started at state 6 (25650us space)
> > [   80.506949] rc rc0: RC6 decode started at state 8 (25

Re: [PATCH v2 7/7] media: rc: mceusb: allow the timeout to be configurable

[Matthias Reichl]

Hi Sean!

[ I trimmed the Cc list, as this is mceusb specific ]

On Sat, Apr 21, 2018 at 07:41:21PM +0200, Matthias Reichl wrote:
> On Sat, Apr 21, 2018 at 03:18:52PM +0200, Matthias Reichl wrote:
> > Another bug report came in, button press results in multiple
> > key down/up events
> > https://forum.kodi.tv/showthread.php?tid=298461=2727837#pid2727837
> > (and following posts).

The original reporter gave up before I could get enough info
to understand what's going on, but now another user with an identical
receiver and the same problems showed up and I could get debug logs.

FYI: I've uploaded the full dmesg here if you need more info
or I snipped off too much:
http://www.horus.com/~hias/tmp/mceusb-settimeout-issue.txt

Here's the info about the IR receiver:
[2.208684] usb 1-1.3: New USB device found, idVendor=1784, idProduct=0011
[2.208699] usb 1-1.3: New USB device strings: Mfr=1, Product=2, 
SerialNumber=3
[2.208708] usb 1-1.3: Product: eHome Infrared Transceiver
[2.208716] usb 1-1.3: Manufacturer: Topseed Technology Corp.
[2.208724] usb 1-1.3: SerialNumber: EID0137AG-8-104054

With timeout configuration in the mceusb driver disabled everything
works fine. But with timeout configuration enabled spurious "keyup"
events show up during a button press and sometimes also a spurious
"ghost" keypress several seconds after the original button press.

Here's the ir-keytable -t output to illustrate the behaviour:

80.385585: event type EV_MSC(0x04): scancode = 0x800f0422
80.385585: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
80.385585: event type EV_SYN(0x00).
80.492469: event type EV_MSC(0x04): scancode = 0x800f0422
80.492469: event type EV_SYN(0x00).
80.633371: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
80.633371: event type EV_SYN(0x00).
80.642478: event type EV_MSC(0x04): scancode = 0x800f0422
80.642478: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
80.642478: event type EV_SYN(0x00).
80.783375: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
80.783375: event type EV_SYN(0x00).
84.318011: event type EV_MSC(0x04): scancode = 0x800f0422
84.318011: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
84.318011: event type EV_SYN(0x00).
84.460049: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
84.460049: event type EV_SYN(0x00).

>From the kernel log the first 2 scancodes are perfectly fine,
we get the timeout space in chunks, followed by the "End of raw IR data"
message and the scancode is properly decoded. Then about 45ms later
the pulses of the following IR message come in.

Snippet from end of second scancode:

[   80.505896] mceusb 1-1.3:1.0: rx data: 81 7f (length=2)
[   80.505902] mceusb 1-1.3:1.0: Raw IR data, 1 pulse/space samples
[   80.505907] mceusb 1-1.3:1.0: Storing space with duration 635
[   80.505911] mceusb 1-1.3:1.0: processed IR data
[   80.506894] mceusb 1-1.3:1.0: rx data: 81 05 (length=2)
[   80.506899] mceusb 1-1.3:1.0: Raw IR data, 1 pulse/space samples
[   80.506904] mceusb 1-1.3:1.0: Storing space with duration 25
[   80.506908] rc rc0: enter idle mode
[   80.506913] rc rc0: sample: (25650us space)
[   80.506918] mceusb 1-1.3:1.0: rx data: 80 (length=1)
[   80.506922] mceusb 1-1.3:1.0: End of raw IR data
[   80.506925] mceusb 1-1.3:1.0: processed IR data
[   80.506943] rc rc0: RC6 decode started at state 6 (25650us space)
[   80.506949] rc rc0: RC6 decode started at state 8 (25650us space)
[   80.506955] rc rc0: RC6(6A) proto 0x0013, scancode 0x800f0422 (toggle: 1)
[   80.506961] rc rc0: Media Center Ed. eHome Infrared Remote Transceiver 
(1784:0011): scancode 0x800f0422 keycode 0x160
[   80.552906] mceusb 1-1.3:1.0: rx data: 81 b5 (length=2)
[   80.552914] mceusb 1-1.3:1.0: Raw IR data, 1 pulse/space samples
[   80.552919] mceusb 1-1.3:1.0: Storing pulse with duration 265
[   80.552924] rc rc0: leave idle mode

But then, the end of the third scancode gets interesting. The
last chunk of the timeout space is missing and instead we get
a combined message with the remaining space and a zero-length
pulse just before the fourth IR message starts. Of course, this is
too late and the keyup timer had already expired:

[   80.612896] mceusb 1-1.3:1.0: rx data: 81 7f (length=2)
[   80.612903] mceusb 1-1.3:1.0: Raw IR data, 1 pulse/space samples
[   80.612908] mceusb 1-1.3:1.0: Storing space with duration 635
[   80.612912] mceusb 1-1.3:1.0: processed IR data
[   80.647880] rc rc0: keyup key 0x0160
[   80.656901] mceusb 1-1.3:1.0: rx data: 82 05 80 (length=3)
[   80.656908] mceusb 1-1.3:1.0: Raw IR data, 2 pulse/space samples
[   80.656913] mceusb 1-1.3:1.0: Storing space with duration 25
[   80.656918] rc rc0: enter idle mode
[   80.656923] rc rc0: sample: (25650us space)
[   80.656928] mceusb 1-1.3:1.0: Storing pulse with duration 0
[   80.656931] rc rc0: leave idle mode
[   80.656935] mceusb 1-1.3:1.0: processed IR data
[   80.656967] rc rc0: RC6 decode started at state 6 (25650us

Re: [PATCH v2 7/7] media: rc: mceusb: allow the timeout to be configurable

[Matthias Reichl]

On Sat, Apr 21, 2018 at 03:18:52PM +0200, Matthias Reichl wrote:
> Hi Sean,
> 
> On Fri, Apr 20, 2018 at 12:17:23AM +0200, Matthias Reichl wrote:
> > One of the affected users reported back that the fix worked fine!
> > 
> > I'm keeping an eye on further reports but so far I'd say everything's
> > working really well.
> 
> Another bug report came in, button press results in multiple
> key down/up events
> https://forum.kodi.tv/showthread.php?tid=298461=2727837#pid2727837
> (and following posts).
> 
> ir-ctl -r output looks fine to me, but ir-keytable -t output suggests
> we'll need a margin between raw timeout and key up timeout:
> 
> https://pastebin.com/6RTSGXvp
> 2199.824106: event type EV_MSC(0x04): scancode = 0x800f0419
> 2199.824106: event type EV_KEY(0x01) key_down: KEY_STOP(0x0080)
> 2199.824106: event type EV_SYN(0x00).
> 2199.887081: event type EV_KEY(0x01) key_up: KEY_STOP(0x0080)
> 2199.887081: event type EV_MSC(0x04): scancode = 0x800f0422
> 2199.887081: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
> 2199.887081: event type EV_SYN(0x00).
> 2200.029804: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
> 2200.029804: event type EV_SYN(0x00).

Sorry, just noticed I snipped off the interesting part, here
is the rest:

2200.036070: event type EV_MSC(0x04): scancode = 0x800f0422
2200.036070: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
2200.036070: event type EV_SYN(0x00).
2200.143067: event type EV_MSC(0x04): scancode = 0x800f0422
2200.143067: event type EV_SYN(0x00).
2200.206061: event type EV_MSC(0x04): scancode = 0x800f0422
2200.206061: event type EV_SYN(0x00).
2200.346472: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
2200.346472: event type EV_SYN(0x00).

I looked at the log again, and now it doesn't make much sense
to me.

I'm not exactly sure what happened with the first KEY_OK
scancode, that seems to have been decoded ~60ms after the
KEY_STOP scancode (.887 vs .824).

The second KEY_OK scancode was decoded ~ 150ms after the first,
(and caused the problem), delay to third is 107ms, to fourth 63ms.

I'll ask the user to change batteries on the remote and check
if the reception is OK, could be that this was a false alarm.

Adding some margin (maybe 20-50ms) for keyup could still make
sense though, as currently we have basically just the timeout
as margin, which can be quite low and round to 1-2 jiffies.

so long,

Hias

Re: [PATCH v2 7/7] media: rc: mceusb: allow the timeout to be configurable

[Matthias Reichl]

Hi Sean,

On Fri, Apr 20, 2018 at 12:17:23AM +0200, Matthias Reichl wrote:
> One of the affected users reported back that the fix worked fine!
> 
> I'm keeping an eye on further reports but so far I'd say everything's
> working really well.

Another bug report came in, button press results in multiple
key down/up events
https://forum.kodi.tv/showthread.php?tid=298461=2727837#pid2727837
(and following posts).

ir-ctl -r output looks fine to me, but ir-keytable -t output suggests
we'll need a margin between raw timeout and key up timeout:

https://pastebin.com/6RTSGXvp
2199.824106: event type EV_MSC(0x04): scancode = 0x800f0419
2199.824106: event type EV_KEY(0x01) key_down: KEY_STOP(0x0080)
2199.824106: event type EV_SYN(0x00).
2199.887081: event type EV_KEY(0x01) key_up: KEY_STOP(0x0080)
2199.887081: event type EV_MSC(0x04): scancode = 0x800f0422
2199.887081: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
2199.887081: event type EV_SYN(0x00).
2200.029804: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
2200.029804: event type EV_SYN(0x00).

so long,

Hias

Re: [PATCH v2 7/7] media: rc: mceusb: allow the timeout to be configurable

[Matthias Reichl]

Hi Sean,

On Wed, Apr 18, 2018 at 07:42:29PM +0200, Matthias Reichl wrote:
> Hi Sean,
> 
> On Wed, Apr 18, 2018 at 12:24:29PM +0100, Sean Young wrote:
> > Hello Hias,
> > 
> > On Tue, Apr 17, 2018 at 09:14:57PM +0200, Matthias Reichl wrote:
> > > On Sun, Apr 08, 2018 at 10:19:42PM +0100, Sean Young wrote:
> > > > mceusb devices have a default timeout of 100ms, but this can be changed.
> > > 
> > > We finally added a backport of the v2 series (and also the mce_kbd
> > > series) to LibreELEC yesterday and ratcher quickly received 2 bugreports
> > > from users using mceusb receivers.
> > > 
> > > Local testing on RPi/gpio-ir and Intel NUC/ite-cir was fine, I've
> > > been using the v2 series for over a week without issues on
> > > LibreELEC (RPi with kernel 4.14).
> > > 
> > > Here are the links to the bugreports and logs:
> > > https://forum.kodi.tv/showthread.php?tid=298461=2726684#pid2726684
> > > https://forum.kodi.tv/showthread.php?tid=298462=2726690#pid2726690
> > > 
> > > Both users are using similar mceusb receivers:
> > > 
> > > Log 1:
> > > [6.418218] rc rc0: Media Center Ed. eHome Infrared Remote Transceiver 
> > > (147a:e017) as 
> > > /devices/platform/soc/3f98.usb/usb1/1-1/1-1.3/1-1.3:1.0/rc/rc0
> > > [6.418358] input: Media Center Ed. eHome Infrared Remote Transceiver 
> > > (147a:e017) as 
> > > /devices/platform/soc/3f98.usb/usb1/1-1/1-1.3/1-1.3:1.0/rc/rc0/input0
> > > [6.419443] rc rc0: lirc_dev: driver ir-lirc-codec (mceusb) registered 
> > > at minor = 0
> > > [6.608114] mceusb 1-1.3:1.0: Registered Formosa21 SnowflakeEmulation 
> > > with mce emulator interface version 1
> > > [6.608125] mceusb 1-1.3:1.0: 0 tx ports (0x0 cabled) and 1 rx sensors 
> > > (0x1 active)
> > > 
> > > Log 2:
> > > [3.023361] rc rc0: Media Center Ed. eHome Infrared Remote Transceiver 
> > > (147a:e03e) as /devices/pci:00/:00:14.0/usb1/1-10/1-10:1.0/rc/rc0
> > > [3.023393] input: Media Center Ed. eHome Infrared Remote Transceiver 
> > > (147a:e03e) as 
> > > /devices/pci:00/:00:14.0/usb1/1-10/1-10:1.0/rc/rc0/input11
> > > [3.023868] rc rc0: lirc_dev: driver ir-lirc-codec (mceusb) registered 
> > > at minor = 0
> > > [3.119384] input: eventlircd as /devices/virtual/input/input21
> > > [3.138625] ip6_tables: (C) 2000-2006 Netfilter Core Team
> > > [3.196830] mceusb 1-10:1.0: Registered Formosa21 eHome Infrared 
> > > Transceiver with mce emulator interface version 2
> > > [3.196836] mceusb 1-10:1.0: 0 tx ports (0x0 cabled) and 1 rx sensors 
> > > (0x1 active)
> > > 
> > > In both cases ir-keytable doesn't report any scancodes and the
> > > ir-ctl -r output contains very odd long space values where I'd expect
> > > a short timeout instead:
> > > 
> > > gap between messages:
> > > space 800
> > > pulse 450
> > > space 16777215
> > > space 25400
> > > pulse 2650
> > > space 800
> > > 
> > > end of last message:
> > > space 800
> > > pulse 450
> > > space 16777215
> > > timeout 31750
> > > 
> > > This patch applied cleanly on 4.14 and the mceusb history from
> > > 4.14 to media/master looked rather unsuspicious. I'm not 100% sure
> > > if I might have missed a dependency when backporting the patch
> > > or if this is indeed an issue of this patch on these particular
> > > (or maybe some more) mceusb receivers.
> > 
> > Thanks again for a great bug report and analysis! So, it seems with the
> > shorter timeout, some mceusb devices add a specific "timeout" code to
> > the IR data stream (0x80) rather than a space. The current mceusb code
> > resets the decoders in this case, causing the IR decoders to reset and
> > lirc to report a space of 0xff.
> > 
> > Turns out that one of my mceusb devices also suffers from this, I don't
> > know how I missed this. Anyway hopefully this will solve the problem.
> 
> Thanks a lot for the quick fix!
> 
> I can't test myself as I don't have the hardware, but we will
> include the patch in tonight's LibreELEC build and I asked the
> users to test it. I'll keep you posted about the outcome.

One of the affected users reported back that the fix worked fine!

I'm keeping an eye on further reports but so far I'd say everything's
working really well.

Great job and thanks a lot for the im

Re: [PATCH v2 7/7] media: rc: mceusb: allow the timeout to be configurable

[Matthias Reichl]

Hi Sean,

On Wed, Apr 18, 2018 at 12:24:29PM +0100, Sean Young wrote:
> Hello Hias,
> 
> On Tue, Apr 17, 2018 at 09:14:57PM +0200, Matthias Reichl wrote:
> > On Sun, Apr 08, 2018 at 10:19:42PM +0100, Sean Young wrote:
> > > mceusb devices have a default timeout of 100ms, but this can be changed.
> > 
> > We finally added a backport of the v2 series (and also the mce_kbd
> > series) to LibreELEC yesterday and ratcher quickly received 2 bugreports
> > from users using mceusb receivers.
> > 
> > Local testing on RPi/gpio-ir and Intel NUC/ite-cir was fine, I've
> > been using the v2 series for over a week without issues on
> > LibreELEC (RPi with kernel 4.14).
> > 
> > Here are the links to the bugreports and logs:
> > https://forum.kodi.tv/showthread.php?tid=298461=2726684#pid2726684
> > https://forum.kodi.tv/showthread.php?tid=298462=2726690#pid2726690
> > 
> > Both users are using similar mceusb receivers:
> > 
> > Log 1:
> > [6.418218] rc rc0: Media Center Ed. eHome Infrared Remote Transceiver 
> > (147a:e017) as 
> > /devices/platform/soc/3f98.usb/usb1/1-1/1-1.3/1-1.3:1.0/rc/rc0
> > [6.418358] input: Media Center Ed. eHome Infrared Remote Transceiver 
> > (147a:e017) as 
> > /devices/platform/soc/3f98.usb/usb1/1-1/1-1.3/1-1.3:1.0/rc/rc0/input0
> > [6.419443] rc rc0: lirc_dev: driver ir-lirc-codec (mceusb) registered 
> > at minor = 0
> > [6.608114] mceusb 1-1.3:1.0: Registered Formosa21 SnowflakeEmulation 
> > with mce emulator interface version 1
> > [6.608125] mceusb 1-1.3:1.0: 0 tx ports (0x0 cabled) and 1 rx sensors 
> > (0x1 active)
> > 
> > Log 2:
> > [3.023361] rc rc0: Media Center Ed. eHome Infrared Remote Transceiver 
> > (147a:e03e) as /devices/pci:00/:00:14.0/usb1/1-10/1-10:1.0/rc/rc0
> > [3.023393] input: Media Center Ed. eHome Infrared Remote Transceiver 
> > (147a:e03e) as 
> > /devices/pci:00/:00:14.0/usb1/1-10/1-10:1.0/rc/rc0/input11
> > [3.023868] rc rc0: lirc_dev: driver ir-lirc-codec (mceusb) registered 
> > at minor = 0
> > [3.119384] input: eventlircd as /devices/virtual/input/input21
> > [3.138625] ip6_tables: (C) 2000-2006 Netfilter Core Team
> > [3.196830] mceusb 1-10:1.0: Registered Formosa21 eHome Infrared 
> > Transceiver with mce emulator interface version 2
> > [3.196836] mceusb 1-10:1.0: 0 tx ports (0x0 cabled) and 1 rx sensors 
> > (0x1 active)
> > 
> > In both cases ir-keytable doesn't report any scancodes and the
> > ir-ctl -r output contains very odd long space values where I'd expect
> > a short timeout instead:
> > 
> > gap between messages:
> > space 800
> > pulse 450
> > space 16777215
> > space 25400
> > pulse 2650
> > space 800
> > 
> > end of last message:
> > space 800
> > pulse 450
> > space 16777215
> > timeout 31750
> > 
> > This patch applied cleanly on 4.14 and the mceusb history from
> > 4.14 to media/master looked rather unsuspicious. I'm not 100% sure
> > if I might have missed a dependency when backporting the patch
> > or if this is indeed an issue of this patch on these particular
> > (or maybe some more) mceusb receivers.
> 
> Thanks again for a great bug report and analysis! So, it seems with the
> shorter timeout, some mceusb devices add a specific "timeout" code to
> the IR data stream (0x80) rather than a space. The current mceusb code
> resets the decoders in this case, causing the IR decoders to reset and
> lirc to report a space of 0xff.
> 
> Turns out that one of my mceusb devices also suffers from this, I don't
> know how I missed this. Anyway hopefully this will solve the problem.

Thanks a lot for the quick fix!

I can't test myself as I don't have the hardware, but we will
include the patch in tonight's LibreELEC build and I asked the
users to test it. I'll keep you posted about the outcome.

so long,

Hias

> 
> 
> Thanks
> 
> Sean
> 
> >>From 92d27b206e51993e927dc0b3aba210a621eef3d0 Mon Sep 17 00:00:00 2001
> From: Sean Young <s...@mess.org>
> Date: Wed, 18 Apr 2018 10:36:25 +0100
> Subject: [PATCH] media: rc: mceusb: IR of length 0 means IR timeout, not reset
> 
> The last usb packet with IR data will end with 0x80 (MCE_IRDATA_TRAILER).
> If we reset the decoder state at this point, IR decoding can fail.
> 
> Signed-off-by: Sean Young <s...@mess.org>
> ---
>  drivers/media/rc/mceusb.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> index c97c

Re: [PATCH v2 7/7] media: rc: mceusb: allow the timeout to be configurable

[Matthias Reichl]

Hi Sean!

On Sun, Apr 08, 2018 at 10:19:42PM +0100, Sean Young wrote:
> mceusb devices have a default timeout of 100ms, but this can be changed.

We finally added a backport of the v2 series (and also the mce_kbd
series) to LibreELEC yesterday and ratcher quickly received 2 bugreports
from users using mceusb receivers.

Local testing on RPi/gpio-ir and Intel NUC/ite-cir was fine, I've
been using the v2 series for over a week without issues on
LibreELEC (RPi with kernel 4.14).

Here are the links to the bugreports and logs:
https://forum.kodi.tv/showthread.php?tid=298461=2726684#pid2726684
https://forum.kodi.tv/showthread.php?tid=298462=2726690#pid2726690

Both users are using similar mceusb receivers:

Log 1:
[6.418218] rc rc0: Media Center Ed. eHome Infrared Remote Transceiver 
(147a:e017) as 
/devices/platform/soc/3f98.usb/usb1/1-1/1-1.3/1-1.3:1.0/rc/rc0
[6.418358] input: Media Center Ed. eHome Infrared Remote Transceiver 
(147a:e017) as 
/devices/platform/soc/3f98.usb/usb1/1-1/1-1.3/1-1.3:1.0/rc/rc0/input0
[6.419443] rc rc0: lirc_dev: driver ir-lirc-codec (mceusb) registered at 
minor = 0
[6.608114] mceusb 1-1.3:1.0: Registered Formosa21 SnowflakeEmulation with 
mce emulator interface version 1
[6.608125] mceusb 1-1.3:1.0: 0 tx ports (0x0 cabled) and 1 rx sensors (0x1 
active)

Log 2:
[3.023361] rc rc0: Media Center Ed. eHome Infrared Remote Transceiver 
(147a:e03e) as /devices/pci:00/:00:14.0/usb1/1-10/1-10:1.0/rc/rc0
[3.023393] input: Media Center Ed. eHome Infrared Remote Transceiver 
(147a:e03e) as 
/devices/pci:00/:00:14.0/usb1/1-10/1-10:1.0/rc/rc0/input11
[3.023868] rc rc0: lirc_dev: driver ir-lirc-codec (mceusb) registered at 
minor = 0
[3.119384] input: eventlircd as /devices/virtual/input/input21
[3.138625] ip6_tables: (C) 2000-2006 Netfilter Core Team
[3.196830] mceusb 1-10:1.0: Registered Formosa21 eHome Infrared Transceiver 
with mce emulator interface version 2
[3.196836] mceusb 1-10:1.0: 0 tx ports (0x0 cabled) and 1 rx sensors (0x1 
active)

In both cases ir-keytable doesn't report any scancodes and the
ir-ctl -r output contains very odd long space values where I'd expect
a short timeout instead:

gap between messages:
space 800
pulse 450
space 16777215
space 25400
pulse 2650
space 800

end of last message:
space 800
pulse 450
space 16777215
timeout 31750

This patch applied cleanly on 4.14 and the mceusb history from
4.14 to media/master looked rather unsuspicious. I'm not 100% sure
if I might have missed a dependency when backporting the patch
or if this is indeed an issue of this patch on these particular
(or maybe some more) mceusb receivers.

so long,

Hias

> Signed-off-by: Sean Young 
> ---
>  drivers/media/rc/mceusb.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> index 69ba57372c05..c97cb2eb1c5f 100644
> --- a/drivers/media/rc/mceusb.c
> +++ b/drivers/media/rc/mceusb.c
> @@ -982,6 +982,25 @@ static int mceusb_set_tx_carrier(struct rc_dev *dev, u32 
> carrier)
>   return 0;
>  }
>  
> +static int mceusb_set_timeout(struct rc_dev *dev, unsigned int timeout)
> +{
> + u8 cmdbuf[4] = { MCE_CMD_PORT_IR, MCE_CMD_SETIRTIMEOUT, 0, 0 };
> + struct mceusb_dev *ir = dev->priv;
> + unsigned int units;
> +
> + units = DIV_ROUND_CLOSEST(timeout, US_TO_NS(MCE_TIME_UNIT));
> +
> + cmdbuf[2] = units >> 8;
> + cmdbuf[3] = units;
> +
> + mce_async_out(ir, cmdbuf, sizeof(cmdbuf));
> +
> + /* get receiver timeout value */
> + mce_async_out(ir, GET_RX_TIMEOUT, sizeof(GET_RX_TIMEOUT));
> +
> + return 0;
> +}
> +
>  /*
>   * Select or deselect the 2nd receiver port.
>   * Second receiver is learning mode, wide-band, short-range receiver.
> @@ -1415,7 +1434,10 @@ static struct rc_dev *mceusb_init_rc_dev(struct 
> mceusb_dev *ir)
>   rc->dev.parent = dev;
>   rc->priv = ir;
>   rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
> + rc->min_timeout = US_TO_NS(MCE_TIME_UNIT);
>   rc->timeout = MS_TO_NS(100);
> + rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
> + rc->s_timeout = mceusb_set_timeout;
>   if (!ir->flags.no_tx) {
>   rc->s_tx_mask = mceusb_set_tx_mask;
>   rc->s_tx_carrier = mceusb_set_tx_carrier;
> -- 
> 2.14.3
> 

Re: [PATCH v2 0/7] Improve latency of IR decoding

[Matthias Reichl]

On Tue, Apr 10, 2018 at 07:39:43PM +0100, Sean Young wrote:
> On Tue, Apr 10, 2018 at 07:53:43PM +0200, Matthias Reichl wrote:
> > Hi Sean,
> > 
> > On Sun, Apr 08, 2018 at 10:19:35PM +0100, Sean Young wrote:
> > > The current IR decoding is much too slow. Many IR protocols rely on
> > > a trailing space for decoding (e.g. rc-6 needs to know when the bits
> > > end). The trailing space is generated by the IR timeout, and if this
> > > is longer than required, buttons can feel slow to respond.
> > > 
> > > The other issue is the keyup timer. IR has no concept of a keyup message,
> > > this is implied by the absence of IR. So, minimising the timeout for
> > > this makes buttons less "sticky"; the are released much quicker.
> > > 
> > > With these patches in place, using IR with the builtin decoders is much
> > > improved and feels very snappy.
> > > 
> > > Changes since v1:
> > >  - lost more testing
> > >  - fixed various issues with mce decoder
> > >  - fixed mceusb so it can use better timeout too
> > 
> > thanks, this version is working fine with meson-ir and gpio-rc-recv
> > (latter on RPi). I mainly tested it with rc-5 remotes so far, more
> > will follow and I'll update LibreELEC in a day or two to include
> > the v2 series.
> 
> Thanks again for testing!
> 
> > Also thanks a lot for the mce_kbd fixes, I was just going to dig
> > into the decoder (we received a bug report about stuck keys with
> > mce_kbd last week), your patches can in just at the right time :)
> > 
> > I had a closer look at ir-mce_kbd-decoder.c and noticed 2 things
> > (which can be handled separately):
> > 
> > It looks like the input_sync call in the state machine error
> > path is not necessary:
> > 
> > out:
> > dev_dbg(>dev, "failed at state %i (%uus %s)\n",
> > data->state, TO_US(ev.duration), TO_STR(ev.pulse));
> > data->state = STATE_INACTIVE;
> > input_sync(data->idev);
> > return -EINVAL;
> > 
> > If I followed the code paths correctly states that generate
> > input events won't go to out, only paths were no events are
> > generated jump to out - but better verify that, I could have
> > missed something.
> 
> The input_sync() patch is in mce_kbd_rx_timeout() code path, which
> doesn't go through this. Hopefully it should fix the keys stuck
> issue.

Yes, I saw that and the input_sync() in mce_kbd_rx_timeout() fixed
this (I could verify that locally).

What I meant is that the input_sync() in the snippet above, which
isn't touched by your patches, doesn't seem necessary, as the
code paths that lead to there don't submit input events. The
happy paths (successful key/mouse event and keyup) call input_sync()
at the end of STATE_FINISHED:

lsc.scancode = scancode;
ir_lirc_scancode_event(dev, );
data->state = STATE_INACTIVE;
input_event(data->idev, EV_MSC, MSC_SCAN, scancode);
input_sync(data->idev);
return 0;

> > The other thing I noticed is that there's no spinlock synchronizing
> > the events from the timeout callback with the ones from the state
> > machine - like keylock in rc-main. So the code could potentially
> > be racy (if the timeout fires while the state machine is outputting
> > events).
> 
> This timeout is for sending keyups in case no keyup is received from
> the keyboard via input_report_key(). The input system does locking
> for us (see drivers/input/input.c:436).

The synchronization code in rc-main looks like it's used to make
code blocks that eg call input_report_key and input_sync do that
in an atomic way (plus there's the special handling of keyup_jiffies
to prevent a race betwen the keyup timeout callback and new
decoded scancodes.

As both the timeout callback and the state machine send out
multiple key events and end them with input_sync it looks to me
like we'd need to make that atomic as well. Otherwise we could
have eg timeout running, starting to report a bunch of shift
and other keys as up, then interruption by a new key/scancode
calling input_report_key down plus input_sync and then timeout
resuming and reporting the rest of the keys down and again
do input_sync().

so long,

Hias

Re: [PATCH v2 0/7] Improve latency of IR decoding

[Matthias Reichl]

Hi Sean,

On Sun, Apr 08, 2018 at 10:19:35PM +0100, Sean Young wrote:
> The current IR decoding is much too slow. Many IR protocols rely on
> a trailing space for decoding (e.g. rc-6 needs to know when the bits
> end). The trailing space is generated by the IR timeout, and if this
> is longer than required, buttons can feel slow to respond.
> 
> The other issue is the keyup timer. IR has no concept of a keyup message,
> this is implied by the absence of IR. So, minimising the timeout for
> this makes buttons less "sticky"; the are released much quicker.
> 
> With these patches in place, using IR with the builtin decoders is much
> improved and feels very snappy.
> 
> Changes since v1:
>  - lost more testing
>  - fixed various issues with mce decoder
>  - fixed mceusb so it can use better timeout too

thanks, this version is working fine with meson-ir and gpio-rc-recv
(latter on RPi). I mainly tested it with rc-5 remotes so far, more
will follow and I'll update LibreELEC in a day or two to include
the v2 series.

Also thanks a lot for the mce_kbd fixes, I was just going to dig
into the decoder (we received a bug report about stuck keys with
mce_kbd last week), your patches can in just at the right time :)

I had a closer look at ir-mce_kbd-decoder.c and noticed 2 things
(which can be handled separately):

It looks like the input_sync call in the state machine error
path is not necessary:

out:
dev_dbg(>dev, "failed at state %i (%uus %s)\n",
data->state, TO_US(ev.duration), TO_STR(ev.pulse));
data->state = STATE_INACTIVE;
input_sync(data->idev);
return -EINVAL;

If I followed the code paths correctly states that generate
input events won't go to out, only paths were no events are
generated jump to out - but better verify that, I could have
missed something.

The other thing I noticed is that there's no spinlock synchronizing
the events from the timeout callback with the ones from the state
machine - like keylock in rc-main. So the code could potentially
be racy (if the timeout fires while the state machine is outputting
events).

so long,

Hias

> 
> Sean Young (7):
>   media: rc: set timeout to smallest value required by enabled protocols
>   media: rc: add ioctl to get the current timeout
>   media: rc: per-protocol repeat period and minimum keyup timer
>   media: rc: mce_kbd decoder: low timeout values cause double keydowns
>   media: rc: mce_kbd protocol encodes two scancodes
>   media: rc: mce_kbd decoder: fix stuck keys
>   media: rc: mceusb: allow the timeout to be configurable
> 
>  Documentation/media/uapi/rc/lirc-func.rst  |  1 +
>  .../media/uapi/rc/lirc-set-rec-timeout.rst | 14 +++--
>  drivers/media/cec/cec-core.c   |  2 +-
>  drivers/media/rc/ir-imon-decoder.c |  1 +
>  drivers/media/rc/ir-jvc-decoder.c  |  1 +
>  drivers/media/rc/ir-mce_kbd-decoder.c  | 36 +++-
>  drivers/media/rc/ir-nec-decoder.c  |  1 +
>  drivers/media/rc/ir-rc5-decoder.c  |  1 +
>  drivers/media/rc/ir-rc6-decoder.c  |  1 +
>  drivers/media/rc/ir-sanyo-decoder.c|  1 +
>  drivers/media/rc/ir-sharp-decoder.c|  1 +
>  drivers/media/rc/ir-sony-decoder.c |  1 +
>  drivers/media/rc/ir-xmp-decoder.c  |  1 +
>  drivers/media/rc/lirc_dev.c|  9 ++-
>  drivers/media/rc/mceusb.c  | 22 +++
>  drivers/media/rc/rc-core-priv.h|  1 +
>  drivers/media/rc/rc-ir-raw.c   | 31 +-
>  drivers/media/rc/rc-main.c | 68 
> +++---
>  include/uapi/linux/lirc.h  |  6 ++
>  19 files changed, 144 insertions(+), 55 deletions(-)
> 
> -- 
> 2.14.3
> 

Re: [PATCH 0/3] Improve latency of IR decoding

[Matthias Reichl]

Hi Sean,

On Wed, Mar 28, 2018 at 08:30:29PM +0200, Matthias Reichl wrote:
> Hi Sean,
> 
> On Sat, Mar 24, 2018 at 02:50:42PM +, Sean Young wrote:
> > The current IR decoding is much too slow. Many IR protocols rely on
> > a trailing space for decoding (e.g. rc-6 needs to know when the bits
> > end). The trailing space is generated by the IR timeout, and if this
> > is longer than required, keys can be perceived as sticky and slugish.
> > 
> > The other issue the keyup timer. IR has no concept of a keyup message,
> > this is implied by the absence of IR. So, minimising the timeout for
> > this further improves the handling.
> > 
> > With these patches in place, using IR with the builtin decoders is much
> > improved and feels very snappy.
> 
> thanks a lot for the patches!
> 
> I didn't have much time to test yet, but quick checks on
> Amlogic/meson-ir (kernel 4.16-rc7 + media tree + your patches)
> and Raspberry Pi (RPi foundation kernel 4.14 + my backport of
> your patches) look really promising.
> 
> I found one issue, though, in ir-sharp-decoder.c max_space must
> be set to SHARP_ECHO_SPACE - otherwise we get a timeout between
> the normal and inverted message part and decoding fails.
> 
> One thing I'm wondering is if the keyup timer marging might
> be too tight now. Basically we have just the fixed 10ms marging
> from idle timeout. The repeat periods of the protocols are rather
> accurate/strict, so (programmable) remotes not sticking to the
> official timing might cause repeated keyup/down events if they
> are repeating a tad to slow.
> 
> I'm not sure if this could be an issue, but maybe we should
> add a safety margin to the repeat periods as well? For example
> 10 or 20 percent of the specced repeat periods. What do you think?
> 
> To get some more test coverage I've asked my colleague to
> include my backport patch in the LibreELEC testbuilds for
> x86 and RPi. We've got some 500 regular users of these so if
> something's not working we should find out soon.

Quick update: testing so far went really smooth, no issues reported
since we included the backport patch in LibreELEC testbuilds.

Quote from 
https://github.com/LibreELEC/LibreELEC.tv/pull/2623#issuecomment-377897518
> This is in my nightly test builds since 28 March, and no problems reported so 
> far.
>
> On my NUC with Harmony One/RC6 remote these commits are working just fine.

I've been using LibreELEC on RPi with the patch (using a gpio ir
receiver and a Hauppauge RC-5 remote) since then and everything
was fine as well.

so long,

Hias

> I just hope I
> didn't mess up the backport... Here's the link to my 4.14 patch:
> 
> https://github.com/HiassofT/LibreELEC.tv/blob/le9-ir-latency/packages/linux/patches/default/linux-999-improve-ir-timeout-handling.patch
> 
> so long & thanks,
> 
> Hias
> 
> > 
> > Sean Young (3):
> >   media: rc: set timeout to smallest value required by enabled protocols
> >   media: rc: add ioctl to get the current timeout
> >   media: rc: per-protocol repeat period and minimum keyup timer
> > 
> >  Documentation/media/uapi/rc/lirc-func.rst  |  1 +
> >  .../media/uapi/rc/lirc-set-rec-timeout.rst | 14 +++--
> >  drivers/media/cec/cec-core.c   |  2 +-
> >  drivers/media/rc/ir-imon-decoder.c |  1 +
> >  drivers/media/rc/ir-jvc-decoder.c  |  1 +
> >  drivers/media/rc/ir-mce_kbd-decoder.c  |  1 +
> >  drivers/media/rc/ir-nec-decoder.c  |  1 +
> >  drivers/media/rc/ir-rc5-decoder.c  |  1 +
> >  drivers/media/rc/ir-rc6-decoder.c  |  1 +
> >  drivers/media/rc/ir-sanyo-decoder.c|  1 +
> >  drivers/media/rc/ir-sharp-decoder.c|  1 +
> >  drivers/media/rc/ir-sony-decoder.c |  1 +
> >  drivers/media/rc/ir-xmp-decoder.c  |  1 +
> >  drivers/media/rc/lirc_dev.c|  9 ++-
> >  drivers/media/rc/rc-core-priv.h|  1 +
> >  drivers/media/rc/rc-ir-raw.c   | 31 +-
> >  drivers/media/rc/rc-main.c | 68 
> > +++---
> >  include/uapi/linux/lirc.h  |  6 ++
> >  18 files changed, 101 insertions(+), 41 deletions(-)
> > 
> > -- 
> > 2.14.3
> > 

Re: [PATCH 0/3] Improve latency of IR decoding

[Matthias Reichl]

Hi Sean,

On Sat, Mar 24, 2018 at 02:50:42PM +, Sean Young wrote:
> The current IR decoding is much too slow. Many IR protocols rely on
> a trailing space for decoding (e.g. rc-6 needs to know when the bits
> end). The trailing space is generated by the IR timeout, and if this
> is longer than required, keys can be perceived as sticky and slugish.
> 
> The other issue the keyup timer. IR has no concept of a keyup message,
> this is implied by the absence of IR. So, minimising the timeout for
> this further improves the handling.
> 
> With these patches in place, using IR with the builtin decoders is much
> improved and feels very snappy.

thanks a lot for the patches!

I didn't have much time to test yet, but quick checks on
Amlogic/meson-ir (kernel 4.16-rc7 + media tree + your patches)
and Raspberry Pi (RPi foundation kernel 4.14 + my backport of
your patches) look really promising.

I found one issue, though, in ir-sharp-decoder.c max_space must
be set to SHARP_ECHO_SPACE - otherwise we get a timeout between
the normal and inverted message part and decoding fails.

One thing I'm wondering is if the keyup timer marging might
be too tight now. Basically we have just the fixed 10ms marging
from idle timeout. The repeat periods of the protocols are rather
accurate/strict, so (programmable) remotes not sticking to the
official timing might cause repeated keyup/down events if they
are repeating a tad to slow.

I'm not sure if this could be an issue, but maybe we should
add a safety margin to the repeat periods as well? For example
10 or 20 percent of the specced repeat periods. What do you think?

To get some more test coverage I've asked my colleague to
include my backport patch in the LibreELEC testbuilds for
x86 and RPi. We've got some 500 regular users of these so if
something's not working we should find out soon. I just hope I
didn't mess up the backport... Here's the link to my 4.14 patch:

https://github.com/HiassofT/LibreELEC.tv/blob/le9-ir-latency/packages/linux/patches/default/linux-999-improve-ir-timeout-handling.patch

so long & thanks,

Hias

> 
> Sean Young (3):
>   media: rc: set timeout to smallest value required by enabled protocols
>   media: rc: add ioctl to get the current timeout
>   media: rc: per-protocol repeat period and minimum keyup timer
> 
>  Documentation/media/uapi/rc/lirc-func.rst  |  1 +
>  .../media/uapi/rc/lirc-set-rec-timeout.rst | 14 +++--
>  drivers/media/cec/cec-core.c   |  2 +-
>  drivers/media/rc/ir-imon-decoder.c |  1 +
>  drivers/media/rc/ir-jvc-decoder.c  |  1 +
>  drivers/media/rc/ir-mce_kbd-decoder.c  |  1 +
>  drivers/media/rc/ir-nec-decoder.c  |  1 +
>  drivers/media/rc/ir-rc5-decoder.c  |  1 +
>  drivers/media/rc/ir-rc6-decoder.c  |  1 +
>  drivers/media/rc/ir-sanyo-decoder.c|  1 +
>  drivers/media/rc/ir-sharp-decoder.c|  1 +
>  drivers/media/rc/ir-sony-decoder.c |  1 +
>  drivers/media/rc/ir-xmp-decoder.c  |  1 +
>  drivers/media/rc/lirc_dev.c|  9 ++-
>  drivers/media/rc/rc-core-priv.h|  1 +
>  drivers/media/rc/rc-ir-raw.c   | 31 +-
>  drivers/media/rc/rc-main.c | 68 
> +++---
>  include/uapi/linux/lirc.h  |  6 ++
>  18 files changed, 101 insertions(+), 41 deletions(-)
> 
> -- 
> 2.14.3
> 

Issue with sharp protocol on ite-cir due to high idle timeout

[Matthias Reichl]

Hi Sean,

yesterday we received an interesting bug report that might
help to motivate using a lower idle timeout on ite-cir:

https://forum.libreelec.tv/thread/11951-sharp-ir-remote-on-intel-nuc-2820-double-presses/

The rather long message time of the sharp protocol (about 86ms)
in combination with the 200ms default idle timeout of ite-cir
leads to the last message being received after the current 250ms
"keyup" timeout. This results in an additional keyup/keydown
event being generated from the last message. Even a short button
press results in 2 keydown events.

I dont't have any ite-cir hardware here but could reproduce it
with gpio-ir-recv set to 200ms idle timeout. Lowering the
timeout to 100 or 125ms fixed the issue both here and on the
reporter's hardware.

so long,

Hias

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Matthias Reichl]

Hi Sean,

On Mon, Mar 12, 2018 at 01:58:11PM +, Sean Young wrote:
> On Mon, Mar 12, 2018 at 02:20:00PM +0100, Matthias Reichl wrote:
> > On Sun, Mar 11, 2018 at 12:55:19PM +, Sean Young wrote:
> > > That makes complete sense. I'm actually keen to get this lowered, since
> > > this makes it possible to lower the repeat period per-protocol, see
> > > commit d57ea877af38 which had to be reverted (the ite driver will
> > > need fixing up as well before this can happen).
> > 
> > I remember the commit, this issue hit us in LibreELEC testbuilds
> > as well :-)
> > 
> > > Lowering to below 125ms does increase the risk of regressions, so I
> > > am weary of that. Do you think there is benefit in doing this?
> > 
> > I'd also say stick to the 125ms default. The default settings
> > should always be safe ones IMO.
> 
> Well, yes. I just wanted to explore the ideal situation before making
> up our minds.
> 
> > People who want to optimize for the last bit of performance can
> > easily do that on their own, at their own risk.
> > 
> > 
> > Personally I've been using gpio-ir-recv on RPi with the default 125ms
> > timeout and a Hauppauge rc-5 remote for about 2 years now and I've
> > always been happy with that.
> 
> Ok. We should try to get this change for meson-ir ready for v4.17. I can
> write a patch later.

Thanks, it worked fine!

> > I have to acknowledge though that the responsiveness of a remote
> > with short messages, like rc-5, in combination with a low timeout
> > (I tested down to 10ms) is pretty impressive.
> 
> I've been thinking about this problem. What we could do is have a 
> per-protocol maximum space length, and repeat period. The timeout
> can then be set to a maximum space length (+safety margin), and the
> keyup timer can be set to timeout + repeat period (+safety margin).

This sounds like a very good idea. It won't help much with IR
receivers that have no configurable timeout or a large minimum
timeout (ite-cir has 100ms min, probably a hardware limitation?),
but for other receivers this'll be a nice improvement.

> If memory serves, the lirc daemon always sets the timeout with
> LIRC_SET_REC_TIMEOUT, so it would not affect lirc daemon decoding.

Current versions of Lirc (0.9.4 and newer) don't seem to use
LIRC_SET_REC_TIMEOUT but handle timeouts on it's own via a
timeout value in poll().

There's still some generic code in lircd.cpp that supports setting
timeouts via LIRC_SET_REC_TIMEOUT but the default plugin (which
handles /dev/lircX) doesn't implement any of the required 
get/set timeout ioctls.

strace on lircd 0.10.0 also shows that only LIRC_GET_FEATURES is
used.

Older Lirc versions (checked with 0.9.1 source I had here)
seem to be using LIRC_SET_REC_TIMEOUT.

So I think we should be fine here.

Not sure if there are other users of the /dev/lirc interface
that could be affected, I'm only familiar with lirc and the
tools from v4l-utils.

> Anyway, just an idea. Not something for v4.17.

No need to rush things, your idea looks good to me but better
test it thoroughly.

Drop me a line if you have a first implementation, I'd be happy
to help with testing.

so long,

Hias

Re: [PATCH] media: rc: meson-ir: lower timeout and make configurable

[Matthias Reichl]

On Mon, Mar 12, 2018 at 09:48:40PM +, Sean Young wrote:
> A timeout of 200ms is much longer than necessary, and delays the decoding
> decoding of a single scancode and the last scancode when a button is being
> held. This makes the remote seem sluggish.
> 
> If the min_timeout and max_timeout values are set, the timeout is
> configurable via the LIRC_SET_REC_TIMEOUT ioctl.
> 
> Signed-off-by: Sean Young <s...@mess.org>

Tested-by: Matthias Reichl <h...@horus.com>

Thanks a lot, this is working fine!

so long,

Hias

> ---
>  drivers/media/rc/meson-ir.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> index 64b0aa4f4db7..f449b35d25e7 100644
> --- a/drivers/media/rc/meson-ir.c
> +++ b/drivers/media/rc/meson-ir.c
> @@ -144,7 +144,9 @@ static int meson_ir_probe(struct platform_device *pdev)
>   ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
>   ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
>   ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
> - ir->rc->timeout = MS_TO_NS(200);
> + ir->rc->min_timeout = 1;
> + ir->rc->timeout = IR_DEFAULT_TIMEOUT;
> + ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
>   ir->rc->driver_name = DRIVER_NAME;
>  
>   spin_lock_init(>lock);
> -- 
> 2.14.3
> 

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Matthias Reichl]

Hi Sean,

On Sun, Mar 11, 2018 at 12:55:19PM +, Sean Young wrote:
> Hi Matthias,
> 
> On Sat, Mar 10, 2018 at 06:38:28PM +0100, Matthias Reichl wrote:
> > On Sat, Mar 10, 2018 at 11:27:45AM +, Sean Young wrote:
> > > So if the timeout is below N then you will never get a space of N or 
> > > larger;
> > > the largest space I know of in an IR message is 40ms in the sanyo 
> > > protocol:
> > > 
> > > https://www.sbprojects.net/knowledge/ir/sharp.php
> > > 
> > > So if timeout is set to less than 40ms, we would have trouble decoding the
> > > sharp protocol.
> > > 
> > > The space between nec repeats is a little less than 100ms. I'm trying to
> > > remember what would could go wrong if the space between them would be
> > > timeouts instead. Mauro do you remember? I can imagine some IR hardware
> > > (e.g. winbond) queuing up IR after generating a timeout, thus delaying
> > > delivering IR to the kernel and we ending up generating a key up.
> > > 
> > > The problem with a higher timeout is that the trailing space (=timeout)
> > > is sometimes needed for decoding, and decoding of the last message is
> > > delayed until the timeout is received. That means the keyup message is
> > > delayed until that time, making keys a bit "sticky" and more likely to
> > > generate repeats. I'm pretty sure that is needed for rc-5 and nec.
> > 
> > Another issue with high timeouts is the response to very short button
> > presses where the remote only transmits a single scancode. It then
> > takes signal transmission time plus timeout, so roughly a quarter
> > of a second on meson-ir and ite-cir with 200ms timeout, until the
> > scancode is decoded and the keydown event is generated.
> > 
> > On longer button presses this is less of an issue as we get the
> > space signal when the first pulse of the repeated scancode is
> > received. So the delay between button press and keydown is determined
> > by the remote scancode repeat interval and with typically ~110ms
> > on nec/rc-5 a lot lower.
> > 
> > This affects both "quick fingers" using a standard remote and
> > users of programmable remotes like the Logitech Harmony where
> > the number of scancodes transmitted on a short press can be
> > configured. With a single scancode transmission we run into
> > the long keydown delay, 2 scancodes is fine, and at 3 or 4 we
> > start running into the key repeat issue.
> > 
> > We received several reports from users that their remote felt
> > "sluggish" when we switched from the downstream "amremote" driver
> > (which IIRC decoded the nec protocol in hardware) to meson-ir.
> > 
> > Lowering the timeout to 125ms or even significantly lower
> > (depending on what the protocol and IR receiver permits)
> > removes this "sluggishness", users report that their remote
> > is more "responsive".
> 
> That makes complete sense. I'm actually keen to get this lowered, since
> this makes it possible to lower the repeat period per-protocol, see
> commit d57ea877af38 which had to be reverted (the ite driver will
> need fixing up as well before this can happen).

I remember the commit, this issue hit us in LibreELEC testbuilds
as well :-)

> Lowering to below 125ms does increase the risk of regressions, so I
> am weary of that. Do you think there is benefit in doing this?

I'd also say stick to the 125ms default. The default settings
should always be safe ones IMO.

People who want to optimize for the last bit of performance can
easily do that on their own, at their own risk.

Personally I've been using gpio-ir-recv on RPi with the default 125ms
timeout and a Hauppauge rc-5 remote for about 2 years now and I've
always been happy with that.

I have to acknowledge though that the responsiveness of a remote
with short messages, like rc-5, in combination with a low timeout
(I tested down to 10ms) is pretty impressive.

so long,

Hias

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Matthias Reichl]

Hi Sean,

On Sat, Mar 10, 2018 at 11:27:45AM +, Sean Young wrote:
> Hi Matthias,
> 
> On Fri, Mar 09, 2018 at 04:54:51PM +0100, Matthias Reichl wrote:
> > On Thu, Mar 08, 2018 at 04:43:27PM +, Sean Young wrote:
> > > On Tue, Mar 06, 2018 at 06:41:22PM +0100, Matthias Reichl wrote:
> > > > +
> > > >  static int meson_ir_probe(struct platform_device *pdev)
> > > >  {
> > > > struct device *dev = >dev;
> > > > @@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device 
> > > > *pdev)
> > > > ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
> > > > ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
> > > > ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
> > > > +   ir->rc->min_timeout = 1;
> > > > ir->rc->timeout = MS_TO_NS(200);
> > > > +   ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
> > > 
> > > Any idea why the default timeout is to 200ms? It seems very high.
> > 
> > Indeed it is very high, but I have no idea where that might be
> > coming from - so I didn't touch it.
> > 
> > I've been testing rc-5 and NEC remotes with 20-50ms timeouts
> > on meson-ir/upstream kernel and a couple of LibreELEC users are
> > using 30-50ms timeouts without issues on Amlogic devices as well
> > (on 3.14 vendor kernel with meson-ir timeout patch):
> > 
> > https://forum.libreelec.tv/thread/11643-le9-0-remote-configs-ir-keytable-amlogic-devices/?postID=83124#post83124
> > 
> > Out of curiosity: where does the 125ms IR_DEFAULT_TIMEOUT value
> > come from? For raw IR signals processed by the decoders this seems
> > rather high to me as well. On my RPi3 with gpio-ir-recv I'm
> > using 30ms timeout (with an rc-5 remote) without issues.
> 
> So if the timeout is below N then you will never get a space of N or larger;
> the largest space I know of in an IR message is 40ms in the sanyo protocol:
> 
> https://www.sbprojects.net/knowledge/ir/sharp.php
> 
> So if timeout is set to less than 40ms, we would have trouble decoding the
> sharp protocol.
> 
> The space between nec repeats is a little less than 100ms. I'm trying to
> remember what would could go wrong if the space between them would be
> timeouts instead. Mauro do you remember? I can imagine some IR hardware
> (e.g. winbond) queuing up IR after generating a timeout, thus delaying
> delivering IR to the kernel and we ending up generating a key up.
> 
> The problem with a higher timeout is that the trailing space (=timeout)
> is sometimes needed for decoding, and decoding of the last message is
> delayed until the timeout is received. That means the keyup message is
> delayed until that time, making keys a bit "sticky" and more likely to
> generate repeats. I'm pretty sure that is needed for rc-5 and nec.

Another issue with high timeouts is the response to very short button
presses where the remote only transmits a single scancode. It then
takes signal transmission time plus timeout, so roughly a quarter
of a second on meson-ir and ite-cir with 200ms timeout, until the
scancode is decoded and the keydown event is generated.

On longer button presses this is less of an issue as we get the
space signal when the first pulse of the repeated scancode is
received. So the delay between button press and keydown is determined
by the remote scancode repeat interval and with typically ~110ms
on nec/rc-5 a lot lower.

This affects both "quick fingers" using a standard remote and
users of programmable remotes like the Logitech Harmony where
the number of scancodes transmitted on a short press can be
configured. With a single scancode transmission we run into
the long keydown delay, 2 scancodes is fine, and at 3 or 4 we
start running into the key repeat issue.

We received several reports from users that their remote felt
"sluggish" when we switched from the downstream "amremote" driver
(which IIRC decoded the nec protocol in hardware) to meson-ir.

Lowering the timeout to 125ms or even significantly lower
(depending on what the protocol and IR receiver permits)
removes this "sluggishness", users report that their remote
is more "responsive".

so long,

Hias

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Matthias Reichl]

Hi Sean,

On Thu, Mar 08, 2018 at 04:43:27PM +, Sean Young wrote:
> On Tue, Mar 06, 2018 at 06:41:22PM +0100, Matthias Reichl wrote:
> > Meson doesn't seem to be able to generate timeout events
> > in hardware. So install a software timer to generate the
> > timeout events required by the decoders to prevent
> > "ghost keypresses".
> > 
> > Signed-off-by: Matthias Reichl <h...@horus.com>
> > ---
> >  drivers/media/rc/meson-ir.c | 22 ++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> > index f2204eb77e2a..f34c5836412b 100644
> > --- a/drivers/media/rc/meson-ir.c
> > +++ b/drivers/media/rc/meson-ir.c
> > @@ -69,6 +69,7 @@ struct meson_ir {
> > void __iomem*reg;
> > struct rc_dev   *rc;
> > spinlock_t  lock;
> > +   struct timer_list timeout_timer;
> >  };
> >  
> >  static void meson_ir_set_mask(struct meson_ir *ir, unsigned int reg,
> > @@ -98,6 +99,10 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
> > rawir.pulse = !!(status & STATUS_IR_DEC_IN);
> >  
> > ir_raw_event_store(ir->rc, );
> > +
> > +   mod_timer(>timeout_timer,
> > +   jiffies + nsecs_to_jiffies(ir->rc->timeout));
> > +
> > ir_raw_event_handle(ir->rc);
> >  
> > spin_unlock(>lock);
> > @@ -105,6 +110,17 @@ static irqreturn_t meson_ir_irq(int irqno, void 
> > *dev_id)
> > return IRQ_HANDLED;
> >  }
> >  
> > +static void meson_ir_timeout_timer(struct timer_list *t)
> > +{
> > +   struct meson_ir *ir = from_timer(ir, t, timeout_timer);
> > +   DEFINE_IR_RAW_EVENT(rawir);
> > +
> > +   rawir.timeout = true;
> > +   rawir.duration = ir->rc->timeout;
> > +   ir_raw_event_store(ir->rc, );
> > +   ir_raw_event_handle(ir->rc);
> > +}
> 
> Now there can be concurrent access to the raw IR kfifo from the interrupt
> handler and the timer. As there is a race condition between the timeout
> timer and new IR arriving from the interrupt handler, the timeout could
> end being generated after new IR and corrupting a message. There is very
> similar functionality in rc-ir-raw.c (with a spinlock).

Ah, thanks for the hint! Now I also noticed your commit a few
weeks ago - must have missed that before.

> > +
> >  static int meson_ir_probe(struct platform_device *pdev)
> >  {
> > struct device *dev = >dev;
> > @@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device *pdev)
> > ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
> > ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
> > ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
> > +   ir->rc->min_timeout = 1;
> > ir->rc->timeout = MS_TO_NS(200);
> > +   ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
> 
> Any idea why the default timeout is to 200ms? It seems very high.

Indeed it is very high, but I have no idea where that might be
coming from - so I didn't touch it.

I've been testing rc-5 and NEC remotes with 20-50ms timeouts
on meson-ir/upstream kernel and a couple of LibreELEC users are
using 30-50ms timeouts without issues on Amlogic devices as well
(on 3.14 vendor kernel with meson-ir timeout patch):

https://forum.libreelec.tv/thread/11643-le9-0-remote-configs-ir-keytable-amlogic-devices/?postID=83124#post83124

Out of curiosity: where does the 125ms IR_DEFAULT_TIMEOUT value
come from? For raw IR signals processed by the decoders this seems
rather high to me as well. On my RPi3 with gpio-ir-recv I'm
using 30ms timeout (with an rc-5 remote) without issues.

> > ir->rc->driver_name = DRIVER_NAME;
> >  
> > spin_lock_init(>lock);
> > @@ -157,6 +175,8 @@ static int meson_ir_probe(struct platform_device *pdev)
> > return ret;
> > }
> >  
> > +   timer_setup(>timeout_timer, meson_ir_timeout_timer, 0);
> > +
> > ret = devm_request_irq(dev, irq, meson_ir_irq, 0, NULL, ir);
> > if (ret) {
> > dev_err(dev, "failed to request irq\n");
> > @@ -198,6 +218,8 @@ static int meson_ir_remove(struct platform_device *pdev)
> > meson_ir_set_mask(ir, IR_DEC_REG1, REG1_ENABLE, 0);
> > spin_unlock_irqrestore(>lock, flags);
> >  
> > +   del_timer_sync(>timeout_timer);
> > +
> > return 0;
> >  }
> >  
> > -- 
> > 2.11.0
> 
> Would you mind trying this patch?

Tested-by: Matthias Reichl <h...@horus.com>

Thanks a lot, this patch works fin

[PATCH] media: rc: meson-ir: add timeout on idle

[Matthias Reichl]

Meson doesn't seem to be able to generate timeout events
in hardware. So install a software timer to generate the
timeout events required by the decoders to prevent
"ghost keypresses".

Signed-off-by: Matthias Reichl <h...@horus.com>
---
 drivers/media/rc/meson-ir.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
index f2204eb77e2a..f34c5836412b 100644
--- a/drivers/media/rc/meson-ir.c
+++ b/drivers/media/rc/meson-ir.c
@@ -69,6 +69,7 @@ struct meson_ir {
void __iomem*reg;
struct rc_dev   *rc;
spinlock_t  lock;
+   struct timer_list timeout_timer;
 };
 
 static void meson_ir_set_mask(struct meson_ir *ir, unsigned int reg,
@@ -98,6 +99,10 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
rawir.pulse = !!(status & STATUS_IR_DEC_IN);
 
ir_raw_event_store(ir->rc, );
+
+   mod_timer(>timeout_timer,
+   jiffies + nsecs_to_jiffies(ir->rc->timeout));
+
ir_raw_event_handle(ir->rc);
 
spin_unlock(>lock);
@@ -105,6 +110,17 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
return IRQ_HANDLED;
 }
 
+static void meson_ir_timeout_timer(struct timer_list *t)
+{
+   struct meson_ir *ir = from_timer(ir, t, timeout_timer);
+   DEFINE_IR_RAW_EVENT(rawir);
+
+   rawir.timeout = true;
+   rawir.duration = ir->rc->timeout;
+   ir_raw_event_store(ir->rc, );
+   ir_raw_event_handle(ir->rc);
+}
+
 static int meson_ir_probe(struct platform_device *pdev)
 {
struct device *dev = >dev;
@@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device *pdev)
ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
+   ir->rc->min_timeout = 1;
ir->rc->timeout = MS_TO_NS(200);
+   ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
ir->rc->driver_name = DRIVER_NAME;
 
spin_lock_init(>lock);
@@ -157,6 +175,8 @@ static int meson_ir_probe(struct platform_device *pdev)
return ret;
}
 
+   timer_setup(>timeout_timer, meson_ir_timeout_timer, 0);
+
ret = devm_request_irq(dev, irq, meson_ir_irq, 0, NULL, ir);
if (ret) {
dev_err(dev, "failed to request irq\n");
@@ -198,6 +218,8 @@ static int meson_ir_remove(struct platform_device *pdev)
meson_ir_set_mask(ir, IR_DEC_REG1, REG1_ENABLE, 0);
spin_unlock_irqrestore(>lock, flags);
 
+   del_timer_sync(>timeout_timer);
+
return 0;
 }
 
-- 
2.11.0

Re: [BUG] ir-ctl: error sending file with multiple scancodes

[Matthias Reichl]

Hi Sean,

On Wed, Nov 29, 2017 at 08:05:21PM +, Sean Young wrote:
> On Wed, Nov 29, 2017 at 03:44:00PM +0100, Matthias Reichl wrote:
> > The goal I'm trying to achieve is to send a repeated signal with ir-ctl
> > (a user reported his sony receiver needs this to actually power up).
> 
> That's interesting.

I'm not sure how common that is. I've got a Panasonic TV here
that needs a 0.5-1sec button press to power up from standby,
but it could well be that this is a rather nieche issue.

I had a look at what it would take to add proper repeat handling
to ir-ctl (similar to the --count  option in lirc's irsend)
but that looks like a larger endeavour: implement automatic
variable length gaps to get fixed repeat times, handle toggle
bits in some protocols, send special repeat codes eg in NEC etc.

As I'm not sure if all of this is even needed I think it's best
to leave it for maybe some time later. For now the current
functionality in ir-ctl looks sufficient.

> > Using the -S option multiple times comes rather close, but the 125ms
> > delay between signals is a bit long for the sony protocol - would be
> > nice if that would be adjustable :)
> 
> Yes, that would be a useful feature.
> 
> I've got some patches for this, I'll send them as a reply to this. Please
> let me know what you think.

[PATCH 1/2] ir-ctl: fix multiple scancodes in one file 
01-multiple-scancodes.patch
[PATCH 2/2] ir-ctl: specify the gap between scancodes or files

Tested-by: Matthias Reichl <h...@horus.com>

Thanks, the patches look and tested fine!

I've tested multiple scancodes in a file with and without explicit
space in between and the gap option with multiple -S scancodes on
the command line. I also tested rc5 protocol in addition to sony12.

So I'd like to say a big thank you for fixing the issue so quickly!

so long,

Hias

[BUG] ir-ctl: error sending file with multiple scancodes

[Matthias Reichl]

Hi Sean!

According to the ir-ctl manpage it should be possible to send a file
containing multiple scancodes, but when trying to do this I get
a warning and an error message.

I initially noticed that on version 1.12.3 but 1.12.5 and master
(rev 85f8e5a99) give the same error.

Sending a file with a single scancode or using the -S option
to specify the scancode on the command line both work fine.

I've tested with the following file:

scancode sony12:0x100015
space 25000
scancode sony12:0x100015

Trying to send it gives this:
$ ./utils/ir-ctl/ir-ctl -s ../sony-test.irctl
warning: ../sony-test.irctl:2: trailing space ignored
/dev/lirc0: failed to send: Invalid argument

Checking with the -v option gives some interesting output - it
looks like the the second half of the buffer hadn't been filled in:

$ ./utils/ir-ctl/ir-ctl -v -s ../sony-test.irctl
warning: ../sony-test.irctl:2: trailing space ignored
Sending:
pulse 2400
space 600
pulse 1200
space 600
pulse 600
space 600
pulse 1200
space 600
pulse 600
space 600
pulse 1200
space 600
pulse 600
space 600
pulse 600
space 600
pulse 600
space 600
pulse 600
space 600
pulse 600
space 600
pulse 600
space 600
pulse 1200
space 600
pulse 0
space 0
pulse 0
space 0
pulse 0
space 0
pulse 0
space 0
pulse 0
space 0
pulse 0
space 0
pulse 0
space 0
pulse 0
space 0
pulse 0
space 0
pulse 0
space 0
pulse 0
space 0
pulse 0
/dev/lirc0: failed to send: Invalid argument

The goal I'm trying to achieve is to send a repeated signal with ir-ctl
(a user reported his sony receiver needs this to actually power up).

Using the -S option multiple times comes rather close, but the 125ms
delay between signals is a bit long for the sony protocol - would be
nice if that would be adjustable :)

so long,

Hias

Re: [PATCH] media: rc: double keypresses due to timeout expiring to early

[Matthias Reichl]

Hi Sean!

On Sun, Nov 19, 2017 at 09:57:27PM +, Sean Young wrote:
> I think for now the best solution is to revert to 250ms for all protocols
> (except for cec which needs 550ms), and reconsider for another kernel.

Thanks, this sounds like a good idea!

> >>From 2f1135f3f9873778ca5c013d1118710152840cb2 Mon Sep 17 00:00:00 2001
> From: Sean Young <s...@mess.org>
> Date: Sun, 19 Nov 2017 21:11:17 +
> Subject: [PATCH] media: rc: partial revert of "media: rc: per-protocol repeat
>  period"
> 
> Since commit d57ea877af38 ("media: rc: per-protocol repeat period"), most
> IR protocols have a lower keyup timeout. This causes problems on the
> ite-cir, which has default IR timeout of 200ms.
> 
> Since the IR decoders read the trailing space, with a IR timeout of 200ms,
> the last keydown will have at least a delay of 200ms. This is more than
> the protocol timeout of e.g. rc-6 (which is 164ms). As a result the last
> IR will be interpreted as a new keydown event, and we get two keypresses.
> 
> Revert the protocol timeout to 250ms, except for cec which needs a timeout
> of 550ms.
> 
> Fixes: d57ea877af38 ("media: rc: per-protocol repeat period")
> Cc: <sta...@vger.kernel.org> # 4.14
> Signed-off-by: Sean Young <s...@mess.org>

Tested-by: Matthias Reichl <h...@horus.com>

I tested this locally with gpio-ir configured to 200ms timeout and
we also received feedback from 2 users that this change fixed the
issue with the ite-cir receiver.

https://forum.kodi.tv/showthread.php?tid=298462=2670637#pid2670637

Thanks a lot for fixing this so quickly!

so long,

Hias
> ---
>  drivers/media/rc/rc-main.c | 32 
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 17950e29d4e3..5057b2ba0c10 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -39,41 +39,41 @@ static const struct {
>   [RC_PROTO_UNKNOWN] = { .name = "unknown", .repeat_period = 250 },
>   [RC_PROTO_OTHER] = { .name = "other", .repeat_period = 250 },
>   [RC_PROTO_RC5] = { .name = "rc-5",
> - .scancode_bits = 0x1f7f, .repeat_period = 164 },
> + .scancode_bits = 0x1f7f, .repeat_period = 250 },
>   [RC_PROTO_RC5X_20] = { .name = "rc-5x-20",
> - .scancode_bits = 0x1f7f3f, .repeat_period = 164 },
> + .scancode_bits = 0x1f7f3f, .repeat_period = 250 },
>   [RC_PROTO_RC5_SZ] = { .name = "rc-5-sz",
> - .scancode_bits = 0x2fff, .repeat_period = 164 },
> + .scancode_bits = 0x2fff, .repeat_period = 250 },
>   [RC_PROTO_JVC] = { .name = "jvc",
>   .scancode_bits = 0x, .repeat_period = 250 },
>   [RC_PROTO_SONY12] = { .name = "sony-12",
> - .scancode_bits = 0x1f007f, .repeat_period = 100 },
> + .scancode_bits = 0x1f007f, .repeat_period = 250 },
>   [RC_PROTO_SONY15] = { .name = "sony-15",
> - .scancode_bits = 0xff007f, .repeat_period = 100 },
> + .scancode_bits = 0xff007f, .repeat_period = 250 },
>   [RC_PROTO_SONY20] = { .name = "sony-20",
> - .scancode_bits = 0x1fff7f, .repeat_period = 100 },
> + .scancode_bits = 0x1fff7f, .repeat_period = 250 },
>   [RC_PROTO_NEC] = { .name = "nec",
> - .scancode_bits = 0x, .repeat_period = 160 },
> + .scancode_bits = 0x, .repeat_period = 250 },
>   [RC_PROTO_NECX] = { .name = "nec-x",
> - .scancode_bits = 0xff, .repeat_period = 160 },
> + .scancode_bits = 0xff, .repeat_period = 250 },
>   [RC_PROTO_NEC32] = { .name = "nec-32",
> - .scancode_bits = 0x, .repeat_period = 160 },
> + .scancode_bits = 0x, .repeat_period = 250 },
>   [RC_PROTO_SANYO] = { .name = "sanyo",
>   .scancode_bits = 0x1f, .repeat_period = 250 },
>   [RC_PROTO_MCIR2_KBD] = { .name = "mcir2-kbd",
> - .scancode_bits = 0x, .repeat_period = 150 },
> + .scancode_bits = 0x, .repeat_period = 250 },
>   [RC_PROTO_MCIR2_MSE] = { .name = "mcir2-mse",
> - .scancode_bits = 0x1f, .repeat_period = 150 },
> + .scancode_bits = 0x1f, .repeat_period = 250 },
>   [RC_PROTO_RC6_0] = { .name = "rc-6-0",
> - .scancode_bits = 0x, .repeat_period = 164 },
> + .scancode_bits = 0x, .repeat_period = 250 },
>   [RC_PROTO_RC6_6A_20] = { .name = "rc-6

Re: [PATCH] media: rc: double keypresses due to timeout expiring to early

[Matthias Reichl]

On Fri, Nov 17, 2017 at 03:52:50PM +0100, Matthias Reichl wrote:
> Hi Sean!
> 
> On Thu, Nov 16, 2017 at 09:54:51PM +, Sean Young wrote:
> > Since commit d57ea877af38 ("media: rc: per-protocol repeat period"),
> > double keypresses are reported on the ite-cir driver. This is due
> > two factors: that commit reduced the timeout used for some protocols
> > (it became protocol dependant) and the high default IR timeout used
> > by the ite-cir driver.
> > 
> > Some of the IR decoders wait for a trailing space, as that is
> > the only way to know if the bit stream has ended (e.g. rc-6 can be
> > 16, 20 or 32 bits). The longer the IR timeout, the longer it will take
> > to receive the trailing space, delaying decoding and pushing it past the
> > keyup timeout.
> > 
> > So, add the IR timeout to the keyup timeout.
> 
> Thanks a lot for the patch, I've asked the people with ite-cir
> receivers to test it.

Just got the first test results from ite-cir + rc-6 remote back,
events were the same as I was seeing with gpio-ir-recv with 200ms
timeout - key repeat event from input layer.

Kernel 4.14 + your patch:
Event: time 1510938801.797335, type 4 (EV_MSC), code 4 (MSC_SCAN), value 
800f041f
Event: time 1510938801.797335, type 1 (EV_KEY), code 108 (KEY_DOWN), value 1
Event: time 1510938801.797335, -- SYN_REPORT 
Event: time 1510938802.034331, type 4 (EV_MSC), code 4 (MSC_SCAN), value 
800f041f
Event: time 1510938802.034331, -- SYN_REPORT 
Event: time 1510938802.301333, type 1 (EV_KEY), code 108 (KEY_DOWN), value 2
Event: time 1510938802.301333, -- SYN_REPORT 
Event: time 1510938802.408003, type 1 (EV_KEY), code 108 (KEY_DOWN), value 0
Event: time 1510938802.408003, -- SYN_REPORT 

Kernel 4.13.2:
Event: time 1510938637.791450, type 4 (EV_MSC), code 4 (MSC_SCAN), value 
800f041f
Event: time 1510938637.791450, type 1 (EV_KEY), code 108 (KEY_DOWN), value 1
Event: time 1510938637.791450, -- SYN_REPORT 
Event: time 1510938638.028301, type 4 (EV_MSC), code 4 (MSC_SCAN), value 
800f041f
Event: time 1510938638.028301, -- SYN_REPORT 
Event: time 1510938638.292323, type 1 (EV_KEY), code 108 (KEY_DOWN), value 0
Event: time 1510938638.292323, -- SYN_REPORT 

so long,

Hias

> 
> In the meanwhile I ran some tests with gpio-ir-recv and timeout
> set to 200ms with a rc-5 remote (that's as close to the original
> setup as I can test right now).
> 
> While the patch fixes the additional key down/up event on longer
> presses, I still get a repeated key event on a short button
> press - which makes it hard to do a single click with the
> remote.
> 
> Test on kernel 4.14 with your patch:
> 1510927844.292126: event type EV_MSC(0x04): scancode = 0x1015
> 1510927844.292126: event type EV_KEY(0x01) key_down: KEY_ENTER(0x001c)
> 1510927844.292126: event type EV_SYN(0x00).
> 1510927844.498773: event type EV_MSC(0x04): scancode = 0x1015
> 1510927844.498773: event type EV_SYN(0x00).
> 1510927844.795410: event type EV_KEY(0x01) key_down: KEY_ENTER(0x001c)
> 1510927844.795410: event type EV_SYN(0x00).
> 1510927844.875412: event type EV_KEY(0x01) key_up: KEY_ENTER(0x001c)
> 1510927844.875412: event type EV_SYN(0x00).
> 
> Same signal received on kernel 4.9:
> 1510927844.280350: event type EV_MSC(0x04): scancode = 0x1015
> 1510927844.280350: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
> 1510927844.280350: event type EV_SYN(0x00).
> 1510927844.506477: event type EV_MSC(0x04): scancode = 0x1015
> 1510927844.506477: event type EV_SYN(0x00).
> 1510927844.763111: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
> 1510927844.763111: event type EV_SYN(0x00).
> 
> If I understand it correctly it's the input layer repeat (500ms delay)
> kicking in, because time between initial scancode and timeout is
> now signal time + 200ms + 164ms + 200ms (about 570-580ms).
> On older kernels this was signal time + 200ms + 250ms, so typically
> just below the 500ms default repeat delay.
> 
> I'm still trying to wrap my head around the timeout code in the
> rc layer. One problem seems to be that we apply the rather large
> timeout twice. Maybe detecting scancodes generated via timeout
> (sth like timestamp - last_timestamp > protocol_repeat_period)
> and in that case immediately signalling keyup could help? Could well
> be that I'm missing somehting important and this is a bad idea.
> I guess I'd better let you figure something out :)
> 
> so long,
> 
> Hias
> 
> > 
> > Cc: <sta...@vger.kernel.org> # 4.14
> > Reported-by: Matthias Reichl <h...@horus.com>
> > Signed-off-by: Sean Young <s...@mess.org>
>

Re: [PATCH] media: rc: double keypresses due to timeout expiring to early

[Matthias Reichl]

Hi Sean!

On Thu, Nov 16, 2017 at 09:54:51PM +, Sean Young wrote:
> Since commit d57ea877af38 ("media: rc: per-protocol repeat period"),
> double keypresses are reported on the ite-cir driver. This is due
> two factors: that commit reduced the timeout used for some protocols
> (it became protocol dependant) and the high default IR timeout used
> by the ite-cir driver.
> 
> Some of the IR decoders wait for a trailing space, as that is
> the only way to know if the bit stream has ended (e.g. rc-6 can be
> 16, 20 or 32 bits). The longer the IR timeout, the longer it will take
> to receive the trailing space, delaying decoding and pushing it past the
> keyup timeout.
> 
> So, add the IR timeout to the keyup timeout.

Thanks a lot for the patch, I've asked the people with ite-cir
receivers to test it.

In the meanwhile I ran some tests with gpio-ir-recv and timeout
set to 200ms with a rc-5 remote (that's as close to the original
setup as I can test right now).

While the patch fixes the additional key down/up event on longer
presses, I still get a repeated key event on a short button
press - which makes it hard to do a single click with the
remote.

Test on kernel 4.14 with your patch:
1510927844.292126: event type EV_MSC(0x04): scancode = 0x1015
1510927844.292126: event type EV_KEY(0x01) key_down: KEY_ENTER(0x001c)
1510927844.292126: event type EV_SYN(0x00).
1510927844.498773: event type EV_MSC(0x04): scancode = 0x1015
1510927844.498773: event type EV_SYN(0x00).
1510927844.795410: event type EV_KEY(0x01) key_down: KEY_ENTER(0x001c)
1510927844.795410: event type EV_SYN(0x00).
1510927844.875412: event type EV_KEY(0x01) key_up: KEY_ENTER(0x001c)
1510927844.875412: event type EV_SYN(0x00).

Same signal received on kernel 4.9:
1510927844.280350: event type EV_MSC(0x04): scancode = 0x1015
1510927844.280350: event type EV_KEY(0x01) key_down: KEY_OK(0x0160)
1510927844.280350: event type EV_SYN(0x00).
1510927844.506477: event type EV_MSC(0x04): scancode = 0x1015
1510927844.506477: event type EV_SYN(0x00).
1510927844.763111: event type EV_KEY(0x01) key_up: KEY_OK(0x0160)
1510927844.763111: event type EV_SYN(0x00).

If I understand it correctly it's the input layer repeat (500ms delay)
kicking in, because time between initial scancode and timeout is
now signal time + 200ms + 164ms + 200ms (about 570-580ms).
On older kernels this was signal time + 200ms + 250ms, so typically
just below the 500ms default repeat delay.

I'm still trying to wrap my head around the timeout code in the
rc layer. One problem seems to be that we apply the rather large
timeout twice. Maybe detecting scancodes generated via timeout
(sth like timestamp - last_timestamp > protocol_repeat_period)
and in that case immediately signalling keyup could help? Could well
be that I'm missing somehting important and this is a bad idea.
I guess I'd better let you figure something out :)

so long,

Hias

> 
> Cc: <sta...@vger.kernel.org> # 4.14
> Reported-by: Matthias Reichl <h...@horus.com>
> Signed-off-by: Sean Young <s...@mess.org>
> ---
>  drivers/media/rc/rc-main.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 17950e29d4e3..fae721534517 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -672,7 +672,8 @@ void rc_repeat(struct rc_dev *dev)
>   input_event(dev->input_dev, EV_MSC, MSC_SCAN, dev->last_scancode);
>   input_sync(dev->input_dev);
>  
> - dev->keyup_jiffies = jiffies + msecs_to_jiffies(timeout);
> + dev->keyup_jiffies = jiffies + msecs_to_jiffies(timeout) +
> + nsecs_to_jiffies(dev->timeout);
>   mod_timer(>timer_keyup, dev->keyup_jiffies);
>  
>  out:
> @@ -744,7 +745,8 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto 
> protocol, u32 scancode,
>  
>   if (dev->keypressed) {
>   dev->keyup_jiffies = jiffies +
> - msecs_to_jiffies(protocols[protocol].repeat_period);
> + msecs_to_jiffies(protocols[protocol].repeat_period) +
> + nsecs_to_jiffies(dev->timeout);
>   mod_timer(>timer_keyup, dev->keyup_jiffies);
>   }
>   spin_unlock_irqrestore(>keylock, flags);
> -- 
> 2.14.3
> 

4.14 regression from commit d57ea877af38 media: rc: per-protocol repeat period

[Matthias Reichl]

The following commit introduced a regression

commit d57ea877af38057b0ef31758cf3b99765dc33695
Author: Sean Young 
Date:   Wed Aug 9 13:19:16 2017 -0400

media: rc: per-protocol repeat period

CEC needs a keypress timeout of 550ms, which is too high for the IR
protocols. Also fill in known repeat times, with 50ms error margin.

Also, combine all protocol data into one structure.

We received a report that an RC6 MCE remote used with the ite-cir
produces "double events" on short button presses:

https://forum.kodi.tv/showthread.php?tid=298462=2667855#pid2667855

Looking at the ir-keytable -t output an additional key event is also
generated after longer button presses:

# ir-keytable -t
Testing events. Please, press CTRL-C to abort.
1510680591.697657: event type EV_MSC(0x04): scancode = 0x800f041f
1510680591.697657: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
1510680591.697657: event type EV_SYN(0x00).
1510680591.867355: event type EV_KEY(0x01) key_up: KEY_DOWN(0x006c)
1510680591.867355: event type EV_SYN(0x00).
1510680591.935026: event type EV_MSC(0x04): scancode = 0x800f041f
1510680591.935026: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c)
1510680591.935026: event type EV_SYN(0x00).
1510680592.104100: event type EV_KEY(0x01) key_up: KEY_DOWN(0x006c)
1510680592.104100: event type EV_SYN(0x00).

1510680597.714055: event type EV_MSC(0x04): scancode = 0x800f0405
1510680597.714055: event type EV_KEY(0x01) key_down: KEY_NUMERIC_5(0x0205)
1510680597.714055: event type EV_SYN(0x00).
1510680597.819939: event type EV_MSC(0x04): scancode = 0x800f0405
1510680597.819939: event type EV_SYN(0x00).
1510680597.925614: event type EV_MSC(0x04): scancode = 0x800f0405
1510680597.925614: event type EV_SYN(0x00).
1510680598.032422: event type EV_MSC(0x04): scancode = 0x800f0405
1510680598.032422: event type EV_SYN(0x00).
...
1510680598.562114: event type EV_MSC(0x04): scancode = 0x800f0405
1510680598.562114: event type EV_SYN(0x00).
1510680598.630641: event type EV_KEY(0x01) key_down: KEY_NUMERIC_5(0x0205)
1510680598.630641: event type EV_SYN(0x00).
1510680598.667906: event type EV_MSC(0x04): scancode = 0x800f0405
1510680598.667906: event type EV_SYN(0x00).
1510680598.760760: event type EV_KEY(0x01) key_down: KEY_NUMERIC_5(0x0205)
1510680598.760760: event type EV_SYN(0x00).
1510680598.837412: event type EV_KEY(0x01) key_up: KEY_NUMERIC_5(0x0205)
1510680598.837412: event type EV_SYN(0x00).
1510680598.905003: event type EV_MSC(0x04): scancode = 0x800f0405
1510680598.905003: event type EV_KEY(0x01) key_down: KEY_NUMERIC_5(0x0205)
1510680598.905003: event type EV_SYN(0x00).
1510680599.074092: event type EV_KEY(0x01) key_up: KEY_NUMERIC_5(0x0205)
1510680599.074092: event type EV_SYN(0x00).

Looking at the timestamps of the scancode events it seems that
signals are received every ~106ms but the last signal seems to be
received 237ms after the last-but-one - which is then interpreted
as a new key press cycle as the delay is longer than the 164ms
"repeat_period" setting of the RC6 protocol (before that commit
250ms was used).

This 237ms delay seems to be coming from the 200ms timeout value
of the ite-cir driver (237ms is in the ballpark of ~30ms rc6 signal
time plus 200ms timeout).

The original author hasn't reported back yet but others confirmed
that changing the timeout to 100ms (minimum idle timeout value
of ite-cir) using "ir-ctl -t 10" fixes the issue.

I could locally reproduce this with gpio-ir-recv (which uses the
default 125ms timeout) and the sony protocol (repeat_period = 100ms):

1510838115.272021: event type EV_MSC(0x04): scancode = 0x110001
1510838115.272021: event type EV_KEY(0x01) key_down: KEY_2(0x0003)
1510838115.272021: event type EV_SYN(0x00).
1510838115.322014: event type EV_MSC(0x04): scancode = 0x110001
1510838115.322014: event type EV_SYN(0x00).
1510838115.362003: event type EV_MSC(0x04): scancode = 0x110001
1510838115.362003: event type EV_SYN(0x00).
1510838115.412002: event type EV_MSC(0x04): scancode = 0x110001
1510838115.412002: event type EV_SYN(0x00).
1510838115.521973: event type EV_KEY(0x01) key_up: KEY_2(0x0003)
1510838115.521973: event type EV_SYN(0x00).
1510838115.532007: event type EV_MSC(0x04): scancode = 0x110001
1510838115.532007: event type EV_KEY(0x01) key_down: KEY_2(0x0003)
1510838115.532007: event type EV_SYN(0x00).
1510838115.641972: event type EV_KEY(0x01) key_up: KEY_2(0x0003)
1510838115.641972: event type EV_SYN(0x00).

Reducing the timeout to 20ms removes the addional key_down/up event.

Another test with a rc-5 remote on gpio-ir-recv worked fine at the
default 125ms timeout but with 200ms as on the ite-cir I again
got the additional key event.

so long,

Hias

Re: [PATCH] keytable: ensure udev rule fires on rc input device

[Matthias Reichl]

Hi Sean!

On Wed, Aug 16, 2017 at 05:56:25PM +0100, Sean Young wrote:
> I've found a shorter way of doing this.

That's a really clever trick of dealing with the RUN/$id issue,
I like it a lot!

> 
> From: Sean Young <s...@mess.org>
> Date: Wed, 16 Aug 2017 17:41:53 +0100
> Subject: [PATCH] keytable: ensure the udev rule fires on creation of the input
>  device
> 
> The rc device is created before the input device, so if ir-keytable runs
> too early the input device does not exist yet.
> 
> Ensure that rule fires on creation of a rc device's input device.
> 
> Note that this also prevents udev from starting ir-keytable on an
> transmit only device, which has no input device.
> 
> Note that $id in RUN will not work, since that is expanded after all the
> rules are matched, at which point the the parent might have been changed
> by another match in another rule. The argument to $env{key} is expanded
> immediately.
> 
> Signed-off-by: Sean Young <s...@mess.org>

Tested-by: Matthias Reichl <h...@horus.com>

so long & thanks for the fix,

Hias
> ---
>  utils/keytable/70-infrared.rules | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utils/keytable/70-infrared.rules 
> b/utils/keytable/70-infrared.rules
> index afffd951..41ca2089 100644
> --- a/utils/keytable/70-infrared.rules
> +++ b/utils/keytable/70-infrared.rules
> @@ -1,4 +1,4 @@
>  # Automatically load the proper keymaps after the Remote Controller device
>  # creation.  The keycode tables rules should be at /etc/rc_maps.cfg
>  
> -ACTION=="add", SUBSYSTEM=="rc", RUN+="/usr/bin/ir-keytable -a 
> /etc/rc_maps.cfg -s $name"
> +ACTION=="add", SUBSYSTEM=="input", SUBSYSTEMS=="rc", KERNEL=="event*", 
> ENV{.rc_sysdev}="$id", RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s 
> $env{.rc_sysdev}"
> -- 
> 2.13.5
> 

Re: [PATCH] keytable: ensure udev rule fires on rc input device

[Matthias Reichl]

Hi Sean!

On Sun, Aug 06, 2017 at 09:56:55AM +0100, Sean Young wrote:
> The rc device is created before the input device, so if ir-keytable runs
> too early the input device does not exist yet.
> 
> Ensure that rule fires on creation of a rc device's input device.
> 
> Note that this also prevents udev from starting ir-keytable on an
> transmit only device, which has no input device.
> 
> Signed-off-by: Sean Young <s...@mess.org>

Signed-off-by: Matthias Reichl <h...@horus.com>

One comment though, see below

> ---
>  utils/keytable/70-infrared.rules | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> Matthias, can I have your Signed-off-by please? Thank you.
> 
> 
> diff --git a/utils/keytable/70-infrared.rules 
> b/utils/keytable/70-infrared.rules
> index afffd951..b3531727 100644
> --- a/utils/keytable/70-infrared.rules
> +++ b/utils/keytable/70-infrared.rules
> @@ -1,4 +1,12 @@
>  # Automatically load the proper keymaps after the Remote Controller device
>  # creation.  The keycode tables rules should be at /etc/rc_maps.cfg
>  
> -ACTION=="add", SUBSYSTEM=="rc", RUN+="/usr/bin/ir-keytable -a 
> /etc/rc_maps.cfg -s $name"
> +ACTION!="add", SUBSYSTEMS!="rc", GOTO="rc_dev_end"

This line doesn't quite what we want it to do.

As SUBSYSTEMS!="rc" is basically a no-op and would only be
evaluated on change/remove events anyways that line boils down to

ACTION!="add", GOTO="rc_dev_end"

and the following rules are evaluated on all add events.

While that'll still work it'll do unnecessary work, like importing
rc_sydev for all input devices and could bite us (or users) later
if we change/extend the ruleset.

Better do it like in my original comment (using positive logic and
a GOTO="begin") or use ACTION!="add", GOTO="rc_dev_end" and add
SUBSYSTEMS=="rc" to the IMPORT and RUN rules below.

so long,

Hias

> +
> +SUBSYSTEM=="rc", ENV{rc_sysdev}="$name"
> +
> +SUBSYSTEM=="input", IMPORT{parent}="rc_sysdev"
> +
> +KERNEL=="event[0-9]*", ENV{rc_sysdev}=="?*", RUN+="/usr/bin/ir-keytable -a 
> /etc/rc_maps.cfg -s $env{rc_sysdev}"
> +
> +LABEL="rc_dev_end"
> -- 
> 2.11.0
> 

[PATCH] [media] rc: gpio-ir-tx: switch to gpiod, fix inverted polarity

[Matthias Reichl]

Manual handling of gpio output polarity was inverted.
Switch to using gpiod, this allows us to simplify the code,
delegate polarity handling to gpiod and remove the buggy local
polarity handling code.

Signed-off-by: Matthias Reichl <h...@horus.com>
---

This patch is against [PATCH v2 3/6] [media] rc: gpio-ir-tx:
add new driver.

Feel free to squash these two patches into one for v3.

so long,

Hias

 drivers/media/rc/gpio-ir-tx.c | 41 +
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/drivers/media/rc/gpio-ir-tx.c b/drivers/media/rc/gpio-ir-tx.c
index 7a5371dbb360..ca6834d09467 100644
--- a/drivers/media/rc/gpio-ir-tx.c
+++ b/drivers/media/rc/gpio-ir-tx.c
@@ -13,11 +13,10 @@
 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -25,8 +24,7 @@
 #define DEVICE_NAME"GPIO Bit Banging IR Transmitter"
 
 struct gpio_ir {
-   int gpio_nr;
-   bool active_low;
+   struct gpio_desc *gpio;
unsigned int carrier;
unsigned int duty_cycle;
/* we need a spinlock to hold the cpu while transmitting */
@@ -101,14 +99,12 @@ static int gpio_ir_tx(struct rc_dev *dev, unsigned int 
*txbuf,
ktime_t last = ktime_add_us(edge, txbuf[i]);
 
while (ktime_get() < last) {
-   gpio_set_value(gpio_ir->gpio_nr,
-  gpio_ir->active_low);
+   gpiod_set_value(gpio_ir->gpio, 1);
edge += pulse;
delta = edge - ktime_get();
if (delta > 0)
ndelay(delta);
-   gpio_set_value(gpio_ir->gpio_nr,
-  !gpio_ir->active_low);
+   gpiod_set_value(gpio_ir->gpio, 0);
edge += space;
delta = edge - ktime_get();
if (delta > 0)
@@ -128,16 +124,7 @@ static int gpio_ir_tx_probe(struct platform_device *pdev)
 {
struct gpio_ir *gpio_ir;
struct rc_dev *rcdev;
-   enum of_gpio_flags flags;
-   int rc, gpio;
-
-   gpio = of_get_gpio_flags(pdev->dev.of_node, 0, );
-   if (gpio < 0) {
-   if (gpio != -EPROBE_DEFER)
-   dev_err(>dev, "Failed to get gpio flags (%d)\n",
-   gpio);
-   return -EINVAL;
-   }
+   int rc;
 
gpio_ir = devm_kmalloc(>dev, sizeof(*gpio_ir), GFP_KERNEL);
if (!gpio_ir)
@@ -147,6 +134,14 @@ static int gpio_ir_tx_probe(struct platform_device *pdev)
if (!rcdev)
return -ENOMEM;
 
+   gpio_ir->gpio = devm_gpiod_get(>dev, NULL, GPIOD_OUT_LOW);
+   if (IS_ERR(gpio_ir->gpio)) {
+   if (PTR_ERR(gpio_ir->gpio) != -EPROBE_DEFER)
+   dev_err(>dev, "Failed to get gpio (%ld)\n",
+   PTR_ERR(gpio_ir->gpio));
+   return PTR_ERR(gpio_ir->gpio);
+   }
+
rcdev->priv = gpio_ir;
rcdev->driver_name = DRIVER_NAME;
rcdev->device_name = DEVICE_NAME;
@@ -154,20 +149,10 @@ static int gpio_ir_tx_probe(struct platform_device *pdev)
rcdev->s_tx_duty_cycle = gpio_ir_tx_set_duty_cycle;
rcdev->s_tx_carrier = gpio_ir_tx_set_carrier;
 
-   gpio_ir->gpio_nr = gpio;
-   gpio_ir->active_low = (flags & OF_GPIO_ACTIVE_LOW) != 0;
gpio_ir->carrier = 38000;
gpio_ir->duty_cycle = 50;
spin_lock_init(_ir->lock);
 
-   rc = devm_gpio_request(>dev, gpio, "gpio-ir-tx");
-   if (rc < 0)
-   return rc;
-
-   rc = gpio_direction_output(gpio, !gpio_ir->active_low);
-   if (rc < 0)
-   return rc;
-
rc = devm_rc_register_device(>dev, rcdev);
if (rc < 0)
dev_err(>dev, "failed to register rc device\n");
-- 
2.11.0

Re: ir-keytable question [Ubuntu 17.04]

[Matthias Reichl]

On Sat, Jul 29, 2017 at 11:23:22AM +0100, Sean Young wrote:
> Hi,
> 
> On Sun, Jul 16, 2017 at 10:26:14PM -0700, Szabolcs Andrasi wrote:
> > Hi,
> > 
> > I'm using Ubuntu 17.04 and I installed the ir-keytable tool. The
> > output of the ir-keytable command is as follows:
> > 
> > 
> > 
> > Found /sys/class/rc/rc0/ (/dev/input/event5) with:
> > Driver ite-cir, table rc-rc6-mce
> > Supported protocols: unknown other lirc rc-5 rc-5-sz jvc sony nec
> > sanyo mce_kbd rc-6 sharp xmp
> > Enabled protocols: lirc rc-6
> > Name: ITE8708 CIR transceiver
> > bus: 25, vendor/product: 1283:, version: 0x
> > Repeat delay = 500 ms, repeat period = 125 ms
> > 
> > 
> > 
> > I'm trying to enable the supported mce_kbd protocol in addition to the
> > lirc and rc-6 protocols with the
> > 
> > $ sudo ir-keytable -p lirc -p rc-6 -p mce_kbd
> > 
> > command which works as expected. If, however, I reboot my computer,
> > ir-keytable forgets this change and only the lirc and rc-6 protocols
> > are enabled. Is there a configuration file I can edit so that after
> > the boot my IR remote still works? Or is that so that the only way to
> > make it work is to write a start-up script that runs the above command
> > to enable the needed protocol?
> 
> So what we have today is /etc/rc_maps.cfg, where you can select the default
> keymap for a particular driver; unfortunately, you can only select one
> keymap and one keymap can only have one protocol.
>
> Ideally we could either have more than one protocol per keymap, which
> would be helpful for the MCE Keyboard, or we could allow multiple keymaps
> which would be great for supporting different remotes at the same time.

Having more than one protocol in the keymap file works fine here,
we have been using that feature in LibreELEC for a long time now.
Maybe it was just forgotten to document it?

$ git show 42511eb505
commit 42511eb505b46b125652d37e764e5c8d1eb99e6b
Author: Mauro Carvalho Chehab 
Date:   Sat Apr 10 21:55:28 2010 -0300

ir-keytable: add support for more than one protocol in a table

Signed-off-by: Mauro Carvalho Chehab 

Quick test with ir-keytable 1.12.3 from Debian Stretch:

$ sudo ir-keytable -c -p lirc,rc-6 -s rc1
Old keytable cleared
Protocols changed to lirc rc-6

$ sudo ir-keytable -r -s rc1
Enabled protocols: lirc rc-6

$ cat /etc/rc_keymaps/rc6_mce_kbd_test
# table test, type:rc-6,mce_kbd
0x01KEY_1

$ cat test-map.cfg
* * rc6_mce_kbd_test

$ sudo ir-keytable -a test-map.cfg -s rc1
Old keytable cleared
Wrote 1 keycode(s) to driver
Protocols changed to mce_kbd rc-6

$ sudo ir-keytable -r -s rc1
scancode 0x0001 = KEY_1 (0x02)
Enabled protocols: lirc mce_kbd rc-6

so long,

Hias

> 
> For now, you could add a udev rule to also enable the mce_kbd protocol.
> 
> 
> Sean

Re: [PATCH v2 4/6] [media] rc: pwm-ir-tx: add new driver

[Matthias Reichl]

Hi Sean,

sorry for double-post, forgot to CC the list and others.

On Fri, Jul 07, 2017 at 10:52:02AM +0100, Sean Young wrote:
> This is new driver which uses pwm, so it is more power-efficient
> than the bit banging gpio-ir-tx driver.
> 
> Signed-off-by: Sean Young <s...@mess.org>

Tested-by: Matthias Reichl <h...@horus.com>

Tested on RPi2, with downstream RPi kernel 4.13-rc1, using
ir-ctl to send RC-5 codes, against another RPi with gpio-ir-recv.
Also verified signal polarity and frequency on scope.

so long & thanks a lot,

Hias

> ---
>  MAINTAINERS  |   6 ++
>  drivers/media/rc/Kconfig |  12 
>  drivers/media/rc/Makefile|   1 +
>  drivers/media/rc/pwm-ir-tx.c | 138 
> +++
>  4 files changed, 157 insertions(+)
>  create mode 100644 drivers/media/rc/pwm-ir-tx.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8a37c2f..446a528 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10449,6 +10449,12 @@ F:   
> Documentation/devicetree/bindings/hwmon/pwm-fan.txt
>  F:   Documentation/hwmon/pwm-fan
>  F:   drivers/hwmon/pwm-fan.c
>  
> +PWM IR Transmitter
> +M:   Sean Young <s...@mess.org>
> +L:   linux-media@vger.kernel.org
> +S:   Maintained
> +F:   drivers/media/rc/pwm-ir-tx.c
> +
>  PWM SUBSYSTEM
>  M:   Thierry Reding <thierry.red...@gmail.com>
>  L:   linux-...@vger.kernel.org
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index ef767d1..bca77f0 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -410,6 +410,18 @@ config IR_GPIO_TX
>  To compile this driver as a module, choose M here: the module will
>  be called gpio-ir-tx.
>  
> +config IR_PWM_TX
> + tristate "PWM IR transmitter"
> + depends on RC_CORE
> + depends on LIRC
> + depends on PWM
> + ---help---
> +Say Y if you want to use a PWM based IR transmitter. This is
> +more power efficient than the bit banging gpio driver.
> +
> +To compile this driver as a module, choose M here: the module will
> +be called pwm-ir-tx.
> +
>  config RC_ST
>   tristate "ST remote control receiver"
>   depends on RC_CORE
> diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
> index 3e64a4e..466c402 100644
> --- a/drivers/media/rc/Makefile
> +++ b/drivers/media/rc/Makefile
> @@ -33,6 +33,7 @@ obj-$(CONFIG_IR_WINBOND_CIR) += winbond-cir.o
>  obj-$(CONFIG_RC_LOOPBACK) += rc-loopback.o
>  obj-$(CONFIG_IR_GPIO_CIR) += gpio-ir-recv.o
>  obj-$(CONFIG_IR_GPIO_TX) += gpio-ir-tx.o
> +obj-$(CONFIG_IR_PWM_TX) += pwm-ir-tx.o
>  obj-$(CONFIG_IR_IGORPLUGUSB) += igorplugusb.o
>  obj-$(CONFIG_IR_IGUANA) += iguanair.o
>  obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o
> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> new file mode 100644
> index 000..27d0f58
> --- /dev/null
> +++ b/drivers/media/rc/pwm-ir-tx.c
> @@ -0,0 +1,138 @@
> +/*
> + * Copyright (C) 2017 Sean Young <s...@mess.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define DRIVER_NAME  "pwm-ir-tx"
> +#define DEVICE_NAME  "PWM IR Transmitter"
> +
> +struct pwm_ir {
> + struct pwm_device *pwm;
> + unsigned int carrier;
> + unsigned int duty_cycle;
> +};
> +
> +static const struct of_device_id pwm_ir_of_match[] = {
> + { .compatible = "pwm-ir-tx", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, pwm_ir_of_match);
> +
> +static int pwm_ir_set_duty_cycle(struct rc_dev *dev, u32 duty_cycle)
> +{
> + struct pwm_ir *pwm_ir = dev->priv;
> +
> + pwm_ir->duty_cycle = duty_cycle;
> +
> + return 0;
> +}
> +
> +static int pwm_ir_set_carrier(struct rc_dev *dev, u32 carrier)
> +{
> + struct pwm_ir *pwm_ir = dev->priv;
> +
> + if (!carrier)
> + return -EINVAL;
> +
> + pwm_ir->carrier = carrier;
> +
> + return 0;
> +}
> +
> +static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
> +  unsigned int count)
>

Re: [PATCH v2 3/6] [media] rc: gpio-ir-tx: add new driver

[Matthias Reichl]

Hi Sean,

On Fri, Jul 21, 2017 at 04:12:45PM +0200, Matthias Reichl wrote:
> Hi Sean,
> 
> On Fri, Jul 07, 2017 at 10:51:59AM +0100, Sean Young wrote:
> > This is a simple bit-banging GPIO IR TX driver.
> 
> thanks a lot for the driver, this is highly appreciated!
> 
> I tested the patch series on a RPi2, against RPi downstream kernel
> 4.13-rc1, and noticed an issue: the polarity of the gpio seems
> to be reversed.
> 
> Other than the polarity issue the driver seems to work fine -
> at least on the scope screen. Didn't have an IR transmitter to
> do some real tests yet.

I've now run a functional test and it worked fine.

I tested with your patch and my proposed changes, using an
active-high IR transmitter and ir-ctl -S rc5:... to send 
rc-5 codes and another RPi with gpio-ir-recv to receive/decode them.

so long & thanks a lot,

Hias

> 
> I've configured the gpio as active high:
> 
> gpio_ir_tx: gpio-ir-transmitter {
>   compatible = "gpio-ir-tx";
>   gpios = < 18 0>;
> };
> 
> However, when loading the gpio-ir-tx driver the gpio pin changed
> to 3.3V. I did some tests with ir-ctl -S, idle state of the signal
> was 3.3V, active state 0V.
> 
> I think it's better to use the descriptor based gpio functions
> instead of the legacy number based ones. That'll simplify the
> driver and it can delegate polarity handling to gpiod.
> 
> Proposed changes and comments are inline. I've also included
> the patch against your patch that I've been testing with at the
> end of the message.
> 
> > diff --git a/drivers/media/rc/gpio-ir-tx.c b/drivers/media/rc/gpio-ir-tx.c
> > new file mode 100644
> > index 000..7a5371d
> > --- /dev/null
> > +++ b/drivers/media/rc/gpio-ir-tx.c
> > @@ -0,0 +1,189 @@
> > +/*
> > + * Copyright (C) 2017 Sean Young <s...@mess.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> 
> use linux/gpio/consumer.h instead of linux/gpio.h
> 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> of_gpio.h can be dropped
> 
> > +#include 
> > +#include 
> > +
> > +#define DRIVER_NAME"gpio-ir-tx"
> > +#define DEVICE_NAME"GPIO Bit Banging IR Transmitter"
> > +
> > +struct gpio_ir {
> > +   int gpio_nr;
> > +   bool active_low;
> 
> Replace these 2 fields with
>   struct gpio_desc *gpio;
> 
> > +   unsigned int carrier;
> > +   unsigned int duty_cycle;
> > +   /* we need a spinlock to hold the cpu while transmitting */
> > +   spinlock_t lock;
> > +};
> > +
> > +static const struct of_device_id gpio_ir_tx_of_match[] = {
> > +   { .compatible = "gpio-ir-tx", },
> > +   { },
> > +};
> > +MODULE_DEVICE_TABLE(of, gpio_ir_tx_of_match);
> > +
> > +static int gpio_ir_tx_set_duty_cycle(struct rc_dev *dev, u32 duty_cycle)
> > +{
> > +   struct gpio_ir *gpio_ir = dev->priv;
> > +
> > +   gpio_ir->duty_cycle = duty_cycle;
> > +
> > +   return 0;
> > +}
> > +
> > +static int gpio_ir_tx_set_carrier(struct rc_dev *dev, u32 carrier)
> > +{
> > +   struct gpio_ir *gpio_ir = dev->priv;
> > +
> > +   if (!carrier)
> > +   return -EINVAL;
> > +
> > +   gpio_ir->carrier = carrier;
> > +
> > +   return 0;
> > +}
> > +
> > +static int gpio_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
> > + unsigned int count)
> > +{
> > +   struct gpio_ir *gpio_ir = dev->priv;
> > +   unsigned long flags;
> > +   ktime_t edge;
> > +   /*
> > +* delta should never exceed 0.5 seconds (IR_MAX_DURATION) and on
> > +* m68k ndelay(s64) does not compile; so use s32 rather than s64.
> > +*/
> > +   s32 delta;
> > +   int i;
> > +   unsigned int pulse, space;
> > +
> > +   /* Ensure the dividend fits into 32 bit */
> > +   pulse = DIV_ROUND_CLOSEST(gpio_ir->duty_cycle * (NSEC_PER_SEC / 100),
> > + gpio_ir->carrier);
> > +   space = DIV_ROUN

Re: [PATCH v2 3/6] [media] rc: gpio-ir-tx: add new driver

[Matthias Reichl]

Hi Sean,

On Fri, Jul 07, 2017 at 10:51:59AM +0100, Sean Young wrote:
> This is a simple bit-banging GPIO IR TX driver.

thanks a lot for the driver, this is highly appreciated!

I tested the patch series on a RPi2, against RPi downstream kernel
4.13-rc1, and noticed an issue: the polarity of the gpio seems
to be reversed.

Other than the polarity issue the driver seems to work fine -
at least on the scope screen. Didn't have an IR transmitter to
do some real tests yet.

I've configured the gpio as active high:

gpio_ir_tx: gpio-ir-transmitter {
compatible = "gpio-ir-tx";
gpios = < 18 0>;
};

However, when loading the gpio-ir-tx driver the gpio pin changed
to 3.3V. I did some tests with ir-ctl -S, idle state of the signal
was 3.3V, active state 0V.

I think it's better to use the descriptor based gpio functions
instead of the legacy number based ones. That'll simplify the
driver and it can delegate polarity handling to gpiod.

Proposed changes and comments are inline. I've also included
the patch against your patch that I've been testing with at the
end of the message.

> diff --git a/drivers/media/rc/gpio-ir-tx.c b/drivers/media/rc/gpio-ir-tx.c
> new file mode 100644
> index 000..7a5371d
> --- /dev/null
> +++ b/drivers/media/rc/gpio-ir-tx.c
> @@ -0,0 +1,189 @@
> +/*
> + * Copyright (C) 2017 Sean Young 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 

use linux/gpio/consumer.h instead of linux/gpio.h

> +#include 
> +#include 
> +#include 
> +#include 

of_gpio.h can be dropped

> +#include 
> +#include 
> +
> +#define DRIVER_NAME  "gpio-ir-tx"
> +#define DEVICE_NAME  "GPIO Bit Banging IR Transmitter"
> +
> +struct gpio_ir {
> + int gpio_nr;
> + bool active_low;

Replace these 2 fields with
struct gpio_desc *gpio;

> + unsigned int carrier;
> + unsigned int duty_cycle;
> + /* we need a spinlock to hold the cpu while transmitting */
> + spinlock_t lock;
> +};
> +
> +static const struct of_device_id gpio_ir_tx_of_match[] = {
> + { .compatible = "gpio-ir-tx", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, gpio_ir_tx_of_match);
> +
> +static int gpio_ir_tx_set_duty_cycle(struct rc_dev *dev, u32 duty_cycle)
> +{
> + struct gpio_ir *gpio_ir = dev->priv;
> +
> + gpio_ir->duty_cycle = duty_cycle;
> +
> + return 0;
> +}
> +
> +static int gpio_ir_tx_set_carrier(struct rc_dev *dev, u32 carrier)
> +{
> + struct gpio_ir *gpio_ir = dev->priv;
> +
> + if (!carrier)
> + return -EINVAL;
> +
> + gpio_ir->carrier = carrier;
> +
> + return 0;
> +}
> +
> +static int gpio_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
> +   unsigned int count)
> +{
> + struct gpio_ir *gpio_ir = dev->priv;
> + unsigned long flags;
> + ktime_t edge;
> + /*
> +  * delta should never exceed 0.5 seconds (IR_MAX_DURATION) and on
> +  * m68k ndelay(s64) does not compile; so use s32 rather than s64.
> +  */
> + s32 delta;
> + int i;
> + unsigned int pulse, space;
> +
> + /* Ensure the dividend fits into 32 bit */
> + pulse = DIV_ROUND_CLOSEST(gpio_ir->duty_cycle * (NSEC_PER_SEC / 100),
> +   gpio_ir->carrier);
> + space = DIV_ROUND_CLOSEST((100 - gpio_ir->duty_cycle) *
> +   (NSEC_PER_SEC / 100), gpio_ir->carrier);
> +
> + spin_lock_irqsave(_ir->lock, flags);
> +
> + edge = ktime_get();
> +
> + for (i = 0; i < count; i++) {
> + if (i % 2) {
> + // space
> + edge = ktime_add_us(edge, txbuf[i]);
> + delta = ktime_us_delta(edge, ktime_get());
> + if (delta > 10) {
> + spin_unlock_irqrestore(_ir->lock, flags);
> + usleep_range(delta, delta + 10);
> + spin_lock_irqsave(_ir->lock, flags);
> + } else if (delta > 0) {
> + udelay(delta);
> + }
> + } else {
> + // pulse
> + ktime_t last = ktime_add_us(edge, txbuf[i]);
> +
> + while (ktime_get() < last) {
> + gpio_set_value(gpio_ir->gpio_nr,
> +gpio_ir->active_low);

gpiod_set_value(gpio_ir->gpio, 1);

> + edge += pulse;
> + delta = edge - 

[v4l-utils] 70-infrared.rules starts ir-keytable too early

[Matthias Reichl]

While testing serial_ir on kernel 4.11.8 with ir-keytable 1.12.3
I noticed that my /etc/rc_maps.cfg configuration wasn't applied.
Manually running "ir-keytable -a /etc/rc_maps.cfg -s rc0" always
worked fine, though.

Digging further into this I tracked it down to the udev rule
being racy. The udev rule triggers on the rc subsystem, but this
is before the input and event devices are created.

The kernel creates 3 events relevant to this context, on rcX
device creation, on inputY creation and on inputY/eventZ creation
- the latter 2 usually in quick succession, but some time can
elapse between the first 2.

In my case ir-keytable -a was executing during this small time
window and failed with an error because it couldn't find the
input/event devices.

Excerpt from log with udev debugging enabled:

Jul 16 11:02:11 LibreELEC systemd-udevd[272]: seq 2099 queued, 'add' 'rc'
...
Jul 16 11:02:11 LibreELEC systemd-udevd[614]: '/usr/bin/ir-keytable -a 
/etc/rc_maps.cfg -s rc0'(err) 'Couldn't find any node at 
/sys/class/rc/rc0/input*.'
Jul 16 11:02:11 LibreELEC systemd-udevd[614]: Process '/usr/bin/ir-keytable -a 
/etc/rc_maps.cfg -s rc0' failed with exit code 255.
...
Jul 16 11:02:11 LibreELEC systemd-udevd[272]: seq 2106 queued, 'add' 'input'
Jul 16 11:02:11 LibreELEC systemd-udevd[272]: seq 2106 forked new worker [622]
Jul 16 11:02:11 LibreELEC systemd-udevd[272]: seq 2107 queued, 'add' 'input'

The full log is available here:
http://www.horus.com/~hias/tmp/journalctl-ir-keytable-failed

One solution is to trigger ir-keytable -a execution from the event device
creation instead. I'm currently testing with the following udev rule:

ACTION=="add", SUBSYSTEMS=="rc", GOTO="begin"
GOTO="end"

LABEL="begin"

SUBSYSTEM=="rc", ENV{rc_sysdev}="$name"

SUBSYSTEM=="input", IMPORT{parent}="rc_sysdev"

KERNEL=="event[0-9]*", ENV{rc_sysdev}=="?*", \
   RUN+="/usr/bin/ir-keytable -a /etc/rc_maps.cfg -s $env{rc_sysdev}"

LABEL="end"

That udev rule is a bit messy, ir-keytable -a needs the rcX sysdev,
which doesn't seem to be easily available from the event node in
the input subsystem, so I'm propagating that info through an
environment variable.

So far testing is working fine, but hints for better/nicer solutions
are welcome!

so long,

Hias

Re: [PATCH] [media] rc: raw decoder for keymap protocol is not loaded on register

[Matthias Reichl]

On Wed, Feb 22, 2017 at 11:11:49PM +, Sean Young wrote:
> When the protocol is set via the sysfs protocols attribute, the
> decoder is loaded. However, when it is not when a device is first
> plugged in or registered.
> 
> Fixes: acc1c3c ("[media] media: rc: load decoder modules on-demand")
> 
> Signed-off-by: Sean Young <s...@mess.org>

Tested-by: Matthias Reichl <h...@horus.com>

I've tested the backported patch below successfully on RPi3 with
kernel 4.10 and decoder modules are loading fine again:

# dmesg | grep "IR "
[3.526404] Registered IR keymap rc-hauppauge
[3.590875] lirc_dev: IR Remote Control driver registered, major 242
[3.600602] IR RC5(x/sz) protocol handler initialized
[3.602111] IR LIRC bridge handler initialized

Thanks a lot for fixing this so quickly!

so long,

Hias

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index dedaf38..9a397da 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1441,6 +1441,7 @@ int rc_register_device(struct rc_dev *dev)
int attr = 0;
int minor;
int rc;
+   u64 rc_type;
 
if (!dev || !dev->map_name)
return -EINVAL;
@@ -1526,14 +1527,18 @@ int rc_register_device(struct rc_dev *dev)
goto out_input;
}
 
+   rc_type = BIT_ULL(rc_map->rc_type);
+
if (dev->change_protocol) {
-   u64 rc_type = (1ll << rc_map->rc_type);
rc = dev->change_protocol(dev, _type);
if (rc < 0)
goto out_raw;
dev->enabled_protocols = rc_type;
}
 
+   if (dev->driver_type == RC_DRIVER_IR_RAW)
+   ir_raw_load_modules(_type);
+
/* Allow the RC sysfs nodes to be accessible */
atomic_set(>initialized, 1);
 

Re: Bug: decoders referenced in kernel rc-keymaps not loaded on boot

[Matthias Reichl]

On Tue, Feb 21, 2017 at 07:34:39PM +, Sean Young wrote:
> On Tue, Feb 21, 2017 at 07:49:29PM +0100, Matthias Reichl wrote:
> > There seems to be a bug in on-demand loading of IR protocol decoders.
> > 
> > After bootup the protocol referenced in the in-kernel rc keymap shows
> > up as enabled (in sysfs and ir-keytable) but the protocol decoder
> > is not loaded and thus no rc input events will be generated.
> > 
> > For example, RPi3 with kernel 4.10 and gpio-ir-recv configured to use
> > the rc-hauppauge keymap in devicetree:
> > 
> > # lsmod | grep '^\(ir\|rc_\)'
> > ir_lirc_codec   5590  0
> > rc_hauppauge2422  0
> > rc_core24320  5 
> > rc_hauppauge,ir_lirc_codec,lirc_dev,gpio_ir_recv
> > 
> > # cat /sys/class/rc/rc0/protocols
> > other unknown [rc-5] nec rc-6 jvc sony rc-5-sz sanyo sharp mce_kbd xmp cec 
> > [lirc]
> > 
> > # dmesg | grep "IR "
> > [4.506728] Registered IR keymap rc-hauppauge
> > [4.554651] lirc_dev: IR Remote Control driver registered, major 242
> > [4.576490] IR LIRC bridge handler initialized
> > 
> > The same happens with other IR receivers, eg the streamzap receiver,
> > which uses the rc-5-sz protocol / ir_rc5_decoder, on x86.
> > 
> > Reverting the on-demand-loading patches
> > 
> > [media] media: rc: remove unneeded code
> > commit c1500ba0b61e9abf95e0e7ecd3c4ad877f019abe
> > 
> > [media] media: rc: move check whether a protocol is enabled to the core
> > commit d80ca8bd71f0b01b2b12459189927cb3299cfab9
> > 
> > [media] media: rc: load decoder modules on-demand
> > commit acc1c3c688ed8cc862ddc007eab0dcef839f4ec8
> > 
> > restores the previous behaviour, all decoders are enabled and IR
> > events can be generated immediately after boot without having to
> > manually trigger loading of a protocol decoder.
> 
> Hmm this seems to be working fine for me. If you write to the protocols
> file, eg. "echo +nec > /sys/class/rc/rc0/protocols", is ir-nec-decoder
> loaded and do you get any messages in dmesg (you should).
> 
> What's your config?

When I do an "echo +nec > /sys/class/rc/rc0/protocols" it triggers
the load of both rc5 and nec decoder modules:

root@rpi3:~# cat /sys/class/rc/rc0/protocols
other unknown [rc-5] nec rc-6 jvc sony rc-5-sz sanyo sharp mce_kbd xmp cec 
[lirc]
root@rpi3:~# echo +nec > /sys/class/rc/rc0/protocols
root@rpi3:~# cat /sys/class/rc/rc0/protocols
other unknown [rc-5] [nec] rc-6 jvc sony rc-5-sz sanyo sharp mce_kbd xmp cec 
[lirc]
root@rpi3:~# dmesg | grep "IR "
[3.565061] Registered IR keymap rc-hauppauge
[3.613031] lirc_dev: IR Remote Control driver registered, major 242
[3.641423] IR LIRC bridge handler initialized
[   41.877263] IR RC5(x/sz) protocol handler initialized
[   41.931575] IR NEC protocol handler initialized

I'm currently testing with downstream RPi kernel 4.9 on Raspbian Jessie
(a Debian derivate).

Kernel config is here:
https://github.com/raspberrypi/linux/blob/rpi-4.9.y/arch/arm/configs/bcm2709_defconfig

To reproduce the issue it's important to disable the udev rule that
runs ir-keytable -a as that can trigger a load of the kernel keytable
via the userspace keymap/protocol.

We ran accross the issue via a bugreport from a LibreELEC user,
his streamzap remote wasn't working anymore on x86 in the beta
releases:
https://forum.libreelec.tv/thread-4873.html

Kernel-config for LibreELEC x86 is here:
https://github.com/LibreELEC/LibreELEC.tv/blob/libreelec-8.0/projects/Generic/linux/linux.x86_64.conf

Our analysis (I hope it's not completely off) is about this:

In the previous version (with kernel 4.4) it worked because
the kernel loaded the keymap and protocol decoders. The udev
rule probably failed as ir-keytable -a couldn't cope with the RC5_SZ
protocol - but that went unnoticed as everything was setup fine
by the kernel.

In current beta (with kernel 4.9) the kernel only loaded the
keymap but didn't enable the decoder. Since ir-keytable -a again
failed to setup the protocol the user was left with a non-functioning
remote.

I then could reproduce this on RPi with Raspbian and LibreELEC
using gpio-ir-recv. With udev/ir-keytable -a working the protocol
decoder is loaded, without that it isn't.

so long,

Hias

Bug: decoders referenced in kernel rc-keymaps not loaded on boot

[Matthias Reichl]

There seems to be a bug in on-demand loading of IR protocol decoders.

After bootup the protocol referenced in the in-kernel rc keymap shows
up as enabled (in sysfs and ir-keytable) but the protocol decoder
is not loaded and thus no rc input events will be generated.

For example, RPi3 with kernel 4.10 and gpio-ir-recv configured to use
the rc-hauppauge keymap in devicetree:

# lsmod | grep '^\(ir\|rc_\)'
ir_lirc_codec   5590  0
rc_hauppauge2422  0
rc_core24320  5 rc_hauppauge,ir_lirc_codec,lirc_dev,gpio_ir_recv

# cat /sys/class/rc/rc0/protocols
other unknown [rc-5] nec rc-6 jvc sony rc-5-sz sanyo sharp mce_kbd xmp cec 
[lirc]

# dmesg | grep "IR "
[4.506728] Registered IR keymap rc-hauppauge
[4.554651] lirc_dev: IR Remote Control driver registered, major 242
[4.576490] IR LIRC bridge handler initialized

The same happens with other IR receivers, eg the streamzap receiver,
which uses the rc-5-sz protocol / ir_rc5_decoder, on x86.

Reverting the on-demand-loading patches

[media] media: rc: remove unneeded code
commit c1500ba0b61e9abf95e0e7ecd3c4ad877f019abe

[media] media: rc: move check whether a protocol is enabled to the core
commit d80ca8bd71f0b01b2b12459189927cb3299cfab9

[media] media: rc: load decoder modules on-demand
commit acc1c3c688ed8cc862ddc007eab0dcef839f4ec8

restores the previous behaviour, all decoders are enabled and IR
events can be generated immediately after boot without having to
manually trigger loading of a protocol decoder.

so long,

Hias

Re: [PATCH] v4l-utils: fix invalid protocol in streamzap keymap

[Matthias Reichl]

On Sat, Feb 18, 2017 at 07:24:43PM +, Sean Young wrote:
> On Fri, Feb 17, 2017 at 10:19:16AM +0100, Matthias Reichl wrote:
> > ir-keytable can't load the streamzap keymap because the
> > protocol type RC5_SZ is invalid:
> > 
> > ./ir-keytable -w rc_keymaps/streamzap
> > Protocol RC5_SZ invalid
> > ...
> > 
> > Fix this by changing the protocol type to RC-5-SZ which
> > matches the kernel protocol rc-5-sz
> > 
> > Signed-off-by: Matthias Reichl <h...@horus.com>
> > ---
> >  utils/keytable/rc_keymaps/streamzap | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/utils/keytable/rc_keymaps/streamzap 
> > b/utils/keytable/rc_keymaps/streamzap
> > index 3512cd8..03d2cb8 100644
> > --- a/utils/keytable/rc_keymaps/streamzap
> > +++ b/utils/keytable/rc_keymaps/streamzap
> 
> This is file is generated by utils/keytable/gen_keytables.pl, so there is no
> use in patching it.

Ouch, I totally missed that. Thanks for pointing it out!

> Actually I think a better solution would be to be less pernickety about how
> the protocol is specified. ir-ctl also does this. How about the following
> patch.

I agree, this is a much better solution. It simplifies the protocol_map
and being more tolerant about spelling is also a benefit to the user.

> Sean
> 
> From: Sean Young <s...@mess.org>
> Subject: [PATCH] [PATCH v4l-utils] ir-keytable: be more permissive on protocol
>  name
> 
> Allowed the protocol to be specified with or without underscores or
> dashes. This also solves the problem of not being able to load
> the streamzap keymap.
> 
> ./ir-keytable -w rc_keymaps/streamzap
> Protocol RC5_SZ invalid
> 
> Reported-by: Matthias Reichl <h...@horus.com>
> Signed-off-by: Sean Young <s...@mess.org>
> ---
>  utils/keytable/keytable.c | 20 
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/utils/keytable/keytable.c b/utils/keytable/keytable.c
> index a6ecc9e..a35db5b 100644
> --- a/utils/keytable/keytable.c
> +++ b/utils/keytable/keytable.c
> @@ -120,9 +120,7 @@ const struct protocol_map_entry protocol_map[] = {
>   { "other",  NULL,   SYSFS_OTHER },
>   { "lirc",   NULL,   SYSFS_LIRC  },
>   { "rc-5",   "/rc5_decoder", SYSFS_RC5   },
> - { "rc5",NULL,   SYSFS_RC5   },
>   { "rc-5x",  NULL,   SYSFS_INVALID   },
> - { "rc5x",   NULL,   SYSFS_INVALID   },
>   { "rc-5-sz",NULL,   SYSFS_RC5_SZ},
>   { "jvc","/jvc_decoder", SYSFS_JVC   },
>   { "sony",   "/sony_decoder",SYSFS_SONY  },
> @@ -134,7 +132,6 @@ const struct protocol_map_entry protocol_map[] = {
>   { "mce_kbd",NULL,   SYSFS_MCE_KBD   },
>   { "mce-kbd",NULL,   SYSFS_MCE_KBD   },
>   { "rc-6",   "/rc6_decoder", SYSFS_RC6   },
> - { "rc6",NULL,   SYSFS_RC6   },
>   { "rc-6-0", NULL,   SYSFS_INVALID   },
>   { "rc-6-6a-20", NULL,   SYSFS_INVALID   },
>   { "rc-6-6a-24", NULL,   SYSFS_INVALID   },
> @@ -145,6 +142,21 @@ const struct protocol_map_entry protocol_map[] = {
>   { NULL, NULL,   SYSFS_INVALID   },
>  };
>  
> +static bool str_like(const char *a, const char *b)
> +{
> + while (*a && *b) {
> + if (*a == '-' || *a == '_')
> + a++;
> + if (*b == '-' || *b == '_')
> + b++;
> + if (tolower(*a) != tolower(*b))
> + return false;
> + a++; b++;
> + }

A small nit: this code will fail if both strings are identical
and end with a dash or underscore. In that case we'll iterate
beyond then end of the strings.

Adding a continue after the dash/underscore increment should fix
this, then we won't increment the pointer by 2 within a loop

if (*a == '-' || *a == '_') {
a++;
continue;
}
if (*b == '-' || *b == '_') {
b++;
continue;
}

Other than that the patch looks fine to me and worked well.

> +
> + return !*a && !*b;
> +}
> +
>  static enum sysfs_protocols parse_sysfs_protocol(const char *name, bool 
> all_allowed)
>  {
>   const struct protocol_map_entry *pme;
> @@ -156,7 +168,7 @@ static enum sysfs_protocols parse_sysfs_protocol(const 
> char *name, bool all_allo
>   return ~0;
>  
>   for (pme = protocol_map; pme->name; pme++) {
> - if (!strcasecmp(name, pme->name))
> + if (str_like(name, pme->name))
>   return pme->sysfs_protocol;
>   }
>  
> -- 
> 2.9.3
> 

[PATCH] v4l-utils: fix invalid protocol in streamzap keymap

[Matthias Reichl]

ir-keytable can't load the streamzap keymap because the
protocol type RC5_SZ is invalid:

./ir-keytable -w rc_keymaps/streamzap
Protocol RC5_SZ invalid
...

Fix this by changing the protocol type to RC-5-SZ which
matches the kernel protocol rc-5-sz

Signed-off-by: Matthias Reichl <h...@horus.com>
---
 utils/keytable/rc_keymaps/streamzap | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/keytable/rc_keymaps/streamzap 
b/utils/keytable/rc_keymaps/streamzap
index 3512cd8..03d2cb8 100644
--- a/utils/keytable/rc_keymaps/streamzap
+++ b/utils/keytable/rc_keymaps/streamzap
@@ -1,4 +1,4 @@
-# table streamzap, type: RC5_SZ
+# table streamzap, type: RC-5-SZ
 0x28c0 KEY_NUMERIC_0
 0x28c1 KEY_NUMERIC_1
 0x28c2 KEY_NUMERIC_2
-- 
2.1.4

Re: [PATCH] Partly revert "[media] rc-core: allow calling rc_open with device not initialized"

[Matthias Reichl]

On Sat, Jul 30, 2016 at 03:19:27PM +0200, Ole Ernst wrote:
> This partly reverts commit 078600f514a12fd763ac84c86af68ef5b5267563.
> 
> Due to the relocation of input_register_device() call, holding down a
> button on an IR remote no longer resulted in repeated key down events.
> 
> Signed-off-by: Ole Ernst <olebo...@gmx.com>

Tested-by: Matthias Reichl <h...@horus.com>

I tested on Raspberry Pi model B with kernel 4.7.0, gpio-rc-recv,
rc-hauppauge keymap and with this patch key repeat is working fine
again.

Thanks a lot for the quick fix!

Hias

> ---
>  drivers/media/rc/rc-main.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 8e7f292..26fd63b 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1460,6 +1460,10 @@ int rc_register_device(struct rc_dev *dev)
>   dev->input_dev->phys = dev->input_phys;
>   dev->input_dev->name = dev->input_name;
>  
> + rc = input_register_device(dev->input_dev);
> + if (rc)
> + goto out_table;
> +
>   /*
>* Default delay of 250ms is too short for some protocols, especially
>* since the timeout is currently set to 250ms. Increase it to 500ms,
> @@ -1475,11 +1479,6 @@ int rc_register_device(struct rc_dev *dev)
>*/
>   dev->input_dev->rep[REP_PERIOD] = 125;
>  
> - /* rc_open will be called here */
> - rc = input_register_device(dev->input_dev);
> - if (rc)
> - goto out_table;
> -
>   path = kobject_get_path(>dev.kobj, GFP_KERNEL);
>   dev_info(>dev, "%s as %s\n",
>   dev->input_name ?: "Unspecified device", path ?: "N/A");
> -- 
> 2.9.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

IR button repeat not working in kernel 4.6 and 4.7

[Matthias Reichl]

In kernel 4.6 and 4.7 holding down a button on an IR remote no longer
results in repeated key down events.

I've reproduced that issue on a Raspberry Pi B using a GPIO IR
receiver. Other systems seem to be affected as well, for
example Intel NUC with an ITE CIR receiver.

Bisecting points to this commit as a possible cause for that issue:

commit 078600f514a12fd763ac84c86af68ef5b5267563
Author: Mauro Carvalho Chehab 
Date:   Wed Mar 2 08:00:15 2016 -0300

[media] rc-core: allow calling rc_open with device not initialized

With upstream kernel 4.6.0 and 4.7.0 the ir-keytable output looks OK:

Found /sys/class/rc/rc0/ (/dev/input/event0) with:
Driver gpio-rc-recv, table rc-hauppauge
Supported protocols: NEC RC-5 RC-6 JVC SONY SANYO RC-5-SZ SHARP XMP 
other
Enabled protocols: RC-5
Name: gpio_ir_recv
bus: 25, vendor/product: 0001:0001, version: 0x0100
Repeat delay = 500 ms, repeat period = 125 ms
 
But running evtest (or ir-keytable -t) only shows EV_KEY events on
initial button down and on release:

Event: time 1469531035.490700, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531035.490700, type 1 (EV_KEY), code 2 (KEY_1), value 1
Event: time 1469531035.490700, -- EV_SYN 
Event: time 1469531035.603725, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531035.603725, -- EV_SYN 
Event: time 1469531035.716778, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531035.716778, -- EV_SYN 
Event: time 1469531035.829849, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531035.829849, -- EV_SYN 
Event: time 1469531035.942893, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531035.942893, -- EV_SYN 
Event: time 1469531036.055932, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531036.055932, -- EV_SYN 
Event: time 1469531036.169004, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531036.169004, -- EV_SYN 
Event: time 1469531036.282043, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531036.282043, -- EV_SYN 
Event: time 1469531036.395103, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531036.395103, -- EV_SYN 
Event: time 1469531036.508157, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531036.508157, -- EV_SYN 
Event: time 1469531036.621216, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531036.621216, -- EV_SYN 
Event: time 1469531036.734255, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531036.734255, -- EV_SYN 
Event: time 1469531036.875917, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531036.875917, -- EV_SYN 
Event: time 1469531037.125875, type 1 (EV_KEY), code 2 (KEY_1), value 0
Event: time 1469531037.125875, -- EV_SYN 

When reverting commit 078600f514a12fd763ac84c86af68ef5b5267563 in
kernel 4.6.0 evtest output is back to normal: EV_KEY events with
value 2 show up between button down and button up:

Event: time 1469531201.086823, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531201.086823, type 1 (EV_KEY), code 2 (KEY_1), value 1
Event: time 1469531201.086823, -- EV_SYN 
Event: time 1469531201.199789, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531201.199789, -- EV_SYN 
Event: time 1469531201.312818, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531201.312818, -- EV_SYN 
Event: time 1469531201.425846, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531201.425846, -- EV_SYN 
Event: time 1469531201.538852, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531201.538852, -- EV_SYN 
Event: time 1469531201.578497, type 1 (EV_KEY), code 2 (KEY_1), value 2
Event: time 1469531201.578497, -- EV_SYN 
Event: time 1469531201.651897, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531201.651897, -- EV_SYN 
Event: time 1469531201.708488, type 1 (EV_KEY), code 2 (KEY_1), value 2
Event: time 1469531201.708488, -- EV_SYN 
Event: time 1469531201.764901, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531201.764901, -- EV_SYN 
Event: time 1469531201.838497, type 1 (EV_KEY), code 2 (KEY_1), value 2
Event: time 1469531201.838497, -- EV_SYN 
Event: time 1469531201.877950, type 4 (EV_MSC), code 4 (MSC_SCAN), value 1e01
Event: time 1469531201.877950, -- EV_SYN