[PATCH libinput] Fix debouncing algorithm
Peter Hutterer
peter.hutterer at who-t.net
Wed Nov 1 02:56:50 UTC 2017
Hi Vicente
sorry about the delay, too many things on right now.
On Wed, Oct 25, 2017 at 09:12:53PM +0200, Vicente Bergas wrote:
> The algorithm assumed that bouncing can only occur once when pressing a
> button.
> This approach did not work with my mouse because that is not 100%
> correct. Bouncing can occur when a button toggles, both press and release
> and can bounce multiple times. Mine bounces mostly on the release event.
> Also, the timeout period was missing more than half of the bounces.
>
> This patch changes the algorithm so that there is no difference between
> press and release events, they are treated the same. The algorithm has
> two states: cancel_noise==false and cancel_noise==true.
> When not canceling noise, the default state, any event received is
> inmediately processed and state is changed to cancel_noise=true.
> The canceling noise state has a timeout. During the limited time of this
> state any event received (from the same button) is considered noise and
> the last value of the event is saved.
> When the timeout expires the state changes back to not canceling noise
> and if the last saved event value is different than the committed one,
> then a new event is generated to restore equilibrium.
>
> Note that all events are inmediately committed. There is no need to
> dynamically enable that feature because of the adverse effects of the
> fixed delay from the previous algorithm.
I like it. I went through the various cases I could think of an except for
the spurious actions (below) it does the right thing afaict. So an ack from
me for the approach, but the patch needs a lot more work, sorry.
> A faulty mouse can have bouncing buttons. A worse mouse can also generate
> spurious actions. A spurious action is when the button is in steady state
> and for a short while it toggles state. A bounce generates an odd number
> of events in fast sequence, eg. press-release-press. A spurious action
> generates an even number of events in fast sequence, eg. release-press.
>
> The new algorithm is well suited for debouncing because events are
> inmediately committed. But it is not well suited for filtering out
> spurious actions.
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.
> 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.
>
> The timeout period has been set to 25ms. I have observed that the noisy
> period after a button toggle can vary from 8ms to around 30ms, the mean
> value at around 20ms. Although those numbers come from one single sample
> mouse, 25ms seems a reasonable value, that is 20 clicks per second!
>
> It would be interesting to have the DEBOUNCE_TIME and DEBOUNCE_AFTER as
> configurable parameters, but that is not part of this patch.
>
> Signed-off-by: Vicente Bergas <vicencb at gmail.com>
> ---
> 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.
> diff --git a/src/evdev-fallback.c b/src/evdev-fallback.c
> index 13e130a..44cb900 100644
> --- a/src/evdev-fallback.c
> +++ b/src/evdev-fallback.c
> @@ -30,7 +30,16 @@
>
> #include "evdev.h"
>
> -#define DEBOUNCE_TIME ms2us(12)
> +/* Time window after an event to ignore extra events */
> +#define DEBOUNCE_TIME ms2us(25)
> +
> +/* 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.
> struct fallback_dispatch {
> struct evdev_dispatch base;
> @@ -89,9 +98,11 @@ struct fallback_dispatch {
> bool ignore_events;
>
> 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.
> - int code = dispatch->debounce.button_code;
> - int button;
> -
> - if (dispatch->debounce.state != DEBOUNCE_ACTIVE)
> - return;
> + bool last_value = dispatch->debounce.last_value;
whoah, 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.
>
> - if (hw_is_key_down(dispatch, code)) {
> - button = evdev_to_left_handed(device, code);
> + 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.
> + ) {
> + 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.
> evdev_pointer_notify_physical_button(device,
> - dispatch->debounce.button_up_time,
> - button,
> - LIBINPUT_BUTTON_STATE_RELEASED);
> - hw_set_key_down(dispatch, code, 0);
> + dispatch->debounce.button_time,
> + evdev_to_left_handed(device, code),
> + last_value ? LIBINPUT_BUTTON_STATE_PRESSED :
> + LIBINPUT_BUTTON_STATE_RELEASED);
> + hw_set_key_down(dispatch, code, last_value);
> + evdev_log_debug(device, "debounce commit %u\n", last_value);
> + } else {
> + 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.
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.
> }
>
> - dispatch->debounce.state = DEBOUNCE_ON;
> + dispatch->debounce.cancel_noise = false;
> }
>
> static void
> @@ -668,113 +681,32 @@ fallback_debounce_timeout(uint64_t now, void *data)
> }
>
> static bool
> -fallback_filter_debounce_press(struct fallback_dispatch *dispatch,
> +fallback_filter_debounce(struct fallback_dispatch *dispatch,
> struct evdev_device *device,
> struct input_event *e,
> uint64_t time)
> {
> - bool filter = false;
> - uint64_t tdelta;
> -
> - /* If other button is pressed while we're holding back the release,
> - * flush the pending release (if any) and continue. We don't handle
> - * this situation, if you have a mouse that needs per-button
> - * debouncing, consider writing to santa for a new mouse.
> - */
> - if (e->code != dispatch->debounce.button_code) {
> - if (dispatch->debounce.state == DEBOUNCE_ACTIVE) {
> + if (dispatch->debounce.cancel_noise) {
> + /* Flush if a button is pressed while holding back another one */
indentation, one tab in please.
> + if (e->code != dispatch->debounce.button_code) {
> libinput_timer_cancel(&dispatch->debounce.timer);
> + evdev_log_debug(device, "debounce reset\n");
> fallback_flush_debounce(dispatch, device);
> + return false;
> }
> - return false;
> - }
> -
> - tdelta = time - dispatch->debounce.button_up_time;
> - assert((int64_t)tdelta >= 0);
> -
> - if (tdelta < DEBOUNCE_TIME) {
> - switch (dispatch->debounce.state) {
> - case DEBOUNCE_INIT:
> - /* This is the first time we debounce, enable proper debouncing
> - from now on but filter this press event */
> - filter = true;
> - 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.
> - dispatch->debounce.state = DEBOUNCE_NEEDED;
> - break;
> - case DEBOUNCE_NEEDED:
> - case DEBOUNCE_ON:
> - break;
> - /* If a release event is pending and, filter press
> - * events until we flushed the release */
> - case DEBOUNCE_ACTIVE:
> - filter = true;
> - break;
> - }
> - } else if (dispatch->debounce.state == DEBOUNCE_ACTIVE) {
> - /* call libinput_dispatch() more frequently */
> - evdev_log_bug_client(device,
> - "Debouncing still active past timeout\n");
> + 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.
> + dispatch->debounce.last_value = e->value;
> + return true;
> }
> -
> - return filter;
> -}
> -
> -static bool
> -fallback_filter_debounce_release(struct fallback_dispatch *dispatch,
> - struct input_event *e,
> - uint64_t time)
> -{
> - bool filter = false;
> -
> + evdev_log_debug(device, "debounce start %u\n", e->value);
> + dispatch->debounce.cancel_noise = true;
> dispatch->debounce.button_code = e->code;
> - dispatch->debounce.button_up_time = time;
> -
> - switch (dispatch->debounce.state) {
> - case DEBOUNCE_INIT:
> - break;
> - case DEBOUNCE_NEEDED:
> - filter = true;
> - dispatch->debounce.state = DEBOUNCE_ON;
> - break;
> - case DEBOUNCE_ON:
> - libinput_timer_set(&dispatch->debounce.timer,
> - time + DEBOUNCE_TIME);
> - filter = true;
> - dispatch->debounce.state = DEBOUNCE_ACTIVE;
> - break;
> - case DEBOUNCE_ACTIVE:
> - filter = true;
> - break;
> - }
> -
> - return filter;
> -}
> -
> -static bool
> -fallback_filter_debounce(struct fallback_dispatch *dispatch,
> - struct evdev_device *device,
> - struct input_event *e, uint64_t time)
> -{
> - bool filter = false;
> -
> - /* Behavior: we monitor the time deltas between release and press
> - * events. Proper debouncing is disabled on init, but the first
> - * time we see a bouncing press event we enable it.
> - *
> - * The first bounced event is simply discarded, which ends up in the
> - * button being released sooner than it should be. Subsequent button
> - * presses are timer-based and thus released a bit later because we
> - * then wait for a timeout before we post the release event.
> - */
> - if (e->value)
> - filter = fallback_filter_debounce_press(dispatch, device, e, time);
> - else
> - filter = fallback_filter_debounce_release(dispatch, e, time);
> -
> - return filter;
> + dispatch->debounce.button_time = time;
> + dispatch->debounce.first_value = e->value;
> + dispatch->debounce.last_value = e->value;
> + 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.
Cheers,
Peter
> }
>
> static inline void
> diff --git a/src/evdev.h b/src/evdev.h
> index 4c42f6e..280b561 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -131,25 +131,6 @@ enum evdev_button_scroll_state {
> BUTTONSCROLL_SCROLLING, /* have sent scroll events */
> };
>
> -enum evdev_debounce_state {
> - /**
> - * Initial state, no debounce but monitoring events
> - */
> - DEBOUNCE_INIT,
> - /**
> - * Bounce detected, future events need debouncing
> - */
> - DEBOUNCE_NEEDED,
> - /**
> - * Debounce is enabled, but no event is currently being filtered
> - */
> - DEBOUNCE_ON,
> - /**
> - * Debounce is enabled and we are currently filtering an event
> - */
> - DEBOUNCE_ACTIVE,
> -};
> -
> struct mt_slot {
> int32_t seat_slot;
> struct device_coords point;
> --
> 2.14.3
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list