From 1684606fdf6c341bd16806c416cb83ab3e17a4dc Mon Sep 17 00:00:00 2001 From: Sam Lantinga Date: Tue, 4 Feb 2020 15:26:56 -0800 Subject: [PATCH] Fixed long delay on main thread caused by blocking rumble writes in HIDAPI 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. --- src/joystick/hidapi/SDL_hidapi_gamecube.c | 5 +++-- src/joystick/hidapi/SDL_hidapi_ps4.c | 3 ++- src/joystick/hidapi/SDL_hidapi_xbox360.c | 3 ++- src/joystick/hidapi/SDL_hidapi_xbox360w.c | 3 ++- src/joystick/hidapi/SDL_hidapi_xboxone.c | 21 +++++++++++---------- src/joystick/hidapi/SDL_hidapijoystick.c | 16 +++++++++++++++- src/joystick/hidapi/SDL_hidapijoystick_c.h | 12 +++++++++--- 7 files changed, 44 insertions(+), 19 deletions(-) diff --git a/src/joystick/hidapi/SDL_hidapi_gamecube.c b/src/joystick/hidapi/SDL_hidapi_gamecube.c index 6634427d05532..4800141017f48 100644 --- 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 index d79121a90170d..c2e590ad1988f 100644 --- 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 index 271af53191290..bc6fe95582dec 100644 --- 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 index f3550f3ef947b..af2286710c7e1 100644 --- 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 index 4fab0af8a1b88..d964351374fad 100644 --- 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 index 1da537b0ddf5a..a2e91a32051d9 100644 --- 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 index ae0326c10e681..562e8fa65c8f1 100644 --- 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;