[PATCH v2 libinput 3/6] touchpad: add a TOUCH_HOVERING state
Hans de Goede
hdegoede at redhat.com
Fri Jan 9 03:35:38 PST 2015
Hi Peter,
On 07-01-15 01:14, Peter Hutterer wrote:
> Some touchpads provide touch information while the finger hovers over the
> touchpad, i.e. before BTN_TOUCH. Add a touch state for those touchpads so we
> can ignore the touches until they actually start.
>
> The approach is now: instead of BEGIN we mark a new touch as HOVERING.
> Use the BTN_TOOL_FINGER, BTN_TOOL_DOUBLETAP information during
> tp_process_state() to mark any touches that are hovering as down or ended.
>
> i.e. provided BTN_TOUCH is down: if BTN_TOOL_FINGER is down, one hovering
> touch gets marked as down, if DOUBLETAP is down, two touches are marked as
> down, etc.
>
> When ending touches, switch them back into HOVERING if the BTN_TOOL_FINGER
> is still set, otherwise end them properly.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=87197
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> changes to v1:
> - rename tp_begin_touch to tp_new_touch
> - add tp_begin_touch to set it to TOUCH_BEGIN
> - change tp_unhover_touches to begin touches left-to-right and end touches
> right-to-left
> - if a touch ended while switching to hover terminate it correctly
> (see tp_post_process_state)
Thanks, I think this version is better, but I still have on concern,
see comments inline.
>
> src/evdev-mt-touchpad-edge-scroll.c | 1 +
> src/evdev-mt-touchpad.c | 113 +++++++++++++++++++++++++++++++-----
> src/evdev-mt-touchpad.h | 1 +
> 3 files changed, 101 insertions(+), 14 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad-edge-scroll.c b/src/evdev-mt-touchpad-edge-scroll.c
> index a4dc093..0e3d648 100644
> --- a/src/evdev-mt-touchpad-edge-scroll.c
> +++ b/src/evdev-mt-touchpad-edge-scroll.c
> @@ -294,6 +294,7 @@ tp_edge_scroll_handle_state(struct tp_dispatch *tp, uint64_t time)
>
> switch (t->state) {
> case TOUCH_NONE:
> + case TOUCH_HOVERING:
> break;
> case TOUCH_BEGIN:
> tp_edge_scroll_handle_event(tp, t, SCROLL_EVENT_TOUCH);
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index 632ad38..a1d2274 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -178,26 +178,48 @@ tp_fake_finger_set(struct tp_dispatch *tp,
> }
>
> static inline void
> -tp_begin_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
> +tp_new_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
> {
> - if (t->state == TOUCH_BEGIN || t->state == TOUCH_UPDATE)
> + if (t->state == TOUCH_BEGIN ||
> + t->state == TOUCH_UPDATE ||
> + t->state == TOUCH_HOVERING)
> return;
>
> + /* we begin the touch as hovering because until BTN_TOUCH happens we
> + * don't know if it's a touch down or not. And BTN_TOUCH may happen
> + * after ABS_MT_TRACKING_ID */
> tp_motion_history_reset(t);
> t->dirty = true;
> + t->state = TOUCH_HOVERING;
> + t->pinned.is_pinned = false;
> + t->millis = time;
> + tp->queued |= TOUCHPAD_EVENT_MOTION;
> +}
> +
> +static inline void
> +tp_begin_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
> +{
> + t->dirty = true;
> t->state = TOUCH_BEGIN;
> - t->pinned.is_pinned = false;
> t->millis = time;
> tp->nfingers_down++;
> assert(tp->nfingers_down >= 1);
> - tp->queued |= TOUCHPAD_EVENT_MOTION;
> }
>
> static inline void
> tp_end_touch(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time)
> {
> - if (t->state == TOUCH_END || t->state == TOUCH_NONE)
> + switch (t->state) {
> + case TOUCH_HOVERING:
> + t->state = TOUCH_NONE;
> + case TOUCH_NONE:
> + case TOUCH_END:
> return;
> + case TOUCH_BEGIN:
> + case TOUCH_UPDATE:
> + break;
> +
> + }
>
> t->dirty = true;
> t->is_pointer = false;
> @@ -260,7 +282,7 @@ tp_process_absolute(struct tp_dispatch *tp,
> break;
> case ABS_MT_TRACKING_ID:
> if (e->value != -1)
> - tp_begin_touch(tp, t, time);
> + tp_new_touch(tp, t, time);
> else
> tp_end_touch(tp, t, time);
> }
> @@ -306,13 +328,10 @@ tp_process_fake_touch(struct tp_dispatch *tp,
> for (i = start; i < tp->ntouches; i++) {
> t = tp_get_touch(tp, i);
> if (i < nfake_touches)
> - tp_begin_touch(tp, t, time);
> + tp_new_touch(tp, t, time);
> else
> tp_end_touch(tp, t, time);
> }
> -
> - /* On mt the actual touch info may arrive after BTN_TOOL_FOO */
> - assert(tp->has_mt || tp->nfingers_down == nfake_touches);
> }
>
> static void
> @@ -327,6 +346,7 @@ tp_process_key(struct tp_dispatch *tp,
> tp_process_button(tp, e, time);
> break;
> case BTN_TOUCH:
> + case BTN_TOOL_FINGER:
> case BTN_TOOL_DOUBLETAP:
> case BTN_TOOL_TRIPLETAP:
> case BTN_TOOL_QUADTAP:
> @@ -580,12 +600,69 @@ tp_remove_scroll(struct tp_dispatch *tp)
> }
>
> static void
> +tp_unhover_touches(struct tp_dispatch *tp, uint64_t time)
> +{
> + struct tp_touch *t;
> + unsigned int nfake_touches;
> + int i;
> +
> + if (!tp->fake_touches && !tp->nfingers_down)
> + return;
> +
> + nfake_touches = tp_fake_finger_count(tp);
> + if (tp->nfingers_down == nfake_touches &&
> + ((tp->nfingers_down == 0 && !tp_fake_finger_is_touching(tp)) ||
> + (tp->nfingers_down > 0 && tp_fake_finger_is_touching(tp))))
> + return;
> +
> + /* if BTN_TOUCH is set and we have less fingers down than fake
> + * touches, switch each hovering touch to BEGIN
> + * until nfingers_down matches nfake_touches
> + */
> + if (tp_fake_finger_is_touching(tp) &&
> + tp->nfingers_down < nfake_touches) {
> + for (i = 0; i < (int)tp->ntouches; i++) {
> + t = tp_get_touch(tp, i);
> +
> + if (t->state == TOUCH_HOVERING) {
> + tp_begin_touch(tp, t, time);
> +
> + if (tp->nfingers_down >= nfake_touches)
> + break;
> + }
> + }
> + }
> +
> + /* if BTN_TOUCH is unset end all touches, we're hovering now. If we
> + * have too many touches also end some of them. This is done in
> + * reverse order.
> + */
> + if (tp->nfingers_down > nfake_touches ||
> + !tp_fake_finger_is_touching(tp)) {
> + for (i = tp->ntouches - 1; i >= 0; i--) {
> + t = tp_get_touch(tp, i);
> +
> + if (t->state == TOUCH_HOVERING)
> + continue;
> +
> + tp_end_touch(tp, t, time);
> +
> + if (tp_fake_finger_is_touching(tp) &&
> + tp->nfingers_down == nfake_touches)
> + break;
> + }
> + }
> +}
> +
> +static void
> tp_process_state(struct tp_dispatch *tp, uint64_t time)
> {
> struct tp_touch *t;
> struct tp_touch *first = tp_get_touch(tp, 0);
> unsigned int i;
>
> + tp_unhover_touches(tp, time);
> +
> for (i = 0; i < tp->ntouches; i++) {
> t = tp_get_touch(tp, i);
>
> @@ -629,15 +706,23 @@ static void
> tp_post_process_state(struct tp_dispatch *tp, uint64_t time)
> {
> struct tp_touch *t;
> + unsigned int i;
> +
> + for (i = 0; i < tp->ntouches; i++) {
> + t = tp_get_touch(tp, i);
>
> - tp_for_each_touch(tp, t) {
> if (!t->dirty)
> continue;
>
> - if (t->state == TOUCH_END)
> - t->state = TOUCH_NONE;
> - else if (t->state == TOUCH_BEGIN)
> + if (t->state == TOUCH_END) {
> + if (!tp_fake_finger_is_touching(tp) &&
> + tp_fake_finger_count(tp) >= i + 1)
Using i here is wrong, there may be unused slots, etc. Also
see below.
> + t->state = TOUCH_HOVERING;
> + else
> + t->state = TOUCH_NONE;
> + } else if (t->state == TOUCH_BEGIN) {
> t->state = TOUCH_UPDATE;
> + }
>
> t->dirty = false;
> }
Sorry, but I'm still not happy about this, before this patch, touches
0 - tp->real_touches begin / end is controlled by ABS_MT_TRACKING_ID
and tp->real_touches - tp->ntouches are controlled by
BTN_TOOL_fooTAP I can see how that is inconsistent, and you're
fixing that inconsistency here.
My problem with the above code is that if a touch gets its
ABS_MT_TRACKING_ID set to -1, but BTN_TOOL_fooTAP does not change
in the same sync, then it will end up in t->state = TOUCH_HOVERING,
and if the next sync BTN_TOOL_fooTAP is still not changed it
will move to TOUCH_BEGIN and now we have a touch with no valid
coordinates in state TOUCH_BEGIN (*).
I think that we need to store the tracking_id in a touch, and decide
whether to move to NONE or HOVERING based on tracking_id == -1.
Storing the tracking_id also allows us to check that the kernel always
sends a -1 before re-using a slot, I know it should do that, but it might
be good to check, and log a kernel bug if it doesn't.
(*) and that is just one scenario, we could also have one finger
being lifted in e.g. slot 1 and another one starting in slot 2, then
the use of i is wrong as remarked, and moving the slot 1 touch to
HOVERING also is wrong.
Regards,
Hans
p.s.
Thinking about this also makes me realize that tp_process_state is
somewhat buggy wrt getting the "first" touch which it uses for
coordinates for the touches tp->real_touches - tp->ntouches, what
if that touch is not valid ? This could happen if the BTN_TOOL_fooTAP
arrives before the slot gets used... I don't know if this ever
actually happens, but at a minimum we should check for this.
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index 1e6152a..0de2e62 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -53,6 +53,7 @@ enum touchpad_model {
>
> enum touch_state {
> TOUCH_NONE = 0,
> + TOUCH_HOVERING,
> TOUCH_BEGIN,
> TOUCH_UPDATE,
> TOUCH_END
>
More information about the wayland-devel
mailing list