[rmkit][input] copy old mt slots from previous event

[raisjn]

• remove wonky backwards event coalescing, instead start with prev_ev for all events
• remove MouseEvent, its not applicable since we started using resim
• set lifted on TouchEvent when a finger is lifted (TRACKING_ID set to -1)
• add count_fingers() to TouchEvent and use TouchEvent.fingers in gestures.cpy for figuring out how many fingers pressed

[mrichards42]

Seems like this should work -- I haven't tried it out myself, but I'll give it a shot in a little bit on puzzles and let you know if something is obviously wrong.

[mrichards42]

I'm not seeing where marshal updates an event. Is this left over from a previous version, or am I missing something (likely since I don't know this code very well)?

[mrichards42]

ok, I did a little testing of my own, just in a mouse.down event.

• fingers was always 0, even when the last slot was > 0
• so is_multitouch was always false, since that's calculated from fingers
• if I ran through slots myself and checked for slots with left > -1, I usually got the right number of fingers
• ... but not always. sometimes I ended up with not as many fingers registered as were actually on the screen. I'd guess it just depends on how many fingers were in the event when the first EV_SYN was received, even if the next EV_SYN picked up more fingers, so I'm not sure what to do about that.
• the worst part of all this is that if I took the stylus and put my hand on the screen as if to write, my hand only registered as 1 slot, so I'm not sure how to reject touch in that case :(

code I tested with:

 target->mouse.down += [=](auto &ev) {
auto touch_ev = dynamic_cast<input::TouchEvent*>(ev.original.get());
if (touch_ev) {
int f = 0;
for (auto s : touch_ev->slots)
if (s.left > -1)
f++;
std::cerr << "touch event with "
<< touch_ev->fingers << " fingers"
<< "; " << touch_ev->slot << " slots"
<< "; " << f << " slots with left > -1"
<< std::endl;
if (touch_ev->is_multitouch) {
std::cerr << "skipping multitouch" << std::endl;
return;
}
}
};

[mrichards42]

Ah, ok I found https://www.kernel.org/doc/html/v4.18/input/multi-touch-protocol.html (you might already be aware of similar documentation, this is all new to me) which describes what all those ABS_MT_* fields mean. I see we aren't capturing ABS_MT_TOUCH_MAJOR or ABS_MT_TOUCH_MINOR, but those describe the size of the touch on the screen. For me a finger shows up as around major=17;minor=8. The edge of my hand has a lot of different sizes, but it seems like it's always at least major >= 26 OR minor >= 17, so maybe that's a reasonable threshold.

[raisjn]

i did know about those docs and have read about the axis minor/major, but didn't think about how to use them for palm detection, nice!

it seems like this diff is not behaving like i thought it would - but at least you have ideas for how to fix palm touch :-D

i think that holding prev_ev and using it as the base makes a lot of sense, so i'd like to achieve that

[raisjn]

ok, i think this works for coalescing the previous event (tested remux, harmony, mines, simple, genie) with touch and stylus. the major piece is in d96ebde.

genie gestures work and now use count_fingers() for filtering instead of the slot.

in addition to those docs, i also used https://elixir.bootlin.com/linux/latest/source/include/linux/input.h.

[raisjn]

have to do some more testing and debugging

was running into strangeness with remux due to remux touch flood, now fixed

i'll likely keep testing this and merge it on the weekend, but preliminary tests seem to show its working at least as well as previously

[mrichards42]

This works great on my end! The only thing that still seems off is the initial finger count (calling count_fingers() does always end up with the right number).

[mrichards42]

I think the event = prev_ev part is redundant? Also should this have a call to event.finalize() somewhere?

[raisjn]

event.finalize() is called on EV_SYN

[mrichards42]

oh you're totally right, it's like 5 lines above this 🤦

[mrichards42]

I'm still seeing fingers=0 in a SynMotionEvent handler when going through dynamic_cast<TouchEvent*>(ev.original().get())->fingers. I think it's b/c that isn't copied here. Does this need an explicit copy constructor, or would the default work?

[raisjn]

i removed explicit constructor, hopefully things improve, but it might be that count_fingers() is always useful to call before checking fingers (and we put that in to main_loop?).

[mrichards42]

When we see tracking_id -1 would we want to reset the slot to its initial -1, -1, -1 values? I assume tracking_id=-1 should mean that any existing x,y coords are invalid.

[raisjn]

if the tracking ID is set to -1, next time the slot is used it will get an X and Y coordinate, i believe.

one potential problem with setting to -1 at this point is that this event is still used (i believe) during mainloop's event dispatch.

[raisjn]

thanks for the help with this - it definitely is better code now. i just glanced at docs again and grepped "palm" and saw MT_TOOL_PALM. i'm curious if we will get this tool type or not. (accidentally put comment in wrong PR a moment ago)

UPDATE: nope, i think evtest says MT_TOOL_PALM is not supported (max of ABS_MT_TOOL_TYPE is 1, should be >= 2 when PALM is supported)

