Add hotplug support #238
Labels
No labels
3rdparty
Android
API
Author does not respond
bsd
bug
build system/CI
ci-virtual-device
CMake
Contributions Welcome
Core
dependencies
device-specific
documentation
don't_merge
duplicate
enhancement
Example
Haiku
hidraw
hotplug
info required
invalid
libusb
macOS
Misc
Not an issue
question
Release
Test
Test GUI
Windows
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-libusb/hidapi#238
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
One of the main missing feature of hidapi is the hotplug support.
Technically there are some Windows codes are out there to support hotplug. For Linux you can consider libudev. For macOS there are similar codes as well. libusb has hotplug support under Linux and macOS but not Windows (complicated due to libusb Windows implementation).
For Windows, not so sure pywinusb can shed some light.
https://github.com/rene-aguirre/pywinusb
Or the codes here.
http://janaxelson.com/hidpage.htm
I think Hidsharp also supports hotplug.
https://github.com/IntergatedCircuits/HidSharp
I'd say that hotplug support for each platform is pretty much straight-forward (I've done it for macOS and Windows several years ago in a propriatary project)
same is true for linux (udev) and libusb backends
it just has to be implemented
#299 seems to work pretty well under Windows. The API function names are now similar to libusb and I think they are fine.
Hi,sir. I found a little error about nullptr in https://github.com/DJm00n/hidapi/blob/device_notify/windows/hid.c#L1036
I noticed, the plugged devices before my testing program startup also can be callback.
I think, hotplug just processes the hot plug events, but not existed devices.
Once hotplug is registered - it has to report all existing devices.
The likely alternative you probably want to have is:
Or the other way around (first register callback, then enumerate existing devices).
In either case you're having a race condition if the device is plugged at the moment you're registering a callback (and that is happening more often than you could imagine).
You either miss a device or have it reported twice.
The only 100% reliable way to use device hotplug events - is to use only hotplug callback events to get a list of all devices (existing and new).
And not use things like
hid_enumerateat all.yes.
registe callback like this will not receive existed devices:
hid_hotplug_register_callback(0, 0, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED| HID_API_HOTPLUG_EVENT_DEVICE_LEFT, 0/*HID_API_HOTPLUG_ENUMERATE*/, hotplug_callback, this, &hotplug_handle);Just to understand this issue: Are you saying that hidapi is not USB hotplug ready? Then how to deal with disconnecting devices (on MacOS this might cause app and even system crashes)?
As of right now - only by using OS-specific API in your application directly.
HIDAPI handles those internally (e.g.: 1) - no crashes are expected.
I am using a library (ZWO EFW SDK) that in turn uses hidapi. I get crashes constantly when plugging and unplugging the device wildly:
More context at https://bbs.astronomy-imaging-camera.com/d/15289-efwgetnum-on-macos-not-thread-safe/5 (That thread is full of assumptions, however, sorry...)
hid_enumerateis not tried to be used from multiple threads concurently?Oh, additionally: not only
hid_enumerate- none ofhid_init/hid_enumerate/hid_open/hid_open_path/hid_close/hid_exitare thread-safe on macOS - and it has nothing to do with hotplug support.Hmm. That simple C ten-liner that I have linked above results in multiple threads:
so I assume they are using threads there. I don't think they are accessing hidapi from multiple threads.
If this doesn't have anything to do with hotplugging, maybe I should open another issue. At least it happens while hot plugging...
Yes please.
At least this thread is about having an additional callbacks to report plugged/unplugged devices, and not about dealing with unplugged devices in runtime.
Thanks.
No need for another issue: I have fixed the problem by replacing the ar packaged version of hidapi within that SDK with a fresh recompile from GitHub. Companies keep their software closed to hide ugly truths... Thank you for your help!
I noticed that the current branch only supports hotplug on windows, so I added a prototype implementation for libusb in #645. It's a pretty straightforward one, taking huge chunks from Windows implementation.
Please review.has a couple of missing spots, will fill them in in the next few hours.UPD: Fixed all the main issues I could find myself, requesting comments.
Also noticed that the issue #538 has been stale for 4 months. #646 should resolve that, but I have not tested it yet (will report when I get to a windows machine to test it).
Added a hidraw implementation (#647 ) that copies a lot from the libusb one (I specifically tried to move the major differences into separate functions, so that the general callback list management is the same exact code). Did not run any tests yet, that will have to wait until at least the weekends.
I don't have a Mac to build or test the last one... It's not going to stop me, but it's gonna take a bit longer than it took for the others.
UPD: Ran some tests on hidraw version, fixed some errors, it works fine now. I still have to ask someone to double-check everything, up to and including code-style.
Couldn't get libusb one to work, libusb itself does not read any events even under sudo. Probably something wrong with udev rules on my system.
I have made a mock-up for a MacOS implementation of the hotplug feature in #653 . Even if it can not be used as-is, it will simplify the work to get this completed.
I beieve this completes the list of platforms that have to have the hotplug support for the hotplug feature to be included into the next release of hidapi. However, this one I am the least sure of, as it took me a whole two weeks and I stil couldn't set up a proper buid environment for it, so I can not even check if it builds (github CI might become my only way to test it), let alone check if it works.
@k1-801 please excuse me for not looking into your PRs. Really appreciate all the work you've done.
Got extremely busy lately.
I promice to get back to it as soon as get enought time for it.
@Youw sorry to disturb you, but is there any chance of getting a review?
Didn't want to be rude, but it's been 3 months 😅
Thant's for the reminder. I actually do have a few hours today. Will take a look.
Somehow - and I am not sure how - I got the MacOS implementation to a buildable state, with all the basic blocks of the implementation in place. All that's left now is to somehow make sure it works - and then give it a full review.
Additionally, I noticed that
clangproduced a few warnings in the pre-existing code that might require their own issue:I think it's best to rebase the
connection-callbackbranch onto the freshmaster. There are some conflicts now due to the updates that happened in the last 5 months.Good point.
@Youw
Please help here. Thanks.
@Youw is there anything else that needs to be done or anything else needed from my side to get the feature reviewed, merged and then considered to be included in the next release?
I understand that there's nothing worse than releasing an untested feature, but so far we've gotten very good results on 3 out of the 4 backends, libusb being the only tricky one.
The libusb PR now works as well.
No driver detachment is needed to receive events.
There are no untested platforms left.
As for the practical applications of this feature, at this moment the project: https://gitlab.com/CalcProgrammer1/OpenRGB/-/merge_requests/1963 is waiting for this feature to be released.
I just found out that there is actually now support for a netbsd backend in hidapi, and the
connection-callbackbranch doesn't have stub callback management functions for that backend. Making a PR now.UPD: #668
Unfortunately, I couldn't find any information on how to get device notifications on netbsd.
A small concern here: calling any Register or Unregister functions from within a callback may (and probably will) cause a deadlock. On Windows, Critical Sections seem to keep an internal counter that allows the same thread to enter on multiple times, while in pthread, unless PTHREAD_MUTEX_RECURSIVE is specified, it will cause a deadlock.
Additionally, just unlocking the mutex before calling the callback (or making it recursive) will allow the user to remove elements in such a way that it disturbs the execution flow, which can already happen on Windows.
Bad scenarios:
currentpointer (as it will be pointing to an area that has just beenfree'd), and even if use-after-free situation doesn't break things, returning a value from the callback that deletes the callback will process the list incorrectlynextpointer is stored in advance (which it isn't in current implementation, but should be warned against in the future), adding a new element while the last one is being processed will only cause the new one to not be processed while removing an element right after the current one will cause unpredictable behavior.There is a way to circumvent this, by adding a flag to each callback that indicates that it should be removed later when it is considered safe, which is set by the Unregister function if the mutex is already locked at the moment of the call. It seems that, as long as we know for a fact that the mutex is locked by the same thread, it should be safe to read and navigate the list of callbacks and even set flags to it, and the list will not be changed (the mutex is still locked and will not be unloced until we leave the callback and the callback processing part). As for the Register function, I see no problem with additional callbacks being registered from a callback. However, this approach still requires to somehow know for a fact that the mutex is locked by the same thread we are currently in.
Another possible resolution would be to warn against the use of Register and Unregister functions inside a callback, however, there are scenarios where the user code could make use of that (a widely used example would be the scenario where the unplug callback is only registered after a device was connected).
After some additional testing, I found that on Linux (hidraw) in some scenarios, the hotplug event returns empty Usage/Page values. The manufacturer string and serial number also seem to not be filled in. It seems that the number of usages is not correct (I assumed it was correct when I started investigating), but all interfaces are present.
The fileds are reported correctly when using
hid_enumeratelater. Both use the same functions to fetch data, so I'm not sure what's going on there.I am currently testing this on an Ubuntu VM, and it didn't play out like this last time I tested on a real machine. The fields are empty for all devices reported through a callback.
Here is what I get as events:
And here are the numbers I get through enumeration (I, U, P are Interface, Usage and Page):
Is it possible that
get_next_hid_usageneeds some time before being called when a device is connected?That is a good hunch. I guess the presense of UDEV event doesn't guarantee that HIDRAW subsystem done initialising.
I believe the patch in #765 should fix this. Gonna do more testing for now, but the interactive test utility reported correct data with it.
What it does is it tries to repeatedly request a report until it succeeds or a timeout expires. For regular hid_enumerate the timeout is 0, so it only tries once, but for a hotplug event it's 4 seconds.
UPD: The interactive test reported all numbers properly even without any retries, while OpenRGB fails to get Usages even with them... Digging deeper.
@Youw found it was my mistake (further - in the closing comment of #765). Everything works correctly if the library is linked at compilation time. If it is loaded through
dlopen, hidapi cannot retrieve the report descriptor. A linker flag-rdynamicseems to fix it, but I am not gonna vouch on that, as I don't understand how and why it works.