[rmkit] Slots not persisted between TouchEvents

[mrichards42]

I was thinking about palm-rejection when using the pen, and figured one way to do that might be rejecting any SynMotionEvents with more than, say, 5 slots. When I tried this out, the number of slots reported in the original TouchEvent wasn't always accurate. I think this is related to #45.

From reading through a log with DEBUG_INPUT_EVENT=1, it looks like what's happening is that each raw event only includes information about the most recently changed slot. I'm not positive, but I think we need to have TouchEvents start with a copy of the previous event (including all slots), since the input appears to be an update, not a full event. For instance, this is a log from a rm2 of placing 4 fingers on the screen, one after the other, and then lifting them up in the same order (i.e. 1 down, 2 down, 3 down, 4 down, 1 up, 2 up, 3 up 4 up). I also did the reverse (1d, 2d, 3d, 4d, 4u, 3u, 2u, 1u) for comparison.

There's a lot of extra junk in there, since each tiny change in location or pressure triggers a new event. The important parts are when a new slot appears, with a corresponding tracking-id, and when that tracking-id goes away.

A small example:

This is a heavily edited excerpt from the first log, showing 2 slots

# Initial event (slot 0)
event: time 1612038446, type: 3, code :39, value 3003  <-- tracking id for slot 0
event: time 1612038446, type: 3, code :35, value 282   <-- x
event: time 1612038446, type: 3, code :36, value 1608  <-- y
event: time 1612038446, type: 3, code :3a, value 113   <-- pressure
event: time 1612038446, type: 3, code :30, value 17
event: time 1612038446, type: 3, code :34, value 3

# Slot 1 appears with a new tracking id
# Note that position and pressure are specific to slot 1 here; slot 0 has not changed
Event: time 1612038446, type: 3, code :2f, value 1     <-- slot 1
Event: time 1612038446, type: 3, code :39, value 3004  <-- tracking id for slot 1
Event: time 1612038446, type: 3, code :35, value 510   <-- x
Event: time 1612038446, type: 3, code :36, value 1650  <-- y
Event: time 1612038446, type: 3, code :3a, value 85    <-- pressure
Event: time 1612038446, type: 3, code :30, value 8
Event: time 1612038446, type: 3, code :31, value 17
Event: time 1612038446, type: 3, code :34, value 2

# Slot 0 updates position and pressure
Event: time 1612038446, type: 3, code :2f, value 0     <-- slot 0
Event: time 1612038446, type: 3, code :36, value 1594  <-- y
Event: time 1612038446, type: 3, code :3a, value 116   <-- pressure

# Slot 0 again, since we haven't gotten another 2f
Event: time 1612038446, type: 3, code :35, value 283   <-- x
Event: time 1612038446, type: 3, code :36, value 1593  <-- y
Event: time 1612038446, type: 3, code :3a, value 115   <-- pressure

# One more slot 0, then slot 2
Event: time 1612038446, type: 3, code :3a, value 113   <-- pressure (slot 0)
Event: time 1612038446, type: 3, code :2f, value 1     <-- slot 1
Event: time 1612038446, type: 3, code :35, value 508   <-- x
Event: time 1612038446, type: 3, code :3a, value 86    <-- pressure

# Slot 0 is lifted up
Event: time 1612038446, type: 3, code :2f, value 0     <-- slot 0
Event: time 1612038446, type: 3, code :39, value -1    <-- tracking-id removed

# Slot 1 keeps getting events
Event: time 1612038446, type: 3, code :2f, value 1     <-- slot 1
Event: time 1612038446, type: 3, code :36, value 1656  <-- y
Event: time 1612038446, type: 3, code :3a, value 86    <-- pressure

# Slot 1 is lifted up
Event: time 1612038446, type: 3, code :39, value -1    <-- tracking-id removed

And at this point there are no more slots left, so we can throw away this event and start the next touch from scratch.

It looks like currently the slot count is set to the most recently seen slot, but I think it actually needs to be a count of the number of slots with positive tracking-ids. I wonder if that might be related to overly sensitive three-finger gesture from #44?

[raisjn]

thanks for finding and looking into this! i think that this can be fixed by merging the slots from the previous event into the current one (as you say), but might require some misc. fixes to accommodate the correct data.

i made the wrong assumption that slot will contain the highest slot edited on a touch event, so current code that uses a touch event will use the slot value as the indicator of how many fingers are touching. this is mainly used in two places: gestures (to detect how many fingers are touching) and main_loop to detect when the gesture has stopped (if any finger is lifted). in both places, this is incorrect behavior but somehow sort of works (because we discard events that look wrong). (can grep for slots to see where its used)

some notes on current behavior:

• we use the tracking_id to detect if "left" button is pressed (see further down in handle_abs function). when a slot has tracking id set to -1, it's left property will be set to false.
• self.x (and y) is set to slot[0].x when slot[0] is updated and slot[0] is evaluated for swipe recognition (iirc).

[raisjn]

here's a first attempt at copying the slots from the last event: 3f759c8

had to pass references around because the TouchEvent is now getting modified.

even with this fix, though, i think the palm will still accidentally invoke three finger gestures. my thought is to create a tolerance for the distance in X and Y direction and if the distance is too high in both directions, we consider it a palm tap.

below is a palm tap that accidentally triggers three finger gesture. can see that there is a large distance in both X and Y direction.

<slot> <x> <y> <left>
0 492 1135 1
1 498 1131 1
2 860 1570 1
0 492 1374 1
1 488 1137 1
2 860 1570 1
0 1059 1374 1
1 488 1137 1
2 860 1570 1
0 1059 1374 1
1 488 1137 1
2 860 1570 0
RUNNING COMMAND echo "three finger tap"