[raisjn]

• remove wonky backwards event coalescing, instead start with prev_ev for all events
• remove MouseEvent, its not applicable since we started using resim
• set lifted on TouchEvent when a finger is lifted (TRACKING_ID set to -1)
• add count_fingers() to TouchEvent and use TouchEvent.fingers in gestures.cpy for figuring out how many fingers pressed

[mrichards42]

Seems like this should work -- I haven't tried it out myself, but I'll give it a shot in a little bit on puzzles and let you know if something is obviously wrong.

[mrichards42]

I'm not seeing where marshal updates an event. Is this left over from a previous version, or am I missing something (likely since I don't know this code very well)?

[mrichards42]

ok, I did a little testing of my own, just in a mouse.down event.

• fingers was always 0, even when the last slot was > 0
• so is_multitouch was always false, since that's calculated from fingers
• if I ran through slots myself and checked for slots with left > -1, I usually got the right number of fingers
• ... but not always. sometimes I ended up with not as many fingers registered as were actually on the screen. I'd guess it just depends on how many fingers were in the event when the first EV_SYN was received, even if the next EV_SYN picked up more fingers, so I'm not sure what to do about that.
• the worst part of all this is that if I took the stylus and put my hand on the screen as if to write, my hand only registered as 1 slot, so I'm not sure how to reject touch in that case :(

code I tested with:

 target->mouse.down += [=](auto &ev) {
auto touch_ev = dynamic_cast<input::TouchEvent*>(ev.original.get());
if (touch_ev) {
int f = 0;
for (auto s : touch_ev->slots)
if (s.left > -1)
f++;
std::cerr << "touch event with "
<< touch_ev->fingers << " fingers"
<< "; " << touch_ev->slot << " slots"
<< "; " << f << " slots with left > -1"
<< std::endl;
if (touch_ev->is_multitouch) {
std::cerr << "skipping multitouch" << std::endl;
return;
}
}
};

[mrichards42]

Ah, ok I found https://www.kernel.org/doc/html/v4.18/input/multi-touch-protocol.html (you might already be aware of similar documentation, this is all new to me) which describes what all those ABS_MT_* fields mean. I see we aren't capturing ABS_MT_TOUCH_MAJOR or ABS_MT_TOUCH_MINOR, but those describe the size of the touch on the screen. For me a finger shows up as around major=17;minor=8. The edge of my hand has a lot of different sizes, but it seems like it's always at least major >= 26 OR minor >= 17, so maybe that's a reasonable threshold.

[raisjn]

i did know about those docs and have read about the axis minor/major, but didn't think about how to use them for palm detection, nice!

it seems like this diff is not behaving like i thought it would - but at least you have ideas for how to fix palm touch :-D

i think that holding prev_ev and using it as the base makes a lot of sense, so i'd like to achieve that

[raisjn]

ok, i think this works for coalescing the previous event (tested remux, harmony, mines, simple, genie) with touch and stylus. the major piece is in d96ebde.

genie gestures work and now use count_fingers() for filtering instead of the slot.

in addition to those docs, i also used https://elixir.bootlin.com/linux/latest/source/include/linux/input.h.

[raisjn]

have to do some more testing and debugging

was running into strangeness with remux due to remux touch flood, now fixed

i'll likely keep testing this and merge it on the weekend, but preliminary tests seem to show its working at least as well as previously

[mrichards42]

This works great on my end! The only thing that still seems off is the initial finger count (calling count_fingers() does always end up with the right number).

[mrichards42]

I think the event = prev_ev part is redundant? Also should this have a call to event.finalize() somewhere?

[raisjn]

event.finalize() is called on EV_SYN

[mrichards42]

oh you're totally right, it's like 5 lines above this 🤦

[mrichards42]

I'm still seeing fingers=0 in a SynMotionEvent handler when going through dynamic_cast<TouchEvent*>(ev.original().get())->fingers. I think it's b/c that isn't copied here. Does this need an explicit copy constructor, or would the default work?

[raisjn]

i removed explicit constructor, hopefully things improve, but it might be that count_fingers() is always useful to call before checking fingers (and we put that in to main_loop?).

[mrichards42]

When we see tracking_id -1 would we want to reset the slot to its initial -1, -1, -1 values? I assume tracking_id=-1 should mean that any existing x,y coords are invalid.

[raisjn]

if the tracking ID is set to -1, next time the slot is used it will get an X and Y coordinate, i believe.

one potential problem with setting to -1 at this point is that this event is still used (i believe) during mainloop's event dispatch.

[raisjn]

thanks for the help with this - it definitely is better code now. i just glanced at docs again and grepped "palm" and saw MT_TOOL_PALM. i'm curious if we will get this tool type or not. (accidentally put comment in wrong PR a moment ago)

UPDATE: nope, i think evtest says MT_TOOL_PALM is not supported (max of ABS_MT_TOOL_TYPE is 1, should be >= 2 when PALM is supported)

