[PATCH v2] touchpad: add wobbling detection

[Konstantin Kharlamov]

On 20.02.2018 09:34, Peter Hutterer wrote:
> On Sun, Feb 18, 2018 at 11:14:55PM +0300, Konstantin Kharlamov wrote:
>> +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...

My touchpad definitely does; however it does so many events for every 
touch that I feel like it produces every possible combination within a 
second or two. The more that the number for the three slots is 2³ = 8, 
which isn't much.

>> +	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?

Thank you, nice catch!

>> +			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.

Well, not really can, the comparison for zero here would check 
coordinate of previous x, whereas I need to check whether the delta 
between prev. and curr. positions is zero, e.g. if a finger is still and 
FW filters.

>>   		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.

Thanks for you comments!

I tried addressing them, and found that the result of the time 
calculation "time - tp->hysteresis.last_motion_time" whilst my finger is 
down is constantly increasing, i.e. like "20, 30, 50, 100, 120…". It 
looks as if "tp->hysteresis.last_motion_time" is not the last motion 
time, but the last finger-down time.

Do you happen to know anything about that?

> Cheers,
>     Peter
> 
>>   	} hysteresis;
>>   
>>   	struct {
>> -- 
>> 2.15.1