[PATCH 2/2] touchpad: don't apply tap config until all fingers are up
Peter Hutterer
peter.hutterer at who-t.net
Sun May 3 23:41:31 PDT 2015
On Fri, May 01, 2015 at 10:19:59AM +0200, Hans de Goede wrote:
> Hi,
>
> On 01-05-15 04:48, Peter Hutterer wrote:
> >On Thu, Apr 30, 2015 at 10:01:55AM +0200, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 30-04-15 08:00, Peter Hutterer wrote:
> >>>If tapping is enabled while at least one finger is down, we underrun
> >>>tp->tap.tap_finger_count on touch release. Avoid this by only switching
> >>>tap config whenever there are no fingers down.
> >>
> >>The immediate applying of tapping enabled changes is deliberate because
> >>we also go through this code path when suspending tapping on trackpoint
> >>activity and then we want to enable tapping directly even though a
> >>finger may be resting on the touchpad (think top soft buttons).
> >
> >is this much of an issue though? afaict the only drawback is that we require
> >the user to lift all fingers once tapping is enabled to allow tapping. I
> >don't think that's a problem.
> >
> >>We could delay the enabling and not the disabling but that seems complex.
> >>
> >>I think that the best solution is actually to completely remove the
> >>tap.tap_finger_count variable, it is only used in one place:
> >>
> >>
> >>static void
> >>tp_tap_dead_handle_event(struct tp_dispatch *tp,
> >> struct tp_touch *t,
> >> enum tap_event event,
> >> uint64_t time)
> >>{
> >>
> >> switch (event) {
> >> case TAP_EVENT_RELEASE:
> >> if (tp->tap.tap_finger_count == 0)
> >> tp->tap.state = TAP_STATE_IDLE;
> >> break;
> >>
> >>
> >>We can simply replace that with:
> >>
> >>static void
> >>tp_tap_dead_handle_event(struct tp_dispatch *tp,
> >> struct tp_touch *t,
> >> enum tap_event event,
> >> uint64_t time)
> >>{
> >>
> >> switch (event) {
> >> case TAP_EVENT_RELEASE:
> >> if (tp->nfingers_down == 0)
> >> tp->tap.state = TAP_STATE_IDLE;
> >> break;
> >>
> >>And completely remove tap.tap_finger_count. As a bonus this new
> >>code is also consistent with how we set tp->tap.state on
> >>enable/disable.
> >
> >right, the problem is that nfingers_down is not useful for us here. this
> >variable was only introduced recently, from 591a41f:
> >"tp->nfingers_down gives us the current state of the touchpad but in
> >the case of the tapping state we need the touchpoints separately. If all
> >touchpoints end in the same SYN_REPORT frame, tp->nfingers_down is 0 when we
> >handle the touch releases."
>
> I see, so if we use tp->nfingers_down then the first released touch would
> move us to idle, and the next one would trigger:
>
> static void
> tp_tap_idle_handle_event(struct tp_dispatch *tp,
> struct tp_touch *t,
> enum tap_event event, uint64_t time)
> {
> struct libinput *libinput = tp->device->base.seat->libinput;
>
> switch (event) {
> ...
> case TAP_EVENT_RELEASE:
> case TAP_EVENT_MOTION:
> log_bug_libinput(libinput,
> "invalid tap event, no fingers are down\n");
> break;
>
> We can easily fix that though by simply ignoring release events in idle.
>
> This way we can remove finger_count simplifying the code, where as your
> patch just adds additional code.
I just tried this and there's a slight problem with it: it messes up the
state machine. if you move from DEAD to IDLE on the first release there may
still be fingers down. That's easy to handle in IDLE but we'd then have to
ignore release events in every other state as well. IMO that makes things
less reliable and harder to real find issues. so back to the drawing board,
I guess.
Cheers,
Peter
> >>>Reported-by: Rui Matos <tiagomatos at gmail.com>
> >>>Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >>>---
> >>> src/evdev-mt-touchpad-tap.c | 47 +++++++++++++++++++++++++++++++++++----------
> >>> src/evdev-mt-touchpad.h | 1 +
> >>> test/touchpad.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 85 insertions(+), 10 deletions(-)
> >>>
> >>>diff --git a/src/evdev-mt-touchpad-tap.c b/src/evdev-mt-touchpad-tap.c
> >>>index bd58bb1..0962899 100644
> >>>--- a/src/evdev-mt-touchpad-tap.c
> >>>+++ b/src/evdev-mt-touchpad-tap.c
> >>>@@ -622,12 +622,33 @@ tp_tap_enabled(struct tp_dispatch *tp)
> >>> return tp->tap.enabled && !tp->tap.suspended;
> >>> }
> >>>
> >>>+static void
> >>>+tp_tap_apply_config(struct tp_dispatch *tp)
> >>>+{
> >>>+ struct tp_touch *t;
> >>>+ bool finger_down = false;
> >>>+
> >>>+ if (tp->tap.enabled == tp->tap.want_enabled)
> >>>+ return;
> >>>+
> >>>+ tp_for_each_touch(tp, t) {
> >>>+ if (t->state != TOUCH_NONE) {
> >>>+ finger_down = true;
> >>>+ break;
> >>>+ }
> >>>+ }
> >>>+
> >>>+ if (!finger_down)
> >>>+ tp->tap.enabled = tp->tap.want_enabled;
> >>>+}
> >>>+
> >>> int
> >>> tp_tap_handle_state(struct tp_dispatch *tp, uint64_t time)
> >>> {
> >>> struct tp_touch *t;
> >>> int filter_motion = 0;
> >>>
> >>>+ tp_tap_apply_config(tp);
> >>> if (!tp_tap_enabled(tp))
> >>> return 0;
> >>>
> >>>@@ -647,6 +668,7 @@ tp_tap_handle_state(struct tp_dispatch *tp, uint64_t time)
> >>>
> >>> if (t->state == TOUCH_BEGIN) {
> >>> tp->tap.tap_finger_count++;
> >>>+ assert(tp->tap.tap_finger_count > 0);
> >>> t->tap.state = TAP_TOUCH_STATE_TOUCH;
> >>> t->tap.initial = t->point;
> >>> tp_tap_handle_event(tp, t, TAP_EVENT_TOUCH, time);
> >>>@@ -659,6 +681,7 @@ tp_tap_handle_state(struct tp_dispatch *tp, uint64_t time)
> >>> tp_tap_handle_event(tp, t, TAP_EVENT_MOTION, time);
> >>>
> >>> } else if (t->state == TOUCH_END) {
> >>>+ assert(tp->tap.tap_finger_count > 0);
> >>> tp->tap.tap_finger_count--;
> >>> tp_tap_handle_event(tp, t, TAP_EVENT_RELEASE, time);
> >>> t->tap.state = TAP_TOUCH_STATE_IDLE;
> >>>@@ -719,12 +742,14 @@ tp_tap_handle_timeout(uint64_t time, void *data)
> >>> }
> >>>
> >>> static void
> >>>-tp_tap_enabled_update(struct tp_dispatch *tp, bool suspended, bool enabled, uint64_t time)
> >>>+tp_tap_enabled_update(struct tp_dispatch *tp,
> >>>+ bool suspended,
> >>>+ uint64_t time)
> >>> {
> >>> bool was_enabled = tp_tap_enabled(tp);
> >>>
> >>>+ tp_tap_apply_config(tp);
> >>> tp->tap.suspended = suspended;
> >>>- tp->tap.enabled = enabled;
> >>>
> >>> if (tp_tap_enabled(tp) == was_enabled)
> >>> return;
> >>>@@ -758,9 +783,8 @@ tp_tap_config_set_enabled(struct libinput_device *device,
> >>> struct tp_dispatch *tp = NULL;
> >>>
> >>> tp = container_of(dispatch, tp, base);
> >>>- tp_tap_enabled_update(tp, tp->tap.suspended,
> >>>- (enabled == LIBINPUT_CONFIG_TAP_ENABLED),
> >>>- libinput_now(device->seat->libinput));
> >>>+ tp->tap.want_enabled = (enabled == LIBINPUT_CONFIG_TAP_ENABLED),
> >>>+ tp_tap_apply_config(tp);
> >>>
> >>> return LIBINPUT_CONFIG_STATUS_SUCCESS;
> >>> }
> >>>@@ -774,8 +798,9 @@ tp_tap_config_is_enabled(struct libinput_device *device)
> >>> dispatch = ((struct evdev_device *) device)->dispatch;
> >>> tp = container_of(dispatch, tp, base);
> >>>
> >>>- return tp->tap.enabled ? LIBINPUT_CONFIG_TAP_ENABLED :
> >>>- LIBINPUT_CONFIG_TAP_DISABLED;
> >>>+ return tp->tap.want_enabled ?
> >>>+ LIBINPUT_CONFIG_TAP_ENABLED :
> >>>+ LIBINPUT_CONFIG_TAP_DISABLED;
> >>> }
> >>>
> >>> static enum libinput_config_tap_state
> >>>@@ -818,7 +843,9 @@ tp_init_tap(struct tp_dispatch *tp)
> >>> tp->device->base.config.tap = &tp->tap.config;
> >>>
> >>> tp->tap.state = TAP_STATE_IDLE;
> >>>- tp->tap.enabled = tp_tap_default(tp->device);
> >>>+ tp->tap.want_enabled = tp_tap_default(tp->device);
> >>>+ tp->tap.enabled = !tp->tap.want_enabled;
> >>>+ tp_tap_apply_config(tp);
> >>>
> >>> libinput_timer_init(&tp->tap.timer,
> >>> tp->device->base.seat->libinput,
> >>>@@ -849,13 +876,13 @@ tp_release_all_taps(struct tp_dispatch *tp, uint64_t now)
> >>> void
> >>> tp_tap_suspend(struct tp_dispatch *tp, uint64_t time)
> >>> {
> >>>- tp_tap_enabled_update(tp, true, tp->tap.enabled, time);
> >>>+ tp_tap_enabled_update(tp, true, time);
> >>> }
> >>>
> >>> void
> >>> tp_tap_resume(struct tp_dispatch *tp, uint64_t time)
> >>> {
> >>>- tp_tap_enabled_update(tp, false, tp->tap.enabled, time);
> >>>+ tp_tap_enabled_update(tp, false, time);
> >>> }
> >>>
> >>> bool
> >>>diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> >>>index 97b17cd..7f90f2b 100644
> >>>--- a/src/evdev-mt-touchpad.h
> >>>+++ b/src/evdev-mt-touchpad.h
> >>>@@ -253,6 +253,7 @@ struct tp_dispatch {
> >>> struct {
> >>> struct libinput_device_config_tap config;
> >>> bool enabled;
> >>>+ bool want_enabled;
> >>> bool suspended;
> >>> struct libinput_timer timer;
> >>> enum tp_tap_state state;
> >>>diff --git a/test/touchpad.c b/test/touchpad.c
> >>>index 830e589..c25a5d0 100644
> >>>--- a/test/touchpad.c
> >>>+++ b/test/touchpad.c
> >>>@@ -2944,6 +2944,51 @@ START_TEST(touchpad_tap_invalid)
> >>> }
> >>> END_TEST
> >>>
> >>>+START_TEST(touchpad_tap_enable_during_tap)
> >>>+{
> >>>+ struct litest_device *dev = litest_current_device();
> >>>+ struct libinput *li = dev->libinput;
> >>>+
> >>>+ libinput_device_config_tap_set_enabled(dev->libinput_device,
> >>>+ LIBINPUT_CONFIG_TAP_DISABLED);
> >>>+ litest_drain_events(li);
> >>>+
> >>>+ litest_touch_down(dev, 0, 50, 50);
> >>>+ libinput_dispatch(li);
> >>>+ libinput_device_config_tap_set_enabled(dev->libinput_device,
> >>>+ LIBINPUT_CONFIG_TAP_ENABLED);
> >>>+ litest_touch_up(dev, 0);
> >>>+ libinput_dispatch(li);
> >>>+
> >>>+ litest_assert_empty_queue(li);
> >>>+}
> >>>+END_TEST
> >>>+
> >>>+START_TEST(touchpad_tap_disable_during_tap)
> >>>+{
> >>>+ struct litest_device *dev = litest_current_device();
> >>>+ struct libinput *li = dev->libinput;
> >>>+
> >>>+ libinput_device_config_tap_set_enabled(dev->libinput_device,
> >>>+ LIBINPUT_CONFIG_TAP_ENABLED);
> >>>+ litest_drain_events(li);
> >>>+
> >>>+ litest_touch_down(dev, 0, 50, 50);
> >>>+ libinput_dispatch(li);
> >>>+ libinput_device_config_tap_set_enabled(dev->libinput_device,
> >>>+ LIBINPUT_CONFIG_TAP_DISABLED);
> >>>+ litest_touch_up(dev, 0);
> >>>+ libinput_dispatch(li);
> >>>+
> >>>+ litest_assert_button_event(li,
> >>>+ BTN_LEFT,
> >>>+ LIBINPUT_BUTTON_STATE_PRESSED);
> >>>+ litest_assert_button_event(li,
> >>>+ BTN_LEFT,
> >>>+ LIBINPUT_BUTTON_STATE_RELEASED);
> >>>+}
> >>>+END_TEST
> >>>+
> >>> static int
> >>> touchpad_has_palm_detect_size(struct litest_device *dev)
> >>> {
> >>>@@ -4163,6 +4208,8 @@ int main(int argc, char **argv) {
> >>> litest_add("touchpad:tap", touchpad_tap_invalid, LITEST_TOUCHPAD, LITEST_ANY);
> >>> litest_add("touchpad:tap", touchpad_tap_is_available, LITEST_TOUCHPAD, LITEST_ANY);
> >>> litest_add("touchpad:tap", touchpad_tap_is_not_available, LITEST_ANY, LITEST_TOUCHPAD);
> >>>+ litest_add("touchpad:tap", touchpad_tap_enable_during_tap, LITEST_TOUCHPAD, LITEST_ANY);
> >>>+ litest_add("touchpad:tap", touchpad_tap_disable_during_tap, LITEST_TOUCHPAD, LITEST_ANY);
> >>>
> >>> litest_add("touchpad:tap", clickpad_1fg_tap_click, LITEST_CLICKPAD, LITEST_ANY);
> >>> litest_add("touchpad:tap", clickpad_2fg_tap_click, LITEST_CLICKPAD, LITEST_SINGLE_TOUCH|LITEST_APPLE_CLICKPAD);
> >>>
More information about the wayland-devel
mailing list