[PATCH 2/2] touchpad: don't apply tap config until all fingers are up

[Hans de Goede]

Hi,

On 04-05-15 08:41, Peter Hutterer wrote:
> On Fri, May 01, 2015 at 10:19:59AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 01-05-15 04:48, Peter Hutterer wrote:
>>> On Thu, Apr 30, 2015 at 10:01:55AM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 30-04-15 08:00, Peter Hutterer wrote:
>>>>> If tapping is enabled while at least one finger is down, we underrun
>>>>> tp->tap.tap_finger_count on touch release. Avoid this by only switching
>>>>> tap config whenever there are no fingers down.
>>>>
>>>> The immediate applying of tapping enabled changes is deliberate because
>>>> we also go through this code path when suspending tapping on trackpoint
>>>> activity and then we want to enable tapping directly even though a
>>>> finger may be resting on the touchpad (think top soft buttons).
>>>
>>> is this much of an issue though? afaict the only drawback is that we require
>>> the user to lift all fingers once tapping is enabled to allow tapping. I
>>> don't think that's a problem.
>>>
>>>> We could delay the enabling and not the disabling but that seems complex.
>>>>
>>>> I think that the best solution is actually to completely remove the
>>>> tap.tap_finger_count variable, it is only used in one place:
>>>>
>>>>
>>>> static void
>>>> tp_tap_dead_handle_event(struct tp_dispatch *tp,
>>>>                           struct tp_touch *t,
>>>>                           enum tap_event event,
>>>>                           uint64_t time)
>>>> {
>>>>
>>>>          switch (event) {
>>>>          case TAP_EVENT_RELEASE:
>>>>                  if (tp->tap.tap_finger_count == 0)
>>>>                          tp->tap.state = TAP_STATE_IDLE;
>>>>                  break;
>>>>
>>>>
>>>> We can simply replace that with:
>>>>
>>>> static void
>>>> tp_tap_dead_handle_event(struct tp_dispatch *tp,
>>>>                           struct tp_touch *t,
>>>>                           enum tap_event event,
>>>>                           uint64_t time)
>>>> {
>>>>
>>>>          switch (event) {
>>>>          case TAP_EVENT_RELEASE:
>>>>                  if (tp->nfingers_down == 0)
>>>>                          tp->tap.state = TAP_STATE_IDLE;
>>>>                  break;
>>>>
>>>> And completely remove tap.tap_finger_count. As a bonus this new
>>>> code is also consistent with how we set tp->tap.state on
>>>> enable/disable.
>>>
>>> right, the problem is that nfingers_down is not useful for us here. this
>>> variable was only introduced recently, from 591a41f:
>>> "tp->nfingers_down gives us the current state of the touchpad but in
>>> the case of the tapping state we need the touchpoints separately. If all
>>> touchpoints end in the same SYN_REPORT frame, tp->nfingers_down is 0 when we
>>> handle the touch releases."
>>
>> I see, so if we use tp->nfingers_down then the first released touch would
>> move us to idle, and the next one would trigger:
>>
>> static void
>> tp_tap_idle_handle_event(struct tp_dispatch *tp,
>>                           struct tp_touch *t,
>>                           enum tap_event event, uint64_t time)
>> {
>>          struct libinput *libinput = tp->device->base.seat->libinput;
>>
>>          switch (event) {
>> 	...
>>          case TAP_EVENT_RELEASE:
>>          case TAP_EVENT_MOTION:
>>                  log_bug_libinput(libinput,
>>                                   "invalid tap event, no fingers are down\n");
>>                  break;
>>
>> We can easily fix that though by simply ignoring release events in idle.
>>
>> This way we can remove finger_count simplifying the code, where as your
>> patch just adds additional code.
>
> I just tried this and there's a slight problem with it: it messes up the
> state machine. if you move from DEAD to IDLE on the first release there may
> still be fingers down. That's easy to handle in IDLE but we'd then have to
> ignore release events in every other state as well.

I do not understand, why do we need to ignore release events in other
tates as well ?

This can only happy if tp->nfingers_down == 0, so then we expect to only
see more release events in the same sync / tp_tap_handle_state call and at
the end of that call we will still be in idle, and all fingers should be
released.

Regards,

Hans