[raisjn]

• remove wonky backwards event coalescing, instead start with prev_ev for all events
• remove MouseEvent, its not applicable since we started using resim
• set lifted on TouchEvent when a finger is lifted (TRACKING_ID set to -1)
• add count_fingers() to TouchEvent and use TouchEvent.fingers in gestures.cpy for figuring out how many fingers pressed

[mrichards42]

Seems like this should work -- I haven't tried it out myself, but I'll give it a shot in a little bit on puzzles and let you know if something is obviously wrong.

[mrichards42]

I'm not seeing where marshal updates an event. Is this left over from a previous version, or am I missing something (likely since I don't know this code very well)?

[mrichards42]

ok, I did a little testing of my own, just in a mouse.down event.

• fingers was always 0, even when the last slot was > 0
• so is_multitouch was always false, since that's calculated from fingers
• if I ran through slots myself and checked for slots with left > -1, I usually got the right number of fingers
• ... but not always. sometimes I ended up with not as many fingers registered as were actually on the screen. I'd guess it just depends on how many fingers were in the event when the first EV_SYN was received, even if the next EV_SYN picked up more fingers, so I'm not sure what to do about that.
• the worst part of all this is that if I took the stylus and put my hand on the screen as if to write, my hand only registered as 1 slot, so I'm not sure how to reject touch in that case :(

code I tested with:

 target->mouse.down += [=](auto &ev) {
auto touch_ev = dynamic_cast<input::TouchEvent*>(ev.original.get());
if (touch_ev) {
int f = 0;
for (auto s : touch_ev->slots)
if (s.left > -1)
f++;
std::cerr << "touch event with "
<< touch_ev->fingers << " fingers"
<< "; " << touch_ev->slot << " slots"
<< "; " << f << " slots with left > -1"
<< std::endl;
if (touch_ev->is_multitouch) {
std::cerr << "skipping multitouch" << std::endl;
return;
}
}
};

[mrichards42]

Ah, ok I found https://www.kernel.org/doc/html/v4.18/input/multi-touch-protocol.html (you might already be aware of similar documentation, this is all new to me) which describes what all those ABS_MT_* fields mean. I see we aren't capturing ABS_MT_TOUCH_MAJOR or ABS_MT_TOUCH_MINOR, but those describe the size of the touch on the screen. For me a finger shows up as around major=17;minor=8. The edge of my hand has a lot of different sizes, but it seems like it's always at least major >= 26 OR minor >= 17, so maybe that's a reasonable threshold.

[raisjn]

i did know about those docs and have read about the axis minor/major, but didn't think about how to use them for palm detection, nice!

it seems like this diff is not behaving like i thought it would - but at least you have ideas for how to fix palm touch :-D

i think that holding prev_ev and using it as the base makes a lot of sense, so i'd like to achieve that

[raisjn]

ok, i think this works for coalescing the previous event (tested remux, harmony, mines, simple, genie) with touch and stylus. the major piece is in d96ebde.

genie gestures work and now use count_fingers() for filtering instead of the slot.

in addition to those docs, i also used https://elixir.bootlin.com/linux/latest/source/include/linux/input.h.

[raisjn]

have to do some more testing and debugging

was running into strangeness with remux due to remux touch flood, now fixed

i'll likely keep testing this and merge it on the weekend, but preliminary tests seem to show its working at least as well as previously

[mrichards42]

This works great on my end! The only thing that still seems off is the initial finger count (calling count_fingers() does always end up with the right number).

[mrichards42]

I think the event = prev_ev part is redundant? Also should this have a call to event.finalize() somewhere?

[raisjn]

event.finalize() is called on EV_SYN

[mrichards42]

oh you're totally right, it's like 5 lines above this 🤦

[mrichards42]

I'm still seeing fingers=0 in a SynMotionEvent handler when going through dynamic_cast<TouchEvent*>(ev.original().get())->fingers. I think it's b/c that isn't copied here. Does this need an explicit copy constructor, or would the default work?

[raisjn]

i removed explicit constructor, hopefully things improve, but it might be that count_fingers() is always useful to call before checking fingers (and we put that in to main_loop?).

[mrichards42]

When we see tracking_id -1 would we want to reset the slot to its initial -1, -1, -1 values? I assume tracking_id=-1 should mean that any existing x,y coords are invalid.

[raisjn]

if the tracking ID is set to -1, next time the slot is used it will get an X and Y coordinate, i believe.

one potential problem with setting to -1 at this point is that this event is still used (i believe) during mainloop's event dispatch.

[raisjn]

thanks for the help with this - it definitely is better code now. i just glanced at docs again and grepped "palm" and saw MT_TOOL_PALM. i'm curious if we will get this tool type or not. (accidentally put comment in wrong PR a moment ago)

UPDATE: nope, i think evtest says MT_TOOL_PALM is not supported (max of ABS_MT_TOOL_TYPE is 1, should be >= 2 when PALM is supported)

