[PATCH v2] touchpad: add wobbling detection

[Konstantin Kharlamov]

On 20.02.2018 17:31, Konstantin Kharlamov wrote:
> On 20.02.2018 13:44, Konstantin Kharlamov wrote:
>> 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?
> 
> Although I did find what's the matter. Turns out, the last_motion_time 
> have been assigned at tp_maybe_disable_hysteresis() which I did remove. 
> But even there it's been guarded by if:
> 
>          if (tp->queued & TOUCHPAD_EVENT_MOTION)
>                 tp->hysteresis.last_motion_time = time;
> 
> What does that condition mean?

Ok, I see, I was mostly confused by "queued" name. It probably means 
"events queued to get processed by libinput". I wrote a code like:

		if (tp->queued & TOUCHPAD_EVENT_MOTION) {
			tp_detect_wobbling(tp, t, time);
			tp->hysteresis.last_motion_time = time;
			t->prev_x = t->point.x;
		}

It should work like it previously did; what bothers me however — 
shouldn't that be specific to a touch, but not touchpad? Couldn't that 
get triggered e.g. by 2-finger scrolling? (whatever that is, I dunno, 
touching with 2 finger doesn't scroll for me — I do scroll instead using 
the right edge of the touchpad).