Merge lp:~vanvugt/ubuntu/oneiric/xserver-xorg-input-synaptics/fix-754000 into lp:ubuntu/oneiric/xserver-xorg-input-synaptics

[Daniel van Vugt]

Number of active touches can be less than number of active fingers/contacts so ask grail for the real number of fingers when clicking. This fixes 3-finger clicks (middle click) when there are geis/grail clients active such as Unity. (LP: #754000)

This change also appears to fix multi-touch bug 798047.

[Chase Douglas (chasedouglas)]

Hi Daniel,

First, thanks a bunch for working on this bug!

Can you explain in more detail the difference you see between the number of "active touches" as calculated in ProcessTouch of synaptics vs the number of touches seen by grail? I thought they would be the same, but obviously your testing proves otherwise :). A more in depth explanation of how this resolves the bug would help me.

Thanks!

[Daniel van Vugt (vanvugt)]

The problem was (when grail/geis clients were actively subscribed to gestures) that active_touches had a value of 1 instead of 3 if you had touched all three fingers down at basically the same time. Meaning a 3-finger touch was being counted as simply one touch.

I don't claim to have found and fixed the absolute root cause of the problem -- that is either in one of the earlier debian/patches/* or even in grail/geis. However this fix is robust and resolves the problem regardless of how the code interprets what a single touch event is. So if there is an issue in geis/grail then than can be isolated and fixed later as a separate bug.

Aside from anything else, it's not safe practice to leave counting the number of active fingers to the high-level client code (xserver-xorg-input-synaptics). If the lower-level code (grail) has the definite answer already then you should get it from the library (via grail_get_contacts). Or in fact even lower level -- the bcm5974 kernel module counts the fingers for you by means of BTN_TOOL_TRIPLETAP and that was being ignored too due to 120_active_touches_num_fingers.patch. I suspect removing 120_active_touches_num_fingers.patch would remove this bug altogether.

[Chase Douglas (chasedouglas)]

> The problem was (when grail/geis clients were actively subscribed to gestures)
> that active_touches had a value of 1 instead of 3 if you had touched all three
> fingers down at basically the same time. Meaning a 3-finger touch was being
> counted as simply one touch.

This shouldn't be happening... I'll try to debug it.

