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 MT slots #1439

Closed
wants to merge 2 commits into from
Closed

Clobbering of MT slots #1439

wants to merge 2 commits into from

Conversation

mapinguari
Copy link

@mapinguari mapinguari commented Dec 9, 2021

As per discussion on #1437.

I have made changes to return an fd on device open and to pass the fd that generated the event as part of the lua table returned.

Having made these changes I think it probably is unnecessary to pass the fd with EVERY event as ultimately the main loop in waitForInput only returns a table of event tables for one device at a time, so could have waitForInput return a triple instead. hypothetically.


This change is Reviewable

… field which contains the source fd of the event
input/input.c Outdated
@@ -312,7 +317,7 @@ static inline size_t drain_input_queue(lua_State* L, struct input_event* input_q

// Iterate over every input event in the queue buffer
for (const struct input_event* event = input_queue; event < input_queue + ev_count; event++) {
set_event_table(L, event); // Pushed a new ev table all filled up at the top of the stack (that's -1)
set_event_table(L, event,fd); // Pushed a new ev table all filled up at the top of the stack (that's -1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, I think clang-format will take care of it by itself?

input/input.c Outdated
@@ -406,7 +411,7 @@ static int waitForInput(lua_State* L)

if ((size_t) len == queue_available_size) {
// If we're out of buffer space in the queue, drain it *now*
j = drain_input_queue(L, input_queue, ev_count, j);
j = drain_input_queue(L, input_queue, ev_count,inputfds[i], j);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same nit

input/input.c Outdated
@@ -150,6 +150,7 @@ static int openInputDevice(lua_State* L)

// We're done w/ inputdevice, pop it
lua_settop(L, 0);
lua_pushnumber(L,inputfds[fd_idx]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double nit

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm besides the formatting nits (spaces for indentation, space after comma), but as I said, clang-format. ^_^

@mapinguari
Copy link
Author

mapinguari commented Dec 9, 2021

ugh sorry, stupid of me. Ok, those should be updated

@NiLuJe
Copy link
Member

NiLuJe commented Dec 10, 2021

I'm gonna go with "Not good enough" for now, sorry. (This is just a quick veto to merging this as-is, until I have more than a shower's worth of time to think about it).

In decreasing order of importance:

  • This breaks initial state handling (I alluded to this in the other thread). The only way I could think of to avoid that would be to:
  • Pass the same info along during the openInputDevice code, so that front's init has it on init, so it can tweak the initial state appropriately.
  • That would probably imply using the inputfds index instead of the fd number, because this would allow us to make sure initial state only applies only to the touch-screen (which is importanty on devices with wonky initial state, because they only have a single iunput device: the scree,).
  • That would require making sure we always open the touchscreen device first, which should be easy enough, if that's not already the case (because otherwise, we'd need to do some kinds of cap checks on open to discriminate screen from pen, and that way lies madness, especially on older kernels).
  • This should probably be using a small-ish seed value in addition to the fd number, because that may be low enough to be lower than the maximum touch points of the input device, which could still leave room for conflicts.
  • I like the idea of passing the info along in w<aitForInput... as long is it doesn't make the frontend code that much more horrible ;).
  • I'd like to see the matching frontend changes before merging this ;).

Copy link
Member

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it wrong, see above ;p.

(This is just to generate a big red cross to avoid drive-by merges).

@mapinguari
Copy link
Author

Ok, points taken.

I will close this for the moment on my end.

@mapinguari mapinguari closed this Dec 10, 2021
@Frenzie
Copy link
Member

Frenzie commented Dec 10, 2021

Pass the same info along during the openInputDevice code, so that front's init has it on init, so it can tweak the initial state appropriately.

I thought that was implied by this PR btw.

@mapinguari
Copy link
Author

I think my lack of clarity didn't aid this PR.

I was thinking that the fd would just be passed between the C and the lua so that frontend can deal with it as it sees appropriate. Not that the ABS_MT_SLOT values would be automatically converted to ABS_MT_SLOT + fd (this would obviously cause major differences with handling gestures and the inited state [not that I fully understand what they would be at this point :)]).

This confusion aside however I totally take that the lack of changes on the lua side makes this a bit futile at this point and adds a not insignificant amount to the size of the tables being sent back to lua. So I am happy for this to sit quietly until I have made some changes on the front that either make this PR worthwhile, or change the PR to reduce overhead and satisfy you guys and myself that it isn't going to cause some pernicious issues in the gesture detecting.

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

Successfully merging this pull request may close these issues.

3 participants