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.
Resolves: #238