Fixes and improvements (#616)

Platforms/platform-esp32/source/drivers/usb/esp32_usb_device_controller.cpp

@@ -35,11 +35,10 @@ static constexpr size_t COMPOSITE_STRING_DESCRIPTOR_MAX = 6;
 
 // ---- Controller state ----
 // One TinyUSB device-mode slot, shared by every USB device class (each a separate child device
-// under usbdevice0 in the devicetree - see esp32_usb_hid_device.cpp / esp32_usb_device_msc.cpp /
-// esp32_usb_midi_device.cpp). Only one primary class may be installed at a time - see
-// UsbDeviceControllerApi::claim(). CDC (esp32_usb_cdc_device.cpp) is a separate, orthogonal
-// devicetree-presence addon composited into whichever primary is active (HID or MIDI; MSC is
-// deliberately excluded, see claim()'s cdc_enabled computation) - see claim() below.
+// under usbdevice0 in the devicetree; \see esp32_usb_hid_device.cpp, esp32_usb_device_msc.cpp,
+// esp32_usb_midi_device.cpp). Only one primary class may be installed at a time, enforced in
+// claim(). CDC (esp32_usb_cdc_device.cpp) is a separate, orthogonal devicetree-presence addon
+// composited into whichever primary is active; \see claim()'s cdc_enabled computation.
 
 struct UsbDeviceControllerCtx {
     enum UsbDeviceClass active_class = USB_DEVICE_CLASS_NONE;
@@ -53,10 +52,10 @@ struct UsbDeviceControllerCtx {
     uint8_t next_in_endpoint = 1;   // EP0 reserved
     uint8_t next_out_endpoint = 1;
 
-    // Composite descriptor scratch, rebuilt on every claim() - can't be `static const` per-class
+    // Composite descriptor scratch, rebuilt on every claim(); can't be `static const` per-class
     // anymore now that CDC's presence is a runtime devicetree fact, not compile-time. Two buffers
-    // since a primary (MSC) may need different bytes per speed (see UsbInterfaceContribution's
-    // hs_descriptor_bytes) - under !TUD_OPT_HIGH_SPEED only the fs buffer is ever used.
+    // since a primary (MSC) may need different bytes per speed (\see UsbInterfaceContribution's
+    // hs_descriptor_bytes). Under !TUD_OPT_HIGH_SPEED only the fs buffer is ever used.
     uint8_t composite_fs_config_descriptor[COMPOSITE_CONFIG_DESCRIPTOR_MAX];
 #if (TUD_OPT_HIGH_SPEED)
     uint8_t composite_hs_config_descriptor[COMPOSITE_CONFIG_DESCRIPTOR_MAX];
@@ -101,7 +100,7 @@ static bool find_cdc_child(struct Device* child, void* context) {
 }
 
 // Devicetree children are constructed/added/started strictly after their parent
-// (kernel_init.cpp's dts_devices[] loop starts each device in list order, parent first) - so the
+// (kernel_init.cpp's dts_devices[] loop starts each device in list order, parent first), so the
 // usbdevicecdc0 child does not exist yet when this controller's own start_device() runs.
 // Discovering it lazily here (called from is_cdc_enabled(), well after all devicetree devices
 // have finished starting) instead of once at start_device() time is the only way to actually see
@@ -168,14 +167,7 @@ static error_t claim(struct Device* device, enum UsbDeviceClass usb_class, const
         return ERROR_INVALID_STATE;
     }
 
-    // MSC is deliberately excluded from CDC compositing - a MSC-presenting device is meant to
-    // look/behave like plain mass storage to the host's storage stack, and there's no real use
-    // case for a console interface while acting as a flash drive, unlike HID/MIDI which
-    // plausibly want a console alongside. (A Windows safe-eject hang was seen during development
-    // of this compositing code but traced to a stale Windows-side driver/device-cache state,
-    // cleared by a host reboot - not caused by CDC compositing or this exclusion. Kept the
-    // exclusion anyway since it matches MSC's actual intended behavior.)
-    const bool cdc_enabled = usb_class != USB_DEVICE_CLASS_MSC && is_cdc_enabled(device);
+    const bool cdc_enabled = is_cdc_enabled(device);
 
     // ---- String table: primary's table, plus CDC's own interface string appended last.
     size_t string_count = config->string_descriptor_count;
@@ -207,7 +199,7 @@ static error_t claim(struct Device* device, enum UsbDeviceClass usb_class, const
 
     // ---- Descriptor byte assembly: primary's bytes, then CDC's (if enabled), behind one config
     // header. Built twice (fs/hs) under TUD_OPT_HIGH_SPEED if the primary supplied distinct
-    // high-speed bytes (see UsbInterfaceContribution::hs_descriptor_bytes) - CDC never varies by
+    // high-speed bytes (\see UsbInterfaceContribution::hs_descriptor_bytes). CDC never varies by
     // speed, so its bytes are reused verbatim in both buffers.
     auto assemble = [&](uint8_t* dest, size_t dest_capacity, const uint8_t* primary_bytes, size_t primary_len) -> error_t {
         const size_t total_len = TUD_CONFIG_DESC_LEN + primary_len + (cdc_enabled ? cdc_contribution.descriptor_bytes_len : 0);
@@ -250,13 +242,6 @@ static error_t claim(struct Device* device, enum UsbDeviceClass usb_class, const
 
     ctx->allocation_open = false;
 
-    // ---- Device descriptor: primary's metadata (idVendor/idProduct/strings), controller decides
-    // the class triad since CDC's presence (not the primary's identity) is what needs IAD in the
-    // general case - except MSC, which always got MISC/IAD unconditionally even standalone in
-    // the pre-refactor code (see esp32_usb_device_msc.cpp history). Kept that behavior verbatim
-    // for MSC (regardless of cdc_enabled, which is always false for MSC anyway - see the
-    // cdc_enabled computation above) simply to match what shipped before rather than introduce
-    // an unrelated behavior change while separating CDC out of HID's descriptor.
     ctx->composite_device_descriptor = *static_cast<const tusb_desc_device_t*>(config->device_descriptor);
     if (cdc_enabled || usb_class == USB_DEVICE_CLASS_MSC) {
         ctx->composite_device_descriptor.bDeviceClass = TUSB_CLASS_MISC;
@@ -277,7 +262,7 @@ static error_t claim(struct Device* device, enum UsbDeviceClass usb_class, const
     ctx->composite_tusb_cfg.fs_configuration_descriptor = ctx->composite_fs_config_descriptor;
     ctx->composite_tusb_cfg.hs_configuration_descriptor = ctx->composite_hs_config_descriptor;
     // The qualifier descriptor's class triad must mirror the device descriptor's (same IAD
-    // reasoning) - built here rather than supplied per-primary since its content is boilerplate
+    // reasoning). Built here rather than supplied per-primary since its content is boilerplate
     // (same bMaxPacketSize0/bNumConfigurations for every class) and the controller already knows
     // the class triad.
     ctx->composite_device_qualifier = {
@@ -366,7 +351,7 @@ extern const UsbDeviceControllerApi usb_device_controller_api = {
 };
 
 // ---- Driver lifecycle ----
-// This device is defined in each board's .dts (usbdevice0), same pattern as usbhost0 - its
+// This device is defined in each board's .dts (usbdevice0), same pattern as usbhost0. Its
 // child classes (usbdevicehid0, usbdevicemsc0, usbdevicemidi0, usbdevicecdc0) are separate .dts
 // nodes with their own drivers, wired to this device as their parent by the devicetree compiler.
 
@@ -377,19 +362,19 @@ static error_t start_device(struct Device* device) {
     auto* ctx = new UsbDeviceControllerCtx();
     device_set_driver_data(device, ctx);
     // cdc_child is NOT resolved here: usbdevicecdc0 (a child of this device in the devicetree)
-    // hasn't been constructed/started yet at this point - kernel_init.cpp starts devicetree
-    // devices in list order, parent before children. Resolved lazily instead, see
-    // find_cdc_child_lazy() / is_cdc_enabled().
+    // hasn't been constructed/started yet at this point; kernel_init.cpp starts devicetree
+    // devices in list order, parent before children.
+    // \see find_cdc_child_lazy(), is_cdc_enabled() for the lazy resolution.
     return ERROR_NONE;
 }
 
 static error_t stop_device(struct Device* device) {
     auto* ctx = static_cast<UsbDeviceControllerCtx*>(device_get_driver_data(device));
     // Safety cleanup: if a class still holds the slot (e.g. this device is stopped while HID/
-    // MSC/MIDI is still active), tear TinyUSB down and restore the PHY route before freeing ctx -
+    // MSC/MIDI is still active), tear TinyUSB down and restore the PHY route before freeing ctx;
     // otherwise the installed TinyUSB driver and mis-routed PHY would outlive the context that
-    // tracks them. Mirrors the same safety-release pattern each child driver's own stop_device
-    // uses (see esp32_usb_hid_device.cpp / esp32_usb_device_msc.cpp / esp32_usb_midi_device.cpp).
+    // tracks them. Mirrors the same safety-release pattern each child driver's own stop_device uses.
+    // \see esp32_usb_hid_device.cpp, esp32_usb_device_msc.cpp, esp32_usb_midi_device.cpp
     if (ctx->active_class != USB_DEVICE_CLASS_NONE) {
         release(device, ctx->active_class);
     }

Platforms/platform-esp32/source/drivers/usb/esp32_usbhost_hid.cpp

@@ -3,6 +3,7 @@
 
 #include <tactility/device.h>
 #include <tactility/driver.h>
+#include <tactility/drivers/keyboard.h>
 #include <tactility/drivers/usb_host_hid.h>
 #include <tactility/log.h>
 
@@ -26,6 +27,7 @@ constexpr auto HID_PROC_TASK_STACK    = 4096;
 constexpr auto HID_PROC_TASK_PRIORITY = 5;
 constexpr auto HID_STOP_TIMEOUT_MS    = 2000;
 constexpr auto MAX_SUBSCRIBERS        = 4;
+constexpr auto USB_HID_KB_QUEUE_SIZE  = 16;
 
 typedef struct {
     hid_host_device_handle_t handle;
@@ -53,8 +55,20 @@ struct UsbHidContext {
 
     QueueHandle_t    subscribers[MAX_SUBSCRIBERS] = {};
     SemaphoreHandle_t sub_mutex                   = nullptr;
+
+    // The controller Device itself (set in start_device()), used as the parent for kb_device.
+    Device* controller_device = nullptr;
+    // Dynamic KEYBOARD_TYPE child device, constructed while a physical USB keyboard is connected.
+    Device kb_device{};
+    bool kb_device_active = false;
 };
 
+extern "C" {
+static void usb_hid_keyboard_device_construct(UsbHidContext* ctx);
+static void usb_hid_keyboard_device_destruct(UsbHidContext* ctx);
+static void usb_hid_keyboard_publish_key(UsbHidContext* ctx, uint32_t lv_key, bool pressed, bool ctrl, bool alt, uint8_t hid_keycode, uint8_t hid_modifier);
+}
+
 static const uint8_t keycode2ascii[57][2] = {
     {0, 0}, {0, 0}, {0, 0}, {0, 0},
     {'a', 'A'}, {'b', 'B'}, {'c', 'C'}, {'d', 'D'}, {'e', 'E'},
@@ -182,8 +196,7 @@ static void hid_interface_callback(hid_host_device_handle_t handle,
                         uint32_t lv_key = ctx->pressed_lv_keys[prev_hid];
                         ctx->pressed_lv_keys[prev_hid] = 0;
                         if (lv_key) {
-                            UsbHidEvent evt = { .type = USB_HID_EVENT_KEY, .key = { lv_key, false, with_ctrl, with_alt } };
-                            publish_event(ctx, &evt);
+                            usb_hid_keyboard_publish_key(ctx, lv_key, false, with_ctrl, with_alt, prev_hid, kb->modifier.val);
                         }
                     }
                 }
@@ -221,11 +234,7 @@ static void hid_interface_callback(hid_host_device_handle_t handle,
                         uint32_t lv_key = hid_keycode_to_key(kb->modifier.val, hid_code,
                                                                   ctx->caps_lock_active, ctx->num_lock_active);
                         if (lv_key) {
-                            UsbHidEvent evt = {
-                                .type = USB_HID_EVENT_KEY,
-                                .key = { lv_key, true, with_ctrl, with_alt }
-                            };
-                            publish_event(ctx, &evt);
+                            usb_hid_keyboard_publish_key(ctx, lv_key, true, with_ctrl, with_alt, hid_code, kb->modifier.val);
                             ctx->pressed_lv_keys[hid_code] = lv_key;
                         }
                     }
@@ -271,6 +280,7 @@ static void hid_interface_callback(hid_host_device_handle_t handle,
             memset(ctx->pressed_lv_keys, 0, sizeof(ctx->pressed_lv_keys));
             ctx->kb_handle.store(nullptr);
             ctx->kb_led_pending.store(false);
+            usb_hid_keyboard_device_destruct(ctx);
         } else if (params.proto == HID_PROTOCOL_MOUSE) {
             ctx->mouse_connected = false;
         }
@@ -349,6 +359,7 @@ static void hid_proc_task(void* arg) {
                              | (ctx->caps_lock_active   ? 0x02 : 0)
                              | (ctx->scroll_lock_active ? 0x04 : 0);
                 hid_class_request_set_report(dev_evt.handle, HID_REPORT_TYPE_OUTPUT, 0, &leds, 1);
+                usb_hid_keyboard_device_construct(ctx);
             } else if (params.proto == HID_PROTOCOL_MOUSE) {
                 ctx->mouse_connected = true;
             }
@@ -419,8 +430,122 @@ static const UsbHidApi hid_api = {
 
 extern "C" {
 
+// region Dynamic KEYBOARD_TYPE device
+//
+// While a physical USB keyboard is connected, a KEYBOARD_TYPE child device is constructed so the
+// rest of the system (lvgl_hardware_keyboard_is_available(), Tactility's KeyboardDeviceListener)
+// sees a real hardware keyboard through the same generic device model as any other keyboard, e.g.
+// Devices/m5stack-tab5/Source/devices/tab5_keyboard.cpp. Real key events are delivered exclusively
+// through this device (kb_handle's hid_interface_callback pushes into its queue below); the
+// generic UsbHidEvent publish/subscribe channel above is unaffected for mouse move/button/scroll
+// and the right-click-as-ESC synthesis it already does.
+
+static error_t usb_hid_kb_device_start(Device* device) {
+    auto* queue = xQueueCreate(USB_HID_KB_QUEUE_SIZE, sizeof(KeyboardKeyData));
+    if (queue == nullptr) {
+        return ERROR_RESOURCE;
+    }
+    device_set_driver_data(device, queue);
+    return ERROR_NONE;
+}
+
+static error_t usb_hid_kb_device_stop(Device* device) {
+    auto* queue = static_cast<QueueHandle_t>(device_get_driver_data(device));
+    vQueueDelete(queue);
+    device_set_driver_data(device, nullptr);
+    return ERROR_NONE;
+}
+
+static error_t usb_hid_kb_device_read_key(Device* device, KeyboardKeyData* data) {
+    auto* queue = static_cast<QueueHandle_t>(device_get_driver_data(device));
+    if (queue == nullptr) {
+        *data = {};
+        return ERROR_NONE;
+    }
+    if (xQueueReceive(queue, data, 0) != pdTRUE) {
+        *data = {};
+        return ERROR_NONE;
+    }
+    data->continue_reading = uxQueueMessagesWaiting(queue) > 0;
+    return ERROR_NONE;
+}
+
+static const KeyboardApi esp32_usbhost_hid_keyboard_api = {
+    .read_key = usb_hid_kb_device_read_key,
+    .get_backlight = nullptr,
+    .is_present = nullptr,
+};
+
+Driver esp32_usbhost_hid_keyboard_driver = {
+    .name = "esp32_usbhost_hid_keyboard",
+    .compatible = (const char*[]) { nullptr },
+    .start_device = usb_hid_kb_device_start,
+    .stop_device = usb_hid_kb_device_stop,
+    .api = &esp32_usbhost_hid_keyboard_api,
+    .device_type = &KEYBOARD_TYPE,
+    .owner = nullptr,
+    .internal = nullptr,
+};
+
+static void usb_hid_keyboard_device_construct(UsbHidContext* ctx) {
+    if (ctx->kb_device_active) {
+        return;
+    }
+
+    ctx->kb_device = Device {
+        .address = 0,
+        .name = "usb_keyboard0",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr,
+    };
+
+    if (device_construct(&ctx->kb_device) != ERROR_NONE) {
+        LOG_E(TAG, "failed to construct USB keyboard device");
+        return;
+    }
+    device_set_parent(&ctx->kb_device, ctx->controller_device);
+    device_set_driver(&ctx->kb_device, &esp32_usbhost_hid_keyboard_driver);
+    if (device_add(&ctx->kb_device) != ERROR_NONE) {
+        LOG_E(TAG, "failed to add USB keyboard device");
+        device_destruct(&ctx->kb_device);
+        return;
+    }
+    if (device_start(&ctx->kb_device) != ERROR_NONE) {
+        LOG_E(TAG, "failed to start USB keyboard device");
+        device_remove(&ctx->kb_device);
+        device_destruct(&ctx->kb_device);
+        return;
+    }
+
+    ctx->kb_device_active = true;
+}
+
+static void usb_hid_keyboard_device_destruct(UsbHidContext* ctx) {
+    if (!ctx->kb_device_active) {
+        return;
+    }
+    ctx->kb_device_active = false;
+
+    device_stop(&ctx->kb_device);
+    device_remove(&ctx->kb_device);
+    device_destruct(&ctx->kb_device);
+}
+
+static void usb_hid_keyboard_publish_key(UsbHidContext* ctx, uint32_t lv_key, bool pressed, bool ctrl, bool alt, uint8_t hid_keycode, uint8_t hid_modifier) {
+    if (!ctx->kb_device_active) {
+        return;
+    }
+    auto* queue = static_cast<QueueHandle_t>(device_get_driver_data(&ctx->kb_device));
+    KeyboardKeyData data = { lv_key, pressed, false, ctrl, alt, hid_keycode, hid_modifier };
+    xQueueSend(queue, &data, 0);
+}
+
+// endregion
+
 static error_t start_device(struct Device* device) {
     auto* ctx = new UsbHidContext();
+    ctx->controller_device = device;
 
     ctx->sub_mutex = xSemaphoreCreateMutex();
     if (!ctx->sub_mutex) {
@@ -486,6 +611,13 @@ static error_t stop_device(struct Device* device) {
     auto* ctx = static_cast<UsbHidContext*>(device_get_driver_data(device));
     if (!ctx) return ERROR_NONE;
 
+    // hid_host_device_close() halts and flushes the pending IN transfer before returning, so no
+    // hid_interface_callback() for this handle can race the queue delete below.
+    if (auto kb_handle = ctx->kb_handle.load()) {
+        hid_host_device_close(kb_handle);
+    }
+    usb_hid_keyboard_device_destruct(ctx);
+
     ctx->hid_proc_running = false;
 
     if (xSemaphoreTake(ctx->hid_proc_task_done, pdMS_TO_TICKS(HID_STOP_TIMEOUT_MS)) != pdTRUE) {