-
Notifications
You must be signed in to change notification settings - Fork 5
Broader handling of architectures; WITH-EVENT-DEVICES macro #8
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
base: master
Are you sure you want to change the base?
Conversation
jtgans
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to not merge this as-is. We need to solve the nonblocking problem in a better way.
Also, I'd like to see the int size problem solved in a better way. Can you split these two things into two different PRs?
| (define-unsigned unsigned-long-int 8)) | ||
| (t 4)) | ||
| #+32-bit(define-unsigned unsigned-long-int 4) | ||
| #+64-bit(define-unsigned unsigned-long-int 8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has always stuck in my craw, and while using these flags seem to work okay, I worry it's the wrong approach.
The more I think about it, the more I think the host CL compiler is the wrong place to put this: it's possible this library could be running in a 32-bit compiler with a 64-bit host kernel, which means these values are entirely wrong for the evdev interface.
We really need a way to identify what the host kernel's word size is, rather than the host compiler's architecture.
| &body body) | ||
| "Opens DEVICE-PATHS for reading, combine individual stream events into | ||
| EVENT-VAR and calls BODY for each event passed in. DEVICE-PATHS must | ||
| exist, otherwise an error condition is signaled." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also worth noting here that the event orders may be out of order, kernel depending, since you're only using a read syscall here instead of poll or otherwise. It's possible that the other devices may be ready, but you're blocked on one earlier in the list.
read-raw-event should probably at the very least be made to work in nonblocking mode for this to work correctly, or these file descriptors should be passed out in a threaded manner, or otherwise poll/select should be setup to do this, otherwise, this function isn't really what you want. I know I'm speaking C here, but that's the underlying interface we have to think about in this case.
When I designed this library, the concept was to keep it focused on doing one thing well (parse out evdev data into something useful for CL), ideally in a reentrant way. That's why I never put in anything relating to nonblocking, threading or anything else.
This meant that I was actually letting threading and nonblocking behavior exist outside of this library in the outer silica project's inputmanager handler, which felt more appropriate. In there, I was using chanl as a means to dispatch events from multiple threads into a producer/consumer setup to solve the blocking problem in a more managed way. See also handler.lisp in cl-event-handler. Effectively, while the code is currently written to only handle one evdev device, the manager could be altered to manage events from multiple handler instances, each running their own with-evdev-device loop.
Please consider the following changes: