[Maintenance] USB HID control packet as struct (#21688)

* ChibiOS: USB HID control request as dedicated struct

Instead of accessing the raw USB setup packet and documenting the values
as the corresponding USB HID control request fields we introduce a
struct that allows direct access to the fields. This is safer and self
documenting.

* Rename usb_request.h to usb_types.h

In the future all shared USB data types can live in this file.

2 files changed, 82 insertions, 92 deletions

diff --git a/tmk_core/protocol/chibios/usb_main.c b/tmk_core/protocol/chibios/usb_main.c
index f2158fd009..66f9ad0318 100644
--- a/tmk_core/protocol/chibios/usb_main.c
+++ b/tmk_core/protocol/chibios/usb_main.c
@@ -43,6 +43,7 @@
 #include "usb_device_state.h"
 #include "usb_descriptor.h"
 #include "usb_driver.h"
+#include "usb_types.h"
 
 #ifdef NKRO_ENABLE
 #    include "keycode_config.h"
@@ -103,30 +104,18 @@ union {
             NULL, /* SETUP buffer (not a SETUP endpoint) */
 #endif
 
-/* HID specific constants */
-#define HID_GET_REPORT 0x01
-#define HID_GET_IDLE 0x02
-#define HID_GET_PROTOCOL 0x03
-#define HID_SET_REPORT 0x09
-#define HID_SET_IDLE 0x0A
-#define HID_SET_PROTOCOL 0x0B
-
-/*
- * Handles the GET_DESCRIPTOR callback
- *
- * Returns the proper descriptor
- */
 static const USBDescriptor *usb_get_descriptor_cb(USBDriver *usbp, uint8_t dtype, uint8_t dindex, uint16_t wIndex) {
-    (void)usbp;
-    static USBDescriptor desc;
-    uint16_t             wValue  = ((uint16_t)dtype << 8) | dindex;
-    uint16_t             wLength = ((uint16_t)usbp->setup[7] << 8) | usbp->setup[6];
-    desc.ud_string               = NULL;
-    desc.ud_size                 = get_usb_descriptor(wValue, wIndex, wLength, (const void **const) & desc.ud_string);
-    if (desc.ud_string == NULL)
+    usb_control_request_t *setup = (usb_control_request_t *)usbp->setup;
+
+    static USBDescriptor descriptor;
+    descriptor.ud_string = NULL;
+    descriptor.ud_size   = get_usb_descriptor(setup->wValue.word, setup->wIndex, setup->wLength, (const void **const) & descriptor.ud_string);
+
+    if (descriptor.ud_string == NULL) {
         return NULL;
-    else
-        return &desc;
+    }
+
+    return &descriptor;
 }
 
 /*
@@ -497,8 +486,7 @@ void usb_event_queue_task(void) {
     }
 }
 
-/* Handles the USB driver global events
- * TODO: maybe disable some things when connection is lost? */
+/* Handles the USB driver global events. */
 static void usb_event_cb(USBDriver *usbp, usbevent_t event) {
     switch (event) {
         case USB_EVENT_ADDRESS:
@@ -570,16 +558,6 @@ static void usb_event_cb(USBDriver *usbp, usbevent_t event) {
     }
 }
 
-/* Function used locally in os/hal/src/usb.c for getting descriptors
- * need it here for HID descriptor */
-static uint16_t get_hword(uint8_t *p) {
-    uint16_t hw;
-
-    hw = (uint16_t)*p++;
-    hw |= (uint16_t)*p << 8U;
-    return hw;
-}
-
 /*
  * Appendix G: HID Request Support Requirements
  *
@@ -596,7 +574,9 @@ static uint16_t get_hword(uint8_t *p) {
 static uint8_t set_report_buf[2] __attribute__((aligned(4)));
 
 static void set_led_transfer_cb(USBDriver *usbp) {
-    if (usbp->setup[6] == 2) { /* LSB(wLength) */
+    usb_control_request_t *setup = (usb_control_request_t *)usbp->setup;
+
+    if (setup->wLength == 2) {
         uint8_t report_id = set_report_buf[0];
         if ((report_id == REPORT_ID_KEYBOARD) || (report_id == REPORT_ID_NKRO)) {
             keyboard_led_state = set_report_buf[1];
@@ -606,24 +586,16 @@ static void set_led_transfer_cb(USBDriver *usbp) {
     }
 }
 
-/* Callback for SETUP request on the endpoint 0 (control) */
-static bool usb_request_hook_cb(USBDriver *usbp) {
-    const USBDescriptor *dp;
-
-    /* usbp->setup fields:
-     *  0:   bmRequestType (bitmask)
-     *  1:   bRequest
-     *  2,3: (LSB,MSB) wValue
-     *  4,5: (LSB,MSB) wIndex
-     *  6,7: (LSB,MSB) wLength (number of bytes to transfer if there is a data phase) */
+static bool usb_requests_hook_cb(USBDriver *usbp) {
+    usb_control_request_t *setup = (usb_control_request_t *)usbp->setup;
 
     /* Handle HID class specific requests */
-    if (((usbp->setup[0] & USB_RTYPE_TYPE_MASK) == USB_RTYPE_TYPE_CLASS) && ((usbp->setup[0] & USB_RTYPE_RECIPIENT_MASK) == USB_RTYPE_RECIPIENT_INTERFACE)) {
-        switch (usbp->setup[0] & USB_RTYPE_DIR_MASK) {
+    if ((setup->bmRequestType & (USB_RTYPE_TYPE_MASK | USB_RTYPE_RECIPIENT_MASK)) == (USB_RTYPE_TYPE_CLASS | USB_RTYPE_RECIPIENT_INTERFACE)) {
+        switch (setup->bmRequestType & USB_RTYPE_DIR_MASK) {
             case USB_RTYPE_DIR_DEV2HOST:
-                switch (usbp->setup[1]) { /* bRequest */
-                    case HID_GET_REPORT:
-                        switch (usbp->setup[4]) { /* LSB(wIndex) (check MSB==0?) */
+                switch (setup->bRequest) {
+                    case HID_REQ_GetReport:
+                        switch (setup->wIndex) {
 #ifndef KEYBOARD_SHARED_EP
                             case KEYBOARD_INTERFACE:
                                 usbSetupTransfer(usbp, (uint8_t *)&keyboard_report_sent, KEYBOARD_REPORT_SIZE, NULL);
@@ -639,59 +611,54 @@ static bool usb_request_hook_cb(USBDriver *usbp) {
 #ifdef SHARED_EP_ENABLE
                             case SHARED_INTERFACE:
 #    ifdef KEYBOARD_SHARED_EP
-                                if (usbp->setup[2] == REPORT_ID_KEYBOARD) {
+                                if (setup->wValue.lbyte == REPORT_ID_KEYBOARD) {
                                     usbSetupTransfer(usbp, (uint8_t *)&keyboard_report_sent, KEYBOARD_REPORT_SIZE, NULL);
-                                    return TRUE;
-                                    break;
+                                    return true;
                                 }
 #    endif
 #    ifdef MOUSE_SHARED_EP
-                                if (usbp->setup[2] == REPORT_ID_MOUSE) {
+                                if (setup->wValue.lbyte == REPORT_ID_MOUSE) {
                                     usbSetupTransfer(usbp, (uint8_t *)&mouse_report_sent, sizeof(mouse_report_sent), NULL);
-                                    return TRUE;
-                                    break;
+                                    return true;
                                 }
 #    endif
 #endif /* SHARED_EP_ENABLE */
                             default:
-                                universal_report_blank.report_id = usbp->setup[2];
-                                usbSetupTransfer(usbp, (uint8_t *)&universal_report_blank, usbp->setup[6], NULL);
-                                return TRUE;
-                                break;
+                                universal_report_blank.report_id = setup->wValue.lbyte;
+                                usbSetupTransfer(usbp, (uint8_t *)&universal_report_blank, setup->wLength, NULL);
+                                return true;
                         }
                         break;
 
-                    case HID_GET_PROTOCOL:
-                        if ((usbp->setup[4] == KEYBOARD_INTERFACE) && (usbp->setup[5] == 0)) { /* wIndex */
-                            usbSetupTransfer(usbp, &keyboard_protocol, 1, NULL);
-                            return TRUE;
+                    case HID_REQ_GetProtocol:
+                        if (setup->wIndex == KEYBOARD_INTERFACE) {
+                            usbSetupTransfer(usbp, &keyboard_protocol, sizeof(uint8_t), NULL);
+                            return true;
                         }
                         break;
 
-                    case HID_GET_IDLE:
-                        usbSetupTransfer(usbp, &keyboard_idle, 1, NULL);
-                        return TRUE;
-                        break;
+                    case HID_REQ_GetIdle:
+                        usbSetupTransfer(usbp, &keyboard_idle, sizeof(uint8_t), NULL);
+                        return true;
                 }
                 break;
 
             case USB_RTYPE_DIR_HOST2DEV:
-                switch (usbp->setup[1]) { /* bRequest */
-                    case HID_SET_REPORT:
-                        switch (usbp->setup[4]) { /* LSB(wIndex) (check MSB==0?) */
+                switch (setup->bRequest) {
+                    case HID_REQ_SetReport:
+                        switch (setup->wIndex) {
                             case KEYBOARD_INTERFACE:
 #if defined(SHARED_EP_ENABLE) && !defined(KEYBOARD_SHARED_EP)
                             case SHARED_INTERFACE:
 #endif
                                 usbSetupTransfer(usbp, set_report_buf, sizeof(set_report_buf), set_led_transfer_cb);
-                                return TRUE;
-                                break;
+                                return true;
                         }
                         break;
 
-                    case HID_SET_PROTOCOL:
-                        if ((usbp->setup[4] == KEYBOARD_INTERFACE) && (usbp->setup[5] == 0)) { /* wIndex */
-                            keyboard_protocol = ((usbp->setup[2]) != 0x00);                    /* LSB(wValue) */
+                    case HID_REQ_SetProtocol:
+                        if (setup->wIndex == KEYBOARD_INTERFACE) {
+                            keyboard_protocol = setup->wValue.word;
 #ifdef NKRO_ENABLE
                             if (!keyboard_protocol && keyboard_idle) {
 #else  /* NKRO_ENABLE */
@@ -704,12 +671,11 @@ static bool usb_request_hook_cb(USBDriver *usbp) {
                             }
                         }
                         usbSetupTransfer(usbp, NULL, 0, NULL);
-                        return TRUE;
-                        break;
+                        return true;
 
-                    case HID_SET_IDLE:
-                        keyboard_idle = usbp->setup[3]; /* MSB(wValue) */
-                                                        /* arm the timer */
+                    case HID_REQ_SetIdle:
+                        keyboard_idle = setup->wValue.hbyte;
+                        /* arm the timer */
 #ifdef NKRO_ENABLE
                         if (!keymap_config.nkro && keyboard_idle) {
 #else  /* NKRO_ENABLE */
@@ -720,19 +686,21 @@ static bool usb_request_hook_cb(USBDriver *usbp) {
                             osalSysUnlockFromISR();
                         }
                         usbSetupTransfer(usbp, NULL, 0, NULL);
-                        return TRUE;
-                        break;
+                        return true;
                 }
                 break;
         }
     }
 
-    /* Handle the Get_Descriptor Request for HID class (not handled by the default hook) */
-    if ((usbp->setup[0] == 0x81) && (usbp->setup[1] == USB_REQ_GET_DESCRIPTOR)) {
-        dp = usbp->config->get_descriptor_cb(usbp, usbp->setup[3], usbp->setup[2], get_hword(&usbp->setup[4]));
-        if (dp == NULL) return FALSE;
-        usbSetupTransfer(usbp, (uint8_t *)dp->ud_string, dp->ud_size, NULL);
-        return TRUE;
+    /* Handle the Get_Descriptor Request for HID class, which is not handled by
+     * the ChibiOS USB driver */
+    if (((setup->bmRequestType & (USB_RTYPE_DIR_MASK | USB_RTYPE_RECIPIENT_MASK)) == (USB_RTYPE_DIR_DEV2HOST | USB_RTYPE_RECIPIENT_INTERFACE)) && (setup->bRequest == USB_REQ_GET_DESCRIPTOR)) {
+        const USBDescriptor *descriptor = usbp->config->get_descriptor_cb(usbp, setup->wValue.lbyte, setup->wValue.hbyte, setup->wIndex);
+        if (descriptor == NULL) {
+            return false;
+        }
+        usbSetupTransfer(usbp, (uint8_t *)descriptor->ud_string, descriptor->ud_size, NULL);
+        return true;
     }
 
     for (int i = 0; i < NUM_USB_DRIVERS; i++) {
@@ -742,10 +710,9 @@ static bool usb_request_hook_cb(USBDriver *usbp) {
         }
     }
 
-    return FALSE;
+    return false;
 }
 
-/* Start-of-frame callback */
 static void usb_sof_cb(USBDriver *usbp) {
     osalSysLockFromISR();
     for (int i = 0; i < NUM_USB_DRIVERS; i++) {
@@ -758,7 +725,7 @@ static void usb_sof_cb(USBDriver *usbp) {
 static const USBConfig usbcfg = {
     usb_event_cb,          /* USB events callback */
     usb_get_descriptor_cb, /* Device GET_DESCRIPTOR request callback */
-    usb_request_hook_cb,   /* Requests hook callback */
+    usb_requests_hook_cb,  /* Requests hook callback */
     usb_sof_cb             /* Start Of Frame callback */
 };
 
diff --git a/tmk_core/protocol/usb_types.h b/tmk_core/protocol/usb_types.h
new file mode 100644
index 0000000000..019775a1c4
--- /dev/null
+++ b/tmk_core/protocol/usb_types.h
@@ -0,0 +1,23 @@
+// Copyright 2023 Stefan Kerkmann
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#pragma once
+
+#include "util.h"
+
+/**
+ * @brief Common USB 2.0 control request structure
+ */
+typedef struct {
+    uint8_t bmRequestType; // [0] (Bitmask)
+    uint8_t bRequest;      // [1]
+    union {
+        struct {
+            uint8_t lbyte; // [2] (LSB)
+            uint8_t hbyte; // [3] (MSB)
+        };
+        uint16_t word; // [2,3] (LSB,MSB)
+    } wValue;
+    uint16_t wIndex;  // [4,5] (LSB,MSB)
+    uint16_t wLength; // [6,7] (LSB,MSB)
+} PACKED usb_control_request_t;