Update for bug 4923 - Calling SDL_GameControllerRumble() often takes 8 ms

meyraud705

Dualshock4 on bluetooth need 78 bytes for the rumble data while SDL_HIDAPI_RumbleRequest can only hold 64 bytes.

'volatile' is not meant for thread synchronization.

The list of rumble request could grow infinitely if user call SDL_JoystickRumble too much. The documentation says "Each call to this function cancels any previous rumble effect", so overwriting pending request seem like a good idea.

src/joystick/hidapi/SDL_hidapi_rumble.c

@@ -34,7 +34,7 @@
 typedef struct SDL_HIDAPI_RumbleRequest
 {
     SDL_HIDAPI_Device *device;
-    Uint8 data[USB_PACKET_LENGTH];
+    Uint8 data[2*USB_PACKET_LENGTH]; /* need enough space for the biggest report: dualshock4 is 78 bytes */
     int size;
     struct SDL_HIDAPI_RumbleRequest *prev;
 
@@ -42,7 +42,8 @@ typedef struct SDL_HIDAPI_RumbleRequest
 
 typedef struct SDL_HIDAPI_RumbleContext
 {
-    volatile SDL_bool running;
+    SDL_atomic_t initialized;
+    SDL_atomic_t running;
     SDL_Thread *thread;
     SDL_mutex *lock;
     SDL_sem *request_sem;
@@ -58,7 +59,7 @@ static int SDL_HIDAPI_RumbleThread(void *data)
 
     SDL_SetThreadPriority(SDL_THREAD_PRIORITY_HIGH);
 
-    while (ctx->running) {
+    while (SDL_AtomicGet(&ctx->running)) {
         SDL_HIDAPI_RumbleRequest *request = NULL;
 
         SDL_SemWait(ctx->request_sem);
@@ -87,7 +88,7 @@ static int SDL_HIDAPI_RumbleThread(void *data)
 static void
 SDL_HIDAPI_StopRumbleThread(SDL_HIDAPI_RumbleContext *ctx)
 {
-    ctx->running = SDL_FALSE;
+    SDL_AtomicSet(&ctx->running, SDL_FALSE);
 
     if (ctx->thread) {
         int result;
@@ -110,6 +111,8 @@ SDL_HIDAPI_StopRumbleThread(SDL_HIDAPI_RumbleContext *ctx)
         SDL_DestroyMutex(ctx->lock);
         ctx->lock = NULL;
     }
+
+    SDL_AtomicSet(&ctx->initialized, SDL_FALSE);
 }
 
 static int
@@ -127,7 +130,7 @@ SDL_HIDAPI_StartRumbleThread(SDL_HIDAPI_RumbleContext *ctx)
         return -1;
     }
 
-    ctx->running = SDL_TRUE;
+    SDL_AtomicSet(&ctx->running, SDL_TRUE);
     ctx->thread = SDL_CreateThreadInternal(SDL_HIDAPI_RumbleThread, "HIDAPI Rumble", 0, ctx);
     if (!ctx->thread) {
         SDL_HIDAPI_StopRumbleThread(ctx);
@@ -136,23 +139,48 @@ SDL_HIDAPI_StartRumbleThread(SDL_HIDAPI_RumbleContext *ctx)
     return 0;
 }
 
-int SDL_HIDAPI_SendRumble(SDL_HIDAPI_Device *device, const Uint8 *data, int size)
+int SDL_HIDAPI_LockRumble(void)
 {
     SDL_HIDAPI_RumbleContext *ctx = &rumble_context;
-    SDL_HIDAPI_RumbleRequest *request;
-
-    if (size > sizeof(request->data)) {
-        return SDL_SetError("Couldn't send rumble, size %d is greater than %d", size, (int)sizeof(request->data));
-    }
 
-    if (!ctx->running) {
+    if (SDL_AtomicCAS(&ctx->initialized, SDL_FALSE, SDL_TRUE)) {
         if (SDL_HIDAPI_StartRumbleThread(ctx) < 0) {
             return -1;
         }
     }
 
+    return SDL_LockMutex(ctx->lock);
+}
+
+SDL_bool SDL_HIDAPI_GetPendingRumbleLocked(SDL_HIDAPI_Device *device, Uint8 **data, int **size, int *maximum_size)
+{
+    SDL_HIDAPI_RumbleContext *ctx = &rumble_context;
+    SDL_HIDAPI_RumbleRequest *request;
+
+    for (request = ctx->requests_tail; request; request = request->prev) {
+        if (request->device == device) {
+            *data = request->data;
+            *size = &request->size;
+            *maximum_size = sizeof(request->data);
+            return SDL_TRUE;
+        }
+    }
+    return SDL_FALSE;
+}
+
+int SDL_HIDAPI_SendRumbleAndUnlock(SDL_HIDAPI_Device *device, const Uint8 *data, int size)
+{
+    SDL_HIDAPI_RumbleContext *ctx = &rumble_context;
+    SDL_HIDAPI_RumbleRequest *request;
+
+    if (size > sizeof(request->data)) {
+        SDL_HIDAPI_UnlockRumble();
+        return SDL_SetError("Couldn't send rumble, size %d is greater than %d", size, (int)sizeof(request->data));
+    }
+
     request = (SDL_HIDAPI_RumbleRequest *)SDL_calloc(1, sizeof(*request));
     if (!request) {
+        SDL_HIDAPI_UnlockRumble();
         return SDL_OutOfMemory();
     }
     request->device = device;
@@ -161,25 +189,59 @@ int SDL_HIDAPI_SendRumble(SDL_HIDAPI_Device *device, const Uint8 *data, int size
 
     SDL_AtomicIncRef(&device->rumble_pending);
     
-    SDL_LockMutex(ctx->lock);
     if (ctx->requests_head) {
         ctx->requests_head->prev = request;
     } else {
         ctx->requests_tail = request;
     }
     ctx->requests_head = request;
-    SDL_UnlockMutex(ctx->lock);
+
+    /* Make sure we unlock before posting the semaphore so the rumble thread can run immediately */
+    SDL_HIDAPI_UnlockRumble();
 
     SDL_SemPost(ctx->request_sem);
 
     return size;
 }
 
+void SDL_HIDAPI_UnlockRumble(void)
+{
+    SDL_HIDAPI_RumbleContext *ctx = &rumble_context;
+
+    SDL_UnlockMutex(ctx->lock);
+}
+
+int SDL_HIDAPI_SendRumble(SDL_HIDAPI_Device *device, const Uint8 *data, int size)
+{
+    Uint8 *pending_data;
+    int *pending_size;
+    int maximum_size;
+
+    if (SDL_HIDAPI_LockRumble() < 0) {
+        return -1;
+    }
+
+    /* check if there is a pending request for the device and update it */
+    if (SDL_HIDAPI_GetPendingRumbleLocked(device, &pending_data, &pending_size, &maximum_size)) {
+        if (size > maximum_size) {
+            SDL_HIDAPI_UnlockRumble();
+            return SDL_SetError("Couldn't send rumble, size %d is greater than %d", size, maximum_size);
+        }
+
+        SDL_memcpy(pending_data, data, size);
+        *pending_size = size;
+        SDL_HIDAPI_UnlockRumble();
+        return size;
+    }
+
+    return SDL_HIDAPI_SendRumbleAndUnlock(device, data, size);
+}
+
 void SDL_HIDAPI_QuitRumble(void)
 {
     SDL_HIDAPI_RumbleContext *ctx = &rumble_context;
 
-    if (ctx->running) {
+    if (SDL_AtomicGet(&ctx->running)) {
         SDL_HIDAPI_StopRumbleThread(ctx);
     }
 }

src/joystick/hidapi/SDL_hidapi_rumble.h

@@ -23,6 +23,14 @@
 #ifdef SDL_JOYSTICK_HIDAPI
 
 /* Handle rumble on a separate thread so it doesn't block the application */
+
+/* Advanced API */
+int SDL_HIDAPI_LockRumble(void);
+SDL_bool SDL_HIDAPI_GetPendingRumbleLocked(SDL_HIDAPI_Device *device, Uint8 **data, int **size, int *maximum_size);
+int SDL_HIDAPI_SendRumbleAndUnlock(SDL_HIDAPI_Device *device, const Uint8 *data, int size);
+void SDL_HIDAPI_UnlockRumble(void);
+
+/* Simple API, will replace any pending rumble with the new data */
 int SDL_HIDAPI_SendRumble(SDL_HIDAPI_Device *device, const Uint8 *data, int size);
 void SDL_HIDAPI_QuitRumble(void);
 

src/joystick/hidapi/SDL_hidapi_xboxone.c

@@ -118,6 +118,7 @@ typedef struct {
     Uint8 sequence;
     Uint8 last_state[USB_PACKET_LENGTH];
     SDL_bool rumble_synchronized;
+    SDL_bool rumble_synchronization_complete;
     SDL_bool has_paddles;
 } SDL_DriverXboxOne_Context;
 
@@ -194,7 +195,10 @@ SynchronizeRumbleSequence(SDL_HIDAPI_Device *device, SDL_DriverXboxOne_Context *
         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 (SDL_HIDAPI_SendRumble(device, init_packet, sizeof(xboxone_rumble_reset)) != sizeof(xboxone_rumble_reset)) {
+            if (SDL_HIDAPI_LockRumble() < 0) {
+                return SDL_FALSE;
+            }
+            if (SDL_HIDAPI_SendRumbleAndUnlock(device, init_packet, sizeof(xboxone_rumble_reset)) != sizeof(xboxone_rumble_reset)) {
                 SDL_SetError("Couldn't write Xbox One initialization packet");
                 return SDL_FALSE;
             }
@@ -371,16 +375,39 @@ 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 };
+    Uint8 *pending_rumble;
+    int *pending_size;
+    int maximum_size;
 
     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 (SDL_HIDAPI_LockRumble() < 0) {
+        return -1;
+    }
 
-    if (SDL_HIDAPI_SendRumble(device, rumble_packet, sizeof(rumble_packet)) != sizeof(rumble_packet)) {
-        return SDL_SetError("Couldn't send rumble packet");
+    if (!ctx->rumble_synchronization_complete) {
+        if (!SDL_HIDAPI_GetPendingRumbleLocked(device, &pending_rumble, &pending_size, &maximum_size)) {
+            /* Rumble synchronization has drained */
+            ctx->rumble_synchronization_complete = SDL_TRUE;
+        }
+    }
+
+    /* Try to overwrite any pending rumble with the new value */
+    if (ctx->rumble_synchronization_complete && 
+        SDL_HIDAPI_GetPendingRumbleLocked(device, &pending_rumble, &pending_size, &maximum_size)) {
+        /* Magnitude is 1..100 so scale the 16-bit input here */
+        pending_rumble[8] = low_frequency_rumble / 655;
+        pending_rumble[9] = high_frequency_rumble / 655;
+        SDL_HIDAPI_UnlockRumble();
+    } else {
+        /* 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 (SDL_HIDAPI_SendRumbleAndUnlock(device, rumble_packet, sizeof(rumble_packet)) != sizeof(rumble_packet)) {
+            return SDL_SetError("Couldn't send rumble packet");
+        }
     }
     return 0;
 }