[raisjn]

• remove wonky backwards event coalescing, instead start with prev_ev for all events
• remove MouseEvent, its not applicable since we started using resim
• set lifted on TouchEvent when a finger is lifted (TRACKING_ID set to -1)
• add count_fingers() to TouchEvent and use TouchEvent.fingers in gestures.cpy for figuring out how many fingers pressed

[mrichards42]

Seems like this should work -- I haven't tried it out myself, but I'll give it a shot in a little bit on puzzles and let you know if something is obviously wrong.

[mrichards42]

I'm not seeing where marshal updates an event. Is this left over from a previous version, or am I missing something (likely since I don't know this code very well)?

[mrichards42]

ok, I did a little testing of my own, just in a mouse.down event.

• fingers was always 0, even when the last slot was > 0
• so is_multitouch was always false, since that's calculated from fingers
• if I ran through slots myself and checked for slots with left > -1, I usually got the right number of fingers
• ... but not always. sometimes I ended up with not as many fingers registered as were actually on the screen. I'd guess it just depends on how many fingers were in the event when the first EV_SYN was received, even if the next EV_SYN picked up more fingers, so I'm not sure what to do about that.
• the worst part of all this is that if I took the stylus and put my hand on the screen as if to write, my hand only registered as 1 slot, so I'm not sure how to reject touch in that case :(

code I tested with:

 target->mouse.down += [=](auto &ev) {
auto touch_ev = dynamic_cast<input::TouchEvent*>(ev.original.get());
if (touch_ev) {
int f = 0;
for (auto s : touch_ev->slots)
if (s.left > -1)
f++;
std::cerr << "touch event with "
<< touch_ev->fingers << " fingers"
<< "; " << touch_ev->slot << " slots"
<< "; " << f << " slots with left > -1"
<< std::endl;
if (touch_ev->is_multitouch) {
std::cerr << "skipping multitouch" << std::endl;
return;
}
}
};

[mrichards42]

Ah, ok I found https://www.kernel.org/doc/html/v4.18/input/multi-touch-protocol.html (you might already be aware of similar documentation, this is all new to me) which describes what all those ABS_MT_* fields mean. I see we aren't capturing ABS_MT_TOUCH_MAJOR or ABS_MT_TOUCH_MINOR, but those describe the size of the touch on the screen. For me a finger shows up as around major=17;minor=8. The edge of my hand has a lot of different sizes, but it seems like it's always at least major >= 26 OR minor >= 17, so maybe that's a reasonable threshold.

[raisjn]

i did know about those docs and have read about the axis minor/major, but didn't think about how to use them for palm detection, nice!

it seems like this diff is not behaving like i thought it would - but at least you have ideas for how to fix palm touch :-D

i think that holding prev_ev and using it as the base makes a lot of sense, so i'd like to achieve that

[raisjn]

ok, i think this works for coalescing the previous event (tested remux, harmony, mines, simple, genie) with touch and stylus. the major piece is in d96ebde.

genie gestures work and now use count_fingers() for filtering instead of the slot.

in addition to those docs, i also used https://elixir.bootlin.com/linux/latest/source/include/linux/input.h.

[raisjn]

have to do some more testing and debugging

was running into strangeness with remux due to remux touch flood, now fixed

i'll likely keep testing this and merge it on the weekend, but preliminary tests seem to show its working at least as well as previously

[mrichards42]

This works great on my end! The only thing that still seems off is the initial finger count (calling count_fingers() does always end up with the right number).

[mrichards42]

I think the event = prev_ev part is redundant? Also should this have a call to event.finalize() somewhere?

[raisjn]

event.finalize() is called on EV_SYN

[mrichards42]

oh you're totally right, it's like 5 lines above this 🤦

[mrichards42]

I'm still seeing fingers=0 in a SynMotionEvent handler when going through dynamic_cast<TouchEvent*>(ev.original().get())->fingers. I think it's b/c that isn't copied here. Does this need an explicit copy constructor, or would the default work?

[raisjn]

i removed explicit constructor, hopefully things improve, but it might be that count_fingers() is always useful to call before checking fingers (and we put that in to main_loop?).

[mrichards42]

When we see tracking_id -1 would we want to reset the slot to its initial -1, -1, -1 values? I assume tracking_id=-1 should mean that any existing x,y coords are invalid.

[raisjn]

if the tracking ID is set to -1, next time the slot is used it will get an X and Y coordinate, i believe.

one potential problem with setting to -1 at this point is that this event is still used (i believe) during mainloop's event dispatch.

[raisjn]

thanks for the help with this - it definitely is better code now. i just glanced at docs again and grepped "palm" and saw MT_TOOL_PALM. i'm curious if we will get this tool type or not. (accidentally put comment in wrong PR a moment ago)

UPDATE: nope, i think evtest says MT_TOOL_PALM is not supported (max of ABS_MT_TOOL_TYPE is 1, should be >= 2 when PALM is supported)

[raisjn]

• remove wonky backwards event coalescing, instead start with prev_ev for all events
• remove MouseEvent, its not applicable since we started using resim
• set lifted on TouchEvent when a finger is lifted (TRACKING_ID set to -1)
• add count_fingers() to TouchEvent and use TouchEvent.fingers in gestures.cpy for figuring out how many fingers pressed

[mrichards42]

Seems like this should work -- I haven't tried it out myself, but I'll give it a shot in a little bit on puzzles and let you know if something is obviously wrong.

[mrichards42]

I'm not seeing where marshal updates an event. Is this left over from a previous version, or am I missing something (likely since I don't know this code very well)?

[mrichards42]

ok, I did a little testing of my own, just in a mouse.down event.

• fingers was always 0, even when the last slot was > 0
• so is_multitouch was always false, since that's calculated from fingers
• if I ran through slots myself and checked for slots with left > -1, I usually got the right number of fingers
• ... but not always. sometimes I ended up with not as many fingers registered as were actually on the screen. I'd guess it just depends on how many fingers were in the event when the first EV_SYN was received, even if the next EV_SYN picked up more fingers, so I'm not sure what to do about that.
• the worst part of all this is that if I took the stylus and put my hand on the screen as if to write, my hand only registered as 1 slot, so I'm not sure how to reject touch in that case :(

code I tested with:

 target->mouse.down += [=](auto &ev) {
auto touch_ev = dynamic_cast<input::TouchEvent*>(ev.original.get());
if (touch_ev) {
int f = 0;
for (auto s : touch_ev->slots)
if (s.left > -1)
f++;
std::cerr << "touch event with "
<< touch_ev->fingers << " fingers"
<< "; " << touch_ev->slot << " slots"
<< "; " << f << " slots with left > -1"
<< std::endl;
if (touch_ev->is_multitouch) {
std::cerr << "skipping multitouch" << std::endl;
return;
}
}
};

[mrichards42]

Ah, ok I found https://www.kernel.org/doc/html/v4.18/input/multi-touch-protocol.html (you might already be aware of similar documentation, this is all new to me) which describes what all those ABS_MT_* fields mean. I see we aren't capturing ABS_MT_TOUCH_MAJOR or ABS_MT_TOUCH_MINOR, but those describe the size of the touch on the screen. For me a finger shows up as around major=17;minor=8. The edge of my hand has a lot of different sizes, but it seems like it's always at least major >= 26 OR minor >= 17, so maybe that's a reasonable threshold.

[raisjn]

i did know about those docs and have read about the axis minor/major, but didn't think about how to use them for palm detection, nice!

it seems like this diff is not behaving like i thought it would - but at least you have ideas for how to fix palm touch :-D

i think that holding prev_ev and using it as the base makes a lot of sense, so i'd like to achieve that

[raisjn]

ok, i think this works for coalescing the previous event (tested remux, harmony, mines, simple, genie) with touch and stylus. the major piece is in d96ebde.

genie gestures work and now use count_fingers() for filtering instead of the slot.

in addition to those docs, i also used https://elixir.bootlin.com/linux/latest/source/include/linux/input.h.

[raisjn]

have to do some more testing and debugging

was running into strangeness with remux due to remux touch flood, now fixed

i'll likely keep testing this and merge it on the weekend, but preliminary tests seem to show its working at least as well as previously

[mrichards42]

This works great on my end! The only thing that still seems off is the initial finger count (calling count_fingers() does always end up with the right number).

[mrichards42]

I think the event = prev_ev part is redundant? Also should this have a call to event.finalize() somewhere?

[raisjn]

event.finalize() is called on EV_SYN

[mrichards42]

oh you're totally right, it's like 5 lines above this 🤦

[mrichards42]

I'm still seeing fingers=0 in a SynMotionEvent handler when going through dynamic_cast<TouchEvent*>(ev.original().get())->fingers. I think it's b/c that isn't copied here. Does this need an explicit copy constructor, or would the default work?

[raisjn]

i removed explicit constructor, hopefully things improve, but it might be that count_fingers() is always useful to call before checking fingers (and we put that in to main_loop?).

[mrichards42]

When we see tracking_id -1 would we want to reset the slot to its initial -1, -1, -1 values? I assume tracking_id=-1 should mean that any existing x,y coords are invalid.

[raisjn]

if the tracking ID is set to -1, next time the slot is used it will get an X and Y coordinate, i believe.

one potential problem with setting to -1 at this point is that this event is still used (i believe) during mainloop's event dispatch.

[raisjn]

thanks for the help with this - it definitely is better code now. i just glanced at docs again and grepped "palm" and saw MT_TOOL_PALM. i'm curious if we will get this tool type or not. (accidentally put comment in wrong PR a moment ago)

UPDATE: nope, i think evtest says MT_TOOL_PALM is not supported (max of ABS_MT_TOOL_TYPE is 1, should be >= 2 when PALM is supported)

