[PATCH libinput 1/1] Fix debouncing algorithm

[Vicente Bergas]

Hi Peter,

(sorry, sent twice, first time I forgot to "reply-all")

On Tue, Nov 14, 2017 at 5:22 AM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> ...

>> -#define      DEBOUNCE_TIME ms2us(12)
>> +#define      DEBOUNCE_TIME ms2us(25)
>
> 25ms is quite risky for the original algorithm, it's possible to
> press/release events within 25ms. why not add a separate timeout?

With my algorithm it is not possible to make them separate because it
is not possible to know if the current "noise" is a bouncing or an
spurious event until the end of the timeout.

>...

>> +             if (dispatch->debounce.debounce_mode !=
>> +                 DEBOUNCE_MODE_BOUNCE_AND_SPURIOUS) {
>> +                     dispatch->debounce.debounce_mode =
>> +                             DEBOUNCE_MODE_BOUNCE_AND_SPURIOUS;
>> +                     evdev_log_info(device,
>> +                            "Enabling spurious event suppression, "
>> +                            "see %sbutton_debouncing.html for details\n",
>> +                            HTTP_DOC_LINK);
>> +             }
>
> this bit doesn't seem right:
> - if I press + release within the timeout but otherwise don't do anything,
>   spurious debounce mode enables, despite the event being fine otherwise.
>   It's ok to delay the release here for the filtering, but we don't need to
>   enable spurious mode here

Here your interpretation is right.
PR within the timeout, that has been an extremly quick press, so fast
that it is considered an spurious event. The same with RP.
If already in spurious mode, then ignore,
else, commit and switch to spurious mode.
As said before, there is not differentiation between press and release, but
if you consider that a quick PR is a legitimate event whereas a quick RP is not,
then an special case could be added.

> - if I press + release + press within the timeout, spurious mode enables,
>   but your algorithm filters over that correctly anyway so we don't need to
>   enable it.

PRP within the timeout, that is a press plus noise.
Your interpretation is wrong, spurious mode is never enabled in this case.
If already in spurious mode (because it was enabled long ago), then
commit the last press, ignore the first two events.
If not in spurious mode, then the first press is instantly committed,
the last two events are ignored.

> - if I release and press within the timeout, spurious mode enables -> good,
>   that's what it's supposed to do

RP and PR are currently treated the same.

>
> the above condition just makes the spurious mode enable after whatever first
> bounce event is detected which seems wrong. we only need it for the release
> + press within timeout case.

>...

>> +     if (dispatch->debounce.debounce_mode ==
>> +         DEBOUNCE_MODE_BOUNCE_AND_SPURIOUS) {
>> +             /* This could be a spurious event, so, filter it out */
>> +             return true;
>>       }
>
> once you get the first bounce and switched to spurious mode, you're now
> filtering (and thus delaying) every button event, including press events.
>
> [this is where I finished on Fri, was working on the state machine since so
> I'm leaving it here]
>
> Cheers,
>    Peter

I'll try to explain more graphically the algorithm with some examples:

http://wavedrom.com/editor.html?%7Bsignal%3A%20%5B%0A%20%20%7Bname%3A%27current%20mode%27%2C%20wave%3A%20%273.....%7C.....%27%2C%20data%3A%20%5B%27bounce%20only%27%5D%7D%2C%0A%20%20%7Bname%3A%27button%20%20%20%20%20%20%27%2C%20wave%3A%20%271010..%7C101..%27%7D%2C%0A%20%20%7Bname%3A%27application%20%27%2C%20wave%3A%20%2710....%7C1....%27%7D%2C%0A%20%20%7B%7D%2C%0A%20%20%7Bname%3A%27current%20mode%27%2C%20wave%3A%20%274.....%7C.....%27%2C%20data%3A%20%5B%27bounce%20and%20spurious%27%5D%7D%2C%0A%20%20%7Bname%3A%27button%20%20%20%20%20%20%27%2C%20wave%3A%20%271010..%7C101..%27%7D%2C%0A%20%20%7Bname%3A%27application%20%27%2C%20wave%3A%20%271....0%7C....1%27%7D%2C%0A%20%20%7B%7D%2C%0A%20%20%7Bname%3A%27current%20mode%27%2C%20wave%3A%20%273....4......%27%2C%20data%3A%20%5B%27bounce%20only%27%2C%20%27bounce%20and%20spurious%27%5D%7D%2C%0A%20%20%7Bname%3A%27button%20%20%20%20%20%20%27%2C%20wave%3A%20%27101.........%27%7D%2C%0A%20%20%7Bname%3A%27application%20%27%2C%20wave%3A%20%2710...1......%27%7D%2C%0A%20%20%7B%7D%2C%0A%20%20%7Bname%3A%27current%20mode%27%2C%20wave%3A%20%273....4......%27%2C%20data%3A%20%5B%27bounce%20only%27%2C%20%27bounce%20and%20spurious%27%5D%7D%2C%0A%20%20%7Bname%3A%27button%20%20%20%20%20%20%27%2C%20wave%3A%20%27010.........%27%7D%2C%0A%20%20%7Bname%3A%27application%20%27%2C%20wave%3A%20%2701...0......%27%7D%2C%0A%5D%7D%0A

(I hope you can open this URL, you will need to remove the newlines
inserted automatically)
The first two waves show a bouncy press followed by a bouncy release,
the top one in bounce-only mode and the second one in
bounce-and-spurious mode.
The only difference is the delay in which the events are notified to
the application.
The last two waves show how the bounce-and-spurious mode is enabled,
the third with an spurious press and the fourth with an spurious release.
If you want I can add an special case, so an extremly quick press is
treated as a legitimate event and not spurious.

On the one hand, if you want, I can implement that special case.
On the other hand, if you want to take over with your v4
implementation, I will be glad to hand it over to you.

Regards,
  Vicente.