Fixesx and improvements

Devices/m5stack-tab5/Source/devices/tab5_keyboard.cpp

@@ -268,10 +268,6 @@ static error_t probe(Device* device) {
     return i2c_controller_has_device_at_address(parent, I2C_ADDRESS, pdMS_TO_TICKS(5));
 }
 
-error_t tab5_keyboard_is_attached(Device* device) {
-    return device_is_ready(device);
-}
-
 // ---------------------------------------------------------------------------
 // LED helpers - LED0 = Sym indicator (blue), LED1 = Aa indicator (red)
 // RGB register layout: [B, G, R] per LED, stride 4 (byte 3 reserved)

Devices/m5stack-tab5/Source/devices/tab5_keyboard.h

@@ -28,11 +28,6 @@ extern struct Driver tab5_keyboard_driver;
  */
 void tab5_create_keyboard(struct Device* i2c2);
 
-/** @brief Proxy for device_is_started().
- * @deprecated Use device_is_started()
- */
-error_t tab5_keyboard_is_attached(struct Device* device);
-
 #ifdef __cplusplus
 }
 #endif

Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.cpp

@@ -24,8 +24,12 @@ static void apply_rotation(bool attached) {
             rotation_override_active = true;
             lv_display_set_rotation(display, LV_DISPLAY_ROTATION_90);
         }
-    } else if (rotation_override_active && lv_display_get_rotation(display) == LV_DISPLAY_ROTATION_90) {
-        lv_display_set_rotation(display, saved_rotation);
+    } else if (rotation_override_active) {
+        // Ownership of the rotation ends here regardless - only restore it if the display is
+        // still at what we set it to (the user may have changed it manually while attached).
+        if (lv_display_get_rotation(display) == LV_DISPLAY_ROTATION_90) {
+            lv_display_set_rotation(display, saved_rotation);
+        }
         rotation_override_active = false;
     }
 
@@ -49,5 +53,6 @@ void tab5_keyboard_lvgl_rotation_start() {
 }
 
 void tab5_keyboard_lvgl_rotation_stop() {
+    apply_rotation(false);
     device_listener_remove(on_device_event);
 }

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

@@ -27,7 +27,6 @@ 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;
@@ -437,47 +436,9 @@ 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,
+    .read_key = nullptr,
     .get_backlight = nullptr,
     .is_present = nullptr,
 };
@@ -485,8 +446,9 @@ static const KeyboardApi esp32_usbhost_hid_keyboard_api = {
 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,
+    .start_device = nullptr,
+    .stop_device = nullptr,
+    .probe = nullptr,
     .api = &esp32_usbhost_hid_keyboard_api,
     .device_type = &KEYBOARD_TYPE,
     .owner = nullptr,
@@ -543,9 +505,8 @@ static void usb_hid_keyboard_publish_key(UsbHidContext* ctx, uint32_t lv_key, bo
     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);
+    keyboard_emit_key(&ctx->kb_device, data);
 }
 
 // endregion

TactilityKernel/include/tactility/device.h

@@ -397,12 +397,16 @@ error_t device_get_first_by_compatible(const char* compatible, struct Device** o
  * Registers @a device with the hotplug poller (see device_hotplug_poll_once()): started once
  * probe() (or immediately if NULL) reports present, stopped when it reports absent. Sets
  * DEVICE_FLAG_HOTPLUG on @a device if its driver has a `probe`. A device never registered here
- * is never touched by the poller.
- * @warning Call only at boot, before polling starts.
+ * is never touched by the poller. No-op if already registered. Safe to call concurrently with
+ * device_hotplug_poll_once() and with device_hotplug_unregister().
  */
 void device_hotplug_register(struct Device* device);
 
-/** Reverses device_hotplug_register(). No-op if not registered. */
+/**
+ * Reverses device_hotplug_register(), clearing DEVICE_FLAG_HOTPLUG. No-op if not registered.
+ * Safe to call concurrently with device_hotplug_poll_once(); call this before
+ * device_remove()/device_destruct() if a registered device is being permanently torn down.
+ */
 void device_hotplug_unregister(struct Device* device);
 
 /**

TactilityKernel/source/device.cpp

@@ -571,7 +571,14 @@ struct HotplugAction {
     bool mandatory; // probe == nullptr; a start failure here is a real error
 };
 
-// Boot-time-only writes, so no lock: registration finishes before polling starts.
+// Guards hotplug_registered_devices only - never held during probe()/device_start()/device_stop().
+struct HotplugRegistryMutex {
+    Mutex handle {};
+    HotplugRegistryMutex() { mutex_construct(&handle); }
+    ~HotplugRegistryMutex() { mutex_destruct(&handle); }
+};
+
+static HotplugRegistryMutex hotplug_registry_mutex;
 static std::vector<Device*> hotplug_registered_devices;
 
 static Timer* hotplug_timer = nullptr;
@@ -581,65 +588,99 @@ static void hotplug_timer_callback(void*) {
 }
 
 void device_hotplug_register(Device* device) {
-    if (device->internal->driver != nullptr && device->internal->driver->probe != nullptr) {
-        device->flags |= DEVICE_FLAG_HOTPLUG;
+    mutex_lock(&hotplug_registry_mutex.handle);
+    if (std::ranges::find(hotplug_registered_devices, device) == hotplug_registered_devices.end()) {
+        if (device->internal->driver != nullptr && device->internal->driver->probe != nullptr) {
+            device->flags |= DEVICE_FLAG_HOTPLUG;
+        }
+        hotplug_registered_devices.push_back(device);
     }
-    hotplug_registered_devices.push_back(device);
+    mutex_unlock(&hotplug_registry_mutex.handle);
 }
 
 void device_hotplug_unregister(Device* device) {
+    mutex_lock(&hotplug_registry_mutex.handle);
     const auto iterator = std::ranges::find(hotplug_registered_devices, device);
     if (iterator != hotplug_registered_devices.end()) {
         hotplug_registered_devices.erase(iterator);
+        device->flags &= ~DEVICE_FLAG_HOTPLUG;
     }
+    mutex_unlock(&hotplug_registry_mutex.handle);
 }
 
 error_t device_hotplug_poll_once(void) {
-    // Snapshot: a device's start_device() may register another device (e.g. dynamically
-    // constructing a child once its own DEVICE_EVENT_STARTED fires), which would otherwise
-    // reallocate hotplug_registered_devices while this loop iterates it.
-    std::vector<Device*> devices = hotplug_registered_devices;
+    // Pin each device (device_get()) so a concurrent unregister+destruct can't free it mid-loop -
+    // device_destruct() refuses to run while a reference is outstanding.
+    std::vector<Device*> devices;
+    mutex_lock(&hotplug_registry_mutex.handle);
+    for (Device* device : hotplug_registered_devices) {
+        if (device_get(device) == ERROR_NONE) {
+            devices.push_back(device);
+        }
+    }
+    mutex_unlock(&hotplug_registry_mutex.handle);
 
     // Collect actions first: start_device()/stop_device() may add/remove child devices
-    // (ledger_lock), so don't call them mid-iteration.
+    // (ledger_lock), so don't call them mid-iteration. Device state is read/written under its own
+    // mutex, released before calling probe() - never run caller code under a lock.
     std::vector<HotplugAction> actions;
     for (Device* device : devices) {
         auto* internal = device->internal;
+
+        lock_internal(internal);
         auto* driver = internal->driver;
+        bool started = internal->state.started;
+        unlock_internal(internal);
+
         if (driver == nullptr) {
             continue;
         }
 
         if (driver->probe == nullptr) {
-            if (!internal->state.started) {
+            if (!started) {
                 actions.push_back({ device, true, true });
             }
             continue;
         }
 
         bool present = driver->probe(device) == ERROR_NONE;
+
+        lock_internal(internal);
+        bool act = false;
         if (present == internal->hotplug.present) {
             internal->hotplug.consecutive = 0;
-            continue;
+        } else if (++internal->hotplug.consecutive >= DEVICE_HOTPLUG_DEBOUNCE_COUNT) {
+            // hotplug.present isn't committed yet - only once the action below actually
+            // succeeds. Otherwise a failed device_start()/device_stop() would leave it matching
+            // the next stable probe(), and the poller would never retry.
+            internal->hotplug.consecutive = 0;
+            act = true;
         }
+        unlock_internal(internal);
 
-        if (++internal->hotplug.consecutive < DEVICE_HOTPLUG_DEBOUNCE_COUNT) {
-            continue;
+        if (act) {
+            actions.push_back({ device, present, false });
         }
-
-        internal->hotplug.present = present;
-        internal->hotplug.consecutive = 0;
-        actions.push_back({ device, present, false });
     }
 
     error_t first_mandatory_error = ERROR_NONE;
     for (const auto& action : actions) {
         error_t error = action.start ? device_start(action.device) : device_stop(action.device);
+        if (error == ERROR_NONE && !action.mandatory) {
+            auto* internal = action.device->internal;
+            lock_internal(internal);
+            internal->hotplug.present = action.start;
+            unlock_internal(internal);
+        }
         if (action.mandatory && action.start && error != ERROR_NONE && first_mandatory_error == ERROR_NONE) {
             first_mandatory_error = error;
         }
     }
 
+    for (Device* device : devices) {
+        device_put(device);
+    }
+
     return first_mandatory_error;
 }
 

TactilityKernel/source/drivers/keyboard.cpp

@@ -109,9 +109,11 @@ error_t keyboard_subscribe(Device* device, KeyboardEventSubscription* sub) {
     mutex_lock(&subscriptions_mutex.handle);
 
     // Avoid cyclic subscription list that would loop forever
-    if (subscriptions == sub) {
-        mutex_unlock(&subscriptions_mutex.handle);
-        return ERROR_INVALID_STATE;
+    for (auto* current = subscriptions; current != nullptr; current = current->internal.next) {
+        if (current == sub) {
+            mutex_unlock(&subscriptions_mutex.handle);
+            return ERROR_INVALID_STATE;
+        }
     }
 
     sub->internal.device = device;