[raisjn]

• remove wonky backwards event coalescing, instead start with prev_ev for all events
• remove MouseEvent, its not applicable since we started using resim
• set lifted on TouchEvent when a finger is lifted (TRACKING_ID set to -1)
• add count_fingers() to TouchEvent and use TouchEvent.fingers in gestures.cpy for figuring out how many fingers pressed

[mrichards42]

Seems like this should work -- I haven't tried it out myself, but I'll give it a shot in a little bit on puzzles and let you know if something is obviously wrong.

[mrichards42]

I'm not seeing where marshal updates an event. Is this left over from a previous version, or am I missing something (likely since I don't know this code very well)?

[mrichards42]

ok, I did a little testing of my own, just in a mouse.down event.

• fingers was always 0, even when the last slot was > 0
• so is_multitouch was always false, since that's calculated from fingers
• if I ran through slots myself and checked for slots with left > -1, I usually got the right number of fingers
• ... but not always. sometimes I ended up with not as many fingers registered as were actually on the screen. I'd guess it just depends on how many fingers were in the event when the first EV_SYN was received, even if the next EV_SYN picked up more fingers, so I'm not sure what to do about that.
• the worst part of all this is that if I took the stylus and put my hand on the screen as if to write, my hand only registered as 1 slot, so I'm not sure how to reject touch in that case :(

code I tested with:

 target->mouse.down += [=](auto &ev) {
auto touch_ev = dynamic_cast<input::TouchEvent*>(ev.original.get());
if (touch_ev) {
int f = 0;
for (auto s : touch_ev->slots)
if (s.left > -1)
f++;
std::cerr << "touch event with "
<< touch_ev->fingers << " fingers"
<< "; " << touch_ev->slot << " slots"
<< "; " << f << " slots with left > -1"
<< std::endl;
if (touch_ev->is_multitouch) {
std::cerr << "skipping multitouch" << std::endl;
return;
}
}
};

[mrichards42]

Ah, ok I found https://www.kernel.org/doc/html/v4.18/input/multi-touch-protocol.html (you might already be aware of similar documentation, this is all new to me) which describes what all those ABS_MT_* fields mean. I see we aren't capturing ABS_MT_TOUCH_MAJOR or ABS_MT_TOUCH_MINOR, but those describe the size of the touch on the screen. For me a finger shows up as around major=17;minor=8. The edge of my hand has a lot of different sizes, but it seems like it's always at least major >= 26 OR minor >= 17, so maybe that's a reasonable threshold.

[raisjn]

i did know about those docs and have read about the axis minor/major, but didn't think about how to use them for palm detection, nice!

it seems like this diff is not behaving like i thought it would - but at least you have ideas for how to fix palm touch :-D

i think that holding prev_ev and using it as the base makes a lot of sense, so i'd like to achieve that

[raisjn]

ok, i think this works for coalescing the previous event (tested remux, harmony, mines, simple, genie) with touch and stylus. the major piece is in d96ebde.

genie gestures work and now use count_fingers() for filtering instead of the slot.

in addition to those docs, i also used https://elixir.bootlin.com/linux/latest/source/include/linux/input.h.

[raisjn]

have to do some more testing and debugging

was running into strangeness with remux due to remux touch flood, now fixed

i'll likely keep testing this and merge it on the weekend, but preliminary tests seem to show its working at least as well as previously

[mrichards42]

This works great on my end! The only thing that still seems off is the initial finger count (calling count_fingers() does always end up with the right number).

[mrichards42]

I think the event = prev_ev part is redundant? Also should this have a call to event.finalize() somewhere?

[raisjn]

event.finalize() is called on EV_SYN

[mrichards42]

oh you're totally right, it's like 5 lines above this 🤦

[mrichards42]

I'm still seeing fingers=0 in a SynMotionEvent handler when going through dynamic_cast<TouchEvent*>(ev.original().get())->fingers. I think it's b/c that isn't copied here. Does this need an explicit copy constructor, or would the default work?

[raisjn]

i removed explicit constructor, hopefully things improve, but it might be that count_fingers() is always useful to call before checking fingers (and we put that in to main_loop?).

[mrichards42]

When we see tracking_id -1 would we want to reset the slot to its initial -1, -1, -1 values? I assume tracking_id=-1 should mean that any existing x,y coords are invalid.

[raisjn]

if the tracking ID is set to -1, next time the slot is used it will get an X and Y coordinate, i believe.

one potential problem with setting to -1 at this point is that this event is still used (i believe) during mainloop's event dispatch.

[raisjn]

thanks for the help with this - it definitely is better code now. i just glanced at docs again and grepped "palm" and saw MT_TOOL_PALM. i'm curious if we will get this tool type or not. (accidentally put comment in wrong PR a moment ago)

UPDATE: nope, i think evtest says MT_TOOL_PALM is not supported (max of ABS_MT_TOOL_TYPE is 1, should be >= 2 when PALM is supported)

