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

Add hid_write_timeout per issue #198 #199

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

amullins83
Copy link
Contributor

This extends the API with an hid_write_timeout function that allows an early exit in the event of a write failure (usually caused by devices failing to respond correctly).

single report), followed by the report data (16 bytes). In
this example, the length passed in would be 17.

hid_write() will send the data on the first OUT endpoint, if

Choose a reason for hiding this comment

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

This should probably be hid_write_timeout().

@amullins83
Copy link
Contributor Author

I don't know how to request a non-blocking write on the Mac platform, nor do I have a Mac (virtual or otherwise) to test this on. For now it just ignores the timeout and calls the blocking hid_write(). I'm looking up the correct way to do this now.

@amullins83
Copy link
Contributor Author

I'm not very confident of this Mac implementation. I noticed that hid_read_timeout relies on the pthread library, so I basically cargo-culted that. It's not clear to me at all that this is correct. According to this StackOverflow answer, Apple's IOHIDDeviceSetReportWithCallback function doesn't actually work. Otherwise, I'd use it.

{
res = 0;
}
}

Choose a reason for hiding this comment

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

I'm not sure this is the right approach. If there's a timeout, the thread is left running in the background -- meaning that the write will likely eventually succeed. I think the semantics of hid_write() are that it is supposed to either succeed or fail. I extrapolate that for hid_write_timeout() to be that the write should either succeed, fail, or timeout (not timeout and then eventually succeed without you knowing).

Know what I mean?

Perhaps you should pthread_cancel() the thread after a timeout...?

(I don't know anything about OS X programming, either, so I don't have any better suggestions :-( )

@amullins83
Copy link
Contributor Author

Now, my question is whether the result of the pthread_cancel call should have an effect on the function's result. I'm leaning toward "no" as evidenced by the fact that the current iteration just throws that result away. It seems like there's still a subtle race condition here: if the write takes longer than the timeout but finishes before it can be canceled, the result returned by hid_write_timeout will be wrong. I'm not sure if there's any way around that.

@jsquyres
Copy link

Good point (and bad Apple for not implementing IOHIDDeviceSetReportWithCallback. Sigh).

Yes, there is a way around it, but it's -- of course -- a bit more complicated (multi-threaded programming is always more complicated...): use a global data structure (e.g., a linked list) to return values from the set_report() thread to the hid_write()-calling thread.

For example, protect a global linked list with a mutex. When a thread running set_report() gets a return value, it gets the mutex, puts an entry on the list with the return value, and unlocks the mutex. The hid_write() function can lock the mutex, see if there's anything in the list (and if so, de-queue it), and then unlock the mutex.

This gives safe access to a pipeline to return values from set_report to the hid_write()-calling thread.

Combine this concept with adding some kind of cookie/sequence number in both _SET_REPORT_ARGS and the data structure of the items on the global list. I.e., hid_write() will put a sequence number in _SET_REPORT_ARGS, and the call_set_report_with() will (eventually) put a return value on the global linked list with that same sequence number.

When hid_write() goes to check the global list for a return value, it can dequeue items from the list and ignore / discard any values that do not contain the sequence number that it is expecting.

I don't think the HID API is defined to be thread safe, so there's never a concern with multiple threads invoking hid_write() simultaneously.

@amullins83 amullins83 mentioned this pull request Nov 25, 2014
erikolofsson pushed a commit to Malterlib/hidapi that referenced this pull request Nov 25, 2020
The open systcall is allowed to return 0 as a valid file descriptor,
and considering it an error breaks environments where unnecessary
fds are closed before executing untrusted processes.

Fixes: signal11#199

Signed-off-by: Marc Zyngier <[email protected]>
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.

2 participants