From 9477791bfe796c1c5d11b02cddc7ca7ed195742c Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Thu, 20 Jul 2017 11:35:27 +0200 Subject: [PATCH 1/2] Use a reference counted USB session Replace the global USB library context with a reference counted session to manage the lifetime of the USB library context. For the libusb based implementation, this is actually a much better match for the underlying libusb api, and allows to eliminate the global state. For the hidapi based implementation, the global state is unavoidable because the hidapi doesn't support multiple sessions. Therefore we use a singleton session. --- src/usbhid.c | 160 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 118 insertions(+), 42 deletions(-) diff --git a/src/usbhid.c b/src/usbhid.c index 0d3bb8a..0606f12 100644 --- a/src/usbhid.c +++ b/src/usbhid.c @@ -68,8 +68,16 @@ typedef pthread_mutex_t dc_mutex_t; #define ISINSTANCE(device) dc_iostream_isinstance((device), &dc_usbhid_vtable) +typedef struct dc_usbhid_session_t { + size_t refcount; +#if defined(USE_LIBUSB) + libusb_context *handle; +#endif +} dc_usbhid_session_t; + struct dc_usbhid_device_t { unsigned short vid, pid; + dc_usbhid_session_t *session; }; #ifdef USBHID @@ -84,6 +92,7 @@ static dc_status_t dc_usbhid_close (dc_iostream_t *iostream); typedef struct dc_usbhid_iterator_t { dc_iterator_t base; dc_filter_t filter; + dc_usbhid_session_t *session; #if defined(USE_LIBUSB) struct libusb_device **devices; size_t count; @@ -97,6 +106,7 @@ typedef struct dc_usbhid_t { /* Base class. */ dc_iostream_t base; /* Internal state. */ + dc_usbhid_session_t *session; #if defined(USE_LIBUSB) libusb_device_handle *handle; int interface; @@ -133,10 +143,9 @@ static const dc_iostream_vtable_t dc_usbhid_vtable = { dc_usbhid_close, /* close */ }; +#ifdef USE_HIDAPI static dc_mutex_t g_usbhid_mutex = DC_MUTEX_INIT; -static size_t g_usbhid_refcount = 0; -#ifdef USE_LIBUSB -static libusb_context *g_usbhid_ctx = NULL; +static dc_usbhid_session_t *g_usbhid_session = NULL; #endif #if defined(USE_LIBUSB) @@ -162,6 +171,7 @@ syserror(int errcode) } #endif +#ifdef USE_HIDAPI static void dc_mutex_lock (dc_mutex_t *mutex) { @@ -183,55 +193,113 @@ dc_mutex_unlock (dc_mutex_t *mutex) pthread_mutex_unlock (mutex); #endif } +#endif static dc_status_t -dc_usbhid_init (dc_context_t *context) +dc_usbhid_session_new (dc_usbhid_session_t **out, dc_context_t *context) { dc_status_t status = DC_STATUS_SUCCESS; + dc_usbhid_session_t *session = NULL; + if (out == NULL) + return DC_STATUS_INVALIDARGS; + +#ifdef USE_HIDAPI dc_mutex_lock (&g_usbhid_mutex); - if (g_usbhid_refcount == 0) { -#if defined(USE_LIBUSB) - int rc = libusb_init (&g_usbhid_ctx); - if (rc != LIBUSB_SUCCESS) { - ERROR (context, "Failed to initialize usb support (%s).", - libusb_error_name (rc)); - status = syserror (rc); - goto error; - } -#elif defined(USE_HIDAPI) - int rc = hid_init(); - if (rc < 0) { - ERROR (context, "Failed to initialize usb support."); - status = DC_STATUS_IO; - goto error; - } + if (g_usbhid_session) { + g_usbhid_session->refcount++; + *out = g_usbhid_session; + dc_mutex_unlock (&g_usbhid_mutex); + return DC_STATUS_SUCCESS; + } #endif + + session = (dc_usbhid_session_t *) malloc (sizeof(dc_usbhid_session_t)); + if (session == NULL) { + ERROR (context, "Failed to allocate memory."); + status = DC_STATUS_NOMEMORY; + goto error_unlock; } - g_usbhid_refcount++; + session->refcount = 1; + +#if defined(USE_LIBUSB) + int rc = libusb_init (&session->handle); + if (rc != LIBUSB_SUCCESS) { + ERROR (context, "Failed to initialize usb support (%s).", + libusb_error_name (rc)); + status = syserror (rc); + goto error_free; + } +#elif defined(USE_HIDAPI) + int rc = hid_init(); + if (rc < 0) { + ERROR (context, "Failed to initialize usb support."); + status = DC_STATUS_IO; + goto error_free; + } + + g_usbhid_session = session; -error: dc_mutex_unlock (&g_usbhid_mutex); +#endif + + *out = session; + + return status; + +error_free: + free (session); +error_unlock: +#ifdef USE_HIDAPI + dc_mutex_unlock (&g_usbhid_mutex); +#endif return status; } -static dc_status_t -dc_usbhid_exit (void) +static dc_usbhid_session_t * +dc_usbhid_session_ref (dc_usbhid_session_t *session) { - dc_mutex_lock (&g_usbhid_mutex); + if (session == NULL) + return NULL; - if (--g_usbhid_refcount == 0) { +#ifdef USE_HIDAPI + dc_mutex_lock (&g_usbhid_mutex); +#endif + + session->refcount++; + +#ifdef USE_HIDAPI + dc_mutex_unlock (&g_usbhid_mutex); +#endif + + return session; +} + +static dc_status_t +dc_usbhid_session_unref (dc_usbhid_session_t *session) +{ + if (session == NULL) + return DC_STATUS_SUCCESS; + +#ifdef USE_HIDAPI + dc_mutex_lock (&g_usbhid_mutex); +#endif + + if (--session->refcount == 0) { #if defined(USE_LIBUSB) - libusb_exit (g_usbhid_ctx); - g_usbhid_ctx = NULL; + libusb_exit (session->handle); #elif defined(USE_HIDAPI) hid_exit (); + g_usbhid_session = NULL; #endif + free (session); } +#ifdef USE_HIDAPI dc_mutex_unlock (&g_usbhid_mutex); +#endif return DC_STATUS_SUCCESS; } @@ -258,6 +326,12 @@ dc_usbhid_device_get_pid (dc_usbhid_device_t *device) void dc_usbhid_device_free(dc_usbhid_device_t *device) { + if (device == NULL) + return; + +#ifdef USBHID + dc_usbhid_session_unref (device->session); +#endif free (device); } @@ -278,7 +352,7 @@ dc_usbhid_iterator_new (dc_iterator_t **out, dc_context_t *context, dc_descripto } // Initialize the usb library. - status = dc_usbhid_init (context); + status = dc_usbhid_session_new (&iterator->session, context); if (status != DC_STATUS_SUCCESS) { goto error_free; } @@ -286,12 +360,12 @@ dc_usbhid_iterator_new (dc_iterator_t **out, dc_context_t *context, dc_descripto #if defined(USE_LIBUSB) // Enumerate the USB devices. struct libusb_device **devices = NULL; - ssize_t ndevices = libusb_get_device_list (g_usbhid_ctx, &devices); + ssize_t ndevices = libusb_get_device_list (iterator->session->handle, &devices); if (ndevices < 0) { ERROR (context, "Failed to enumerate the usb devices (%s).", libusb_error_name (ndevices)); status = syserror (ndevices); - goto error_usb_exit; + goto error_session_unref; } iterator->devices = devices; @@ -301,7 +375,7 @@ dc_usbhid_iterator_new (dc_iterator_t **out, dc_context_t *context, dc_descripto struct hid_device_info *devices = hid_enumerate(0x0, 0x0); if (devices == NULL) { status = DC_STATUS_IO; - goto error_usb_exit; + goto error_session_unref; } iterator->devices = devices; @@ -313,8 +387,8 @@ dc_usbhid_iterator_new (dc_iterator_t **out, dc_context_t *context, dc_descripto return DC_STATUS_SUCCESS; -error_usb_exit: - dc_usbhid_exit (); +error_session_unref: + dc_usbhid_session_unref (iterator->session); error_free: dc_iterator_deallocate ((dc_iterator_t *) iterator); return status; @@ -407,6 +481,7 @@ dc_usbhid_iterator_next (dc_iterator_t *abstract, void *out) return DC_STATUS_NOMEMORY; } + device->session = dc_usbhid_session_ref (iterator->session); device->vid = dev.idVendor; device->pid = dev.idProduct; @@ -432,6 +507,7 @@ dc_usbhid_iterator_next (dc_iterator_t *abstract, void *out) return DC_STATUS_NOMEMORY; } + device->session = dc_usbhid_session_ref (iterator->session); device->vid = current->vendor_id; device->pid = current->product_id; @@ -454,7 +530,7 @@ dc_usbhid_iterator_free (dc_iterator_t *abstract) #elif defined(USE_HIDAPI) hid_free_enumeration (iterator->devices); #endif - dc_usbhid_exit (); + dc_usbhid_session_unref (iterator->session); return DC_STATUS_SUCCESS; } @@ -480,7 +556,7 @@ dc_usbhid_open (dc_iostream_t **out, dc_context_t *context, unsigned int vid, un } // Initialize the usb library. - status = dc_usbhid_init (context); + status = dc_usbhid_session_new (&usbhid->session, context); if (status != DC_STATUS_SUCCESS) { goto error_free; } @@ -491,12 +567,12 @@ dc_usbhid_open (dc_iostream_t **out, dc_context_t *context, unsigned int vid, un int rc = 0; // Enumerate the USB devices. - ssize_t ndevices = libusb_get_device_list (g_usbhid_ctx, &devices); + ssize_t ndevices = libusb_get_device_list (usbhid->session->handle, &devices); if (ndevices < 0) { ERROR (context, "Failed to enumerate the usb devices (%s).", libusb_error_name (ndevices)); status = syserror (ndevices); - goto error_usb_exit; + goto error_session_unref; } // Find the first device matching the VID/PID. @@ -615,7 +691,7 @@ dc_usbhid_open (dc_iostream_t **out, dc_context_t *context, unsigned int vid, un if (usbhid->handle == NULL) { ERROR (context, "Failed to open the usb device."); status = DC_STATUS_IO; - goto error_usb_exit; + goto error_session_unref; } usbhid->timeout = -1; @@ -633,8 +709,8 @@ error_usb_free_config: error_usb_free_list: libusb_free_device_list (devices, 1); #endif -error_usb_exit: - dc_usbhid_exit (); +error_session_unref: + dc_usbhid_session_unref (usbhid->session); error_free: dc_iostream_deallocate ((dc_iostream_t *) usbhid); return status; @@ -656,7 +732,7 @@ dc_usbhid_close (dc_iostream_t *abstract) #elif defined(USE_HIDAPI) hid_close(usbhid->handle); #endif - dc_usbhid_exit(); + dc_usbhid_session_unref (usbhid->session); return status; } From 3d394c92622b31ba9d74a3881aa9ca3373ec5269 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Thu, 20 Jul 2017 12:08:14 +0200 Subject: [PATCH 2/2] Don't use the USB VID/PID for opening the device When two or more identical (or very similar) dive computers are connected, the USB VID/PID can be ambiguous. That's because the VID/PID identifies the type of the USB device, and not the individual device. But each USB HID device descriptor returned by the device discovery represents a single connected device, and thus guarantees to open the correct USB device. To obtain the same behaviour as before, an application can simply open the first discovered device. --- examples/common.c | 9 +- include/libdivecomputer/usbhid.h | 5 +- src/usbhid.c | 146 ++++++++----------------------- 3 files changed, 40 insertions(+), 120 deletions(-) diff --git a/examples/common.c b/examples/common.c index d23982b..09635f0 100644 --- a/examples/common.c +++ b/examples/common.c @@ -401,28 +401,24 @@ dctool_usbhid_open (dc_iostream_t **out, dc_context_t *context, dc_descriptor_t { dc_status_t status = DC_STATUS_SUCCESS; dc_iostream_t *iostream = NULL; - unsigned int vid = 0, pid = 0; // Discover the usbhid device. dc_iterator_t *iterator = NULL; dc_usbhid_device_t *device = NULL; dc_usbhid_iterator_new (&iterator, context, descriptor); while (dc_iterator_next (iterator, &device) == DC_STATUS_SUCCESS) { - vid = dc_usbhid_device_get_vid (device); - pid = dc_usbhid_device_get_pid (device); - dc_usbhid_device_free (device); break; } dc_iterator_free (iterator); - if (vid == 0 && pid == 0) { + if (device == NULL) { ERROR ("No dive computer found."); status = DC_STATUS_NODEVICE; goto cleanup; } // Open the usbhid device. - status = dc_usbhid_open (&iostream, context, vid, pid); + status = dc_usbhid_open (&iostream, context, device); if (status != DC_STATUS_SUCCESS) { ERROR ("Failed to open the usbhid device."); goto cleanup; @@ -431,6 +427,7 @@ dctool_usbhid_open (dc_iostream_t **out, dc_context_t *context, dc_descriptor_t *out = iostream; cleanup: + dc_usbhid_device_free (device); return status; } diff --git a/include/libdivecomputer/usbhid.h b/include/libdivecomputer/usbhid.h index 70823d1..f1c542a 100644 --- a/include/libdivecomputer/usbhid.h +++ b/include/libdivecomputer/usbhid.h @@ -78,13 +78,12 @@ dc_usbhid_iterator_new (dc_iterator_t **iterator, dc_context_t *context, dc_desc * * @param[out] iostream A location to store the USB HID connection. * @param[in] context A valid context object. - * @param[in] vid The USB Vendor ID of the device. - * @param[in] pid The USB Product ID of the device. + * @param[in] device A valid USB HID device. * @returns #DC_STATUS_SUCCESS on success, or another #dc_status_t code * on failure. */ dc_status_t -dc_usbhid_open (dc_iostream_t **iostream, dc_context_t *context, unsigned int vid, unsigned int pid); +dc_usbhid_open (dc_iostream_t **iostream, dc_context_t *context, dc_usbhid_device_t *device); #ifdef __cplusplus } diff --git a/src/usbhid.c b/src/usbhid.c index 0606f12..0338bfe 100644 --- a/src/usbhid.c +++ b/src/usbhid.c @@ -24,6 +24,7 @@ #endif #include +#include #ifdef HAVE_PTHREAD_H #include #endif @@ -78,6 +79,14 @@ typedef struct dc_usbhid_session_t { struct dc_usbhid_device_t { unsigned short vid, pid; dc_usbhid_session_t *session; +#if defined(USE_LIBUSB) + struct libusb_device *handle; + int interface; + unsigned char endpoint_in; + unsigned char endpoint_out; +#elif defined(USE_HIDAPI) + char *path; +#endif }; #ifdef USBHID @@ -330,6 +339,11 @@ dc_usbhid_device_free(dc_usbhid_device_t *device) return; #ifdef USBHID +#if defined(USE_LIBUSB) + libusb_unref_device (device->handle); +#elif defined(USE_HIDAPI) + free (device->path); +#endif dc_usbhid_session_unref (device->session); #endif free (device); @@ -484,6 +498,10 @@ dc_usbhid_iterator_next (dc_iterator_t *abstract, void *out) device->session = dc_usbhid_session_ref (iterator->session); device->vid = dev.idVendor; device->pid = dev.idProduct; + device->handle = libusb_ref_device (current); + device->interface = interface->bInterfaceNumber; + device->endpoint_in = ep_in->bEndpointAddress; + device->endpoint_out = ep_out->bEndpointAddress; *(dc_usbhid_device_t **) out = device; @@ -510,6 +528,7 @@ dc_usbhid_iterator_next (dc_iterator_t *abstract, void *out) device->session = dc_usbhid_session_ref (iterator->session); device->vid = current->vendor_id; device->pid = current->product_id; + device->path = strdup (current->path); *(dc_usbhid_device_t **) out = device; @@ -537,16 +556,16 @@ dc_usbhid_iterator_free (dc_iterator_t *abstract) #endif dc_status_t -dc_usbhid_open (dc_iostream_t **out, dc_context_t *context, unsigned int vid, unsigned int pid) +dc_usbhid_open (dc_iostream_t **out, dc_context_t *context, dc_usbhid_device_t *device) { #ifdef USBHID dc_status_t status = DC_STATUS_SUCCESS; dc_usbhid_t *usbhid = NULL; - if (out == NULL) + if (out == NULL || device == NULL) return DC_STATUS_INVALIDARGS; - INFO (context, "Open: vid=%04x, pid=%04x", vid, pid); + INFO (context, "Open: vid=%04x, pid=%04x", device->vid, device->pid); // Allocate memory. usbhid = (dc_usbhid_t *) dc_iostream_allocate (context, &dc_usbhid_vtable, DC_TRANSPORT_USBHID); @@ -556,117 +575,22 @@ dc_usbhid_open (dc_iostream_t **out, dc_context_t *context, unsigned int vid, un } // Initialize the usb library. - status = dc_usbhid_session_new (&usbhid->session, context); - if (status != DC_STATUS_SUCCESS) { + usbhid->session = dc_usbhid_session_ref (device->session); + if (usbhid->session == NULL) { goto error_free; } #if defined(USE_LIBUSB) - struct libusb_device **devices = NULL; - struct libusb_config_descriptor *config = NULL; - int rc = 0; - - // Enumerate the USB devices. - ssize_t ndevices = libusb_get_device_list (usbhid->session->handle, &devices); - if (ndevices < 0) { - ERROR (context, "Failed to enumerate the usb devices (%s).", - libusb_error_name (ndevices)); - status = syserror (ndevices); - goto error_session_unref; - } - - // Find the first device matching the VID/PID. - struct libusb_device *device = NULL; - for (size_t i = 0; i < ndevices; i++) { - struct libusb_device_descriptor desc; - rc = libusb_get_device_descriptor (devices[i], &desc); - if (rc < 0) { - ERROR (context, "Failed to get the device descriptor (%s).", - libusb_error_name (rc)); - status = syserror (rc); - goto error_usb_free_list; - } - - if (desc.idVendor == vid && desc.idProduct == pid) { - device = devices[i]; - break; - } - } - - if (device == NULL) { - ERROR (context, "No matching USB device found."); - status = DC_STATUS_NODEVICE; - goto error_usb_free_list; - } - - // Get the active configuration descriptor. - rc = libusb_get_active_config_descriptor (device, &config); - if (rc != LIBUSB_SUCCESS) { - ERROR (context, "Failed to get the configuration descriptor (%s).", - libusb_error_name (rc)); - status = syserror (rc); - goto error_usb_free_list; - } - - // Find the first HID interface. - const struct libusb_interface_descriptor *interface = NULL; - for (unsigned int i = 0; i < config->bNumInterfaces; i++) { - const struct libusb_interface *iface = &config->interface[i]; - for (unsigned int j = 0; j < iface->num_altsetting; j++) { - const struct libusb_interface_descriptor *desc = &iface->altsetting[j]; - if (desc->bInterfaceClass == LIBUSB_CLASS_HID && interface == NULL) { - interface = desc; - } - } - } - - if (interface == NULL) { - ERROR (context, "No HID interface found."); - status = DC_STATUS_IO; - goto error_usb_free_config; - } - - // Find the first input and output interrupt endpoints. - const struct libusb_endpoint_descriptor *ep_in = NULL, *ep_out = NULL; - for (unsigned int i = 0; i < interface->bNumEndpoints; i++) { - const struct libusb_endpoint_descriptor *desc = &interface->endpoint[i]; - - unsigned int type = desc->bmAttributes & LIBUSB_TRANSFER_TYPE_MASK; - unsigned int direction = desc->bEndpointAddress & LIBUSB_ENDPOINT_DIR_MASK; - - if (type != LIBUSB_TRANSFER_TYPE_INTERRUPT) - continue; - - if (direction == LIBUSB_ENDPOINT_IN && ep_in == NULL) { - ep_in = desc; - } - - if (direction == LIBUSB_ENDPOINT_OUT && ep_out == NULL) { - ep_out = desc; - } - } - - if (ep_in == NULL || ep_out == NULL) { - ERROR (context, "No interrupt endpoints found."); - status = DC_STATUS_IO; - goto error_usb_free_config; - } - - usbhid->interface = interface->bInterfaceNumber; - usbhid->endpoint_in = ep_in->bEndpointAddress; - usbhid->endpoint_out = ep_out->bEndpointAddress; - usbhid->timeout = 0; - INFO (context, "Open: interface=%u, endpoints=%02x,%02x", - usbhid->interface, usbhid->endpoint_in, usbhid->endpoint_out); + device->interface, device->endpoint_in, device->endpoint_out); // Open the USB device. - rc = libusb_open (device, &usbhid->handle); + int rc = libusb_open (device->handle, &usbhid->handle); if (rc != LIBUSB_SUCCESS) { ERROR (context, "Failed to open the usb device (%s).", libusb_error_name (rc)); status = syserror (rc); - goto error_usb_free_config; + goto error_session_unref; } #if defined(LIBUSB_API_VERSION) && (LIBUSB_API_VERSION >= 0x01000102) @@ -674,7 +598,7 @@ dc_usbhid_open (dc_iostream_t **out, dc_context_t *context, unsigned int vid, un #endif // Claim the HID interface. - rc = libusb_claim_interface (usbhid->handle, usbhid->interface); + rc = libusb_claim_interface (usbhid->handle, device->interface); if (rc != LIBUSB_SUCCESS) { ERROR (context, "Failed to claim the usb interface (%s).", libusb_error_name (rc)); @@ -682,12 +606,16 @@ dc_usbhid_open (dc_iostream_t **out, dc_context_t *context, unsigned int vid, un goto error_usb_close; } - libusb_free_config_descriptor (config); - libusb_free_device_list (devices, 1); + usbhid->interface = device->interface; + usbhid->endpoint_in = device->endpoint_in; + usbhid->endpoint_out = device->endpoint_out; + usbhid->timeout = 0; #elif defined(USE_HIDAPI) + INFO (context, "Open: path=%s", device->path); + // Open the USB device. - usbhid->handle = hid_open (vid, pid, NULL); + usbhid->handle = hid_open_path (device->path); if (usbhid->handle == NULL) { ERROR (context, "Failed to open the usb device."); status = DC_STATUS_IO; @@ -704,10 +632,6 @@ dc_usbhid_open (dc_iostream_t **out, dc_context_t *context, unsigned int vid, un #if defined(USE_LIBUSB) error_usb_close: libusb_close (usbhid->handle); -error_usb_free_config: - libusb_free_config_descriptor (config); -error_usb_free_list: - libusb_free_device_list (devices, 1); #endif error_session_unref: dc_usbhid_session_unref (usbhid->session);