Fixed long delay on main thread caused by blocking rumble writes in H…

…IDAPI drivers There is now a thread that handles all HIDAPI rumble requests and a lock that guarantees that we're not reading and writing the device at the same time.
libsdl-org · Feb 4, 2020 · 1684606 · 1684606
1 parent 6efebf1
commit 1684606
Show file tree

Hide file tree

Showing 7 changed files with 44 additions and 19 deletions.
diff --git a/src/joystick/hidapi/SDL_hidapi_gamecube.c b/src/joystick/hidapi/SDL_hidapi_gamecube.c
@@ -31,6 +31,7 @@
 #include "SDL_gamecontroller.h"
 #include "../SDL_sysjoystick.h"
 #include "SDL_hidapijoystick_c.h"
+#include "SDL_hidapi_rumble.h"
 
 
 #ifdef SDL_JOYSTICK_HIDAPI_GAMECUBE
@@ -285,7 +286,7 @@ HIDAPI_DriverGameCube_UpdateDevice(SDL_HIDAPI_Device *device)
 
     /* Write rumble packet */
     if (ctx->rumbleUpdate) {
-        hid_write(device->dev, ctx->rumble, sizeof(ctx->rumble));
+        SDL_HIDAPI_SendRumble(device, ctx->rumble, sizeof(ctx->rumble));
         ctx->rumbleUpdate = SDL_FALSE;
     }
 
@@ -343,7 +344,7 @@ HIDAPI_DriverGameCube_CloseJoystick(SDL_HIDAPI_Device *device, SDL_Joystick *joy
 
     /* Stop rumble activity */
     if (ctx->rumbleUpdate) {
-        hid_write(device->dev, ctx->rumble, sizeof(ctx->rumble));
+        SDL_HIDAPI_SendRumble(device, ctx->rumble, sizeof(ctx->rumble));
         ctx->rumbleUpdate = SDL_FALSE;
     }
 }

diff --git a/src/joystick/hidapi/SDL_hidapi_ps4.c b/src/joystick/hidapi/SDL_hidapi_ps4.c
@@ -33,6 +33,7 @@
 #include "SDL_gamecontroller.h"
 #include "../SDL_sysjoystick.h"
 #include "SDL_hidapijoystick_c.h"
+#include "SDL_hidapi_rumble.h"
 
 
 #ifdef SDL_JOYSTICK_HIDAPI_PS4
@@ -305,7 +306,7 @@ HIDAPI_DriverPS4_RumbleJoystick(SDL_HIDAPI_Device *device, SDL_Joystick *joystic
         SDL_memcpy(&data[report_size - sizeof(unCRC)], &unCRC, sizeof(unCRC));
     }
 
