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.
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