normal 3 finger tap:

<slot> <x> <y> <left>
0 550 1627 1
1 600 1392 1
2 756 1306 1
0 550 1627 1
1 604 1395 1
2 756 1308 1
0 550 1627 0
1 604 1395 1
2 756 1308 1
RUNNING COMMAND echo "three finger tap"

i don't think palm will create 5 slots - i tend to get 2 - 4 slots. when i press with 4 fingers on each hand, i get 8 slots.

[mrichards42]

Cool, I'll play around with that commit. I think this is getting pretty close to making a fully copy of prev_ev in marshal and then "overlaying" the new event on top . . . would it make sense to actually start each new event with a copy of the previous one? It looks like there's code here in handle_event_fd that does exactly that if DEV is defined. It seems like the event stream is designed to be a delta from the previous event, but I'm not sure if that would have unintended consequences downstream. Since there's already some event filtering in place ("we discard events that look wrong") it might mean a big increase in the total number of events for instance.

... main_loop to detect when the gesture has stopped (if any finger is lifted).

self.x (and y) is set to slot[0].x when slot[0] is updated and slot[0] is evaluated for swipe recognition (iirc).

Ah, interesting -- what happens if you were to put two fingers down, and then lift one (slot 0) up, but leave the other (slot 1) down? Does that trigger mouse.up? Do we ignore further touch events at that point?

even with this fix, though, i think the palm will still accidentally invoke three finger gestures. my thought is to create a tolerance for the distance in X and Y direction and if the distance is too high in both directions, we consider it a palm tap.

i don't think palm will create 5 slots - i tend to get 2 - 4 slots. when i press with 4 fingers on each hand, i get 8 slots.

hmm, that's too bad, sounds like palm rejection might be more complicated than I was thinking, although the x,y tolerance idea sounds promising.

[raisjn]

Cool, I'll play around with that commit. I think this is getting pretty close to making a fully copy of prev_ev in marshal and then "overlaying" the new event on top . . . would it make sense to actually start each new event with a copy of the previous one? It looks like there's code here in handle_event_fd that does exactly that if DEV is defined. It seems like the event stream is designed to be a delta from the previous event, but I'm not sure if that would have unintended consequences downstream. Since there's already some event filtering in place ("we discard events that look wrong") it might mean a big increase in the total number of events for instance.

it seems reasonable to try that approach (starting off with prev as the base and overlaying on top of it), we just need to test out a few apps / behavior and check that things work. multi-touch was added on after the fact (not originally accounted for in rmkit) and is mostly (only?) used by gesture recognition (multi-tap, swipes, etc in gesture.cpy), the rest of rmkit uses SynMotionEvent which has x,y,left (but no slots), and when i say we discard events - thats referring to during gesture processing (where they are TouchEvents). the events will still be processed as SynMotionEvents when dispatching them to widgets, iirc.

Ah, interesting -- what happens if you were to put two fingers down, and then lift one (slot 0) up, but leave the other (slot 1) down? Does that trigger mouse.up? Do we ignore further touch events at that point?

unsure, but i think it would trigger mouse up. now i think the event system of rmkit should ignore any touch events that have multiple fingers involved

hmm, that's too bad, sounds like palm rejection might be more complicated than I was thinking, although the x,y tolerance idea sounds promising.

could also disable touch (or multi touch) if the stylus is hovering over the screen (on a per app basis, maybe?)

[mrichards42]

could also disable touch (or multi touch) if the stylus is hovering over the screen (on a per app basis, maybe?)

I thought about this, but I think usually the bottom of your hand starts touching the screen before the stylus is close enough to hover, but I'm not positive.

now i think the event system of rmkit should ignore any touch events that have multiple fingers involved

That makes a lot of sense to me. Maybe it should be configurable, but I bet almost all of the use-cases for multitouch are based around gestures, which don't translate well to standard mouse events.

[raisjn]

93df17e is modified attempt. i think it is ok, but not great.

i tried to use the coalescing from DEV that you pointed out above but didn't have much success. in specific, i installed a four finger tap handler and three finger tap handler in genie. when i triple tap, it works. when i four tap it works. but if i try triple tap after a four finger tap, it never works. i haven't understood why yet.

iirc, that DEV coalescing is due to how resim (remarkable simulator) works on PC. when resim emits events, the events get cut up across read() calls. on the rM (and in libevdev?), the SYN_EV is always delivered in the same block as the events it is related to.

[mrichards42]

i tried to use the coalescing from DEV that you pointed out above but didn't have much success. in specific, i installed a four finger tap handler and three finger tap handler in genie. when i triple tap, it works. when i four tap it works. but if i try triple tap after a four finger tap, it never works. i haven't understood why yet.

huh, I don't know the code well enough to know for sure, but I wonder if prev_ev needs to be "reset" back to an empty state once the last slot has been lifted?

[mrichards42]

iirc, that DEV coalescing is due to how resim (remarkable simulator) works on PC. when resim emits events, the events get cut up across read() calls. on the rM (and in libevdev?), the SYN_EV is always delivered in the same block as the events it is related to.

Oh, I just read through that code one more time. It looks like event is cleared when it gets EV_SYN like you said, so it seems like that wouldn't do what I'm expecting it to. I think what I was trying to get at would look more like this:

    void handle_event_fd():
      ...

      T event = prev_ev // start with the last event

      for int i = 0; i < bytes / sizeof(struct input_event); i++:
        ...

[raisjn]

fixed this in #77, thanks for opening and helping clean this area up. i think the overall topic is palm detection, but i'll close this task out (i have some small note in another task about it)