[PATCH libinput 1/1] Fix debouncing algorithm
Peter Hutterer
peter.hutterer at who-t.net
Tue Nov 14 04:22:02 UTC 2017
disclaimer: I was halfway through this, started fixing some comments myself
and got stuck in "this is too complicated" land.
See
https://lists.freedesktop.org/archives/wayland-devel/2017-November/035782.html
for the most recent set of patches.
On Thu, Nov 02, 2017 at 12:34:02AM +0100, Vicente Bergas wrote:
> Bouncing can occur when a button toggles, both press and release, and can
> bounce multiple times for each toggling.
>
> The algorithm has two states: cancel_noise==false and cancel_noise==true.
> When not canceling noise, the default state, any event received is
> inmediately committed 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, no delays. This is the
> default mode.
>
> 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.
>
> When an spurious action is detected, the algorithm switches to an alternate
> mode in which events are committed after noise suppression. This mode is
> well suited for both misbehaviours of the button. The drawback is that
> there is a delay, 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!
>
> Signed-off-by: Vicente Bergas <vicencb at gmail.com>
> ---
> doc/button_debouncing.dox | 16 +++--
> src/evdev-fallback.c | 175 +++++++++++++++++-----------------------------
> src/evdev.h | 19 -----
> test/litest.c | 2 +-
> test/test-pointer.c | 33 ++-------
> 5 files changed, 82 insertions(+), 163 deletions(-)
>
> diff --git a/doc/button_debouncing.dox b/doc/button_debouncing.dox
> index fd52986..fd7a8e6 100644
> --- a/doc/button_debouncing.dox
> +++ b/doc/button_debouncing.dox
> @@ -9,12 +9,16 @@ though the user only pressed or clicked the button once. This effect can be
> counteracted by "debouncing" the buttons, usually by ignoring erroneous
> events.
>
> -libinput has a built-in debouncing for hardware defects. This feature is
> -available for all button-base devices but not active by default. When
> -libinput detects a faulty button on a device, debouncing is enabled and a
> -warning is printed to the log. Subsequent button events are handled
> -correctly in that bouncing button events are ignored, a user should thus see
> -the expected behavior.
> +libinput has a built-in debouncing for hardware defects. The debouncing
> +algorithm can operate in two modes. The first mode is the default, it just
> +filters out the bouncing after a press or a release. It has no adverse
> +effects like inserting a delay.
> +
> +Another related issue with buttons is that while pressed they can send
> +spurious release events. If libinput detects this kind of fault then it
> +enables the second mode of the debouncing algorithm and a warning is printed
> +to the log. Subsequent spurious events are handled correctly. This second
> +mode inserts a few milliseconds delay.
>
> Note that libinput's debouncing intended to correct hardware damage or
> substandard hardware. Debouncing is also used as an accessibility feature
> diff --git a/src/evdev-fallback.c b/src/evdev-fallback.c
> index 4da07e7..4436c46 100644
> --- a/src/evdev-fallback.c
> +++ b/src/evdev-fallback.c
> @@ -30,7 +30,13 @@
>
> #include "evdev.h"
>
> -#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?
> +
> +enum debounce_mode {
> + /* The debouncing algorithm handles: */
> + DEBOUNCE_MODE_BOUNCE, /* just bounces */
> + DEBOUNCE_MODE_BOUNCE_AND_SPURIOUS, /* bounces and spurious events */
ah, the enum boolean trap :)
you're not initializing dispatch->debounce.mode anywhere and you're still
relying on it being 0. That's not ideal, I try to avoid this by having enums
start with a nonzero value unless that's part of the feature. And having
asserts to catchin out-of-range values during development.
> +};
>
> struct fallback_dispatch {
> struct evdev_dispatch base;
> @@ -89,9 +95,12 @@ struct fallback_dispatch {
> bool ignore_events;
>
> struct {
> - enum evdev_debounce_state state;
> + bool cancel_noise;
> + int first_value;
> + int last_value;
> + enum debounce_mode debounce_mode;
s/debounce_mode/mode/ because it's only ever used as member of the
'debounce' field anyway.
> unsigned int button_code;
> - uint64_t button_up_time;
> + uint64_t button_time;
> struct libinput_timer timer;
> } debounce;
>
> @@ -639,22 +648,47 @@ static inline void
> fallback_flush_debounce(struct fallback_dispatch *dispatch,
> struct evdev_device *device)
> {
> - int code = dispatch->debounce.button_code;
> - int button;
> + int last_value = dispatch->debounce.last_value;
> + bool commit_last_value;
>
> - if (dispatch->debounce.state != DEBOUNCE_ACTIVE)
> + if (!dispatch->debounce.cancel_noise)
> return;
>
> - if (hw_is_key_down(dispatch, code)) {
> - button = evdev_to_left_handed(device, code);
> + dispatch->debounce.cancel_noise = false;
> +
> + if (dispatch->debounce.debounce_mode ==
> + DEBOUNCE_MODE_BOUNCE_AND_SPURIOUS)
> + /* first_value has not been commited.
typo: committed
> + * Commit last_value only if it is the same.
> + */
> + commit_last_value =
> + dispatch->debounce.first_value == last_value;
> + else
> + /* first_value has already been commited.
typo: committed
> + * Commit last_value only if it has changed.
> + */
> + commit_last_value =
> + dispatch->debounce.first_value != last_value;
just a style nitpick: even though it's just one line for the compiler, use
{} here to make it visually easier to group
> +
> + if (commit_last_value) {
> + int code = dispatch->debounce.button_code;
> +
> 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);
swap the notify and hw_set_key_down order please, to make sure we have the
same order as the main event processing.
also, I wouldn't mind an empty line here to have the logical group above
(notify + hw update) and a logical group below (debounce mode switching).
> + 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
- 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.
- if I release and press within the timeout, spurious mode enables -> good,
that's what it's supposed to do
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.
> }
> -
> - dispatch->debounce.state = DEBOUNCE_ON;
> }
>
> static void
> @@ -668,113 +702,36 @@ 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
> + */
> + if (e->code != dispatch->debounce.button_code) {
> libinput_timer_cancel(&dispatch->debounce.timer);
> fallback_flush_debounce(dispatch, device);
> + } else {
> + dispatch->debounce.last_value = e->value;
> + return true;
> }
> - 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);
> - 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");
> }
>
> - return filter;
> -}
> -
> -static bool
> -fallback_filter_debounce_release(struct fallback_dispatch *dispatch,
> - struct input_event *e,
> - uint64_t time)
> -{
> - bool filter = false;
> -
> + 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;
> + 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);
> + 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
>
> - 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;
> + return false;
> }
>
> 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;
> diff --git a/test/litest.c b/test/litest.c
> index 021e19f..baa588f 100644
> --- a/test/litest.c
> +++ b/test/litest.c
> @@ -3167,7 +3167,7 @@ litest_timeout_tapndrag(void)
> void
> litest_timeout_debounce(void)
> {
> - msleep(15);
> + msleep(28);
> }
>
> void
> diff --git a/test/test-pointer.c b/test/test-pointer.c
> index 25ddc76..f784fab 100644
> --- a/test/test-pointer.c
> +++ b/test/test-pointer.c
> @@ -2287,12 +2287,7 @@ START_TEST(debounce_no_debounce_for_otherbutton)
> BTN_LEFT,
> LIBINPUT_BUTTON_STATE_RELEASED);
>
> - litest_assert_button_event(li,
> - BTN_RIGHT,
> - LIBINPUT_BUTTON_STATE_PRESSED);
> - litest_assert_button_event(li,
> - BTN_RIGHT,
> - LIBINPUT_BUTTON_STATE_RELEASED);
> + litest_timeout_debounce();
>
> litest_assert_empty_queue(li);
> }
> @@ -2323,8 +2318,7 @@ START_TEST(debounce_cancel_debounce_otherbutton)
> litest_event(dev, EV_SYN, SYN_REPORT, 0);
> litest_event(dev, EV_KEY, BTN_LEFT, 1);
> litest_event(dev, EV_SYN, SYN_REPORT, 0);
> - /* release is now held back, press was ignored,
> - * other button should flush the release */
> + /* press is now held back, other button should flush */
> litest_event(dev, EV_KEY, BTN_RIGHT, 1);
> litest_event(dev, EV_SYN, SYN_REPORT, 0);
> litest_event(dev, EV_KEY, BTN_RIGHT, 0);
> @@ -2335,16 +2329,8 @@ START_TEST(debounce_cancel_debounce_otherbutton)
> litest_assert_button_event(li,
> BTN_LEFT,
> LIBINPUT_BUTTON_STATE_PRESSED);
> - litest_assert_button_event(li,
> - BTN_LEFT,
> - LIBINPUT_BUTTON_STATE_RELEASED);
>
> - litest_assert_button_event(li,
> - BTN_RIGHT,
> - LIBINPUT_BUTTON_STATE_PRESSED);
> - litest_assert_button_event(li,
> - BTN_RIGHT,
> - LIBINPUT_BUTTON_STATE_RELEASED);
> + litest_timeout_debounce();
>
> litest_assert_empty_queue(li);
> }
> @@ -2374,8 +2360,7 @@ START_TEST(debounce_switch_to_otherbutton)
> litest_event(dev, EV_SYN, SYN_REPORT, 0);
> litest_event(dev, EV_KEY, BTN_LEFT, 1);
> litest_event(dev, EV_SYN, SYN_REPORT, 0);
> - /* release is now held back, press was ignored,
> - * other button should flush the release */
> + /* press is now held back, other button should flush */
> litest_event(dev, EV_KEY, BTN_RIGHT, 1);
> litest_event(dev, EV_SYN, SYN_REPORT, 0);
> litest_event(dev, EV_KEY, BTN_RIGHT, 0);
> @@ -2392,16 +2377,8 @@ START_TEST(debounce_switch_to_otherbutton)
> litest_assert_button_event(li,
> BTN_LEFT,
> LIBINPUT_BUTTON_STATE_PRESSED);
> - litest_assert_button_event(li,
> - BTN_LEFT,
> - LIBINPUT_BUTTON_STATE_RELEASED);
>
> - litest_assert_button_event(li,
> - BTN_RIGHT,
> - LIBINPUT_BUTTON_STATE_PRESSED);
> - litest_assert_button_event(li,
> - BTN_RIGHT,
> - LIBINPUT_BUTTON_STATE_RELEASED);
> + litest_timeout_debounce();
>
> litest_assert_empty_queue(li);
> }
> --
> 2.15.0
>
More information about the wayland-devel
mailing list