[raisjn]

• remove wonky backwards event coalescing, instead start with prev_ev for all events
• remove MouseEvent, its not applicable since we started using resim
• set lifted on TouchEvent when a finger is lifted (TRACKING_ID set to -1)
• add count_fingers() to TouchEvent and use TouchEvent.fingers in gestures.cpy for figuring out how many fingers pressed

[mrichards42]

Seems like this should work -- I haven't tried it out myself, but I'll give it a shot in a little bit on puzzles and let you know if something is obviously wrong.

[mrichards42]

I'm not seeing where marshal updates an event. Is this left over from a previous version, or am I missing something (likely since I don't know this code very well)?

[mrichards42]

ok, I did a little testing of my own, just in a mouse.down event.

• fingers was always 0, even when the last slot was > 0
• so is_multitouch was always false, since that's calculated from fingers
• if I ran through slots myself and checked for slots with left > -1, I usually got the right number of fingers
• ... but not always. sometimes I ended up with not as many fingers registered as were actually on the screen. I'd guess it just depends on how many fingers were in the event when the first EV_SYN was received, even if the next EV_SYN picked up more fingers, so I'm not sure what to do about that.
• the worst part of all this is that if I took the stylus and put my hand on the screen as if to write, my hand only registered as 1 slot, so I'm not sure how to reject touch in that case :(

code I tested with:

 target->mouse.down += [=](auto &ev) {
auto touch_ev = dynamic_cast<input::TouchEvent*>(ev.original.get());
if (touch_ev) {
int f = 0;
for (auto s : touch_ev->slots)
if (s.left > -1)
f++;
std::cerr << "touch event with "
<< touch_ev->fingers << " fingers"
<< "; " << touch_ev->slot << " slots"
<< "; " << f << " slots with left > -1"
<< std::endl;
if (touch_ev->is_multitouch) {
std::cerr << "skipping multitouch" << std::endl;
return;
}
}
};

[mrichards42]

Ah, ok I found https://www.kernel.org/doc/html/v4.18/input/multi-touch-protocol.html (you might already be aware of similar documentation, this is all new to me) which describes what all those ABS_MT_* fields mean. I see we aren't capturing ABS_MT_TOUCH_MAJOR or ABS_MT_TOUCH_MINOR, but those describe the size of the touch on the screen. For me a finger shows up as around major=17;minor=8. The edge of my hand has a lot of different sizes, but it seems like it's always at least major >= 26 OR minor >= 17, so maybe that's a reasonable threshold.

[raisjn]

i did know about those docs and have read about the axis minor/major, but didn't think about how to use them for palm detection, nice!

it seems like this diff is not behaving like i thought it would - but at least you have ideas for how to fix palm touch :-D

i think that holding prev_ev and using it as the base makes a lot of sense, so i'd like to achieve that

[raisjn]

ok, i think this works for coalescing the previous event (tested remux, harmony, mines, simple, genie) with touch and stylus. the major piece is in d96ebde.

genie gestures work and now use count_fingers() for filtering instead of the slot.

in addition to those docs, i also used https://elixir.bootlin.com/linux/latest/source/include/linux/input.h.

[raisjn]

have to do some more testing and debugging

was running into strangeness with remux due to remux touch flood, now fixed

i'll likely keep testing this and merge it on the weekend, but preliminary tests seem to show its working at least as well as previously

[mrichards42]

This works great on my end! The only thing that still seems off is the initial finger count (calling count_fingers() does always end up with the right number).

[mrichards42]

I think the event = prev_ev part is redundant? Also should this have a call to event.finalize() somewhere?

[raisjn]

event.finalize() is called on EV_SYN

[mrichards42]

oh you're totally right, it's like 5 lines above this 🤦

[mrichards42]

I'm still seeing fingers=0 in a SynMotionEvent handler when going through dynamic_cast<TouchEvent*>(ev.original().get())->fingers. I think it's b/c that isn't copied here. Does this need an explicit copy constructor, or would the default work?

[raisjn]

i removed explicit constructor, hopefully things improve, but it might be that count_fingers() is always useful to call before checking fingers (and we put that in to main_loop?).

[mrichards42]

When we see tracking_id -1 would we want to reset the slot to its initial -1, -1, -1 values? I assume tracking_id=-1 should mean that any existing x,y coords are invalid.

[raisjn]

if the tracking ID is set to -1, next time the slot is used it will get an X and Y coordinate, i believe.

one potential problem with setting to -1 at this point is that this event is still used (i believe) during mainloop's event dispatch.

[raisjn]

thanks for the help with this - it definitely is better code now. i just glanced at docs again and grepped "palm" and saw MT_TOOL_PALM. i'm curious if we will get this tool type or not. (accidentally put comment in wrong PR a moment ago)

UPDATE: nope, i think evtest says MT_TOOL_PALM is not supported (max of ABS_MT_TOOL_TYPE is 1, should be >= 2 when PALM is supported)

