[PATCH v2] touchpad: add wobbling detection

[Konstantin Kharlamov]

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?