[PATCH v2] touchpad: add wobbling detection

[Peter Hutterer]

On Sun, Feb 18, 2018 at 11:14:55PM +0300, Konstantin Kharlamov wrote:
> The details are explained in comment in the code. That aside, I shall
> mention the check is so light, that it shouldn't influence CPU
> performance even a bit, and can blindly be kept always enabled.

I like it! And it does work with a change (see below), so provided it still
works for you, it's worth testing on more laptops.
 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104828
> 
> v2: rename the function, change comment style, and add calculation of
> x_diff (the one that was used is an absolute coordinate).
> 
> FWIW, the algorithm don't care that now added "prev_x" is unintialized,

prev_x is initialized to 0 because we calloc everything in libinput.

> because that happening means the time threshold won't get satisfied
> either. It's not like I can't default-init it — it's just that asking
> oneself a question "what default value should it have" results in "none".
> 
> Signed-off-by: Konstantin Kharlamov <Hi-Angel at yandex.ru>
> ---
>  src/evdev-mt-touchpad.c | 33 +++++++++++++++++++++++++++++++++
>  src/evdev-mt-touchpad.h |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index ead76456..80f56072 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -131,6 +131,36 @@ tp_motion_history_push(struct tp_touch *t)
>  	t->history.index = motion_index;
>  }
>  
> +static inline void
> +tp_detect_wobbling(struct tp_dispatch *tp, int x_diff, uint64_t time)
> +{
> +	if (tp->hysteresis.enabled)
> +		return;
> +
> +	/* Idea: if we got a tuple of *very* quick moves like {Left, Right, Left}, or
> +	 * {Right, Left, Right}, it means touchpad jitters since no human supposed to
> +	 * be able to move like that within thresholds
> +	 *
> +	 * Algo: we encode left moves as zeroes, and right as ones. We also drop the
> +	 * array to all zeroes when contraints are not satisfied. Then we search for
> +	 * the pattern {1,0,1}. It can't match {Left, Right, Left}, but it does match
> +	 * {Left, Right, Left, Right}, so it's okay.
> +	 */

the interesting bit here will be whether there are touchpads that wobble but
with more than one event per direction. I suspect there are but this should
be simple enough that it catches the first RLR movement (bound to happen at
some point) and enable it then. The main question is whether there is a
noticeable period of wobbling until this happens.
I honestly don't know, we won't know until ppl start shouting at us...

> +	if (time - tp->hysteresis.last_motion_time > ms2us(20) || x_diff == 0) {
> +		tp->hysteresis.x_motion_in_threshold = 0;
> +		return;
> +	}

empty line here please.

> +	tp->hysteresis.x_motion_in_threshold <<= 1;
> +	if (x_diff > 0) { /* right move */

s/x_diff/dx/, we use that everywhere else for deltas.

> +		tp->hysteresis.x_motion_in_threshold |= 1;
> +		static const char r_l_r = 5; /* {Right, Left, Right} */

declarations at the top of the block please, separated by an empty line. See
CODING_STYLE in the repository.

> +		if (tp->hysteresis.x_motion_in_threshold & r_l_r) {

this one will trigger on the first right movement, you need
  if ((tp->hysteresis.x_motion_in_threshold & r_l_r) == r_l_r)

With that change, it seems to work correctly on my non-wobbling touchpad
here. Does it still work for you when you add this?

> +			tp->hysteresis.enabled = true;
> +			evdev_log_debug(tp->device, "hysteresis enabled\n");
> +		}
> +	}
> +}
> +
>  static inline void
>  tp_motion_hysteresis(struct tp_dispatch *tp,
>  		     struct tp_touch *t)
> @@ -1405,6 +1435,8 @@ tp_process_state(struct tp_dispatch *tp, uint64_t time)
>  		tp_thumb_detect(tp, t, time);
>  		tp_palm_detect(tp, t, time);
>  
> +		tp_detect_wobbling(tp, tp->hysteresis.prev_x - t->point.x, time);
> +		tp->hysteresis.prev_x = t->point.x;

the dx calculation should happen within tp_detect_wobbling, just pass the
touch in and do the subtraction there. Plus that way you can do a simple:
    if (tp->prev.x == 0) dx = 0; 
which makes things more obvious.

>  		tp_motion_hysteresis(tp, t);
>  		tp_motion_history_push(t);
>  
> @@ -2918,6 +2950,7 @@ tp_init_hysteresis(struct tp_dispatch *tp)
>  	tp->hysteresis.margin.x = res_x/2;
>  	tp->hysteresis.margin.y = res_y/2;
>  	tp->hysteresis.enabled = false;
> +	tp->hysteresis.x_motion_in_threshold = 0;

'threshold' usually refers to a distance in libinput, here it's more a
timeout. But calling it x_motion_history would be best here, IMO.

>  }
>  
>  static void
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index 442f34a3..398a18ed 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -276,6 +276,8 @@ struct tp_dispatch {
>  		struct device_coords margin;
>  		unsigned int other_event_count;
>  		uint64_t last_motion_time;
> +		char x_motion_in_threshold;
> +		int prev_x;

both of these need to be in the struct tp_touch, otherwise 2-finger
scrolling may enable the hysteresis when the fingers move independently from
each other but you're adding them to the same bitmask.

Cheers,
   Peter

>  	} hysteresis;
>  
>  	struct {
> -- 
> 2.15.1