Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clobbering of slots in MT protocol handling #1437

Open
mapinguari opened this issue Dec 6, 2021 · 12 comments
Open

Clobbering of slots in MT protocol handling #1437

mapinguari opened this issue Dec 6, 2021 · 12 comments

Comments

@mapinguari
Copy link

mapinguari commented Dec 6, 2021

Hi all,

If a device has multiple input fds each of which is conforming to the MT B protocol you end up with clobbering of MT slots.

For example, if I have two touch devices as above.

fd 1 produces:
`ABS_MT_SLOT 0

ABS_MT_TRACKING_ID 45

ABS_MT_POSITION_X 0

ABS_MT_POSITION_Y 0

SYN_REPORT
`
then some time later produces

`ABS_MT_POSITION_X 100

ABS_MT_POSITION_Y 100

SYN_REPORT`

and fd2 produces:
`ABS_MT_SLOT 0

ABS_MT_TRACKING_ID 6

ABS_MT_POSITION_X 1000

ABS_MT_POSITION_Y 1000

SYN_REPORT`

From two distinct touch events, say, one from a finger and one from a pen tool then depending on the order of the reads from the fds you can end up with any permutation of touches at (0,0), (100,100) and (1000,1000).

The input functions need to make sure they are keeping track of which fd was the last to be read from, what the last slot it was producing events for was and if it is a different fd to ensure an ABS_MT_SLOT event for the current fd's last slot is inserted into the table of events being generated. It probably also needs to maintain a mapping between an individual fd's slot number and a "globally unique" slot number so you don't end up with clobbering of slots between different input devices.

I don't think this is a driver issue, each individual driver is doing the correct thing and respecting the MT protocol, it is just when you combine multiple input sources together into a combined stream you end up with unfortunate interlacing of the event streams.

I am not sure how much appetite you have to look at this tho.

Any feedback apprecited!

@NiLuJe
Copy link
Member

NiLuJe commented Dec 6, 2021

It's a design simplification, as this should be an exceedingly rare occurrence in practice.

(Also, we also clobber slots on contact losses on some Kobo protocols later, anyway).

@mapinguari
Copy link
Author

mapinguari commented Dec 6, 2021

Fair enough. I am working with the remarkable and its happening regularly.

I will do by best to handle it further downstream.

Want me to close this?

@NiLuJe
Copy link
Member

NiLuJe commented Dec 6, 2021

Want me to close this?

Not necessarily, base's issue tracker isn't crowded, so leaving this open isn't really problematic.

As far as I'm concerned, this would be fairly low on the priority list, even if I actually had time to work on this (spoiler: I don't have any time ;p).

@NiLuJe
Copy link
Member

NiLuJe commented Dec 6, 2021

Could possibly be as simple as passing along the fd number from the C module to Lua-land and simply using fd + ABS_MT_SLOT in GestureManager, as we don't actually care about the slots value per se (in fact, some Kobo devices don't even emit a slot 0).

@mapinguari
Copy link
Author

Yeah I did think about something along those lines, but I am not entirely sure what goes on in KOReader wrt slots and ids once they go into gesture detector.

I am just getting some funny double free exceptions from somewhere and it seems to be related to touch and tool events mingling.

@NiLuJe
Copy link
Member

NiLuJe commented Dec 6, 2021

Might need to be slightly more complex, because of the initial state in https://github.com/koreader/koreader/blob/19a607b5484299623cc57563f7a4baa3f49faff5/frontend/device/input.lua#L220-L226 (among possibly other things).

If we had a guarantee that the first event device we open is the touch screen, we might be able to use the inputfds index instead, but we don't.

@NiLuJe
Copy link
Member

NiLuJe commented Dec 6, 2021

It would probably not be too gnarly to deal with, I just have zero time to devote to this (or anything else, really :/) for the foreseeable future.

@mapinguari
Copy link
Author

ok cool.

I will leave it be.

@NiLuJe
Copy link
Member

NiLuJe commented Dec 6, 2021

There might be a similar issue with cross-device ABS_MT_TRACKING_ID values, too (although a collision there is possibly much rarer).

@mapinguari
Copy link
Author

From my little bit of watching the event streams on this one device then yes, it looks rarer. One device always uses the same ABS_MT_TRACKING_ID 0, and the other just continuously increments it.

@NiLuJe
Copy link
Member

NiLuJe commented Dec 6, 2021

As far as passing along the fd info, it should only take some minor contortions to stuff it in the table ;).

@mapinguari
Copy link
Author

that is probably the simplest unobtrusive fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants