Conversation
…back (#299) - initial API; - Windows backend implementation;
- fix merge conflict - fix indentation in a few places
* netbsd hotplug stubs * Make cygwin happy (fix copied from libusb)
|
keeping it as draft for now |
…EnterCriticalSection (#689)
Fix the possible issues that happen when a register or de-register call is made from inside a callback. The way it works is the same for all platforms and is described below. Resolves: #673 1) The mutex is made recursive, so it can be locked by the same thread multiple times 2) `mutex_ready` kept intact, added 2 more flags, `mutex_in_use` and `cb_list_dirty`. 3) When the `mitex_in_use` flag is set, the Deregister call is no longer allowed to immediately remove any callbacks from the list. Instead, the `events` field in the callback is set to 0, which makes it "invalid", as it will no longer match any events, and the `cb_list_dirty` flag is set to 1 to indicate that the list needs to be checked for invalid events later. 4) When a callback returns a non-zero value, indicating that the callback is to be disarmed and removed from the list, it is marked in the same manner until the processing finishes (unless the callback was called directly by the Register call, in which case it's return value is ignored on purpose) 5) After all the callbacks are processed, if `cb_list_dirty` flag is set, the list of callbacks is checked for any callbacks marked for removal (`events` field set to 0), and those are only removed after all the processing is finished. 6) The Register call is allowed to register callbacks, as it causes no issues so long as the mutex it locks is recursive 7) Since the Register call can also call the new callback if `HID_API_HOTPLUG_ENUMERATE` is specified, `mutex_in_use` flag is set to prevent callback removal in that new callback. 8) The return value of any callbacks called for pre-existing devices is still ignored as per documentation and does not mark them invalid.
| hidapi_thread_mutex_lock(&hid_hotplug_context.libusb_thread); | ||
| while (hid_hotplug_context.queue) { | ||
| struct hid_hotplug_queue *cur_event = hid_hotplug_context.queue; | ||
| process_hotplug_event(cur_event); |
There was a problem hiding this comment.
would it be possible/feasible to postpone the processing of the event until libusb has closed the device? I know that might get complicate but otherwise enumerate would not work (at least as long as libusb/libusb#1532 has not been merged).
There was a problem hiding this comment.
I don't follow your thoughts here. Can you elaborate more details?
There was a problem hiding this comment.
well, libusb opens the device and fires the callback, then hidapi tries to get infos from the device but may fail as it is still open by libusb.
if I run my example program with the libusb backend, I don't get e.g. manufacturer infos but with the hidraw backend I get them.
There was a problem hiding this comment.
I was wrong, libusb does not open the device. but for some reason, hid_enumerate_from_libusb() is not able to open the device in my case (LIBUSB_ERROR_ACCESS) when called by the hotplug handler. If I insert a small delay before libusb_open() it works.
There was a problem hiding this comment.
Interesting finding...
As per documentation - it should be able to use libusb_open from a hotplug callback function.
What version of libusb are you using?
There was a problem hiding this comment.
I use libusb v1.0.27 and this branch for hidapi. Everything can be found here: https://github.com/bearsh/hid/tree/feature/connection-callback including the test program. The project is a go-wrapper around hiadpi. The test application is in cmd/hid-hotplug.
maybe I should file a bug at libusb... I've created libusb/libusb#1586
There was a problem hiding this comment.
something like the following would solve my issue and is also suggested as workaround by @mcuee in libusb/libusb#1586 (comment)
diff --git a/hidapi/libusb/hid.c b/hidapi/libusb/hid.c
index ca8555c..b1c31c5 100644
--- a/hidapi/libusb/hid.c
+++ b/hidapi/libusb/hid.c
@@ -944,7 +944,7 @@ static int should_enumerate_interface(unsigned short vendor_id, const struct lib
return 0;
}
-static struct hid_device_info* hid_enumerate_from_libusb(libusb_device *dev, unsigned short vendor_id, unsigned short product_id)
+static struct hid_device_info* hid_enumerate_from_libusb(libusb_device *dev, unsigned short vendor_id, unsigned short product_id, int hotplug)
{
struct hid_device_info *root = NULL; /* return object */
struct hid_device_info *cur_dev = NULL;
@@ -977,7 +977,11 @@ static struct hid_device_info* hid_enumerate_from_libusb(libusb_device *dev, uns
if (should_enumerate_interface(dev_vid, intf_desc)) {
struct hid_device_info *tmp;
- res = libusb_open(dev, &handle);
+ /* after a hotplug event, retry it 5 time (max 50ms extra latency) */
+ unsigned try = hotplug ? 6 : 1;
+ while (try-- && (res = libusb_open(dev, &handle)) == LIBUSB_ERROR_ACCESS) {
+ usleep(10000);
+ }
#ifdef __ANDROID__
if (handle) {
/* There is (a potential) libusb Android backend, in which
@@ -1058,7 +1062,7 @@ struct hid_device_info HID_API_EXPORT *hid_enumerate(unsigned short vendor_id, u
return NULL;
while ((dev = devs[i++]) != NULL) {
- struct hid_device_info *tmp = hid_enumerate_from_libusb(dev, vendor_id, product_id);
+ struct hid_device_info *tmp = hid_enumerate_from_libusb(dev, vendor_id, product_id, 0);
if (cur_dev) {
cur_dev->next = tmp;
}
@@ -1168,7 +1172,7 @@ static int hid_libusb_hotplug_callback(libusb_context *ctx, libusb_device *devic
static void process_hotplug_event(struct hid_hotplug_queue* msg)
{
if (msg->event == LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED) {
- struct hid_device_info* info = hid_enumerate_from_libusb(msg->device, 0, 0);
+ struct hid_device_info* info = hid_enumerate_from_libusb(msg->device, 0, 0, 1);
struct hid_device_info* info_cur = info;
while (info_cur) {
/* For each device, call all matching callbacks */
There was a problem hiding this comment.
I do not know anything about Go, but let me try out the Go-wrapper over the weekend to see if I can reproduce the issue under Linux or not.
|
is there something I can help with to get this merged? |
|
@Youw any chance the branch could be updated to the fresh master? |
|
Sure, I'll update the branch. Answering to last two comments: lets make a deal here: I'll start looking into this in more details very shortly (within days, at most weeks) - starting with the overall API design, etc. I'll post all my findings (and issues if any), and if anything needed to be adjusted/fixed - I'll probably be asking for help. Also, I really hope we would have a good semi-automatic test for this as well (semi-automatic, since it would probably involve some physical device plug/unplug). If all good when - will merge it into master. |
This doesn't really describe my side of the deal, but - sure, sounds good. I'll do what I can to help in getting this ready.
This PR does already feature an update to the hidtest utility. The update makes it interactive & allows to test the callbacks. I don't know how to make it any more automatic. |
(No I haven't looke at what the modified hidtest currently does, if it already does something like that - that might be good-enought) |
|
I've updated this branch to latest master and fixed conflicts (hopefully right). Next step - I'll review the API and test the usability. |
I don't think my test app fits the description, and if the current hidtest is used in any automated testing scenario, I shall revert it to how it is in the current release. I'm not sure if the new interactive solution has any use, but it can be preserved under a different subfolder (will require a new cmake target, though). As for the automated hotplug test, I'm not sure this is even necessary, but I can implement that too. I believe this can wait until after the review of the API - if the API is to change, the test will have to change too. |
Hi, any news since? |
|
We really would like to use this functionality in OpenRGB's upcoming 1.0 release. Is there any plan to merge this into a hidapi release in the near future? If not, we plan on creating a fork called hidapi-hotplug (just hidapi master with the changes from this branch) and building hidapi-hotplug packages to ship alongside OpenRGB 1.0. I do plan to make OpenRGB 1.0 buildable against standard non-hotplug hidapi but without hotplug capability. Hotplug capability would be a major improvement in user experience for us and @k1-801 has been pushing for it for years. I've been hesitant because of lack of upstream support but I really want to find a way to get this in and maintaining a (hopefully temporary) fork seems like the best action right now. |
|
I failed to find capacity to review this once again, and I apologize for that. |
|
@CalcProgrammer1 does this mean you have tested the hotplug implementation in-depth? Did you test it with the libusb backend? |
|
I personally haven't, I tested the hidraw implementation.
However, @k1-801's been working on this hotplug system for years and has
kept it continuously rebased, and his implementation had support for
libusb-based devices as well has hidraw (which I haven't pulled into mine
yet but intend to). There are a handful of devices with odd or broken HID
descriptors that Linux's hidraw driver fails to properly detect, but we can
still access via the libusb-based hidapi.
…On Thu, Jan 15, 2026 at 8:56 AM bearsh ***@***.***> wrote:
*bearsh* left a comment (libusb/hidapi#674)
<#674 (comment)>
@CalcProgrammer1 <https://github.com/CalcProgrammer1> does this mean you
have tested the hotplug implementation in-depth? Did you test it with the
libusb backend?
I still think something like #674 (comment)
<#674 (comment)> is
needed to make it fully functional...
—
Reply to this email directly, view it on GitHub
<#674 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIAY7GZGZZO36QVP46G2YT4G6TBVAVCNFSM6AAAAABF2KZ5Z2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTONJVGI3DINRZGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I've updated my next_hotplug branch again on OpenRGB, see: https://gitlab.com/CalcProgrammer1/OpenRGB/-/merge_requests/3162 I pulled in support for libusb-wrapped devices and can confirm that the libusb backend also seems to work. For some reason, certain devices (like my HyperX Quadcast S microphone) use HID but have a descriptor that causes hidraw not to detect it, so the only way to access on Linux is using the libusb backend. We have a wrapper for these devices in OpenRGB, so that it normally links against the hidraw backend and uses that for most devices, but we dynamically load the libusb backend and use a wrapper to call its version of the functions for these select few problem devices. With this latest push to next_hotplug, I can hotplug (add device on plug in, remove device on unplug) my HyperX Quadcast S microphone via the libusb wrapper as well as a variety of Razer devices I've been using to test the hidraw path. All is working well on Linux. Have not tested other OSes yet. |
|
I built Windows and MacOS builds of my fork today and got successful builds of OpenRGB (next_hotplug branch) on Windows and MacOS. I've confirmed both are working, both inserting and removing. |
There was a problem hiding this comment.
Pull request overview
This PR introduces cross-platform hotplug (device arrival/removal) support to hidapi, adding a public callback registration API and implementing platform-specific event backends (Windows cfgmgr32 notifications, macOS IOHIDManager, Linux udev monitor, libusb hotplug), plus extending hidtest to exercise the new functionality.
Changes:
- Add new public hotplug API (
hid_hotplug_register_callback/hid_hotplug_deregister_callback) and related types inhidapi.h. - Implement hotplug backends for Windows, macOS, Linux, and libusb; add NetBSD stubs.
- Extend
hidtestwith an interactive hotplug testing mode and stress scenarios.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
hidapi/hidapi.h |
Adds the public hotplug API types, flags, callback signature, and function declarations. |
windows/hidapi_cfgmgr32.h |
Adds cfgmgr32 notification type definitions and function pointer typedefs needed for Windows hotplug. |
windows/hid.c |
Implements Windows hotplug registration, notification callback, and lifecycle cleanup. |
mac/hid.c |
Implements macOS hotplug monitoring thread/runloop, callback list management, and device tracking. |
linux/hid.c |
Implements Linux udev-based hotplug monitor thread and callback dispatch. |
libusb/hid.c |
Implements libusb hotplug callback + internal event queue + callback dispatch thread model. |
netbsd/hid.c |
Adds stub hotplug functions returning unsupported. |
hidtest/test.c |
Adds interactive hotplug tests, keypress handling, and callback stress tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Fill already connected devices so we can use this info in disconnection notification */ | ||
| hid_hotplug_context.devs = hid_enumerate(0, 0); | ||
|
|
||
| hid_hotplug_context.hotplug_cbs = hotplug_cb; | ||
|
|
||
| if (hid_hotplug_context.notify_handle != NULL) { | ||
| register_global_error(L"Device notification have already been registered"); | ||
| LeaveCriticalSection(&hid_hotplug_context.critical_section); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Retrieve HID Interface Class GUID | ||
| https://docs.microsoft.com/windows-hardware/drivers/install/guid-devinterface-hid */ | ||
| HidD_GetHidGuid(&interface_class_guid); | ||
|
|
||
| notify_filter.cbSize = sizeof(notify_filter); | ||
| notify_filter.FilterType = CM_NOTIFY_FILTER_TYPE_DEVICEINTERFACE; | ||
| notify_filter.u.DeviceInterface.ClassGuid = interface_class_guid; | ||
|
|
||
| /* Register for a HID device notification when adding the first callback */ | ||
| if (CM_Register_Notification(¬ify_filter, NULL, hid_internal_notify_callback, &hid_hotplug_context.notify_handle) != CR_SUCCESS) { | ||
| register_global_error(L"hid_hotplug_register_callback/CM_Register_Notification"); | ||
| LeaveCriticalSection(&hid_hotplug_context.critical_section); | ||
| return -1; |
There was a problem hiding this comment.
In the first-callback registration path, hid_hotplug_context.devs = hid_enumerate(0, 0) can return NULL (e.g., if hid_init() / DLL resolution fails), but the function continues and calls CM_Register_Notification(...) anyway. That can crash in the failure case and also registers a callback even though initial enumeration failed. Consider calling hid_init() (and failing fast) at the start of hid_hotplug_register_callback(), and/or checking the result of hid_enumerate() and rolling back state on failure.
| cur_dev = &(cur_dev_ex->info); | ||
| if (cur_dev == NULL) { | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
calloc() result (cur_dev_ex) is dereferenced before being NULL-checked (cur_dev = &(cur_dev_ex->info);). If allocation fails, this is an immediate NULL dereference. Check cur_dev_ex for NULL before accessing its fields (and return NULL on failure).
| cur_dev = &(cur_dev_ex->info); | |
| if (cur_dev == NULL) { | |
| return NULL; | |
| } | |
| if (cur_dev_ex == NULL) { | |
| return NULL; | |
| } | |
| cur_dev = &(cur_dev_ex->info); |
| /* Wait for read_thread() to end. */ | ||
| pthread_join(hid_hotplug_context.thread, NULL); |
There was a problem hiding this comment.
hid_internal_hotplug_cleanup() calls pthread_join(hid_hotplug_context.thread, NULL) while it is intended to be invoked while holding hid_hotplug_context.mutex. This cleanup can be reached from IOHID callbacks that are executed on the hotplug thread itself; in that case, joining hid_hotplug_context.thread from within the same thread will deadlock. Guard against self-join (e.g., pthread_equal(pthread_self(), hid_hotplug_context.thread)) and/or redesign shutdown so the join happens from a different thread or the hotplug thread is detached.
| /* Wait for read_thread() to end. */ | |
| pthread_join(hid_hotplug_context.thread, NULL); | |
| /* Wait for hotplug_thread() to end, avoiding self-join deadlock. */ | |
| if (pthread_equal(pthread_self(), hid_hotplug_context.thread)) { | |
| /* If called from the hotplug thread itself, detach so resources are | |
| reclaimed on exit, and do not attempt to join. */ | |
| pthread_detach(hid_hotplug_context.thread); | |
| } else { | |
| pthread_join(hid_hotplug_context.thread, NULL); | |
| } |
| info->next = NULL; | ||
| hid_internal_invoke_callbacks(info, HID_API_HOTPLUG_EVENT_DEVICE_LEFT); | ||
| /* Free every removed device */ | ||
| free(info); |
There was a problem hiding this comment.
On device removal, the code detaches a hid_device_info node from hid_hotplug_context.devs and then frees it with free(info). Each hid_device_info contains heap-allocated fields (path, strings), so this leaks memory; also this list was built by create_device_info*() which expects hid_free_enumeration() for cleanup. Set info->next = NULL (already done) and call hid_free_enumeration(info) instead.
| free(info); | |
| hid_free_enumeration(info); |
| info->next = NULL; | ||
| hid_internal_invoke_callbacks(info, HID_API_HOTPLUG_EVENT_DEVICE_LEFT); | ||
| /* Free every removed device */ | ||
| free(info); |
There was a problem hiding this comment.
On device removal handling in the udev hotplug thread, the code frees a removed hid_device_info node with free(info). This leaks the heap-allocated members of hid_device_info (path and strings) which are normally freed by hid_free_enumeration(). Since info->next is already set to NULL, use hid_free_enumeration(info) to free the node correctly.
| free(info); | |
| hid_free_enumeration(info); |
|
|
||
| /* Free removed device */ | ||
| if (hotplug_event == HID_API_HOTPLUG_EVENT_DEVICE_LEFT) { | ||
| free(device); |
There was a problem hiding this comment.
When processing a device removal hotplug event, the code frees only the struct hid_device_info node with free(device), but hid_internal_get_device_info() allocates path, serial_number, manufacturer_string, and product_string. This leaks those allocations; use the existing hid_free_enumeration(device) (after ensuring device->next is NULL) to free the node and its fields correctly.
| free(device); | |
| /* Ensure only this node is freed by hid_free_enumeration() */ | |
| device->next = NULL; | |
| hid_free_enumeration(device); |
| hid_hotplug_context.hotplug_cbs = hotplug_cb; | ||
|
|
||
| if (hid_hotplug_context.notify_handle != NULL) { | ||
| register_global_error(L"Device notification have already been registered"); | ||
| LeaveCriticalSection(&hid_hotplug_context.critical_section); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Retrieve HID Interface Class GUID | ||
| https://docs.microsoft.com/windows-hardware/drivers/install/guid-devinterface-hid */ | ||
| HidD_GetHidGuid(&interface_class_guid); | ||
|
|
||
| notify_filter.cbSize = sizeof(notify_filter); | ||
| notify_filter.FilterType = CM_NOTIFY_FILTER_TYPE_DEVICEINTERFACE; | ||
| notify_filter.u.DeviceInterface.ClassGuid = interface_class_guid; | ||
|
|
||
| /* Register for a HID device notification when adding the first callback */ | ||
| if (CM_Register_Notification(¬ify_filter, NULL, hid_internal_notify_callback, &hid_hotplug_context.notify_handle) != CR_SUCCESS) { | ||
| register_global_error(L"hid_hotplug_register_callback/CM_Register_Notification"); | ||
| LeaveCriticalSection(&hid_hotplug_context.critical_section); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
On the error returns in the first-callback path (e.g., notify_handle != NULL or CM_Register_Notification(...) != CR_SUCCESS), the newly allocated hotplug_cb (and the devs list from hid_enumerate) are not freed, and hid_hotplug_context.hotplug_cbs may already be set to hotplug_cb. This leaves the hotplug context in a partially-initialized state and leaks memory; the error path should undo any state changes and free allocations before returning.
| hid_internal_hotplug_exit(); | ||
| } | ||
|
|
There was a problem hiding this comment.
hid_exit() only calls hid_internal_hotplug_exit() when hid_mgr is non-NULL. Since hotplug registration does not require hid_mgr to be initialized (and callers may rely on hid_exit() to clean up regardless), this can leak hotplug threads/mutex state. Call hid_internal_hotplug_exit() unconditionally (or at least whenever the hotplug context is initialized).
| hid_internal_hotplug_exit(); | |
| } | |
| } | |
| hid_internal_hotplug_exit(); |
| printf("Compile-time version matches runtime version of hidapi.\n\n"); | ||
| } | ||
| else { | ||
| printf("Compile-time version is different than runtime version of hidapi.\n]n"); |
There was a problem hiding this comment.
This printf string ends with \n]n, which looks like a typo and will print a stray ] instead of a newline sequence. It should likely be \n\n (or similar) to match the surrounding output formatting.
| printf("Compile-time version is different than runtime version of hidapi.\n]n"); | |
| printf("Compile-time version is different than runtime version of hidapi.\n\n"); |
| int match_ref_to_info(IOHIDDeviceRef device, struct hid_device_info *info) | ||
| { | ||
| if (!device || !info) { | ||
| return 0; | ||
| } | ||
|
|
||
| struct hid_device_info_ex* ex = (struct hid_device_info_ex*)info; | ||
| io_service_t service = IOHIDDeviceGetService(device); | ||
|
|
||
| return (service == ex->service); | ||
| } |
There was a problem hiding this comment.
match_ref_to_info() is a file-local helper but is not declared static, which unnecessarily exports a global symbol from this compilation unit (and increases the risk of symbol collisions). Make it static to keep it private to this file.
Resolves: #238