[PATCH libinput 0/1] Fix debouncing algorithm

[Vicente Bergas]

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.

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

>> ...
>> +/* 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.