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

[Peter Hutterer]

On Mon, May 04, 2015 at 09:11:46AM +0200, Hans de Goede wrote:
> 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 ?

nevermind, my fault. I forgot the nfingers_down == 0 check.
patches passes test suite, coming up in a second

Cheers,
   Peter

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