[PATCH libinput 0/1] Fix debouncing algorithm

[Peter Hutterer]

On Thu, Nov 02, 2017 at 12:34:01AM +0100, Vicente Bergas wrote:
> Hi Peter,
> 
> >Hi Vicente
> >
> >sorry about the delay, too many things on right now.
> 
> Thank you for taking the time to review the patch.
> 
> >...
> >Note: the current algorithm does ignore spurious actions, at least after the
> >very first one. AIUI it's not that uncommon either when the contacts are
> >wobbly. So removing it means we re-introduce bugs, you'll have to modify
> >your patch to fit in addition to the current one.
> 
> For this reason I suggested (below) making the DEBOUNCE_AFTER macro a
> configurable parameter. When it is set to true then it also filters out
> spurious events, as explained in the next paragraph.
> In any case, this new patch enables the debounce_after mode dynamically, so,
> feature wise, will be equivalent to the current version.

I'm really disinclined to make this a configurable parameter. Generally we
try not to have anything configurable that's a hardware-specific property
and try to autodetect it where possible or use hwdb entries where that's not
the case. The biggest problem with any parameter is that you're locking
yourself in for a specific behaviour. This is doubly so when you go beyond a
on/off switch.

Take debouncing as example: assume the first version was a on/off debounce
toggle. with your patch we'd now have to add another toggle to decide which
algorithm to enable. If someone comes up with a newer version in a few
months time, we'd need another toggle. We couldn't easily remove the old
versions when something better comes along because now the configuration
options have spread through the various forums like an STD in the seventies.
And each of those posts will stay online forever, so in 10 years time people
will still copy/paste config snippets for options that haven't worked
properly in years (this was and is the case for many xorg.conf options).

Of course, you now have to also explain what the difference is between the
'debounce on' option and the 'detectable-debounce on' options (or whatever
better name you'll come up with your approach :).

Even worse is the case where you have more specific toggles than on/off.
Timeouts sound like a good idea until we've locked ourselves into a
behaviour because we can't change the definition of the timeout (e.g.
"applies from button release" vs "applies from any button press or release
event").

Sure, you can always add new options and deprecate older ones but now you're
making it a lot harder for anyone else to deal with it. And libinput is a
library, it's not immediately user-facing. So someone may just ship a new
version of foo that uses the config option you've deprecated 2 days ago.
and that is on top of the problem of 'how do you name the API for the third
version of a debouncing algorithm' without upsetting the grammar gods :)

config options are always the last option when we've exhausted everything
else and really throw up or hands and admit that this is too difficult.

> >> There is a macro named DEBOUNCE_AFTER that changes the algorithm in a way
> >> that events are committed after noise suppression. In that mode it is
> >> well suited for both misbehaviours of the button. The drawback is that
> >> there is a delay in that mode, so, it is not enabled by default.
> 
> >> ...
> >>  src/evdev-fallback.c | 158 +++++++++++++++------------------------------------
> >>  src/evdev.h          |  19 -------
> >>  2 files changed, 45 insertions(+), 132 deletions(-)
> >
> >I generally worry when I see significant changes to src but nothing in
> >test :) and indeed, most (all?) button debouncing tests fail now. and there
> >are no new tests for the various new behaviours. we definitely need those
> >before we can merge it.
> 
> I hope it is fixed, but not been able to run the test-suite.
> Please, can you confirm if the new test-suite passes?

running the full test suite should be a simple sudo ninja test, or you run
it directly with
 sudo ./build/libinput-test-suite-runner --filter-test="*sometestname*"
That is particularly useful when you are working on a specific feature and
want a quick test turnover time (the full suite can take up 20 min or
longer)

https://wayland.freedesktop.org/libinput/doc/latest/test-suite.html
has more documentation and we can add to that what's missing

