Bug 90172

Created attachment 115324 [details]
Recording of double tap when it is recognized.

What version of libinput is this? I've replayed this here with the debugging tool (sudo ./tools/event-debug --enable-tap) and it works fine here. Can you reproduce this with the debugging tool or is this a client bug?

May be related to this: http://cgit.freedesktop.org/xorg/driver/xf86-input-synaptics/commit/?id=37d34f0356cc556dd8a49ec5d1ed64d49417a9b2 which got fixed as part of Bug 89511.

I'm running latest libinput git code, but the problem has been happening for a while. It's happening in the fedora 22 beta too. I can reproduce it with a debugging tool. Depending on the delay between the first tap released and the second tap started sometimes the tap is recognized, sometimes it's not. But if I play the recording of a tap that is not recognized using evemu-play, it gets recognized every time.

can you append --verbose to event-debug? that shows the tap state machine and should provide more info what's going wrong. also, there's an "if 0" in evdev_process_event(), if you comment that out it'll show all incoming events as well, that should help too.

Note that this will also print key events (passwords!), so better run as
sudo ./tools/event-debug --verbose --enable-tapping --device /dev/input/eventXYZ

Created attachment 115370 [details]
event-debug output of a double tap that isn't recognized

Right, this line here is what matters:
tap state: TAP_STATE_DRAGGING_OR_DOUBLETAP → TAP_EVENT_TIMEOUT → TAP_STATE_DRAGGING

It recognised the first tap and the second finger down event but then times out waiting for the second finger up event. And the cause for that seems to be that the timeout is too short, the timeout for a single tap continues through to the doubletap so you only get 180ms in total. Patch coming up in a tick.

Created attachment 115387 [details] [review]
0001-touchpad-re-set-the-tap-timer-for-doubletaps.patch

Give this one a try please, that should make it much more reliable.

With the patch applied event-debug reports 2 button presses, but client application doesn't register double tap (if I'm tapping with normal speed). I'm using weston terminal and gedit as client applications. Double tap gets registered if I tap slowly so that libinput reports two separate taps, or if I double tap really fast. I've compared tap states between normal speed double tap and fast double tap and I can't find the difference. I'm attaching both recordings.

Created attachment 115399 [details]
event-debug recording of tapping with normal speed - double tap isn't recognized in client application

Created attachment 115400 [details]
event-debug recording of fast tapping - double tap gets recognized in client application

The only difference I see here is the timestamps. The first one has ~200ms between press/release, then 180ms to the next one which comes in at the same time. The second recording (115400) has 170ms + 180ms.

I wonder if the extended time now exceeds the time required by the client? Can you reduce the DEFAULT_TAP_TIMEOUT_PERIOD to say 150 or 120 in libinput and rebuild/test?

