search

linux-surface / iptsd

Userspace daemon for Intel Precise Touch & Stylus
GNU General Public License v2.0
98 stars 47 forks source link

WIP: Cone-based palm rejection #16

Closed tmarkov closed 4 years ago

tmarkov commented 4 years ago

This prototypes a cone-based palm-rejection. When the pen is down or hovering, a 60-degree cone is projected towards a dynamically-determined direction (30 degrees to either side of the direction vector). Touches in the cone are marked as palms.

1) There is only one cone - if there are multiple styluses it will work poorly. 2) Cone direction is determined based on location of palms detected by the shape-detection algorithm. The direction is a weighted average based on previously detected palms. The weight decays with 1 second half-life. 3) Touches in the cone are marked as palms after the the palm proximity detection, so nearby touches will not be marked as palms for proximity. 4) The cone is cut at a specific distance. Touches further than that distance remain touches, but palms further away are used to update direction.

tmarkov commented 4 years ago

I've fixed up the issues above. Though I seem to have introduced a bug causing the cone to forget itself, so for now you should probably do any performance testing on HEAD~2.

tmarkov commented 4 years ago

It seems that making struct iptsd_touch_rejection_cone a part of iptsd_touch_processor instead of dynamically allocated, causes it to get zeroed out occasionally (not through the update functions). Not sure exactly why that is, though.

StollD commented 4 years ago

So I tried this and it seems to work fine for me. Even with HEAD it works, but I didnt test it for long, so maybe I didn't run into the issue you described yet.

A few questions / ideas though:

Also, I think we should move the code for calculating the rejection cone into a seperate, generic file, like contact.c. I.E. removing the iptsd_ prefixes and operating on struct rejection_cone (or just struct cone maybe?) instead of struct iptsd_touch_processor. That way we could keep the function names a bit shorter and dont have to jam all of it into touch-processor.c

tmarkov commented 4 years ago

So I tried this and it seems to work fine for me. Even with HEAD it works, but I didnt test it for long, so maybe I didn't run into the issue you described yet.

I noticed it because it's not rejecting well and by using debug prints. Here's an experiment that should catch it as well: 1) Hold the pen to the screen while touching your palm to set up cone direction. 2) Lift up both. 3) Touch with the pen (no palm thin time). 4) Circle the pen with a finger. For me, it's accepted in all directions.

Could we get rid of the timestamps by adding a bool active; field to the rejection cone struct, and changing that depending on the value of the proximity bit? So if the stylus is in proximity, the cone gets activated, and when it moves out it gets disabled immediately.

I don't think this will work, since when writing or drawing you lift the pen up sometimes - but you still want palm rejection. So I have it active when the pen is either touching or hovering, so for any pressure.

The second timestamp is used to determine how "fast" to update the cone direction when we detect a new palm (using its shape). I think it's better to have that happen smoothly.

Could we store one rejection cone per stylus and check all of the active ones once a touch comes in? We can't really test it, but I think it would still be nice if we at least theoretically supported multiple styli.

I think it can work theoretically.

Also, I think we should move the code for calculating the rejection cone into a separate, generic file, like contact.c.

I'd like that, too. I tried it already, but I ended up having .h files wanting to include each other:

If you have better suggestions on how it can work, I'd appreciate that.

StollD commented 4 years ago

Do we know which is the currently processed stylus in iptsd_stylus_handle_data, to know which cone to move?

Yes, the one that we emit events for ;) (iptsd->devices.active_stylus). Based on what we figured out about the protocol, it seems that it works a bit like HID: The hardware will send a report containing the serial of the stylus. Until a new serial is sent, all reports come from the stylus with that serial. If iptsd parses a new serial, it will change the active stylus in iptsd_stylus_change_serial.

If you have better suggestions on how it can work, I'd appreciate that.

I would invert the stylus coordinates directly in iptsd_stylus_handle_data (since that has direct access to the config). Then iptsd_touch_rejection_cone_set_tip doesn't need the touch processor struct, and can work with the rejection cone struct directly.

tmarkov commented 4 years ago

I would invert the stylus coordinates directly in iptsd_stylus_handle_data (since that has direct access to the config). Then iptsd_touch_rejection_cone_set_tip doesn't need the touch processor struct, and can work with the rejection cone struct directly.

I also use tp->hm.width and tp->hm.height from the touch processor - besides inverting the coordinates, I need to re-scale them. I considered instead re-scaling everything to 7200x9600, but it's not proportional to the screen size, so it'll make the cone differently wide in different directions. Though I guess that can be worked around somehow.

StollD commented 4 years ago

Would the width / height that we load from the config work? Thats the actual size of the screen (I am not sure if the heatmap has different sizes on different devices), pulled from the HID descriptors. Currently we only use it to calculate the resolution for the input devices.

https://github.com/linux-surface/iptsd/blob/master/config/surface-book2-13.conf#L9-L10

(this could be stored in the rejection cone struct when the stylus is created in devices.c)

tmarkov commented 4 years ago

OK, I figured out why making the cone static doesn't work. Here, https://github.com/linux-surface/iptsd/blob/e4627e8a3a1e88b452811a1414836e9433ff89e7/src/touch.c#L26 you make a copy of the touch device . Then all the updates are made on the copy and discarded. Do we need that to be a copy for some reason?

StollD commented 4 years ago

Ouch, yeah, we should be getting a pointer to the touch device there. Thanks for spotting it.

tmarkov commented 4 years ago

Would you object if I moved all constants to a new constants.h?

StollD commented 4 years ago

I'm fine with that. Although my plan was to try and move as many constants as possible into the configuration files, so that users can tweak them if needed.

tmarkov commented 4 years ago

I added one rejection cone per stylus and tweaked increased the cone radius.

I tried once more to separate the cone code into its own file, but it just doesn't work out cleanly (I can work around the problems I've mentioned here, but then new problems appear, etc). You can give it a try if you want, but adding multiple cones actually made it even more intertwined with the touch processor, so I think I'll give up on it.

However, there seems to be an outstanding bug which somehow permanently blocks out a part of the screen from touch (but not stylus) until iptsd is restarted. I can't yet reliably reproduce it, but so far it happens when I use the stylus to write in the lower right quarter (with my right hand).

StollD commented 4 years ago

Thank you for your work (again)! Merged the changes, since they are working as intended.