[PATCH libinput 1/1] Fix debouncing algorithm

[Peter Hutterer]

On Tue, Nov 14, 2017 at 07:40:08PM +0100, Vicente Bergas wrote:
> 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.

I think that's a case we'd have to add. Your approach covers the bouncing on
state changes nicely, so the spurious relase really only now applies to
buttons held down but released. I'm not sure how common the issue is where a
button sends click events randomly even when the button is physically up.
That is certainly a case where I'd argue for just buying a new mouse :)

> 
> > - 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.

oh, right, because commit_last_value is false so we never enter this bit.
thanks, I misread that.

> > - 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)

worked, but this would've been a good case for an URL shortener :)
but wow, this is a pretty awesome tool, thanks. Might add an SVG or two to
the documentation for that.

> 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.

I think going with the v4 is better right now, if only because debugging
that is a lot easier than keeping the logical state in mind. It's mostly a
matter of opening the SVG and going through the state machine one-by-one
until you figure out where we're going wrong. It's a lot of boilerplate code
but speaking from the experience of debugging the tapping code it saves a
lot of time in the long run. I'd appreciate it if you could give it a test
though.

Cheers,
   Peter