RFC: Add hid_read_interrupt API for thread-safe read cancellation#799
Draft
Youw wants to merge 6 commits into
Draft
RFC: Add hid_read_interrupt API for thread-safe read cancellation#799Youw wants to merge 6 commits into
Youw wants to merge 6 commits into
Conversation
New functions on hid_device, implemented across all five backends (linux, libusb, mac, windows, netbsd): hid_read_interrupt - asynchronously cancel a blocked read hid_is_read_interrupted - query the sticky interrupt state hid_read_clear_interrupt - clear the state, allowing reads to resume hid_read_interrupt is the only hidapi function safe to call concurrently with another function on the same device. Other device operations (write, feature/output reports, info getters) are unaffected. Allows a dedicated reader thread to be stopped without busy-polling on a short timeout. New hid_read_test tool, to check new functionality. Resolves: #146 Assisted-by: Claude:claude-opus-4.7
Youw
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds a new public HIDAPI mechanism to asynchronously interrupt a blocked hid_read()/hid_read_timeout() call in a thread-safe way (intended to allow clean shutdown of dedicated reader threads without short timeouts/busy polling), and introduces a small hid_read_test utility to exercise the behavior.
Changes:
- Adds
hid_read_interrupt(),hid_is_read_interrupted(), andhid_read_clear_interrupt()to the public header and implements them across all backends. - Updates backend
hid_read_timeout()implementations to return-1with an “operation interrupted” read error when interruption is requested. - Adds a new optional
hid_read_testtool and a top-level CMake option to build it.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
windows/hid.c |
Implements interrupt via a manual-reset event + interlocked flag and integrates it into WaitForMultipleObjects() in hid_read_timeout(). |
netbsd/hid.c |
Implements interrupt via a nonblocking pipe added to the poll() fd set, plus a sticky interrupt flag. |
mac/hid.c |
Implements interrupt via a mutex-protected flag + condition broadcast integrated into the read wait loops. |
linux/hid.c |
Implements interrupt via eventfd added to poll() plus a sticky interrupt flag; adjusts failure cleanup path to call hid_close(). |
libusb/hid.c |
Implements interrupt via a mutex-protected flag + condition broadcast integrated into the read wait loops. |
hidapi/hidapi.h |
Adds the public API declarations and documents the intended thread-safety/semantics. |
hid_read_test/main.cpp |
New command-line tool that opens a device, runs a blocking read thread, and interrupts on Enter/Ctrl+C. |
hid_read_test/CMakeLists.txt |
Adds build/install rules for the new test tool (standalone or as a subdir). |
CMakeLists.txt |
Adds HIDAPI_BUILD_HID_READ_TEST option to include the new tool in the build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1160
to
+1176
| @@ -1149,12 +1165,15 @@ int HID_API_EXPORT hid_read_timeout(hid_device *dev, unsigned char *data, size_t | |||
| properly report device disconnection through read() when | |||
| in non-blocking mode. */ | |||
| int ret; | |||
| struct pollfd fds; | |||
|
|
|||
| fds.fd = dev->device_handle; | |||
| fds.events = POLLIN; | |||
| fds.revents = 0; | |||
| ret = poll(&fds, 1, milliseconds); | |||
| struct pollfd fds[2]; | |||
|
|
|||
| fds[0].fd = dev->device_handle; | |||
| fds[0].events = POLLIN; | |||
| fds[0].revents = 0; | |||
| fds[1].fd = dev->interrupt_efd; | |||
| fds[1].events = POLLIN; | |||
| fds[1].revents = 0; | |||
| ret = poll(fds, 2, milliseconds); | |||
Comment on lines
+1216
to
+1219
| /* Interrupt fired and no data is ready. The pending ReadFile is | ||
| left in flight; it will resume on the next hid_read_timeout() | ||
| call after hid_read_clear_interrupt(). */ | ||
| register_string_error_to_buffer(&dev->last_read_error_str, L"hid_read_timeout: operation interrupted"); |
Comment on lines
+130
to
+133
| #ifdef _WIN32 | ||
| std::signal(SIGINT, on_signal); | ||
| #ifdef SIGTERM | ||
| std::signal(SIGTERM, on_signal); |
Comment on lines
+55
to
+59
| extern "C" void on_signal(int) | ||
| { | ||
| /* async-signal-safe: atomic store only. Main thread will call | ||
| hid_read_interrupt() once cin.get() returns from EINTR. */ | ||
| g_terminate.store(true, std::memory_order_release); |
Comment on lines
+95
to
+97
| option(HIDAPI_BUILD_HID_READ_TEST "Build hid_read_test cmd-line tool" OFF) | ||
| if(HIDAPI_BUILD_HID_READ_TEST) | ||
| add_subdirectory(hid_read_test) |
Comment on lines
+480
to
+481
| 1 if the read pipeline is interrupted, 0 if not, -1 on error. | ||
| Call hid_error(dev) to get the failure reason. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- linux/hid.c: poll() unconditionally in hid_read_timeout(), including the blocking (milliseconds < 0) path, so hid_read_interrupt() can wake a blocking read via the interrupt eventfd. Previously the blocking path went straight to read() and never observed the interrupt, so a dedicated reader thread blocked in hid_read() could not be woken — defeating the documented interrupt -> join -> hid_close() shutdown. - hidapi/hidapi.h: hid_is_read_interrupted() is documented as returning only 1/0; drop the misleading "-1 on error" wording since no backend has an error path here. - hid_read_test/main.cpp: use a volatile sig_atomic_t flag (the type the standard guarantees safe to write from a signal handler) instead of std::atomic<bool>, and observe it from the main thread via a detached input-wait helper so Ctrl+C reliably triggers shutdown on all platforms (including Windows, where the console read isn't interrupted by signals). - CMakeLists.txt: add the hid_read_test* targets to the ASan target list. Assisted-by: Claude:claude-opus-4.8
Add a backend-generic HIDAPI test harness and a unit test for the
hid_read_interrupt() / hid_is_read_interrupted() / hid_read_clear_interrupt()
API.
src/tests/test_virtual_device.h
An opaque, backend-agnostic "virtual HID device" interface (create,
inject input, set feature reply, capture output, open via HIDAPI,
destroy). Test scenarios depend only on this, so the same scenarios can
be reused for other backends by adding a provider.
src/tests/test_virtual_device_uhid.c
Linux provider, backed by the kernel /dev/uhid interface: it makes the
kernel expose a real /dev/hidrawN node that the hidraw backend opens.
Answers GET_REPORT and captures OUTPUT/SET_REPORT via an event pump
thread (useful for future read/write/feature tests too).
src/tests/test_read_interrupt.c
Scenarios: normal read delivery, interrupt-before-read (non-blocking
return), blocking read interrupted from another thread (the regression
guard for the hidraw blocking-path fix), timed read interrupted, sticky
interrupt, clear resumes reads, idempotent interrupt/clear. Uses
pthread_timedjoin_np so a regression fails instead of hanging.
The test self-skips (CTest SKIP_RETURN_CODE 77) when no virtual device can
be created (e.g. no 'uhid' module / insufficient privileges), so it never
fails spuriously.
Build/CI:
- HIDAPI_WITH_TESTS is now offered on Linux (default OFF) and builds
src/tests for the hidraw backend.
- The ubuntu-cmake CI job configures with -DHIDAPI_WITH_TESTS=ON, loads
the uhid module (from linux-modules-extra) and runs ctest as root
(LeakSanitizer off, ASan use-after-free detection on).
Assisted-by: Claude:claude-opus-4.8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New functions on hid_device, implemented across all five backends (linux, libusb, mac, windows, netbsd):
hid_read_interrupt - asynchronously cancel a blocked read
hid_is_read_interrupted - query the sticky interrupt state
hid_read_clear_interrupt - clear the state, allowing reads to resume
hid_read_interrupt is the only hidapi function safe to call concurrently with another function on the same device. Other device operations (write, feature/output reports, info getters) are unaffected. Allows a dedicated reader thread to be stopped without busy-polling on a short timeout.
New hid_read_test tool, to check new functionality.
Resolves: #146
Assisted-by: Claude:claude-opus-4.7