rest of the comments are in the v2 patch (haven't looked at it yet though),
many thanks for the v2

Cheers,
   Peter

> >> ...
> >> +/* Commit an event before or after DEBOUNCE_TIME
> >> + *   before: events are notified without any delay
> >> + *           spurious events are also notified
> >> + *   after: events are notified after a delay
> >> + *          spurious events are filtered out
> >> + */
> >> +#define	DEBOUNCE_AFTER false
> >
> >definitive no to this bit. Use an enum, it makes the code a lot more
> >readable, especially because you're mixing the boolean value and the
> >human-readable DEBOUNCE_AFTER. I recommend using an enum with the first enum
> >value not being 0, it avoids the fake boolean temptation.
> 
> Fixed.
> 
> >> ...
> >>  	struct {
> >> -		enum evdev_debounce_state state;
> >> +		bool cancel_noise;
> >> +		bool first_value;
> >> +		bool last_value;
> >>  		unsigned int button_code;
> >> -		uint64_t button_up_time;
> >> +		uint64_t button_time;
> >>  		struct libinput_timer timer;
> >>  	} debounce;
> >>  
> >> @@ -639,22 +650,24 @@ static inline void
> >>  fallback_flush_debounce(struct fallback_dispatch *dispatch,
> >>  			struct evdev_device *device)
> >>  {
> >
> >this function needs to check for cancel_noise == true and return early if
> >that's not the case. we can't guarantee that the timer won't be called after
> >an event has already cancelled it.
> 
> Fixed.
> 
> >> ...
> >> +	bool last_value = dispatch->debounce.last_value;
> >
> >whoah,
> 
> please, be more respectful, what you claim is going on here is not a reason
> for that exclamation.
> 
> >you're secretly changing int to bool here. this needs to be more
> >expressive like e.g.
> > bool is_down = !!dispatch->debounce.last_value; 
> >
> >you can't just change the type, herein lays mayhem.
> 
> your claim is incorrect, the local variable last_value is of type bool and
> the variable dispatch->debounce.last_value is of the same type.
> That applied further below, but not here.
> Does that also apply when an int is used as a boolean condition in an if
> statement or the ternary operator?
> 
> >> ...
> >> +	if (DEBOUNCE_AFTER !=
> >> +		(dispatch->debounce.first_value != last_value)
> >
> >the DEBOUNCE_AFTER is a #define, so you're effectively just checking if the
> >first value != last value or something here (a double negation like this
> >makes my brain go fuzzy). I actually had to write this out, it reads as:
> >    if (false != (first_value != last_value))
> >which may be the most confusing way to write this condition :)
> >and again, you have a #define but then use the readable name of it. Either
> >you use a boolean and make the code so that it's self-explanatory or you use
> >an enum.
> 
> Re-written in a more understandable way.
> 
> >> ...
> >> +		int code = dispatch->debounce.button_code;
> >
> >empty line after declarations please, and watch out for alignments and
> >function arguments (one per line where multiple lines are needed). See
> >CODING_STYLE.
> 
> Fixed.
> 
> >> ...
> >> +		evdev_log_debug(device, "debounce ignore %u\n", last_value);
> >
> >In regards to debug messages: these need to be prefixed so we can grep for
> >it (the touchpad code e.g. uses things like "pressure:", "thumb state",
> >etc.) But I'd say for the debouncing code only ever log when anything kicks
> >in and we're filtering or otherwise modifying an event. Drop the rest so it
> >doesn't get too confusing and it stands out more when something actually
> >happens.
> >
> >Also note that the debug messages will be seen by users searching for a bug
> >so they need to be good readable. 'debounce commit 1' doesn't explain much.
> 
> Dropped all messages that were not there before.
> 
> >Finally here: value is a signed integer in the event and should be kept at
> >that, even if we only ever expect 0/1 as values.
> 
> Fixed. That removes the "int to bool mayhem"
> 
> >> ...
> >> +	if (dispatch->debounce.cancel_noise) {
> >> +	/* Flush if a button is pressed while holding back another one */
> >
> >indentation, one tab in please.
> 
> Fixed.
> 
> >> ...
> >> -			evdev_log_info(device,
> >> -				       "Enabling button debouncing, "
> >> -				       "see %sbutton_debouncing.html for details\n",
> >> -				       HTTP_DOC_LINK);
> >
> >you should really log this the first time the debouncing kicks in so there's
> >some reference in the logs that we're messing with button events now.
> 
> This algorithm was intended to be user configurable once the DEBOUNCE_AFTER
> macro was converted to a parameter, so, there was no need to enable it
> dynamically and printing this message.
> Personally I would prefer it as a configuration parameter, but not a
> deal-breaker. Because you prefer it automagically enabled, the new version
> restores the dynamic behaviour and this message.
> 
> >> ...
> >> +		evdev_log_debug(device, "debounce noise  %u %8lu\n",
> >> +			e->value, time - dispatch->debounce.button_time);
> >
> >use us2ms() here, no-one cares about microseconds granularity here. That
> >also means you don't have to use PRIu64, %lu isn't reliably a uint64.
> 
> Fixed. (removed)
> 
> >> ...
> >> +	libinput_timer_set(&dispatch->debounce.timer, time + DEBOUNCE_TIME);
> >> +	return DEBOUNCE_AFTER;
> >
> >empty line before return, goes for all of the hunks in this patch.
> >same with an empty line after a } to close the block.
> 
> Fixed.
> 
> >Cheers,
> >   Peter
> 
> Thanks for the review!
> 
> Regards,
>   Vicente.