(In reply to Peter Hutterer from comment #11)
> The only difference I see here is the timestamps. The first one has ~200ms
> between press/release, then 180ms to the next one which comes in at the same
> time. The second recording (115400) has 170ms + 180ms.
> 
> I wonder if the extended time now exceeds the time required by the client?

I don't think that's the case because if i tap slowly (so slowly that libinput internally registers these taps as 2 separate taps, not one multitap) client registers that as double click. I can attach that recording too if you want.

> Can you reduce the DEFAULT_TAP_TIMEOUT_PERIOD to say 150 or 120 in libinput
> and rebuild/test?

I've reduced it to 120 and it doesn't help. If I tap really fast it gets registered, tapping with normal speed doesn't get registered and tapping slowly gets registered but libinput recognizes it as two separate taps.

I've been playing with this some more and it turns out your patch works. The problem was I didn't clean git repositories before compiling. Because of that every time I started weston it started from the very first folder I compiled to, and that one didn't have patch applied. I always started event-debug from the right folder and that's why it reported two button presses but client only registered one.

Also, DEFAULT_TAP_TIMEOUT_PERIOD should be reduced to 150 ms (or maybe 140?). When I was using gedit some double taps didn't get registered. That happened when I tapped slower so I guess those taps exceeded the time required by the application. That happened with DEFAULT_TAP_TIMEOUT_PERIOD set to 180. When I reduced it to 150 it always worked. And with smaller timeout period there's also smaller delay between lifting finger for the second time and that getting registered as a second tap. With timeout period set to 150 I could still reliably do tap and drag. I haven't tried anything smaller than 150, but i think it would work with 140. I'll try that tomorrow.

Created attachment 115422 [details] [review]
0001-touchpad-add-separate-shorter-timeout-for-doubletap.patch

I don't think we can actually reduce the first tap timeout by much. If you hit a ~200ms delay on a "normal tap" (comment #9) then reducing it further risks missing it altogether. 

Note that we don't actually control the double click, we only convert tap to button clicks, everything else is handled in the client (GNOME has a doubleclick time setting).

Here's a patch that reduces the second tap timeout, I think that's better to rely on. That should then also reduce the total time so we're more likely to be interpreted as a click in the default settings of the client.

(In reply to Peter Hutterer from comment #14)
> I don't think we can actually reduce the first tap timeout by much. If you
> hit a ~200ms delay on a "normal tap" (comment #9) then reducing it further
> risks missing it altogether. 

Yes, but if I'm not mistaken in the current implementation timer is reset after the first release event arrives. That means that delay between first touch down event and first release event is DEFAULT_TAP_TIMEOUT_PERIOD, and delay between first release event and second touch event is also DEFAULT_TAP_TIMEOUT_PERIOD. 200 ms from "normal tap" is delay between first and second touch down event. So with current implementation, if DEFAULT_TAP_TIMEOUT_PERIOD is set at 180, delay between first and second touch down events can theoretically go up to 360 ms (180 + 180) and the tap would still be recognized.

> Here's a patch that reduces the second tap timeout, I think that's better to
> rely on. That should then also reduce the total time so we're more likely to
> be interpreted as a click in the default settings of the client.

I get the best results with the first patch applied and DEFAULT_TAP_TIMEOUT_PERIOD set to 150 ms so I think you should use that one.

Also, DEFAULT_TAP_TIMEOUT_PERIOD is used as a timeout between finger lifted during drag and finger lowered to continue drag. That time should be increased, and I think 180 ms is to low, 300 (or 350?) should be good value for that timeout (that's a guess, i haven't tried it. Under synaptics driver I use a timeout of 700 ms, but that is to long as a default. Ideally, that value should be configurable).

I've been experimenting with drag timeout. Currently it's set to DEFAULT_TAP_TIMEOUT_PERIOD and that is too fast. If that get's reduced to 150 ms, it will be even worse. I think 300 ms for "DEFAULT_DRAG_TIMEOUT_PERIOD" is a good value. It's not to fast, but also not to slow. Also, it would be great if the drag could be ended by just tapping on the touchpad. That way user doesn't have to wait for a DEFAULT_DRAG_TIMEOUT_PERIOD to expire, it can just tap and the drag will end. I'll attach a patch for that.

Created attachment 115457 [details] [review]
End the drag by tapping

Great to see this is getting fixed, it was driving me crazy.

Do you know which version of the libinput driver will include this fix?

Thanks

Not a bad idea re:tap to end drag timeout and increasing that timeout in general, please file that as separate bugs though, it's too hard to keep track of everything otherwise.

(In reply to Velimir Lisec from comment #15)
> (In reply to Peter Hutterer from comment #14)
> > I don't think we can actually reduce the first tap timeout by much. If you
> > hit a ~200ms delay on a "normal tap" (comment #9) then reducing it further
> > risks missing it altogether. 
> 
> Yes, but if I'm not mistaken in the current implementation timer is reset
> after the first release event arrives. That means that delay between first
> touch down event and first release event is DEFAULT_TAP_TIMEOUT_PERIOD, and
> delay between first release event and second touch event is also
> DEFAULT_TAP_TIMEOUT_PERIOD. 200 ms from "normal tap" is delay between first
> and second touch down event. So with current implementation, if
> DEFAULT_TAP_TIMEOUT_PERIOD is set at 180, delay between first and second
> touch down events can theoretically go up to 360 ms (180 + 180) and the tap
> would still be recognized.

true but that assumes no delay whatsoever. depending on what the system is up to, there can be a delay between the timer triggering and us actually processing the event. Which is, I think, the reason you had a 200ms delay in one of your recordings. And note that you labelled that recording as "normal speed", shortening the timeout would've missed the first tap. So for double-tap I think a longer + slightly shorter combination may be better.

> I get the best results with the first patch applied and
> DEFAULT_TAP_TIMEOUT_PERIOD set to 150 ms so I think you should use that one.

again, don't forget that you thought normal speed was 200ms. and don't underestimate the training effect, it's what makes things like this really really hard to test. you pretty much need to stop after 2-3 tests and come back a few hours later (yes, it's as annoying as it sounds, tell me about it :)

(In reply to Peter Hutterer from comment #19)
> Not a bad idea re:tap to end drag timeout and increasing that timeout in
> general, please file that as separate bugs though, it's too hard to keep
> track of everything otherwise.

Will do.

> true but that assumes no delay whatsoever. depending on what the system is
> up to, there can be a delay between the timer triggering and us actually
> processing the event. Which is, I think, the reason you had a 200ms delay in
> one of your recordings. And note that you labelled that recording as "normal
> speed", shortening the timeout would've missed the first tap. So for
> double-tap I think a longer + slightly shorter combination may be better.

But what I don't understand is why was the tap from that recording recognized? Because DEFAULT_TAP_TIMEOUT was set to 180 ms, if the delay is 200 ms, it shouldn't have been recognized as a tap, but as a pointer move.

hard to be sure without profiling, but what I speculate is: input events and the timerfd are both epoll sources in libinput. but input events are read in a loop to pull all of them off the device, which can potentially starve the timerfd. i.e. while the device keeps producing events as fast as we process them, we never check the timerfd for timeouts and thus go past the allotted time. the event processing doesn't double-check if the event time is newer than a previously scheduled timer (it should, feel free to file a bug for this) so we process an event that comes in late as within the timeout - simply because we haven't checked the timeout yet.

I've tried your second patch again and it just didn't work as expected. Then I figured out what is wrong. We don't actually want to reduce the time for second tap, we want to reduce the time required for button events to appear after tap happened. With your patch (the second one), even if I double tap really fast, the client application will receive  button events 180 ms after I lifted my finger from the touchpad. I'll attach the patch that reduces that delay to 150 ms.

Created attachment 115476 [details] [review]
Reduces the timeout for button events to appear after the double tap to 150 ms

hah, thanks. that patch isn't quite the solution but it pointed me to what should be it. The problem with the delays is that we're setting the timeout at the wrong time. We need to always start the timeout on touch _down_, not on touch up. That subtracts the time the finger is down from the timeout and makes it appear a lot more responsive while giving sufficient time to get to the next touch down.

I've sent the latest patch to the list now, it replaces all other patches here.
http://lists.freedesktop.org/archives/wayland-devel/2015-April/021715.html

commit 182b7b7da92e634d6bd3786076014d1f9e4d5121
Author: Peter Hutterer <peter.hutterer@who-t.net>
Date:   Tue Apr 28 09:50:02 2015 +1000

    touchpad: fix double/multitap timeouts

Hi Peter,

Do you know which version of libinput comes with this fix?

Currently I have version 0.15.0 and while double tap-to-click works fine now, if let's say I'm dragging a window, the window keeps on being dragged even when my finger is removed from the touchpad. So for example, if I start to drag a window, then lift up my finger, and place it down again on the touchpad to perform another action, the window keeps on following the pointer as if I'm still dragging it.

It seems I need to wait slightly more than a second before I can perform any actions after a window is being dragged. This does not happen if I use the physical button on the touchpad rather than tap-to-click.

I thought this might be related to this bug but if needed I can create a new one.

Thank you!

that's actually intentional and it's hard to get rid of this timeout, there are two mutually exclusive features at play:
On the one hand, the tap should stop as soon as possible after lifting the finger. On the other hand, it should be possible to repeatedly lift the finger, move it and set it down again to cover distances otherwise not possible on a touchpad.

The latter requires a timeout, which is the one you're seeing (500ms after the release). We can't have both, but the solution to this is Bug 90255 which was merged today.

Velimir: what's the physical size of your touchpad? we need this for some other feature. thanks

(In reply to Peter Hutterer from comment #28)
> Velimir: what's the physical size of your touchpad? we need this for some
> other feature. thanks

It's 10cm x 5.5cm. That's the size of a touch area. If you need size with physical buttons included it's 10cm x 7cm.