-    if (hid_write(device->dev, data, report_size) != report_size) {
+    if (SDL_HIDAPI_SendRumble(device, data, report_size) != report_size) {
         return SDL_SetError("Couldn't send rumble packet");
     }
     return 0;

diff --git a/src/joystick/hidapi/SDL_hidapi_xbox360.c b/src/joystick/hidapi/SDL_hidapi_xbox360.c
@@ -30,6 +30,7 @@
 #include "SDL_gamecontroller.h"
 #include "../SDL_sysjoystick.h"
 #include "SDL_hidapijoystick_c.h"
+#include "SDL_hidapi_rumble.h"
 
 
 #ifdef SDL_JOYSTICK_HIDAPI_XBOX360
@@ -416,7 +417,7 @@ HIDAPI_DriverXbox360_RumbleJoystick(SDL_HIDAPI_Device *device, SDL_Joystick *joy
     rumble_packet[4] = (high_frequency_rumble >> 8);
 #endif
 
-    if (hid_write(device->dev, rumble_packet, sizeof(rumble_packet)) != sizeof(rumble_packet)) {
+    if (SDL_HIDAPI_SendRumble(device, rumble_packet, sizeof(rumble_packet)) != sizeof(rumble_packet)) {
         return SDL_SetError("Couldn't send rumble packet");
     }
 #endif /* __WIN32__ */

diff --git a/src/joystick/hidapi/SDL_hidapi_xbox360w.c b/src/joystick/hidapi/SDL_hidapi_xbox360w.c
@@ -30,6 +30,7 @@
 #include "SDL_gamecontroller.h"
 #include "../SDL_sysjoystick.h"
 #include "SDL_hidapijoystick_c.h"
+#include "SDL_hidapi_rumble.h"
 
 
 #ifdef SDL_JOYSTICK_HIDAPI_XBOX360
@@ -151,7 +152,7 @@ HIDAPI_DriverXbox360W_RumbleJoystick(SDL_HIDAPI_Device *device, SDL_Joystick *jo
     rumble_packet[5] = (low_frequency_rumble >> 8);
     rumble_packet[6] = (high_frequency_rumble >> 8);
 
-    if (hid_write(device->dev, rumble_packet, sizeof(rumble_packet)) != sizeof(rumble_packet)) {
+    if (SDL_HIDAPI_SendRumble(device, rumble_packet, sizeof(rumble_packet)) != sizeof(rumble_packet)) {
         return SDL_SetError("Couldn't send rumble packet");
     }
     return 0;

diff --git a/src/joystick/hidapi/SDL_hidapi_xboxone.c b/src/joystick/hidapi/SDL_hidapi_xboxone.c
@@ -30,6 +30,7 @@
 #include "SDL_gamecontroller.h"
 #include "../SDL_sysjoystick.h"
 #include "SDL_hidapijoystick_c.h"
+#include "SDL_hidapi_rumble.h"
 
 
 #ifdef SDL_JOYSTICK_HIDAPI_XBOXONE
@@ -177,7 +178,7 @@ ControllerNeedsRumbleSequenceSynchronized(Uint16 vendor_id, Uint16 product_id)
 }
 
 static SDL_bool
-SynchronizeRumbleSequence(hid_device *dev, SDL_DriverXboxOne_Context *ctx)
+SynchronizeRumbleSequence(SDL_HIDAPI_Device *device, SDL_DriverXboxOne_Context *ctx)
 {
     Uint16 vendor_id = ctx->vendor_id;
     Uint16 product_id = ctx->product_id;
@@ -193,7 +194,7 @@ SynchronizeRumbleSequence(hid_device *dev, SDL_DriverXboxOne_Context *ctx)
         SDL_memcpy(init_packet, xboxone_rumble_reset, sizeof(xboxone_rumble_reset));
         for (i = 0; i < 255; ++i) {
             init_packet[2] = ((ctx->sequence + i) % 255);
-            if (hid_write(dev, init_packet, sizeof(xboxone_rumble_reset)) != sizeof(xboxone_rumble_reset)) {
+            if (SDL_HIDAPI_SendRumble(device, init_packet, sizeof(xboxone_rumble_reset)) != sizeof(xboxone_rumble_reset)) {
                 SDL_SetError("Couldn't write Xbox One initialization packet");
                 return SDL_FALSE;
             }
@@ -226,7 +227,7 @@ ControllerSendsWaitingForInit(Uint16 vendor_id, Uint16 product_id)
 }
 
 static SDL_bool
-SendControllerInit(hid_device *dev, SDL_DriverXboxOne_Context *ctx)
+SendControllerInit(SDL_HIDAPI_Device *device, SDL_DriverXboxOne_Context *ctx)
 {
     Uint16 vendor_id = ctx->vendor_id;
     Uint16 product_id = ctx->product_id;
@@ -258,7 +259,7 @@ SendControllerInit(hid_device *dev, SDL_DriverXboxOne_Context *ctx)
             if (init_packet[0] != 0x01) {
                 init_packet[2] = ctx->sequence++;
             }
-            if (hid_write(dev, init_packet, packet->size) != packet->size) {
+            if (hid_write(device->dev, init_packet, packet->size) != packet->size) {
                 SDL_SetError("Couldn't write Xbox One initialization packet");
                 return SDL_FALSE;
             }
@@ -272,7 +273,7 @@ SendControllerInit(hid_device *dev, SDL_DriverXboxOne_Context *ctx)
                     Uint8 data[USB_PACKET_LENGTH];
                     int size;
 
-                    while ((size = hid_read_timeout(dev, data, sizeof(data), 0)) > 0) {
+                    while ((size = hid_read_timeout(device->dev, data, sizeof(data), 0)) > 0) {
 #ifdef DEBUG_XBOX_PROTOCOL
                         DumpPacket("Xbox One INIT packet: size = %d", data, size);
 #endif
@@ -288,7 +289,7 @@ SendControllerInit(hid_device *dev, SDL_DriverXboxOne_Context *ctx)
         }
     }
 
-    SynchronizeRumbleSequence(dev, ctx);
+    SynchronizeRumbleSequence(device, ctx);
 
     return SDL_TRUE;
 }
@@ -371,14 +372,14 @@ HIDAPI_DriverXboxOne_RumbleJoystick(SDL_HIDAPI_Device *device, SDL_Joystick *joy
     SDL_DriverXboxOne_Context *ctx = (SDL_DriverXboxOne_Context *)device->context;
     Uint8 rumble_packet[] = { 0x09, 0x00, 0x00, 0x09, 0x00, 0x0F, 0x00, 0x00, 0x00, 0x00, 0xFF, 0x00, 0xFF };
 
-    SynchronizeRumbleSequence(device->dev, ctx);
+    SynchronizeRumbleSequence(device, ctx);
 
     /* Magnitude is 1..100 so scale the 16-bit input here */
     rumble_packet[2] = ctx->sequence++;
     rumble_packet[8] = low_frequency_rumble / 655;
     rumble_packet[9] = high_frequency_rumble / 655;
 
-    if (hid_write(device->dev, rumble_packet, sizeof(rumble_packet)) != sizeof(rumble_packet)) {
+    if (SDL_HIDAPI_SendRumble(device, rumble_packet, sizeof(rumble_packet)) != sizeof(rumble_packet)) {
         return SDL_SetError("Couldn't send rumble packet");
     }
     return 0;
@@ -523,7 +524,7 @@ HIDAPI_DriverXboxOne_UpdateDevice(SDL_HIDAPI_Device *device)
     if (!ctx->initialized &&
         !ControllerSendsWaitingForInit(device->vendor_id, device->product_id)) {
         if (SDL_TICKS_PASSED(SDL_GetTicks(), ctx->start_time + CONTROLLER_INIT_DELAY_MS)) {
-            if (!SendControllerInit(device->dev, ctx)) {
+            if (!SendControllerInit(device, ctx)) {
                 HIDAPI_JoystickDisconnected(device, joystick->instance_id);
                 return SDL_FALSE;
             }
@@ -542,7 +543,7 @@ HIDAPI_DriverXboxOne_UpdateDevice(SDL_HIDAPI_Device *device)
 #ifdef DEBUG_XBOX_PROTOCOL
                 SDL_Log("Delay after init: %ums\n", SDL_GetTicks() - ctx->start_time);
 #endif
-                if (!SendControllerInit(device->dev, ctx)) {
+                if (!SendControllerInit(device, ctx)) {
                     HIDAPI_JoystickDisconnected(device, joystick->instance_id);
                     return SDL_FALSE;
                 }

diff --git a/src/joystick/hidapi/SDL_hidapijoystick.c b/src/joystick/hidapi/SDL_hidapijoystick.c
@@ -32,6 +32,7 @@
 #include "SDL_joystick.h"
 #include "../SDL_sysjoystick.h"
 #include "SDL_hidapijoystick_c.h"
+#include "SDL_hidapi_rumble.h"
 #include "../../SDL_hints_c.h"
 
 #if defined(__WIN32__)
@@ -682,6 +683,7 @@ HIDAPI_AddDevice(struct hid_device_info *info)
         device->guid.data[14] = 'h';
         device->guid.data[15] = 0;
     }
+    device->dev_lock = SDL_CreateMutex();
 
     /* Need the device name before getting the driver to know whether to ignore this device */
     if (!device->name) {
@@ -768,6 +770,7 @@ HIDAPI_DelDevice(SDL_HIDAPI_Device *device)
 
             HIDAPI_CleanupDeviceDriver(device);
 
+            SDL_DestroyMutex(device->dev_lock);
             SDL_free(device->name);
             SDL_free(device->path);
             SDL_free(device);
@@ -904,7 +907,10 @@ HIDAPI_UpdateDevices(void)
         device = SDL_HIDAPI_devices;
         while (device) {
             if (device->driver) {
-                device->driver->UpdateDevice(device);
+                if (SDL_TryLockMutex(device->dev_lock) == 0) {
+                    device->driver->UpdateDevice(device);
+                    SDL_UnlockMutex(device->dev_lock);
+                }
             }
             device = device->next;
         }
@@ -1029,6 +1035,11 @@ HIDAPI_JoystickClose(SDL_Joystick * joystick)
     if (joystick->hwdata) {
         SDL_HIDAPI_Device *device = joystick->hwdata->device;
 
+        /* Wait for pending rumble to complete */
+        while (SDL_AtomicGet(&device->rumble_pending) > 0) {
+            SDL_Delay(10);
+        }
+
         device->driver->CloseJoystick(device, joystick);
 
         SDL_free(joystick->hwdata);
@@ -1048,6 +1059,9 @@ HIDAPI_JoystickQuit(void)
     while (SDL_HIDAPI_devices) {
         HIDAPI_DelDevice(SDL_HIDAPI_devices);
     }
+
+    SDL_HIDAPI_QuitRumble();
+
     for (i = 0; i < SDL_arraysize(SDL_HIDAPI_drivers); ++i) {
         SDL_HIDAPI_DeviceDriver *driver = SDL_HIDAPI_drivers[i];
         SDL_DelHintCallback(driver->hint, SDL_HIDAPIDriverHintChanged, NULL);

diff --git a/src/joystick/hidapi/SDL_hidapijoystick_c.h b/src/joystick/hidapi/SDL_hidapijoystick_c.h
@@ -23,6 +23,10 @@
 #ifndef SDL_JOYSTICK_HIDAPI_H
 #define SDL_JOYSTICK_HIDAPI_H
 
+#include "SDL_atomic.h"
+#include "SDL_mutex.h"
+#include "SDL_joystick.h"
+#include "SDL_gamecontroller.h"
 #include "../../hidapi/hidapi/hidapi.h"
 #include "../usb_ids.h"
 
@@ -50,6 +54,9 @@
 #define SDL_JOYSTICK_HIDAPI_STEAM
 #endif
 
+/* The maximum size of a USB packet for HID devices */
+#define USB_PACKET_LENGTH   64
+
 /* Forward declaration */
 struct _SDL_HIDAPI_DeviceDriver;
 
@@ -70,7 +77,9 @@ typedef struct _SDL_HIDAPI_Device
 
     struct _SDL_HIDAPI_DeviceDriver *driver;
     void *context;
+    SDL_mutex *dev_lock;
     hid_device *dev;
+    SDL_atomic_t rumble_pending;
     int num_joysticks;
     SDL_JoystickID *joysticks;
 
@@ -98,9 +107,6 @@ typedef struct _SDL_HIDAPI_DeviceDriver
 } SDL_HIDAPI_DeviceDriver;
 
 
-/* The maximum size of a USB packet for HID devices */
-#define USB_PACKET_LENGTH   64
-
 /* HIDAPI device support */
 extern SDL_HIDAPI_DeviceDriver SDL_HIDAPI_DriverPS4;
 extern SDL_HIDAPI_DeviceDriver SDL_HIDAPI_DriverSteam;