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 hotplug support #238

Open
mcuee opened this issue Jan 26, 2021 · 31 comments · May be fixed by #674
Open

Add hotplug support #238

mcuee opened this issue Jan 26, 2021 · 31 comments · May be fixed by #674
Labels
Core Related to common codes like hidapi.h enhancement New feature or request hidraw Related to Linux/hidraw backend libusb Related to libusb backend macOS Related to macOS backend Windows Related to Windows backend

Comments

@mcuee
Copy link
Member

mcuee commented Jan 26, 2021

One of the main missing feature of hidapi is the hotplug support.

@mcuee mcuee added enhancement New feature or request macOS Related to macOS backend Windows Related to Windows backend hidraw Related to Linux/hidraw backend libusb Related to libusb backend labels Jan 26, 2021
@mcuee
Copy link
Member Author

mcuee commented Jun 13, 2021

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).

@mcuee
Copy link
Member Author

mcuee commented Jun 23, 2021

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

@Youw
Copy link
Member

Youw commented Jun 23, 2021

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

@mcuee
Copy link
Member Author

mcuee commented Jul 18, 2021

#299 seems to work pretty well under Windows. The API function names are now similar to libusb and I think they are fine.

@mcuee mcuee added the Core Related to common codes like hidapi.h label Jul 18, 2021
@807183087
Copy link

Hi,sir. I found a little error about nullptr in https://github.com/DJm00n/hidapi/blob/device_notify/windows/hid.c#L1036
before change
after change

@807183087
Copy link

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.

@Youw
Copy link
Member

Youw commented Jan 3, 2022

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:

  1. enumerate existing devices;
  2. register callback for new devices;

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_enumerate at all.

@807183087
Copy link

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);

@gekart
Copy link

gekart commented Nov 21, 2022

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)?

@Youw
Copy link
Member

Youw commented Nov 22, 2022

Then how to deal with disconnecting devices

As of right now - only by using OS-specific API in your application directly.

(on MacOS this might cause app and even system crashes)?

HIDAPI handles those internally (e.g.: 1) - no crashes are expected.

@gekart
Copy link

gekart commented Nov 22, 2022

I am using a library (ZWO EFW SDK) that in turn uses hidapi. I get crashes constantly when plugging and unplugging the device wildly:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
  * frame #0: 0x00007ff807fb363e CoreFoundation`_CFAssertMismatchedTypeID + 110
    frame #1: 0x00007ff807ea83ca CoreFoundation`CFRunLoopStop + 129
    frame #2: 0x00007ff80a6d5e5e IOKit`__IOHIDDeviceNotification + 195
    frame #3: 0x00007ff80a69bcda IOKit`IODispatchCalloutFromCFMessage + 318
    frame #4: 0x00007ff807ea826b CoreFoundation`__CFMachPortPerform + 288
    frame #5: 0x00007ff807e7baf1 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ + 41
    frame #6: 0x00007ff807e7b9d3 CoreFoundation`__CFRunLoopDoSource1 + 615
    frame #7: 0x00007ff807e7a047 CoreFoundation`__CFRunLoopRun + 2419
    frame #8: 0x00007ff807e79014 CoreFoundation`CFRunLoopRunSpecific + 562
    frame #9: 0x00000001000060a2 test_console`hid_enumerate + 178
    frame #10: 0x0000000100004a6b test_console`EFWGetNum + 59
    frame #11: 0x000000010000378d test_console`crashMeC() + 13
    frame #12: 0x00000001000037e9 test_console`main + 9
    frame #13: 0x000000020008952e dyld`start + 462

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...)

@Youw
Copy link
Member

Youw commented Nov 22, 2022

  1. What version of HIDAPI is used here?
  2. Can you confirm that hid_enumerate is not tried to be used from multiple threads concurently?

@Youw
Copy link
Member

Youw commented Nov 22, 2022

Oh, additionally: not only hid_enumerate - none of hid_init/hid_enumerate/hid_open/hid_open_path/hid_close/hid_exit are thread-safe on macOS - and it has nothing to do with hotplug support.

@gekart
Copy link

gekart commented Nov 22, 2022

Hmm. That simple C ten-liner that I have linked above results in multiple threads:

(lldb) thread list
Process 8318 stopped
  thread #1: tid = 0x94125, 0x00007ff807d7b2be libsystem_kernel.dylib`__semwait_signal + 10, queue = 'com.apple.main-thread'
  thread #2: tid = 0x9415e, 0x00007ff807d7a05a libsystem_kernel.dylib`__workq_kernreturn + 10
