From 6664a83e1de11b40d527124751b1d918bff484b0 Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Sun, 20 Sep 2026 16:59:14 +0200 Subject: [PATCH] Fixesx and improvements --- .../Source/devices/tab5_keyboard.cpp | 4 - .../Source/devices/tab5_keyboard.h | 5 -- .../devices/tab5_keyboard_lvgl_rotation.cpp | 9 ++- .../source/drivers/usb/esp32_usbhost_hid.cpp | 49 ++---------- TactilityKernel/include/tactility/device.h | 10 ++- TactilityKernel/source/device.cpp | 75 ++++++++++++++----- TactilityKernel/source/drivers/keyboard.cpp | 8 +- 7 files changed, 82 insertions(+), 78 deletions(-) diff --git a/Devices/m5stack-tab5/Source/devices/tab5_keyboard.cpp b/Devices/m5stack-tab5/Source/devices/tab5_keyboard.cpp index 1c5dcfc50..098e5eb07 100644 --- a/Devices/m5stack-tab5/Source/devices/tab5_keyboard.cpp +++ b/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) diff --git a/Devices/m5stack-tab5/Source/devices/tab5_keyboard.h b/Devices/m5stack-tab5/Source/devices/tab5_keyboard.h index c044df797..9e05faa94 100644 --- a/Devices/m5stack-tab5/Source/devices/tab5_keyboard.h +++ b/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 diff --git a/Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.cpp b/Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.cpp index 12a6f36f0..6f4f783bf 100644 --- a/Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.cpp +++ b/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); } diff --git a/Platforms/platform-esp32/source/drivers/usb/esp32_usbhost_hid.cpp b/Platforms/platform-esp32/source/drivers/usb/esp32_usbhost_hid.cpp index bf97de672..f4bd25062 100644 --- a/Platforms/platform-esp32/source/drivers/usb/esp32_usbhost_hid.cpp +++ b/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(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(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(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 diff --git a/TactilityKernel/include/tactility/device.h b/TactilityKernel/include/tactility/device.h index 8c14fd3e4..ee1713046 100644 --- a/TactilityKernel/include/tactility/device.h +++ b/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); /** diff --git a/TactilityKernel/source/device.cpp b/TactilityKernel/source/device.cpp index b5a971bc5..94e4834a0 100644 --- a/TactilityKernel/source/device.cpp +++ b/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 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 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 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 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; } diff --git a/TactilityKernel/source/drivers/keyboard.cpp b/TactilityKernel/source/drivers/keyboard.cpp index a2cd2a849..30765d392 100644 --- a/TactilityKernel/source/drivers/keyboard.cpp +++ b/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;