JoergAtGithub
commented
10 months ago @DJm00n Could you have a look on the code in this PR?
Closed k1-801 closed 6 months ago
JoergAtGithub
commented
10 months ago @DJm00n Could you have a look on the code in this PR?
k1-801
commented
10 months ago @DJm00n all suggestions applied, thank you!
k1-801
commented
10 months ago Just noticed that the cleanup code on exit won't really do anything if any callbacks are still armed; going to fix that in a moment.
k1-801
commented
10 months ago Would it be fine to throw in a small change for the hidtest (in the hotplug testing section)? It needs a fflush(stdout) in the callback, otherwise the output gets stuck and events don't pop up immediately (sometimes at all).
Ran some tests, had to add this in, with it everything seems to work.
UPD: added the change. Should help with testing the events with the added mutex.
k1-801
commented
10 months ago Apart from the formatting issues: is there're a specific reason to init/deinit the hotplug routines during hid_init/hid_exit?
The main reason here is allowing the register/deregister functions being called from multiple threads simultaneously. This is the original reason to add the mutex at all, and, unfortunately, the mutex has to be initialized somewhere before use. hid_init seemed like the most reasonable place to do so, as the chance of getting a second call to it from a different thread at the same time (before the mutex is initialized) is negligible.
If we happen to get two simultaneous register calls from two different threads with an uninitialized mutex, both could attempt to make a new one, which would lead to either thread locking a different mutex, from where we can encounter all sorts of weird behaviors, including possible memory leak scenarios where the first cached device list is leaked, or one of the callbacks is lost.
I believe the mutex initialization overhead to be a sacrifice small enough to ignore.
UPD: I found a way to avoid the mutex initialization with critical sectiona, investigating.
Youw
commented
10 months ago I have to apoligize for not asking this earlier: why do we even need to allow calling register/deregister functions from different threads? HIDAPI was never intended to have a thread-safe API in general case.
hid_init/hid_exit<->hid_enumerate<->hid_open/hid_open_path/hid_close are not thread-safe in general case (as per my testing on 4 platforms) and I don't see good reason why making register/deregister functions as thread safe
Originally I assumed that mutex is needed for some kind of data syncronisation internally and I didn't actually payed attention to what actually was being done.
k1-801
commented
10 months ago Originally I assumed that mutex is neede for some kind of data syncronisation internally and I didn't actually payed attention to what actually was being done.
That too, by the way, the events are processed in a background thread, and the register calls can potentially interfere with the processing.
If we don't want to ensure the register/deregister call safety, I could move the initialization to the register function. However, I don't know how good of an idea it would be to deinitialize it in the deregister function, need to think on that for a bit.
Youw
commented
10 months ago If we don't want to ensure the register/deregister call safety, I could move the initialization to the register function.
go for it
I don't know how good of an idea it would be to deinitialize it in the deregister function, need to think on that for a bit.
I see two trade offs here: 1) Having deinitialize inside of the deregister makes the implementation symetric and simple. 2) Not doing the deinitialisation in deregister potentially has a bit less overhead if we consider scenarios when different callback functions has to be registered often. But that makes both register/deregister function potentially more complicated and non-symetric.
NOTE: in any case, user may not call deregister function at all, and that could be fine, i.e. hid_close should gracefully perform the deinitialisation if not performed explicitly.
More than that - multiple calls to deregister has to be safe (for the similar reason why free(NULL) is safe).
k1-801
commented
10 months ago
- Not doing the deinitialisation in deregister potentially has a bit less overhead if we consider scenarios when different callback functions has to be registered often. But that makes both register/deregister function potentially more complicated and non-symetric, as it would .
Took the asymmetric approach. The register call now initializes the synchronization. It is only deleted in hid_exit. We lose nothing by keeping it initialized. Replaced the mutex with a CriticalSection. It is the same in function (for unnamed mutexes, at least), but it works ~20 times faster.
mcuee
commented
7 months ago Simple test results seem to be good. Tested under Windows.
1) Unplug and plug of Plantronics Headset (USB composite device)
2) Unplug and plug of Xiaomi USB mouse receiver
3) Unplug and plug of USBasp programmer (USB Composite device)
mcuee
commented
6 months ago Testing again under Windows 10 Dell Laptop to make sure it works -- the results are good.
1) Unplug and plug of Xiaomi USB mouse receiver 2) Unplug and plug of Plantronics Headset (USB composite device) 3) Plug-in (from sleep to active) and Unplug (take out battery) and plug-in (put battery back in) of Microsoft Bluetooth mouse
mcuee
commented
6 months ago Another test with WIndows 11 Acer laptop and it is also good.
1) Unplug and then plug in Logitech USB receiver 2) Plug in and then unplug Plantronics Headset 3) Unplug (turning off power) and then plug in (turning on power) Logitech Pebble Bluetooth mouse
Resolves #538