> I don't claim to have found and fixed the absolute root cause of the problem
> -- that is either in one of the earlier debian/patches/* or even in
> grail/geis. However this fix is robust and resolves the problem regardless of
> how the code interprets what a single touch event is. So if there is an issue
> in geis/grail then than can be isolated and fixed later as a separate bug.
>
> Aside from anything else, it's not safe practice to leave counting the number
> of active fingers to the high-level client code (xserver-xorg-input-
> synaptics). If the lower-level code (grail) has the definite answer already
> then you should get it from the library (via grail_get_contacts). Or in fact
> even lower level -- the bcm5974 kernel module counts the fingers for you by
> means of BTN_TOOL_TRIPLETAP and that was being ignored too due to
> 120_active_touches_num_fingers.patch. I suspect removing
> 120_active_touches_num_fingers.patch would remove this bug altogether.

The kernel and grail report the number of touches over the entire touchpad surface. However, users may want to mask out a portion of the touchpad where touches are ignored. For example, Synaptics Clickpad users may want to mask out the button clicking areas at the bottom of the touchpad. When a touch begins in these areas we don't want to count it as an active touch. This is why we keep track of the number of active touches separately from what the kernel or grail reports.

The input module should be able to keep track of how many touches are active on the touchpad, even if it's not quite as clean as just asking grail or the kernel.

[Daniel van Vugt (vanvugt)]

Understood. And I know that's why the patch set for this package has become so complicated due to so many use-cases.

However in the masking example you cite, that use-case only conflicts with this patch if you expect the user to be multi-touching while also clicking on the masked click-area, and that you wish to ignore some of the multiple touches in that case. More likely if you're using a mask area then you will be clicking with a single finger.

I'm not sure there is any use-case, even with masked areas, in which it is unsafe to count all actual touches when a physical "click" on the clickpad happens.

But if you find a more elegant fix with respect to active_touches then that would be good too...

[Chase Douglas (chasedouglas)]

> Understood. And I know that's why the patch set for this package has become so
> complicated due to so many use-cases.
>
> However in the masking example you cite, that use-case only conflicts with
> this patch if you expect the user to be multi-touching while also clicking on
> the masked click-area, and that you wish to ignore some of the multiple
> touches in that case. More likely if you're using a mask area then you will be
> clicking with a single finger.
>
> I'm not sure there is any use-case, even with masked areas, in which it is
> unsafe to count all actual touches when a physical "click" on the clickpad
> happens.

I'll give an example about what happens when you want to click and drag on a Clickpad. You press on the masked area to click, and then you start dragging with a second finger in the active area. The kernel and grail will report two touches, but we don't want to send events based on two touches. We want to send events based only on the touch in the active area and the button press. If we used both touches we would generate scroll or right click events (if you have two finger scrolling or tap to click enabled).

The current code attempts to rectify this issue by checking if a touch is in the active area before it increments the count of active touches.

[Daniel van Vugt (vanvugt)]

OK, agreed that is a valid use-case right now and my changes will break that use case.

However it seems we have discussed this before in passing -- a fix for bug 681942 will solve that use case and would then mean the below changes no longer break that use case :)

[Chris Halse Rogers (raof)]

Marking as WIP; we don't want to break Clickpad support. Chase - do we have a way forward here?

[Daniel van Vugt (vanvugt)]

I was thinking about this exact proposal just yesterday and how to move forward. Maybe move the call to grail_get_contacts into a count_fingers function which also checks each contact (finger) location against the active area, and excludes those that should not be counted. It would address Chase's concerns from what I can see. But I thought chase was going to look into fixing the root cause instead... Though that might be more difficult and undoubtedly more risky for side-effects.

[Chase Douglas (chasedouglas)]

Sorry, I've been on vacation and otherwise busy with other things to have a chance to look into this in more depth. In theory I would like to resolve this issue of course, but I'm not sure I will have the time necessary to be able to resolve it anytime soon :(.

I see no issues with splitting the touch counting code into a separate function. Maybe it would help diagnose the root cause of the bug? Daniel, are you willing to continue working on this?

Thanks!

[Chase Douglas (chasedouglas)]

Just noting that I believe a different fix is needed.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Yes, I can have a look at improving the fix... at a later date.

[Daniel van Vugt (vanvugt)]

Rewrote the fix, addressing the problem Chase mentioned about testing the active area.

It's a small and elegant patch, but doesn't address the root cause. That, I believe, would require a much larger architectural rethink across grail and xserver-xorg-input-synaptics.

[Chase Douglas (chasedouglas)]

Hmmm, I tested this patch, but it doesn't seem like the check for the touch being in the active area is working. If I had to guess, I'd say that the active area check is in device units, but the grail coordinates are in "normalized" units.

review: Needs Fixing

[Chase Douglas (chasedouglas)]

I should have noted that the touches always seem to fall in the active area. When I try to select text by clicking in the masked out zone where the button is while I move a second finger in the active zone, I get a two finger scroll event instead.

[Daniel van Vugt (vanvugt)]

The fix works for me on a Macbook Air clickpad (where it appears device units == grail coordinates) but obviously there are other devices to account for. What is the correct way of converting the units?

[Daniel van Vugt (vanvugt)]

I have double-checked and can't reproduce the bug Chase describes (any more). Certainly the bug existed in rev 38, but it is fixed in the latest revision 40.

Please test the build of revision 40 (vv2) in my PPA:
https://launchpad.net/~vanvugt/+archive/unity/+packages
(version 1.3.99+git20110116.0e27ce3a-0ubuntu12vv2)

I have tested masking out the bottom quarter of my 1280x800 clickpad using:
  xinput set-prop bcm5974 "Synaptics Area" 0 0 0 600
And then I can do the two-handed text selection you describe (although it sporadically fails to sense the touch at all, which appears to be an unrelated bug).

As an added bonus, the fix in revision 40 also appears to fix bug 798047 :)

When I add debug output I see grail is outputting device coordinates. And these appear to be exactly the same coordinates required by is_inside_active_area. So it does not appear I have made any coordinate conversion mistakes.

Could some one else please test the fix using the debs in the above PPA?

[Chase Douglas (chasedouglas)]

Sorry for the delay, I was at a conference last week.

I retested the package in your PPA, and it had the same behavior for me as before. It does not allow me to click and drag by masking out the bottom area of the trackpad.

To help debugging, I have created a recording of my device. I attempted multiple drags to highlight the issue. The recording files are:

http://people.canonical.com/~cndougla/utouch/device.prop
http://people.canonical.com/~cndougla/utouch/device.record

See https://wiki.ubuntu.com/Multitouch/Testing/uTouchEvEmu#Debugging for instructions on how to play back the recording. When I use the synaptics package in Natty, I get the correct behavior of dragging and selecting text. When I use your proposed package, I get two finger scroll instead.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Thanks Chase. Unfortunately evemu-device is not working at all. It silently fails to create a device entry and outputs nothing. And I tried it on two machines running natty.

Instead, could you please give me output from:
  xinput list-props <x-device> (where <x-device> is from the list of "xinput list")
and
  grep range /var/log/Xorg.?.log

And please advise on the exact command you're using to restrict the active area on your machine.

Also, if you have any hints regarding my previous question "What is the correct way of converting the units?" it would be much appreciated.

[Daniel van Vugt (vanvugt)]

@Chase: I assume two-handed dragging works for you normally, and you tested it before installing my changes?

[Chase Douglas (chasedouglas)]

Here's what you requested:

props:
Device 'SynPS/2 Synaptics TouchPad':
 Device Enabled (135): 1
 Coordinate Transformation Matrix (137): 1.000000, 0.000000, 0.000000, 0.000000, 1.000000, 0.000000, 0.000000, 0.000000, 1.000000
 Device Accel Profile (252): 1
 Device Accel Constant Deceleration (253): 2.500000
 Device Accel Adaptive Deceleration (254): 1.000000
 Device Accel Velocity Scaling (255): 12.500000
 Synaptics Edges (256): 1760, 5302, 1639, 4479
 Synaptics Finger (257): 24, 29, 255
 Synaptics Tap Time (258): 180
 Synaptics Tap Move (259): 232
 Synaptics Tap Durations (260): 180, 180, 100
 Synaptics Tap FastTap (261): 0
 Synaptics Middle Button Timeout (262): 75
 Synaptics Two-Finger Pressure (263): 280
 Synaptics Two-Finger Width (264): 6
 Synaptics Scrolling Distance (265): 250, 250
 Synaptics Edge Scrolling (266): 0, 0, 0
 Synaptics Two-Finger Scrolling (267): 1, 1
 Synaptics Move Speed (268): 1.000000, 1.750000, 0.037893, 40.000000
 Synaptics Edge Motion Pressure (269): 29, 159
 Synaptics Edge Motion Speed (270): 1, 422
 Synaptics Edge Motion Always (271): 0
 Synaptics Off (272): 1
 Synaptics Locked Drags (273): 0
 Synaptics Locked Drags Timeout (274): 5000
 Synaptics Tap Action (275): 2, 3, 0, 0, 1, 3, 2
 Synaptics Click Action (276): 1, 1, 1
 Synaptics Circular Scrolling (277): 0
 Synaptics Circular Scrolling Distance (278): 0.100000
 Synaptics Circular Scrolling Trigger (279): 0
 Synaptics Circular Pad (280): 0
 Synaptics Palm Detection (281): 0
 Synaptics Palm Dimensions (282): 9, 199
 Synaptics Coasting Speed (283): 20.000000, 50.000000
 Synaptics Pressure Motion (284): 29, 159
 Synaptics Pressure Motion Factor (285): 1.000000, 1.000000
 Synaptics Grab Event Device (286): 0
 Synaptics Gestures (287): 1
 Synaptics Capabilities (288): 1, 0, 1, 1, 1, 1, 1
 Synaptics Pad Resolution (289): 121, 58
 Synaptics Area (290): 0, 0, 0, 4100
 Synaptics Jumpy Cursor Threshold (291): 90

grep range /var/log/Xorg.0.log:
[ 83.896] (--) SynPS/2 Synaptics TouchPad: x-axis range 1472 - 5590
[ 83.896] (--) SynPS/2 Synaptics TouchPad: y-axis range 1408 - 4710
[ 83.896] (--) SynPS/2 Synaptics TouchPad: pressure range 0 - 255
[ 83.896] (--) SynPS/2 Synaptics TouchPad: finger width range 0 - 15
[ 21001.277] (--) cndougla’s trackpad: x-axis range -2909 - 3167
[ 21001.278] (--) cndougla’s trackpad: y-axis range -2456 - 2565
[ 21001.278] (--) cndougla’s trackpad: invalid pressure range. defaulting to 0 - 256
[ 21001.278] (--) cndougla’s trackpad: invalid finger width range. defaulting to 0 - 16

From /usr/share/X11/xorg.conf.d/51-synaptics-quirks.conf:
Section "InputClass"
        Identifier "Dell Inspiron embedded buttons quirks"
        MatchTag "inspiron_1011|inspiron_1012"
        MatchDevicePath "/dev/input/event*"
        Driver "synaptics"
        Option "JumpyCursorThreshold" "90"
        Option "AreaBottomEdge" "4100"
EndSection

I'm not sure what the correct conversion is off the top of my head.

I also tested two-handed dragging before and after your changes. It works correctly right now, and it stops working after installing your changes.

[Daniel van Vugt (vanvugt)]

@Chase: I don't have access to a machine to test with right now, but do have new theories as to what I could try changing. In the mean time, could you please try:

1. Finding out if your touchpad is semi_mt, and if so, whether I should have an additional check in the style of:
is_inside_active_area(priv, ecpriv->min_x, ecpriv->min_y) &&
is_inside_active_area(priv, ecpriv->max_x, ecpriv->max_y)
???

2. Insert a log message in GrailCountFingers to discover the exact range of grail_contact values that grail is reporting for your hardware. When I did this on a Macbook Air 11", I found it was the same coordinate range 1280x800 as reported in Xorg.0.log. But obviously your hardware is a little different.

3. Change your Synaptics Area to 0 0 0 3059, so the bottom 50% is available to use as a button. I am suspicious that your current area size is too small to fit a finger on the Dell Inspiron touchpad. I know it seems to work in the current version, but there is a chance that is actually by side-effect of a bug. Not likely, but worth testing. :)

Unmerged revisions

40. By Daniel van Vugt on 2011-06-10

Improved fix for LP: #754000, testing the active area so as to not break
support for two-handed dragging etc on clickpads.

39. By Daniel van Vugt on 2011-06-08

* Drop libxtst-dev build dependency so syndaemon does not use XRecord,
  preventing a wide range of crashes in _CallCallbacks. (LP: #774978)
* Re-add 116_resolution_detect_option.patch as 101_resolution_detect_option.patch:
  - This patch was introduced in 1.2.2-2ubuntu7 but got erroneously dropped in the
    merge for 1.3.99+git20110116.0e27ce3a-0ubuntu1.
    (LP: #327428)

38. By Daniel van Vugt on 2011-05-17

Number of active touches can be less than number of active fingers/contacts
so ask grail for the real number of fingers when clicking. This fixes
3-finger clicks (middle click) when there are geis/grail clients active
such as Unity. (LP: #754000)

=== added file 'debian/patches/130_multifinger_clicks_lp754000.patch'
--- debian/patches/130_multifinger_clicks_lp754000.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/130_multifinger_clicks_lp754000.patch	2011-06-10 09:11:34 +0000
@@ -0,0 +1,57 @@
+Index: mine/src/eventcomm.c
+===================================================================
+--- mine.orig/src/eventcomm.c	2011-06-10 16:22:03.081669777 +0800
++++ mine/src/eventcomm.c	2011-06-10 16:40:31.321669981 +0800
+@@ -533,6 +533,33 @@
+     return rc;
+ }
+ 
++static int
++CountGrailFingers(const SynapticsPrivate *priv, const EventcommPrivate *ecpriv)
++{
++    int count = -1;
++    /*
++     * We need to count the fingers manually when grail is active.
++     * This is because if grail matches a gesture (like 3 fingers touching)
++     * then ABS_MT_SLOT is never sent to EventProcessEvent and active_touches
++     * remains zero. So we ask grail how many fingers really are touching,
++     * but only count those that are inside the active area.
++     */
++    if (priv->has_touch && ecpriv->grail) {
++        struct grail_contact contact[5];
++        int i, contacts;
++        contacts = grail_get_contacts(ecpriv->grail, contact,
++                                      sizeof(contact)/sizeof(contact[0]));
++        count = 0;
++        for (i = 0; i < contacts; i++) {
++            if (is_inside_active_area(priv, contact[i].pos.x,
++                                      contact[i].pos.y)) {
++                count++;
++            }
++        }
++    }
++    return count;
++}
++
+ Bool
+ EventProcessEvent(InputInfoPtr pInfo, struct CommData *comm,
+                   struct SynapticsHwState *hwRet, const struct input_event *ev)
+@@ -542,14 +569,16 @@
+     SynapticsParameters *para = &priv->synpara;
+     struct SynapticsHwState *hw = &(comm->hwState);
+     Bool v;
++    int numfingers;
+ 
+     switch (ev->type) {
+     case EV_SYN:
+         switch (ev->code) {
+         case SYN_REPORT:
+             ProcessTouch(pInfo, priv);
+-            if (priv->has_touch && ecpriv->active_touches < 2)
+-                hw->numFingers = ecpriv->active_touches;
++            numfingers = CountGrailFingers(priv, ecpriv);
++            if (numfingers >= 0)
++                hw->numFingers = numfingers;
+             else if (comm->oneFinger)
+                 hw->numFingers = 1;
+             else if (comm->twoFingers)

=== modified file 'debian/patches/series'
--- debian/patches/series	2011-05-20 11:14:44 +0000
+++ debian/patches/series	2011-06-10 09:11:34 +0000
@@ -17,3 +17,4 @@
 122_revert_pressure_finger_default.patch
 123_order_ProcessTouch_for_numFingers.patch
 124_syndaemon_events.patch
+130_multifinger_clicks_lp754000.patch