* thread #3: tid = 0x9416a, 0x00007ff807fb363e CoreFoundation`_CFAssertMismatchedTypeID + 110, stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
  thread #4: tid = 0x9415f, 0x00007ff807d7a05a libsystem_kernel.dylib`__workq_kernreturn + 10
(lldb) bt
* thread #3, stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
  * frame #0: 0x00007ff807fb363e CoreFoundation`_CFAssertMismatchedTypeID + 110
    frame #1: 0x00007ff807ea83ca CoreFoundation`CFRunLoopStop + 129
    frame #2: 0x00007ff80a6d5e5e IOKit`__IOHIDDeviceNotification + 195
    frame #3: 0x00007ff80a69bcda IOKit`IODispatchCalloutFromCFMessage + 318
    frame #4: 0x00007ff807ea826b CoreFoundation`__CFMachPortPerform + 288
    frame #5: 0x00007ff807e7baf1 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ + 41
    frame #6: 0x00007ff807e7b9d3 CoreFoundation`__CFRunLoopDoSource1 + 615
    frame #7: 0x00007ff807e7a047 CoreFoundation`__CFRunLoopRun + 2419
    frame #8: 0x00007ff807e79014 CoreFoundation`CFRunLoopRunSpecific + 562
    frame #9: 0x0000000100007032 test_console`read_thread + 258
    frame #10: 0x00007ff807db54e1 libsystem_pthread.dylib`_pthread_start + 125
    frame #11: 0x00007ff807db0f6b libsystem_pthread.dylib`thread_start + 15

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...

@Youw
Copy link
Member

Youw commented Nov 22, 2022

If this doesn't have anything to do with hotplugging, maybe I should open another issue.

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.

@gekart
Copy link

gekart commented Nov 22, 2022

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!

@Youw Youw added this to To do in Hotplug Support via automation May 1, 2023
@Youw Youw added this to To do in API v1 via automation Sep 4, 2023
@k1-801
Copy link

k1-801 commented Nov 21, 2023

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.

@k1-801
Copy link

k1-801 commented Nov 21, 2023

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).

@k1-801
Copy link

k1-801 commented Nov 22, 2023

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.

@k1-801
Copy link

k1-801 commented Dec 2, 2023

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.

@Youw
Copy link
Member

Youw commented Dec 14, 2023

@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.

@k1-801
Copy link

k1-801 commented Mar 4, 2024

@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 😅

@Youw
Copy link
Member

Youw commented Mar 4, 2024

Thant's for the reminder. I actually do have a few hours today. Will take a look.

@k1-801
Copy link

k1-801 commented Mar 7, 2024

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 clang produced a few warnings in the pre-existing code that might require their own issue:
image

@k1-801
Copy link

k1-801 commented Mar 9, 2024

I think it's best to rebase the connection-callback branch onto the fresh master. There are some conflicts now due to the updates that happened in the last 5 months.

@mcuee
Copy link
Member Author

mcuee commented Mar 9, 2024

I think it's best to rebase the connection-callback branch onto the fresh master. There are some conflicts now due to the updates that happened in the last 5 months.

Good point.

@Youw
Please help here. Thanks.

@k1-801
Copy link

k1-801 commented Mar 11, 2024

@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.

@k1-801
Copy link

k1-801 commented Mar 12, 2024

The libusb PR now works as well.
No driver detachment is needed to receive events.
There are no untested platforms left.

@k1-801
Copy link

k1-801 commented Mar 13, 2024

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.

@k1-801
Copy link

k1-801 commented Mar 14, 2024

I just found out that there is actually now support for a netbsd backend in hidapi, and the connection-callback branch 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.

@k1-801
Copy link

k1-801 commented Apr 6, 2024

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:

  • Removing the element right before the one currently being processed will invalidate the current pointer (as it will be pointing to an area that has just been free'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 incorrectly
  • Removing the current element will lead to exactly the same use-after-free scenario
  • Removing all elements may cause even more severe side-effects, as removing the last element triggers a cleanup sequence
  • If the next pointer 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Related to common codes like hidapi.h enhancement New feature or request hidraw Related to Linux/hidraw backend libusb Related to libusb backend macOS Related to macOS backend Windows Related to Windows backend
Projects
API v1
To do
5 participants