[raisjn]

• remove wonky backwards event coalescing, instead start with prev_ev for all events
• remove MouseEvent, its not applicable since we started using resim
• set lifted on TouchEvent when a finger is lifted (TRACKING_ID set to -1)
• add count_fingers() to TouchEvent and use TouchEvent.fingers in gestures.cpy for figuring out how many fingers pressed

[mrichards42]

Seems like this should work -- I haven't tried it out myself, but I'll give it a shot in a little bit on puzzles and let you know if something is obviously wrong.

[mrichards42]

I'm not seeing where marshal updates an event. Is this left over from a previous version, or am I missing something (likely since I don't know this code very well)?

[mrichards42]

ok, I did a little testing of my own, just in a mouse.down event.

• fingers was always 0, even when the last slot was > 0
• so is_multitouch was always false, since that's calculated from fingers
• if I ran through slots myself and checked for slots with left > -1, I usually got the right number of fingers
• ... but not always. sometimes I ended up with not as many fingers registered as were actually on the screen. I'd guess it just depends on how many fingers were in the event when the first EV_SYN was received, even if the next EV_SYN picked up more fingers, so I'm not sure what to do about that.
• the worst part of all this is that if I took the stylus and put my hand on the screen as if to write, my hand only registered as 1 slot, so I'm not sure how to reject touch in that case :(

code I tested with:

 target->mouse.down += [=](auto &ev) {
auto touch_ev = dynamic_cast<input::TouchEvent*>(ev.original.get());
if (touch_ev) {
int f = 0;
for (auto s : touch_ev->slots)
if (s.left > -1)
f++;
std::cerr << "touch event with "
<< touch_ev->fingers << " fingers"
<< "; " << touch_ev->slot << " slots"
<< "; " << f << " slots with left > -1"
<< std::endl;
if (touch_ev->is_multitouch) {
std::cerr << "skipping multitouch" << std::endl;
return;
}
}
};

[mrichards42]

Ah, ok I found https://www.kernel.org/doc/html/v4.18/input/multi-touch-protocol.html (you might already be aware of similar documentation, this is all new to me) which describes what all those ABS_MT_* fields mean. I see we aren't capturing ABS_MT_TOUCH_MAJOR or ABS_MT_TOUCH_MINOR, but those describe the size of the touch on the screen. For me a finger shows up as around major=17;minor=8. The edge of my hand has a lot of different sizes, but it seems like it's always at least major >= 26 OR minor >= 17, so maybe that's a reasonable threshold.

[raisjn]

i did know about those docs and have read about the axis minor/major, but didn't think about how to use them for palm detection, nice!

it seems like this diff is not behaving like i thought it would - but at least you have ideas for how to fix palm touch :-D

i think that holding prev_ev and using it as the base makes a lot of sense, so i'd like to achieve that

[raisjn]

ok, i think this works for coalescing the previous event (tested remux, harmony, mines, simple, genie) with touch and stylus. the major piece is in d96ebde.

genie gestures work and now use count_fingers() for filtering instead of the slot.

in addition to those docs, i also used https://elixir.bootlin.com/linux/latest/source/include/linux/input.h.

[raisjn]

have to do some more testing and debugging

was running into strangeness with remux due to remux touch flood, now fixed

i'll likely keep testing this and merge it on the weekend, but preliminary tests seem to show its working at least as well as previously

[mrichards42]

This works great on my end! The only thing that still seems off is the initial finger count (calling count_fingers() does always end up with the right number).

[mrichards42]

I think the event = prev_ev part is redundant? Also should this have a call to event.finalize() somewhere?

[raisjn]

event.finalize() is called on EV_SYN

[mrichards42]

oh you're totally right, it's like 5 lines above this 🤦

[mrichards42]

I'm still seeing fingers=0 in a SynMotionEvent handler when going through dynamic_cast<TouchEvent*>(ev.original().get())->fingers. I think it's b/c that isn't copied here. Does this need an explicit copy constructor, or would the default work?

[raisjn]

i removed explicit constructor, hopefully things improve, but it might be that count_fingers() is always useful to call before checking fingers (and we put that in to main_loop?).

[mrichards42]

When we see tracking_id -1 would we want to reset the slot to its initial -1, -1, -1 values? I assume tracking_id=-1 should mean that any existing x,y coords are invalid.

[raisjn]

if the tracking ID is set to -1, next time the slot is used it will get an X and Y coordinate, i believe.

one potential problem with setting to -1 at this point is that this event is still used (i believe) during mainloop's event dispatch.

[raisjn]

thanks for the help with this - it definitely is better code now. i just glanced at docs again and grepped "palm" and saw MT_TOOL_PALM. i'm curious if we will get this tool type or not. (accidentally put comment in wrong PR a moment ago)

UPDATE: nope, i think evtest says MT_TOOL_PALM is not supported (max of ABS_MT_TOOL_TYPE is 1, should be >= 2 when PALM is supported)