Created attachment 132213 [details] [review] patch to add upper of touchpad into exclusion zones Hi there, I'm using a ThinkPad X1 Carbon 5th gen. I use both TrackPoint and touchpad, and the cursor move around randomly sometime when using TrackPoint. I found out that I frequently put my right thumb finger near TrackPoint buttons and upper edge of touchpad[1], but current palm detecion are not handle this. I tested a simple patch to add upper of touchpad into exclusion zones and it works. Since this only happens with laptops have TrackPoint buttons, how do you think about implement such a feature like this? [1]: it looks like this http://i.imgur.com/OAqVd0C.jpg I'm not a native speaker. Please forgive me for my bad grammar.
Comment on attachment 132213 [details] [review] patch to add upper of touchpad into exclusion zones Review of attachment 132213 [details] [review]: ----------------------------------------------------------------- Thanks for the patch. I'm not opposed to the idea itself, but I'm a bit worried about introducing this and ending up with a lot of palm misdetections. So we need to have quite high confidence in the area that is ignored now, the proposed 25% seem a bit high for this. fwiw, for merging I'd need a patch in the form of git-format-patch. And the documentation updates, and a few test cases to make sure the behaviour is correct :) ::: src/evdev-mt-touchpad.c @@ +553,5 @@ > if (t->state != TOUCH_BEGIN) > return false; > > + if (t->pressure < 150) > + return false; that seems part of a different patch. And we definitely can't hard-code the 150, every device has different thresholds. This is bug 94236 btw. @@ +562,1 @@ > return false; please make sure you keep the indentation correct, i.e. lined-up. This applies to all hunks where appropriate @@ +666,3 @@ > delta = device_delta(t->point, t->palm.first); > dirs = phys_get_direction(tp_phys_delta(tp, delta)); > if ((dirs & DIRECTIONS) && !(dirs & ~DIRECTIONS)) this one won't work correctly. Have a look at http://wayland.freedesktop.org/libinput/doc/latest/palm_detection.html (which btw would need updates for this patch) and the A and B arrows in the graphic there. we only check for left/right directions here, if you started a touch in the top palm zone and moved vertically down, this won't detect it and the touch remains a palm. That interaction is relatively common though so we'd have misdetection here. @@ +729,5 @@ > /* palm must start in exclusion zone, it's ok to move into > the zone without being a palm */ > if (t->state != TOUCH_BEGIN || > + (t->point.x > tp->palm.left_edge && t->point.x < tp->palm.right_edge && > + t->point.y > tp->palm.upper_edge)) We have this condition 3 times now (possibly more, outside the patch context). I think at this point it's best to have a static inline tp_palm_in_edge() helper that returns true/false. Makes the code more readable and less likely to introduce typos/copy-paste errors. @@ +2313,5 @@ > tp->palm.right_edge = edges.x; > + > + mm.y = height * 0.25; > + edges = evdev_device_mm_to_units(device, &mm); > + tp->palm.upper_edge = edges.y; tbh, I think this is way too big. On my touchpad here this would be 2.5cm from the top which is well within the normal finger interaction area. That's a T440 without separate trackpoint buttons, but still. Did you measure the locations of your palm touches to figure out where the affected zone is?
One other question that comes up here is: why doesn't the palm detection work during trackpoint use? We ignore any touches during trackpoint use, so I'm wondering what's going on here. Also, do you use the touchpad at all? If not, turning it off is a better option.
Sorry for the messed up patch, I did some quick & dirty changes and just want to show the idea. Thank you for looking into it. > proposed 25% seem a bit high for this. Yes, evemu-record logs told me 5% should be enough. BTW, how do you think about making the exclusion zone sizes a user specified parameter or udev hwdb entries? I set it to 25% because I locally modified right zone to 25% too. (logs show that my palm often put on 20%~25% of right zone) > One other question that comes up here is: > why doesn't the palm detection work during trackpoint use? > We ignore any touches during trackpoint use, so I'm wondering what's going on here. I guess that I mostly stop moving trackpoint before clicking trackpoint buttons, and my thumb finger accidentally touch the top-middle part of touchpad. Also, libinput doesn't consider trackpoint button events as trackpoint usage if I'm right, the palm detection didn't works well sometimes while double-clicking. > Also, do you use the touchpad at all? If not, turning it off is a better option. Yes, I use both about 7:3 (trackpoint vs. touchpad). I bind a hotkey to toggle it as a quick solution for now. I might able to do some research and/or experiments on weekends to see is it worth or not... Thanks anyway.
tbh, I don't want this exposed as configuration option. udev hwdb entries are a possibility if we struggle with the initial value but I'd rather avoid it. config options are something that is hard to remove once they're there and they promise behaviour too. Often we find a better solution down the road once hardware improves but then cannot easily remove it because users rely on the previous configuration. I pushed a branch for pressure-based palm detection yesterday, see Bug 94236. I suggest given that a try first because it may help with a bunch of issues like the 25% change you need atm. (In reply to Ming-Yang Lu from comment #3) > Also, libinput doesn't consider trackpoint button events as > trackpoint usage if I'm right, the palm detection didn't works well > sometimes while double-clicking. true, it's intended. we have a lot of users that use the touchpad for movement but the trackpoint buttons for clicking - disabling the touchpad is not useful in that case. I guess one of the possibilities could also be to increase the trackpoint timeout on demand when true trackpoint usage is detected. e.g. more than X events from the trackpoint -> timeout is now 1s instead of the few ms we have now. I'm not sure that would help in your case though, I think a 5% edge may still be the best option.
(In reply to Peter Hutterer from comment #4) > Often we find a better solution down the > road once hardware improves but then cannot easily remove it because users > rely on the previous configuration. You're right. That's a fair point. > I pushed a branch for pressure-based palm detection yesterday, see Bug > 94236. I suggest given that a try first because it may help with a bunch of > issues like the 25% change you need atm. This helps a lots. I reverted back the 25% change. > I guess one of the possibilities could also be to increase the trackpoint > timeout on demand when true trackpoint usage is detected. I have tried set the trackpoint timeout to 1s, but still no luck. > I think a 5% edge may still be the best option. Yes. a 5% setup still works great to solve the issue.
Created attachment 132394 [details] libinput-debug-events log describing the issue To make it more clear, I recorded libinput-debug-events while simply clicking trackpoint button a couple of times. The log shows unexpected cursors moves between buttons events. Furthermore, It will get more unexpected mouse clicking events when tapping is enabled.
Created attachment 132395 [details] [review] patch to add tp_palm_in_edge helper function
Created attachment 132396 [details] [review] 0002-touchpad-add-upper-edge-into-exclusion-zone.patch I tried to implement tp_palm_detect_move_out_of_edge correctly by adding S(south) into DIRECTIONS. Also, I tried run tests and this failed on `touchpad_palm_detect_palm_stays_palm` with some devices, but I have no idea why some devices failed while other ones pass the test.
(In reply to Ming-Yang Lu from comment #8) > Also, I tried run tests and this failed on > `touchpad_palm_detect_palm_stays_palm` with some devices, but I have no idea > why some devices failed while other ones pass the test. not all devices have palm detection available so the ones that don't just return early and give false positives. That applies to the whole test suite btw, look for anything failing, there are quite a few false positives due to how the test suite setup works.
Comment on attachment 132395 [details] [review] patch to add tp_palm_in_edge helper function Review of attachment 132395 [details] [review]: ----------------------------------------------------------------- just one comment on this one, I've changed a few things, will attach the new patch in a tick ::: src/evdev-mt-touchpad.c @@ +562,5 @@ > { > if (t->state != TOUCH_BEGIN) > return false; > > + if (tp_palm_in_edge(tp, t)) This bit looks definitely buggy: the previous check returned false if it was *not* in the palm edge area, this one returns false if it's inside the palm edge area now
Created attachment 132402 [details] [review] 0001-touchpad-add-upper-edge-into-exclusion-zone.patch Give this one a try please, it's your patch squashed together with a few minor fixups.
(In reply to Peter Hutterer from comment #10) > This bit looks definitely buggy: the previous check returned false if it was > *not* in the palm edge area, this one returns false if it's inside the palm > edge area now okay, so this issue also happens on master branch since I didn't modified original behavior here. (In reply to Peter Hutterer from comment #11) Just tried the patch (with `wip/touchpad-palm-pressure`). It seems to break something since palm detection(edge) only works in top-left/top-right corner now.
Created attachment 132421 [details] [review] 0001-touchpad-add-upper-edge-into-exclusion-zone.patch Updated, this time with the right checks
Comment on attachment 132421 [details] [review] 0001-touchpad-add-upper-edge-into-exclusion-zone.patch Review of attachment 132421 [details] [review]: ----------------------------------------------------------------- ::: src/evdev-mt-touchpad.c @@ +749,4 @@ > > /* palm must start in exclusion zone, it's ok to move into > the zone without being a palm */ > + if (t->state != TOUCH_BEGIN || tp_palm_in_edge(tp, t)) It works as expected after changing this to !tp_palm_in_edge.
fixed locally. thanks for testing, I'll get this patch ready for merging, there are a few bells and whistles that need to be attached.
Thank you for helping with this. I'm excited to see this get merged :)
Patch is on list now, took me a bit of work to get the tests to succeed :) https://lists.freedesktop.org/archives/wayland-devel/2017-July/034417.html
pushed as 5dc330bdea6fc0ba6ec380b746dac5763c50cfeb, thanks!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.