RFC: Hotplug implementation#674
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.
There was a problem hiding this comment.
A general rule: a retry w/o knowing exactly a reason for the behavior is not a good thing - that is a high indication somewhere is a bug, or something fundamentally done wrong...
In any case - lets have it as a separate issue/PR, this PR has too many comments/threads, things easily get lost...
|
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 agree that this might be a worthy addition, if during testing it revealed such a problem. |
|
@k1-801 lets continue the conversation her: #784 @bearsh see: #674 (comment) Before marking this a Ready for Review/Merging to master - at least #783 needs to be reviewed/merged |
|
|
||
| /** @brief Deregister a callback from a HID hotplug. | ||
|
|
||
| This function is safe to call from within a hotplug callback. |
There was a problem hiding this comment.
while we want this to be true, I have a few conserns:
-
why do we wan't to make this possible? one could simply return non-zero from a callback function, and that would effectively deregister, w/o a need to call
deregistermanually... -
register/deregisterfunctions are not really thread-safe with regard to each other, mostly because context/mutex initisalisation in not thread-safe. even if we allow callingderegisterfrom within the callback, there should be a big NOTE regarding the fact, that all calls toregister/deregister, including the one from withing the callback function - should be serialized / protected by a mutex or so.
There was a problem hiding this comment.
- one could simply return non-zero from a callback function, and that would effectively deregister, w/o a need to call
deregistermanually
It could be deregistering a different callback than the one currently running, OR the user could need to perform certain actions after the callback is deregistered.
There was a problem hiding this comment.
2. there should be a big NOTE regarding the fact, that all calls to
register/deregister, including the one from withing the callback function - should be serialized / protected by a mutex or so.
True. We do need such a note.
Some thread safety did actually exist there and was removed later during review.
There was a problem hiding this comment.
There is also things like: #674 (comment) that needs to be resolved first, and taken into account in general
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Would also be delighted to see this land, FWIW |
Callback_thread used hidapi_thread_cond_timedwait with an absolute timespec already in the past, causing a tight spin loop. Replaced with cond_wait + predicate pattern so the thread sleeps until an event or a shutdown signal arrives. Multithreading fixes folded in along the way: - Queue mutex moved from libusb_thread to callback_thread (its natural condvar partner), with signal-under-mutex to avoid a lost-wakeup race at shutdown. - process_hotplug_event() now runs under hid_hotplug_context.mutex to match hid_hotplug_register_callback()'s HID_API_HOTPLUG_ENUMERATE traversal of hid_hotplug_context.devs — fixes a use-after-free where a concurrent disconnect could free a list node mid-traversal. - devs teardown moved to hid_internal_hotplug_cleanup() (after hidapi_thread_join) so every access to devs happens under hid_hotplug_context.mutex. - hid_hotplug_register_callback() now frees devs on the libusb_hotplug_register_callback() error path, which previously leaked. Closes: #782 Assisted-by: Copilot:claude-sonnet-4.6 Assisted-by: Copilot:claude-opus-4.6 Assisted-by: Claude:claude-opus-4.7
|
I'll refine the suggested behavior a bit in scope of #790 (this is documentation-only) - and will make corresponding changes to the code later. |
|
Any update on hotplug? Is it going to make it into a release anytime soon? I have merged OpenRGB next into master, currently depending on our hidapi-hotplug fork to support hotplugging. I would prefer if there was an official release we could link to but I don't want to delay OpenRGB 1.0 much longer. |
|
I'll spend some time on it right now, but no promices. |
The windows-cmake job set up the MSVC environment by calling a hard-coded "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvars64.bat" in its NMake, clang-cl and Meson steps. The hosted windows-latest image no longer has Visual Studio under that "Enterprise" path, so those steps started failing with "The system cannot find the path specified." Set up the developer environment once per job with the TheMrMilchmann/setup-msvc-dev action -- it locates the installed toolchain (no hard-coded path or edition) -- and drop all the hard-coded vcvars64.bat calls. That environment is job-wide, so the MinGW build (which must use gcc, not cl) is split out into its own windows-mingw job that runs without the MSVC environment. The MSVC, NMake, clang-cl and Meson builds stay in windows-cmake with the action. Assisted-by: claude-code:claude-opus-4-8 (cherry picked from commit 2514836)
) Document the full hotplug API contract in hidapi.h: thread-safety rules (re-entrant hotplug mutex, safe-to-call list inside callbacks), execution context (callbacks only ever run on HIDAPI's internal event context), HID_API_HOTPLUG_ENUMERATE redefined as an asynchronous initial pass from a registration-time snapshot (exactly-once, delivered before live events, return value honoured), device lifetime rules (device->next == NULL, cached DEVICE_LEFT info), deregistration post-conditions, handle non-reuse, argument validation, and Since-version annotations. Implementation gaps vs this contract are tracked as follow-up issues (see PR description). Supersedes #784. Assisted-by: claude-code:claude-fable-5
Brings in #813 (IWYU + leak guards), #815 (virtual-device test harness), #817 (CI vcvars fix, already cherry-picked) and #708 (libusb hid_error). Conflicts in libusb/hid.c (hotplug context vs last_global_error, refactored hid_enumerate vs its error reporting) and linux/hid.c (includes) resolved by keeping both sides' functionality. Assisted-by: claude-code:claude-fable-5
The HIDAPI_BUILD_AS_CXX check (from #814, merged from master) rejected the hotplug code: out-of-order/partial designated initializers on the static hotplug contexts, int-to-enum conversions on the callback events bitmask, and an uncast calloc. Replace the static initializers with zero-initialized storage plus lazy next_handle setup, make the internal events field a plain int bitmask, and cast the remaining sites. Assisted-by: claude-code:claude-fable-5
…ug calls hid_hotplug_register_callback() and hid_hotplug_deregister_callback() set the global error string on failure, so they must be serialized against hid_error(NULL) even though they need no serialization against each other. Also state the rule the backends implement: HIDAPI's internal threads never write the global error string, since an application cannot serialize against them. Assisted-by: claude-code:claude-opus-4-8
… error The callback runs on HIDAPI's internal event context, which never writes the global error string (an application cannot serialize against an internal thread, so writing it there is a use-after-free hazard). Return values still report failure, and hid_error(dev) still works for a device handle. Assisted-by: claude-code:claude-opus-4-8
…t-init thread binding Two clarifications surfaced by a cross-backend consistency review: when several arguments are invalid, which one the error string names is unspecified and differs between backends (only the -1 return and zeroed handle are guaranteed); and implicit hid_init() on registration binds the device-monitoring facilities to the calling thread on some backends (e.g. macOS run-loop scheduling), so an application that cares should call hid_init() from the intended thread first. Assisted-by: claude-code:claude-opus-4-8
Resolves: #238