On Tue, Apr 10, 2018 at 09:24:19PM +0200, Matthias Reichl wrote: > 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->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, &lsc); > data->state = STATE_INACTIVE; > input_event(data->idev, EV_MSC, MSC_SCAN, scancode); > input_sync(data->idev); > return 0; Yes, you're completely right. > > > 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(). Again I stand corrected. I'm just testing some patches now for this. Thanks for being patient and explaining that again :) Sean
