From 89f1804184b5f93a89ec66e6393ebe1d2c73de7c Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Sun, 20 Sep 2026 17:31:07 +0200 Subject: [PATCH] Async keyboard API and device probing support (#654) - Implemented async keyboard API. Made it backwards compatible. - `KeyboardApi::read_key` and `keyboard_read_key()` automatically forward to subscribing pollers. - Implement `Device::probe` for device hot-plug support. - Implement probing for Tab5 keyboard - Updated coding style guide --- .claude/rules/coding-style.md | 28 +-- .../Source/devices/devices_common.cpp | 12 +- .../Source/devices/devices_common.h | 4 + .../Source/devices/tab5_keyboard.cpp | 47 ++-- .../Source/devices/tab5_keyboard.h | 33 +-- .../devices/tab5_keyboard_attach_detect.cpp | 173 -------------- .../devices/tab5_keyboard_attach_detect.h | 12 - .../devices/tab5_keyboard_lvgl_rotation.cpp | 58 +++++ .../devices/tab5_keyboard_lvgl_rotation.h | 8 + Devices/m5stack-tab5/Source/module.cpp | 6 +- .../source/drivers/usb/esp32_usbhost_hid.cpp | 49 +--- TactilityKernel/include/tactility/device.h | 34 ++- TactilityKernel/include/tactility/driver.h | 9 + .../include/tactility/drivers/keyboard.h | 98 ++++++-- TactilityKernel/source/device.cpp | 135 ++++++++++- TactilityKernel/source/driver.cpp | 5 + TactilityKernel/source/drivers/keyboard.cpp | 124 +++++++++- TactilityKernel/source/kernel_init.cpp | 14 +- .../tests/source/device_hotplug_test.cpp | 129 ++++++++++ .../tests/source/keyboard_test.cpp | 225 ++++++++++++++++++ 20 files changed, 870 insertions(+), 333 deletions(-) delete mode 100644 Devices/m5stack-tab5/Source/devices/tab5_keyboard_attach_detect.cpp delete mode 100644 Devices/m5stack-tab5/Source/devices/tab5_keyboard_attach_detect.h create mode 100644 Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.cpp create mode 100644 Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.h create mode 100644 TactilityKernel/tests/source/device_hotplug_test.cpp create mode 100644 TactilityKernel/tests/source/keyboard_test.cpp diff --git a/.claude/rules/coding-style.md b/.claude/rules/coding-style.md index 86af2023b..e3d36cd62 100644 --- a/.claude/rules/coding-style.md +++ b/.claude/rules/coding-style.md @@ -2,8 +2,8 @@ Two conventions coexist; which one to use depends on the project layer: -- **C code** (TactilityKernel, drivers): `lower_snake_case` for files, functions, variables. `UpperCamelCase` for types. Files in `source/`, `include/`, `private/` directories. -- **C++ code** (Tactility, apps, services): `UpperCamelCase` for files and types. `lowerCamelCase` for functions. Files in `Source/`, `Include/`, `Private/` directories. +- **C code**: `lower_snake_case` for files, functions, variables. `UpperCamelCase` for types. Files in `source/`, `include/`, `private/` directories. +- **C++ code**: `UpperCamelCase` for files and types. `lowerCamelCase` for functions. Files in `Source/`, `Include/`, `Private/` directories. For projects that emit C headers and have a C++ implementation file: the internal C++ function naming should be snake_case. @@ -11,24 +11,10 @@ Formatting is enforced by `.clang-format` (LLVM-based, 4-space indent, no column Never throw exceptions — use return types for error handling. Use `enum class` over plain `enum` when writing C++ code. Do not add redundant null checks for parameters with an explicit non-null precondition. -Code Comments: +Code Comments (important!): -- Should be as short as possible, leaving only important context. -- Should avoid explaining what the code does, unless the code complexity is high enough to warrant an explanation. -- Must avoid explaining how the code was before, or how it was changed. -- Should explain why code is implemented. -- Should be as brief as possible without losing critical information. +- Should be minimal, but must contain critical information. +- Must not explain how code was before, or how it was changed. - Should avoid explaining what was not implemented. -- Should avoid referring to designs of other subsystems. -- Must avoid interjections: avoid hyphens or braces to interject. If interjections provide crucial info, use Doxygen entity/anchor references like: -/** - * A dedicated completion \signal for one app instance's task. - * Whichever \side finishes with it last is the one that deletes `semaphore` and frees this struct. - * - * \signal Not the task's shared default FreeRTOS notification, which app_event.cpp's AppEventSubscription also uses. - * An unrelated event delivered to the same task could otherwise unblock a waiter early. - * \side The exiting task or a concurrent app_scheduler_stop() that found the entry in time and is waiting on `semaphore`. - */ -``` - - +- Must avoid referring to designs of other subsystems. +- Must avoid interjections: avoid hyphens or braces to interject. diff --git a/Devices/m5stack-tab5/Source/devices/devices_common.cpp b/Devices/m5stack-tab5/Source/devices/devices_common.cpp index f93da7ecd..79f4c6010 100644 --- a/Devices/m5stack-tab5/Source/devices/devices_common.cpp +++ b/Devices/m5stack-tab5/Source/devices/devices_common.cpp @@ -9,7 +9,7 @@ constexpr auto* TAG = "Tab5"; // Mirrors device_construct_add_start(), but with a device_set_parent() call inserted between // construct and add - device_set_parent() asserts on device->internal (only valid after // construct), and device_construct_add_start() doesn't expose a hook to call it before add(). -bool construct_add_start(Device* device, Device* parent, const char* compatible) { +bool construct_add(Device* device, Device* parent, const char* compatible) { error_t error = device_construct(device); if (error != ERROR_NONE) { LOG_E(TAG, "display_detect: failed to construct %s: %s", device->name, error_to_string(error)); @@ -33,7 +33,15 @@ bool construct_add_start(Device* device, Device* parent, const char* compatible) return false; } - error = device_start(device); + return true; +} + +bool construct_add_start(Device* device, Device* parent, const char* compatible) { + if (!construct_add(device, parent, compatible)) { + return false; + } + + error_t error = device_start(device); if (error != ERROR_NONE) { LOG_E(TAG, "display_detect: failed to start %s: %s", device->name, error_to_string(error)); device_remove(device); diff --git a/Devices/m5stack-tab5/Source/devices/devices_common.h b/Devices/m5stack-tab5/Source/devices/devices_common.h index 8cb73468e..ef542a2cd 100644 --- a/Devices/m5stack-tab5/Source/devices/devices_common.h +++ b/Devices/m5stack-tab5/Source/devices/devices_common.h @@ -10,6 +10,10 @@ // device_construct_add_start(), but with a device_set_parent() call inserted between construct and add bool construct_add_start(Device* device, Device* parent, const char* compatible); +// Same as construct_add_start(), but doesn't start the device - for a device registered with +// device_hotplug_register() instead, whose driver has a Driver::probe deciding if/when it starts. +bool construct_add(Device* device, Device* parent, const char* compatible); + // Unpacks a vendor-typed init-cmd array (ili9881c_lcd_init_cmd_t / st7123_lcd_init_cmd_t - same // field layout) into the flattened [cmd, data_len, delay_ms, data_len bytes...] byte encoding that // ili9881c-module/st7123-module's init-sequence config field expects (the same encoding a diff --git a/Devices/m5stack-tab5/Source/devices/tab5_keyboard.cpp b/Devices/m5stack-tab5/Source/devices/tab5_keyboard.cpp index 09aee53b5..098e5eb07 100644 --- a/Devices/m5stack-tab5/Source/devices/tab5_keyboard.cpp +++ b/Devices/m5stack-tab5/Source/devices/tab5_keyboard.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -262,9 +263,9 @@ static bool write_reg_fast(Device* device, uint8_t reg, uint8_t value) { return i2c_controller_write_register(parent, I2C_ADDRESS, reg, &value, 1, pdMS_TO_TICKS(2)) == ERROR_NONE; } -bool tab5_keyboard_is_attached(Device* device) { +static error_t probe(Device* device) { auto* parent = device_get_parent(device); - return i2c_controller_has_device_at_address(parent, I2C_ADDRESS, pdMS_TO_TICKS(5)) == ERROR_NONE; + return i2c_controller_has_device_at_address(parent, I2C_ADDRESS, pdMS_TO_TICKS(5)); } // --------------------------------------------------------------------------- @@ -462,7 +463,7 @@ static void drain_events(Device* device, Tab5KeyboardInternal* internal) { // mode and interrupt configuration are volatile and reset to power-on defaults when the keyboard // is unplugged and reconnected. // --------------------------------------------------------------------------- -void tab5_keyboard_reinit(Device* device) { +static void tab5_keyboard_reinit(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); write_reg_fast(device, REG_KEYBOARD_MODE, 0x00); // Normal mode write_reg_fast(device, REG_EVENT_NUM, 0x00); // flush event queue @@ -476,7 +477,7 @@ void tab5_keyboard_reinit(Device* device) { } } -void tab5_keyboard_reset_state(Device* device) { +static void tab5_keyboard_reset_state(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); for (uint8_t idx = 0; idx < 70U; idx++) { @@ -594,28 +595,26 @@ static error_t start(Device* device) { // device_get_driver_data(). device_set_driver_data(device, internal); - // This device is constructed speculatively at boot so it can be hot-plug-detected later - if - // the keyboard isn't physically attached yet, skip reinit here (tab5_keyboard_attach_detect.cpp - // calls it again once attach is confirmed) rather than issuing register writes that are certain - // to fail: unlike tab5_keyboard_is_attached()'s plain probe, write_register() logs at error - // level on failure (see esp32_i2c_master.cpp), which would be misleading noise for what's just - // "not plugged in yet". - if (tab5_keyboard_is_attached(device)) { - tab5_keyboard_reinit(device); - } + // Driver::probe (tab5_keyboard_is_attached) already confirmed presence before driver_bind() + // called this. + tab5_keyboard_reinit(device); return ERROR_NONE; } +// How long to give LVGL's own indev timer to drain the release events tab5_keyboard_reset_state() +// pushes below before the queue carrying them is deleted - longer than LVGL's default indev poll +// period so a key held across an unplug doesn't read as stuck forever. +static constexpr uint32_t STOP_DRAIN_GRACE_MS = 50; + static error_t stop(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); remove_irq_pin(internal); write_reg(device, REG_INT_CFG, 0x00); // disable all interrupts - internal->sym_active = false; - internal->aa_sticky = false; - internal->aa_held = false; - update_leds(device, internal); // turn LEDs off + + tab5_keyboard_reset_state(device); + vTaskDelay(pdMS_TO_TICKS(STOP_DRAIN_GRACE_MS)); vQueueDelete(internal->queue); free(internal); @@ -659,7 +658,6 @@ static error_t tab5_keyboard_read_key(Device* device, KeyboardKeyData* data) { static const KeyboardApi tab5_keyboard_api = { .read_key = tab5_keyboard_read_key, .get_backlight = nullptr, - .is_present = tab5_keyboard_is_attached, }; // Defined in module.cpp - this driver is registered directly by m5stack-tab5's own module, @@ -671,6 +669,7 @@ Driver tab5_keyboard_driver = { .compatible = (const char*[]) { "m5stack,tab5-keyboard", nullptr }, .start_device = start, .stop_device = stop, + .probe = probe, .api = &tab5_keyboard_api, .device_type = &KEYBOARD_TYPE, .owner = &m5stack_tab5_module, @@ -684,10 +683,10 @@ static Device tab5_keyboard_device {}; // The keyboard accessory is a kernel driver device (m5stack,tab5-keyboard, defined directly in // this project). Unlike the display/touch, it isn't gated on the display-variant detection at -// all (it lives on i2c2, a separate bus) - lvgl-module binds its indev unconditionally at boot -// regardless of physical attach state. Hot-plug attach/detach handling (register reinit, LVGL -// rotation) lives in tab5_keyboard_attach_detect.cpp, not this driver - see module.cpp for where -// that gets started. +// all (it lives on i2c2, a separate bus). Registered with the kernel's hotplug poller instead of +// started here - Driver::probe (tab5_keyboard_is_attached) decides when it starts/stops. LVGL +// rotation-on-attach lives in tab5_keyboard_attach_detect.cpp, not this driver - see module.cpp +// for where that gets started. void tab5_create_keyboard(Device* i2c2) { tab5_keyboard_device = Device { .address = 0, @@ -715,7 +714,9 @@ void tab5_create_keyboard(Device* i2c2) { // Parented to i2c2 itself (not root, unlike the display): the keyboard driver's start() uses // device_get_parent() as its I2C bus controller. - construct_add_start(&tab5_keyboard_device, i2c2, "m5stack,tab5-keyboard"); + if (construct_add(&tab5_keyboard_device, i2c2, "m5stack,tab5-keyboard")) { + device_hotplug_register(&tab5_keyboard_device); + } } // endregion diff --git a/Devices/m5stack-tab5/Source/devices/tab5_keyboard.h b/Devices/m5stack-tab5/Source/devices/tab5_keyboard.h index 77fae5df1..9e05faa94 100644 --- a/Devices/m5stack-tab5/Source/devices/tab5_keyboard.h +++ b/Devices/m5stack-tab5/Source/devices/tab5_keyboard.h @@ -11,38 +11,23 @@ extern "C" { #include struct Tab5KeyboardConfig { - // Fixed 0x6D on this keyboard accessory - kept as a field for parity/documentation with - // other dynamically-constructed devices in this project, not used to probe the bus. + /** Fixed 0x6D - not used to probe the bus, kept only for parity with other dynamic devices. */ uint8_t address; - // Native SoC GPIO (e.g. gpio0 pin 50) wired directly to the keyboard's INT line - not routed - // through an IO expander, since it needs a real hardware ISR for responsive key events. - // GPIO_PIN_SPEC_NONE falls back to polling REG_INT_STAT instead. + /** + * Wired directly to a native SoC GPIO (not an IO expander) for a real ISR. + * GPIO_PIN_SPEC_NONE falls back to polling REG_INT_STAT. + */ struct GpioPinSpec pin_interrupt; }; extern struct Driver tab5_keyboard_driver; -// Constructs and starts the keyboard accessory device on i2c2. Called from display_detect.cpp's -// on_display_detect_event() once i2c2 is up. +/** + * @brief Constructs (but doesn't start) the keyboard device on i2c2 and registers it with the + * kernel's hotplug poller. Called once i2c2 is up (see display_detect.cpp). + */ void tab5_create_keyboard(struct Device* i2c2); -// Returns true if the keyboard accessory currently ACKs on the I2C bus. Cheap bus probe, no -// debouncing - callers wanting hot-plug-stable state (e.g. tab5_keyboard_attach_detect.cpp) -// should debounce across their own polling interval. -bool tab5_keyboard_is_attached(struct Device* device); - -// (Re)applies the device's register configuration - RGB mode, brightness, interrupt config, LED -// state. Volatile on this chip: reset to power-on defaults whenever the keyboard is unplugged and -// reconnected, so callers must call this again after confirming a reattach (see -// tab5_keyboard_attach_detect.cpp). -void tab5_keyboard_reinit(struct Device* device); - -// Emits a release for every currently-held key (a hardware release can't arrive once the keyboard -// is unplugged), then clears held-key/software-repeat/modifier state. Callers must call this on -// confirmed detach (see tab5_keyboard_attach_detect.cpp) so a key held across an unplug doesn't -// leave consumers with a stuck key or spurious repeats/modifiers after reattach. -void tab5_keyboard_reset_state(struct Device* device); - #ifdef __cplusplus } #endif diff --git a/Devices/m5stack-tab5/Source/devices/tab5_keyboard_attach_detect.cpp b/Devices/m5stack-tab5/Source/devices/tab5_keyboard_attach_detect.cpp deleted file mode 100644 index 7c8555b67..000000000 --- a/Devices/m5stack-tab5/Source/devices/tab5_keyboard_attach_detect.cpp +++ /dev/null @@ -1,173 +0,0 @@ -#include "tab5_keyboard_attach_detect.h" - -#include "tab5_keyboard.h" - -#include -#include - -#include -#include - -#include -#include - -constexpr auto* TAG = "Tab5"; - -// Hot-plug attach-state check interval. I2C probes can false-positive on a floating/half-connected -// bus (e.g. mid-unplug), so a state change is only acted on once it's seen on two consecutive -// checks in a row. -constexpr auto ATTACH_CHECK_INTERVAL_MS = 1000; - -static TimerHandle_t attach_detect_timer = nullptr; - -static bool was_attached = false; -static bool pending_attach_state = false; -static uint8_t pending_attach_confirm_count = 0; - -// Tracks LVGL's own readiness so a restart (lvgl_is_running() going from false back to true - -// e.g. an app that took over the display for direct rendering, stopping and letting LVGL rebind) -// can be told apart from the keyboard itself attaching/detaching. See apply_state()'s comment for -// why that distinction matters. -static bool was_lvgl_ready = false; - -static lv_display_rotation_t saved_rotation = LV_DISPLAY_ROTATION_0; -static bool rotation_override_active = false; - -// Applies the current attach state to LVGL (landscape rotation while attached, restoring the -// prior rotation on detach unless the user changed it manually since attaching) and to the -// keyboard device's own register state (reinit on attach - RGB mode/interrupt config are volatile -// across an unplug/replug on this chip). Ported as-is from the deprecated HAL's -// Tab5Keyboard::applyAutoRotation() / the pre-refactor tab5_keyboard.cpp driver logic. -// @return true once handled; false to be retried on the next tick (e.g. LVGL lock busy). -static bool apply_state(Device* keyboard_device, bool attached) { - if (!lvgl_try_lock(pdMS_TO_TICKS(100))) { - return false; // retry next tick - } - - // Resolved inside the lock, not before: the default display can start/stop between an - // unlocked probe and actually taking the lock, and lv_display_get_default() itself isn't - // safe to call without holding it (unlike lvgl_is_running(), used for the readiness check in - // attach_detect_callback()). - auto* display = lv_display_get_default(); - if (display == nullptr) { - lvgl_unlock(); - return false; // LVGL not ready yet - retry next tick - } - - if (attached) { - tab5_keyboard_reinit(keyboard_device); - - if (lv_display_get_rotation(display) != LV_DISPLAY_ROTATION_90) { - saved_rotation = lv_display_get_rotation(display); - rotation_override_active = true; - lv_display_set_rotation(display, LV_DISPLAY_ROTATION_90); - } - } else { - // A key held at the moment of unplug can never get its hardware release - reset tracked - // state so it doesn't read as stuck or feed stale modifiers into whatever's pressed next - // after reattach. - tab5_keyboard_reset_state(keyboard_device); - - // Only restore if rotation is still what we set it to - if the user manually changed it - // since attaching, respect their choice instead. - if (rotation_override_active && lv_display_get_rotation(display) == LV_DISPLAY_ROTATION_90) { - lv_display_set_rotation(display, saved_rotation); - } - rotation_override_active = false; - } - - lvgl_unlock(); - return true; -} - -static void attach_detect_callback(TimerHandle_t /*timer*/) { - Device* keyboard_device = nullptr; - if (device_get_by_name("keyboard0", &keyboard_device) != ERROR_NONE) { - return; // Not constructed yet - will retry on next tick - } - - // LVGL restarting is a distinct event from the keyboard physically attaching/detaching: the - // accessory may never have moved, but whatever apply_state() last set (rotation) may have - // been reset in the meantime by the restart. Forcing was_attached false makes the block below - // see a fresh "attached" transition, still going through the normal two-check debounce, so - // apply_state() re-announces the current state instead of staying silent forever, waiting for - // an edge that will never come because the keyboard was never actually unplugged. - // lvgl_is_running() is safe to call unlocked (unlike lv_display_get_default(), resolved inside - // the lock in apply_state() instead). - const bool lvgl_ready = lvgl_is_running(); - if (lvgl_ready && !was_lvgl_ready) { - // If the keyboard is (and, per was_attached, already was) physically detached, forcing - // was_attached false below means the detach transition below will never fire again for - // this unplug - it already happened before this restart. Reset software state here - // instead, since apply_state()'s own detach path may never have run: it could have bailed - // out early (LVGL lock busy, or display not ready yet) before reaching its - // tab5_keyboard_reset_state() call, and now never will, because that transition is about - // to be erased. This reset doesn't touch LVGL/display state, so it doesn't need - // apply_state()'s lock/display gating. - if (was_attached && !tab5_keyboard_is_attached(keyboard_device)) { - tab5_keyboard_reset_state(keyboard_device); - } - was_attached = false; - pending_attach_confirm_count = 0; - } - was_lvgl_ready = lvgl_ready; - - const bool attached = tab5_keyboard_is_attached(keyboard_device); - if (attached != was_attached) { - // Require the new state to be confirmed on a second consecutive check before acting: a - // single probe on a floating/half-connected bus (e.g. mid-unplug) can false-positive. - if (attached != pending_attach_state || pending_attach_confirm_count == 0) { - pending_attach_state = attached; - pending_attach_confirm_count = 1; - } else { - pending_attach_confirm_count = 0; - if (apply_state(keyboard_device, attached)) { - was_attached = attached; - } - // else: not handled yet (e.g. LVGL lock busy); retry on the next confirmed check - } - } else { - pending_attach_confirm_count = 0; - } - - device_put(keyboard_device); -} - -void tab5_keyboard_attach_detect_start() { - if (attach_detect_timer != nullptr) { - LOG_W(TAG, "keyboard attach-detect timer already running"); - return; - } - - was_attached = false; - pending_attach_confirm_count = 0; - was_lvgl_ready = false; - rotation_override_active = false; - - attach_detect_timer = xTimerCreate("kb_attach_detect", pdMS_TO_TICKS(ATTACH_CHECK_INTERVAL_MS), pdTRUE, nullptr, attach_detect_callback); - if (!attach_detect_timer) { - LOG_E(TAG, "Failed to create keyboard attach-detect timer"); - return; - } - if (xTimerStart(attach_detect_timer, pdMS_TO_TICKS(100)) != pdPASS) { - LOG_E(TAG, "Failed to start keyboard attach-detect timer"); - xTimerDelete(attach_detect_timer, pdMS_TO_TICKS(100)); - attach_detect_timer = nullptr; - } -} - -void tab5_keyboard_attach_detect_stop() { - if (attach_detect_timer == nullptr) { - return; - } - - if (xTimerStop(attach_detect_timer, pdMS_TO_TICKS(100)) != pdPASS) { - LOG_W(TAG, "Failed to stop keyboard attach-detect timer"); - } - if (xTimerDelete(attach_detect_timer, pdMS_TO_TICKS(100)) != pdPASS) { - LOG_E(TAG, "Failed to delete keyboard attach-detect timer"); - } - // Always clear the handle - stale non-null handle is worse than a resource leak, as it would - // cause tab5_keyboard_attach_detect_start() to silently skip re-creating the timer. - attach_detect_timer = nullptr; -} diff --git a/Devices/m5stack-tab5/Source/devices/tab5_keyboard_attach_detect.h b/Devices/m5stack-tab5/Source/devices/tab5_keyboard_attach_detect.h deleted file mode 100644 index 860972c53..000000000 --- a/Devices/m5stack-tab5/Source/devices/tab5_keyboard_attach_detect.h +++ /dev/null @@ -1,12 +0,0 @@ -#pragma once - -// Starts/stops the periodic keyboard-accessory hot-plug poll: reapplies register configuration on -// reattach (see tab5_keyboard_reinit()) and switches LVGL to landscape while attached, restoring -// the prior rotation on detach - unless the user changed it manually since attaching, in which -// case their choice is respected. Also re-announces the current attach state after LVGL itself -// restarts (e.g. an app that took over the display for direct rendering, stopping and letting -// LVGL rebind), since that's a distinct event from the keyboard physically attaching/detaching - -// the accessory may never have moved, but the rotation override it applied may have been reset in -// the meantime. Called from module.cpp's start()/stop(). -void tab5_keyboard_attach_detect_start(); -void tab5_keyboard_attach_detect_stop(); diff --git a/Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.cpp b/Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.cpp new file mode 100644 index 000000000..6f4f783bf --- /dev/null +++ b/Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.cpp @@ -0,0 +1,58 @@ +#include "tab5_keyboard_lvgl_rotation.h" + +#include +#include + +#include +#include + +static lv_display_rotation_t saved_rotation = LV_DISPLAY_ROTATION_0; +static bool rotation_override_active = false; + +static void apply_rotation(bool attached) { + lvgl_lock(); + + auto* display = lv_display_get_default(); + if (display == nullptr) { + lvgl_unlock(); + return; + } + + if (attached) { + if (lv_display_get_rotation(display) != LV_DISPLAY_ROTATION_90) { + saved_rotation = lv_display_get_rotation(display); + rotation_override_active = true; + lv_display_set_rotation(display, LV_DISPLAY_ROTATION_90); + } + } 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; + } + + lvgl_unlock(); +} + +static void on_device_event(Device* device, DeviceEvent event, void*) { + if (!device_is_compatible(device, "m5stack,tab5-keyboard")) { + return; + } + if (event == DEVICE_EVENT_STARTED) { + apply_rotation(true); + } else if (event == DEVICE_EVENT_STOPPED) { + apply_rotation(false); + } +} + +void tab5_keyboard_lvgl_rotation_start() { + rotation_override_active = false; + device_listener_add(on_device_event, nullptr); +} + +void tab5_keyboard_lvgl_rotation_stop() { + apply_rotation(false); + device_listener_remove(on_device_event); +} diff --git a/Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.h b/Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.h new file mode 100644 index 000000000..98f0cd424 --- /dev/null +++ b/Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.h @@ -0,0 +1,8 @@ +#pragma once + +// Applies landscape rotation while the keyboard accessory ("keyboard0") is started, restoring the +// prior rotation on stop - unless the user changed it manually since attaching, in which case +// their choice is respected. Start/stop are now driven by the kernel's hotplug poller (see +// tab5_keyboard.cpp's Driver::probe), not polled here. Called from module.cpp's start()/stop(). +void tab5_keyboard_lvgl_rotation_start(); +void tab5_keyboard_lvgl_rotation_stop(); diff --git a/Devices/m5stack-tab5/Source/module.cpp b/Devices/m5stack-tab5/Source/module.cpp index b757e8465..912d0af95 100644 --- a/Devices/m5stack-tab5/Source/module.cpp +++ b/Devices/m5stack-tab5/Source/module.cpp @@ -13,7 +13,7 @@ #include "devices/detect.h" #include "devices/tab5_headphone_detect.h" #include "devices/tab5_keyboard.h" -#include "devices/tab5_keyboard_attach_detect.h" +#include "devices/tab5_keyboard_lvgl_rotation.h" #include "devices/tab5_power_control.h" #include "devices/tab_5_camera.h" @@ -70,12 +70,12 @@ static error_t start() { tab5_camera_init(); device_listener_add(on_io_expander0_started, nullptr); tab5_headphone_detect_start(); - tab5_keyboard_attach_detect_start(); + tab5_keyboard_lvgl_rotation_start(); return ERROR_NONE; } static error_t stop() { - tab5_keyboard_attach_detect_stop(); + tab5_keyboard_lvgl_rotation_stop(); tab5_headphone_detect_stop(); device_listener_remove(on_io_expander0_started); tab5_detect_stop(); 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 d919d9f23..ee1713046 100644 --- a/TactilityKernel/include/tactility/device.h +++ b/TactilityKernel/include/tactility/device.h @@ -35,7 +35,7 @@ typedef uint8_t device_flags_t; #define DEVICE_FLAG_VIRTUAL BIT(2) /* No physical hardware */ #define DEVICE_FLAG_REMOVABLE BIT(3) /* May disappear (USB, SDIO, etc.) */ -#define DEVICE_FLAG_HOTPLUG BIT(4) /* Supports hotplug */ +#define DEVICE_FLAG_HOTPLUG BIT(4) /* Automatically set by device_hotplug_register() when the driver has a probe */ /** Represents a piece of hardware */ struct Device { @@ -113,6 +113,7 @@ error_t device_remove(struct Device* device); * device's lock for its entire body, so doing so would deadlock. * @param[in,out] device non-null device pointer * @retval ERROR_INVALID_STATE if the device is already started or not added + * @retval ERROR_NOT_FOUND probe() reported the device isn't present * @retval ERROR_RESOURCE when driver binding fails * @retval ERROR_NONE on success */ @@ -389,6 +390,37 @@ bool device_has_active_by_type(const struct DeviceType* type); */ error_t device_get_first_by_compatible(const char* compatible, struct Device** out_device); +/** Consecutive device_hotplug_poll_once() calls a probe() result must hold before it takes effect. */ +#define DEVICE_HOTPLUG_DEBOUNCE_COUNT 2 + +/** + * 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. 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(), 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); + +/** + * Starts/stops every device_hotplug_register()'d device per its driver's probe() (or + * unconditionally, once, if probe is NULL), debounced across DEVICE_HOTPLUG_DEBOUNCE_COUNT + * calls. Never removes/destructs a device. + * @retval ERROR_NONE on success + * @retval (other) a mandatory (probe == NULL) device failed to start + */ +error_t device_hotplug_poll_once(void); + +/** Starts a periodic timer that calls device_hotplug_poll_once(). */ +void device_hotplug_start_polling(void); + #ifdef __cplusplus } #endif diff --git a/TactilityKernel/include/tactility/driver.h b/TactilityKernel/include/tactility/driver.h index 7ae115a3a..67858330a 100644 --- a/TactilityKernel/include/tactility/driver.h +++ b/TactilityKernel/include/tactility/driver.h @@ -23,6 +23,14 @@ struct Driver { error_t (*start_device)(struct Device* dev); /** Function to deinitialize the driver for a device */ error_t (*stop_device)(struct Device* dev); + /** + * Optional: reports whether the hardware is physically present. Checked by driver_bind() and periodically by device_hotplug_poll_once(). + * NULL means always present. + * @retval ERROR_NONE the hardware is present + * @retval (other) not present (or the probe itself failed - treated the same) + * @warning Must not call device_add()/device_remove() - may run while the ledger is locked. + */ + error_t (*probe)(struct Device* device); /** Contains the driver's functions */ const void* api; /** Which type of devices this driver creates (can be NULL) */ @@ -100,6 +108,7 @@ error_t driver_remove_destruct(struct Driver* driver); * * @param driver The driver to bind. * @param device The device to bind to. + * @retval ERROR_NOT_FOUND probe() reported the device isn't present * @return ERROR_NONE if successful, or an error code otherwise. */ error_t driver_bind(struct Driver* driver, struct Device* device); diff --git a/TactilityKernel/include/tactility/drivers/keyboard.h b/TactilityKernel/include/tactility/drivers/keyboard.h index 067596ea3..87dcf7998 100644 --- a/TactilityKernel/include/tactility/drivers/keyboard.h +++ b/TactilityKernel/include/tactility/drivers/keyboard.h @@ -14,12 +14,9 @@ extern "C" { /** * @brief Named Unicode codepoints for KeyboardKeyData::key. * - * Keys that produce an ordinary character use that character's own codepoint directly (e.g. 'a', - * ' ') and don't need a name here. These are the standard Unicode symbol codepoints used instead - * of LVGL's LV_KEY_* sentinels for keys with no character of their own (arrows, Home/End, - * Tab/Shift+Tab-as-focus-navigation), plus names for the handful of C0 control keys every driver - * needs (Enter/Escape/Backspace/Delete/Tab). Modules/lvgl-module/source/devices/keyboard.cpp - * translates these back into LV_KEY_* internally for LVGL. + * Some common codepoints. + * There are no official codepoints for arrow keys, so we use the symbols as placeholder. + * Subsystems like LVGL can translate these special codepoints to specific actions. */ typedef enum { CODEPOINT_ENTER = '\r', @@ -77,11 +74,11 @@ struct KeyboardKeyData { bool alt; /** * @brief Standard USB HID keyboard usage code for this key (USB HID Usage Tables, page 0x07), - * or 0 if this driver doesn't compute one (most don't - `key` is the only field most consumers - * need). Populated by drivers whose hardware layout maps cleanly onto HID usage codes, so - * consumers that want to mirror physical key presses as real HID reports (e.g. USB HID output) - * don't have to reverse-engineer one out of `key`'s codepoint encoding - which is lossy for - * keys with no Unicode/LVGL representation at all, e.g. F1-F12. + * or 0 if this driver doesn't compute one (most don't - `key` is the only field most consumers need). + * Populated by drivers whose hardware layout maps cleanly onto HID usage codes, so consumers that + * want to mirror physical key presses as real HID reports (e.g. USB HID output) don't have to + * reverse-engineer one out of `key`'s codepoint encoding. + * Encoding is lossy for keys with no Unicode/LVGL representation at all, e.g. F1-F12. */ uint8_t hid_keycode; /** @@ -91,12 +88,35 @@ struct KeyboardKeyData { uint8_t hid_modifier; }; +/** + * @brief Number of key events that can be queued per subscription in the default + * subscribe/unsubscribe/poll implementation before older subscribers' fanned-out events start + * being dropped for that subscriber (silently, preserving FIFO order of what's already queued). + */ +#define KEYBOARD_EVENT_QUEUE_CAPACITY 4 + +/** + * @brief Caller-owned subscription node for keyboard_subscribe()/keyboard_unsubscribe()/ + * keyboard_poll(). + * @warning `internal` is private to keyboard.cpp; do not read or write it directly. + */ +struct KeyboardEventSubscription { + struct { + struct Device* device; + struct KeyboardKeyData queue[KEYBOARD_EVENT_QUEUE_CAPACITY]; + uint8_t head; + uint8_t count; + struct KeyboardEventSubscription* next; + } internal; +}; + /** * @brief API for keyboard drivers. */ struct KeyboardApi { /** * @brief Reads the next pending key event, if any. + * May be NULL if the driver instead pushes events itself via keyboard_emit_key() (e.g. from a dedicated interrupt-handling task). * @param[in] device the keyboard device * @param[out] data the key event data * @retval ERROR_NONE when the operation was successful @@ -104,7 +124,7 @@ struct KeyboardApi { error_t (*read_key)(struct Device* device, struct KeyboardKeyData* data); /** - * @brief Returns the baclight if the keyboard has one. + * @brief Returns the backlight if the keyboard has one. * @warning Returns a referenced device. Must call device_put() afterwards. * @param[in] device the keyboard device * @param[out] backlight_device the output backlight device @@ -114,22 +134,34 @@ struct KeyboardApi { error_t (*get_backlight)(struct Device* device, struct Device** backlight_device); /** - * @brief Optional: reports whether the keyboard is physically present right now. Only - * meaningful for hot-pluggable/detachable keyboards (e.g. a removable accessory) whose - * kernel device is constructed and started once at boot regardless of physical attachment - - * leave NULL for a keyboard that's always physically present whenever its device is active - * (the common case; callers must treat NULL the same as "always present"). - * @param[in] device the keyboard device - * @return true if physically attached/present + * @deprecated Use Driver::probe + device_hotplug_register() instead (see DEVICE_FLAG_HOTPLUG) + * The kernel then starts/stops the device itself instead of leaving it started forever. + * When DEVICE_FLAG_HOTPLUG is used, use device_is_ready() only for checking if a keyboard is usable. */ bool (*is_present)(struct Device* device); }; /** - * @brief Reads the next pending key event using the specified keyboard device. + * @brief Reads the next pending key event using the specified keyboard device. Also fans the + * result out to every keyboard_subscribe()'d subscriber, same as keyboard_emit_key(). + * @retval ERROR_NONE @a data filled - key == 0 if nothing is pending + * @retval ERROR_INVALID_STATE @a device isn't currently started (e.g. hotplug-absent) + * @retval ERROR_NOT_SUPPORTED the driver has no KeyboardApi::read_key (it pushes events via + * keyboard_emit_key() instead) */ error_t keyboard_read_key(struct Device* device, struct KeyboardKeyData* data); +/** + * @brief Push a key event from @a device to every current subscriber (see keyboard_subscribe()). + * For a driver whose hardware delivers key events asynchronously (e.g. from a dedicated + * interrupt-handling task) rather than on demand - KeyboardApi::read_key may be left NULL when + * the driver calls this instead. + * @warning Not ISR-safe (takes a mutex); call from a task, not directly from interrupt context. + * @param[in] device the keyboard device the event originated from + * @param[in] data the key event + */ +void keyboard_emit_key(struct Device* device, struct KeyboardKeyData data); + /** * @brief Returns the backlight if the keyboard has one. * @warning Returns a referenced device. Must call device_put() afterwards. @@ -141,13 +173,33 @@ error_t keyboard_read_key(struct Device* device, struct KeyboardKeyData* data); error_t keyboard_get_backlight(struct Device* device, struct Device** backlight_device); /** - * @brief Whether the keyboard device is physically present right now. True when the driver - * doesn't implement KeyboardApi::is_present (i.e. it's always physically present whenever its - * device is active) - see that field's doc comment. + * @deprecated See KeyboardApi::is_present. * @param[in] device the keyboard device */ bool keyboard_is_present(struct Device* device); +/** + * @brief Register @a sub for async key events from @a device, derived from `read_key`. + * @param[in,out] sub subscription to register; owns the storage, must stay alive (and stationary) + * until unsubscribed + * @retval ERROR_NONE on success + */ +error_t keyboard_subscribe(struct Device* device, struct KeyboardEventSubscription* sub); + +/** + * @brief Remove a subscription previously registered with keyboard_subscribe(). + * @retval ERROR_NONE on success + */ +error_t keyboard_unsubscribe(struct Device* device, struct KeyboardEventSubscription* sub); + +/** + * @brief Non-blocking: pop the next event for @a sub if one is already queued. + * @warning Never blocks. + * @retval ERROR_NONE @a out_data was filled + * @retval ERROR_TIMEOUT nothing queued right now + */ +error_t keyboard_poll(struct Device* device, struct KeyboardEventSubscription* sub, struct KeyboardKeyData* out_data); + extern const struct DeviceType KEYBOARD_TYPE; #ifdef __cplusplus diff --git a/TactilityKernel/source/device.cpp b/TactilityKernel/source/device.cpp index b852a68e6..94e4834a0 100644 --- a/TactilityKernel/source/device.cpp +++ b/TactilityKernel/source/device.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -43,6 +44,11 @@ struct DeviceInternal { } state; /** Attached child devices */ std::vector children {}; + // device_hotplug_poll_once() debounce state. + struct { + uint8_t consecutive = 0; // probe() results differing from `present` so far + bool present = false; + } hotplug; // Outstanding device_get() holders. Guarded by `mutex`. Independent of state.started - // device_get()/device_put() bracket construct/destruct, not start/stop, so a ref can be held // across a device_stop(). device_destruct() refuses to run while this is > 0. @@ -225,7 +231,7 @@ error_t device_start(Device* device) { unlock_internal(internal); if (bind_error != ERROR_NONE) { - return ERROR_RESOURCE; + return bind_error == ERROR_NOT_FOUND ? ERROR_NOT_FOUND : ERROR_RESOURCE; } device_listener_notify(device, DEVICE_EVENT_STARTED); @@ -559,4 +565,131 @@ error_t device_get_first_by_compatible(const char* compatible, Device** out_devi return error; } +struct HotplugAction { + Device* device; + bool start; // vs. stop + bool mandatory; // probe == nullptr; a start failure here is a real error +}; + +// 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; + +static void hotplug_timer_callback(void*) { + device_hotplug_poll_once(); +} + +void device_hotplug_register(Device* device) { + 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); + } + 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) { + // 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. 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 (!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; + } 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 (act) { + 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; +} + +void device_hotplug_start_polling(void) { + if (hotplug_timer != nullptr) { + return; + } + hotplug_timer = timer_alloc(TIMER_TYPE_PERIODIC, pdMS_TO_TICKS(500), hotplug_timer_callback, nullptr); + timer_start(hotplug_timer); +} + } // extern "C" diff --git a/TactilityKernel/source/driver.cpp b/TactilityKernel/source/driver.cpp index 9a05d6ec5..9df0cb4fa 100644 --- a/TactilityKernel/source/driver.cpp +++ b/TactilityKernel/source/driver.cpp @@ -129,6 +129,11 @@ error_t driver_bind(Driver* driver, Device* device) { goto error; } + if (driver->probe != nullptr && driver->probe(device) != ERROR_NONE) { + error = ERROR_NOT_FOUND; + goto error; + } + if (driver->start_device != nullptr) { error = driver->start_device(device); if (error != ERROR_NONE) { diff --git a/TactilityKernel/source/drivers/keyboard.cpp b/TactilityKernel/source/drivers/keyboard.cpp index 9938b6a96..30765d392 100644 --- a/TactilityKernel/source/drivers/keyboard.cpp +++ b/TactilityKernel/source/drivers/keyboard.cpp @@ -3,21 +3,86 @@ #include #include +#include + #define KEYBOARD_DRIVER_API(driver) ((struct KeyboardApi*)driver->api) +// Subscriptions, filtered by `device` on fan-out. +static KeyboardEventSubscription* subscriptions = nullptr; + +struct KeyboardEventMutex { + Mutex handle {}; + KeyboardEventMutex() { mutex_construct(&handle); } + ~KeyboardEventMutex() { mutex_destruct(&handle); } +}; + +// One coarse mutex is fine: fan-out is just a struct copy, never caller code. +static KeyboardEventMutex subscriptions_mutex; + +// Caller must hold subscriptions_mutex. +static void fan_out_to_subscribers(Device* device, const KeyboardKeyData& data) { + for (KeyboardEventSubscription* sub = subscriptions; sub != nullptr; sub = sub->internal.next) { + if (sub->internal.device != device) { + continue; + } + if (sub->internal.count < KEYBOARD_EVENT_QUEUE_CAPACITY) { + uint8_t tail = (sub->internal.head + sub->internal.count) % KEYBOARD_EVENT_QUEUE_CAPACITY; + sub->internal.queue[tail] = data; + sub->internal.count++; + } + } +} + +static bool try_pop(KeyboardEventSubscription* sub, KeyboardKeyData* out_data) { + mutex_lock(&subscriptions_mutex.handle); + bool has_event = sub->internal.count > 0; + if (has_event) { + *out_data = sub->internal.queue[sub->internal.head]; + sub->internal.head = (sub->internal.head + 1) % KEYBOARD_EVENT_QUEUE_CAPACITY; + sub->internal.count--; + } + mutex_unlock(&subscriptions_mutex.handle); + return has_event; +} + extern "C" { error_t keyboard_read_key(Device* device, KeyboardKeyData* data) { + // Under the hotplug poller a keyboard device can be constructed but not started (probe + // hasn't confirmed presence, or it's since detached) - a sync caller like LVGL's indev still + // polls it every tick regardless, so bail out rather than touching the driver's (possibly + // unallocated) state. + if (!device_is_ready(device)) { + *data = {}; + return ERROR_INVALID_STATE; + } + const auto* driver = device_get_driver(device); - // Default the modifier/HID fields here rather than in each driver: only drivers whose hardware - // can report them set them, and the rest would otherwise leave whatever the caller's stack held. + if (KEYBOARD_DRIVER_API(driver)->read_key == nullptr) { + return ERROR_NOT_SUPPORTED; + } + + // Defaulted here, not per-driver: only drivers whose hardware reports them set them. data->ctrl = false; data->alt = false; data->hid_keycode = 0; data->hid_modifier = 0; - return KEYBOARD_DRIVER_API(driver)->read_key(device, data); + error_t result = KEYBOARD_DRIVER_API(driver)->read_key(device, data); + if (result == ERROR_NONE && data->key != 0) { + // A sync caller (e.g. LVGL) isn't the only consumer of this key. + mutex_lock(&subscriptions_mutex.handle); + fan_out_to_subscribers(device, *data); + mutex_unlock(&subscriptions_mutex.handle); + } + return result; +} + +void keyboard_emit_key(Device* device, KeyboardKeyData data) { + mutex_lock(&subscriptions_mutex.handle); + fan_out_to_subscribers(device, data); + mutex_unlock(&subscriptions_mutex.handle); } error_t keyboard_get_backlight(Device* device, Device** backlight_device) { @@ -40,8 +105,59 @@ bool keyboard_is_present(Device* device) { return KEYBOARD_DRIVER_API(driver)->is_present(device); } +error_t keyboard_subscribe(Device* device, KeyboardEventSubscription* sub) { + mutex_lock(&subscriptions_mutex.handle); + + // Avoid cyclic subscription list that would loop forever + 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; + sub->internal.head = 0; + sub->internal.count = 0; + sub->internal.next = subscriptions; + subscriptions = sub; + + mutex_unlock(&subscriptions_mutex.handle); + return ERROR_NONE; +} + +error_t keyboard_unsubscribe(Device*, KeyboardEventSubscription* sub) { + error_t result = ERROR_NOT_FOUND; + + mutex_lock(&subscriptions_mutex.handle); + for (KeyboardEventSubscription** link = &subscriptions; *link != nullptr; link = &(*link)->internal.next) { + if (*link == sub) { + *link = sub->internal.next; + result = ERROR_NONE; + break; + } + } + mutex_unlock(&subscriptions_mutex.handle); + + return result; +} + +error_t keyboard_poll(Device* device, KeyboardEventSubscription* sub, KeyboardKeyData* out_data) { + if (try_pop(sub, out_data)) { + return ERROR_NONE; + } + + // keyboard_read_key() fans the result out to every subscriber, including `sub`. + KeyboardKeyData data; + if (keyboard_read_key(device, &data) != ERROR_NONE || data.key == 0) { + return ERROR_TIMEOUT; + } + + return try_pop(sub, out_data) ? ERROR_NONE : ERROR_TIMEOUT; +} + const DeviceType KEYBOARD_TYPE { - .name = "keyboard" + .name = "keyboard", }; } diff --git a/TactilityKernel/source/kernel_init.cpp b/TactilityKernel/source/kernel_init.cpp index c31cc8531..2af14de6a 100644 --- a/TactilityKernel/source/kernel_init.cpp +++ b/TactilityKernel/source/kernel_init.cpp @@ -65,13 +65,16 @@ error_t kernel_init(Module* const dts_modules[], const DtsDevice dts_devices[]) dts_module++; } + // Starting is deferred to device_hotplug_poll_once() below so probe() can run first. + // DISABLED devices are never registered, so the poller never starts them. const DtsDevice* dts_device = dts_devices; while (dts_device->device != nullptr) { if (dts_device->status == DTS_DEVICE_STATUS_OKAY) { - if (device_construct_add_start(dts_device->device, dts_device->compatible) != ERROR_NONE) { - LOG_E(TAG, "kernel_init failed to construct+add+start device: %s (%s)", dts_device->device->name, dts_device->compatible); + if (device_construct_add(dts_device->device, dts_device->compatible) != ERROR_NONE) { + LOG_E(TAG, "kernel_init failed to construct+add device: %s (%s)", dts_device->device->name, dts_device->compatible); return ERROR_RESOURCE; } + device_hotplug_register(dts_device->device); } else if (dts_device->status == DTS_DEVICE_STATUS_DISABLED) { if (device_construct_add(dts_device->device, dts_device->compatible) != ERROR_NONE) { LOG_E(TAG, "kernel_init failed to construct+add device: %s (%s)", dts_device->device->name, dts_device->compatible); @@ -83,6 +86,13 @@ error_t kernel_init(Module* const dts_modules[], const DtsDevice dts_devices[]) dts_device++; } + // A mandatory (probe == NULL) device's start failure aborts boot, as before. + if (device_hotplug_poll_once() != ERROR_NONE) { + LOG_E(TAG, "kernel_init failed to start a mandatory device"); + return ERROR_RESOURCE; + } + device_hotplug_start_polling(); + LOG_I(TAG, "init done"); return ERROR_NONE; } diff --git a/TactilityKernel/tests/source/device_hotplug_test.cpp b/TactilityKernel/tests/source/device_hotplug_test.cpp new file mode 100644 index 000000000..c23230308 --- /dev/null +++ b/TactilityKernel/tests/source/device_hotplug_test.cpp @@ -0,0 +1,129 @@ +#include "doctest.h" +#include +#include +#include + +static Module hotplug_test_module = { + .name = "hotplug_test_module", + .start = nullptr, + .stop = nullptr +}; + +static bool probe_present = false; +static int probe_driver_start_called = 0; +static int probe_driver_stop_called = 0; + +static error_t probe_driver_probe(Device*) { return probe_present ? ERROR_NONE : ERROR_NOT_FOUND; } +static error_t probe_driver_start(Device*) { probe_driver_start_called++; return ERROR_NONE; } +static error_t probe_driver_stop(Device*) { probe_driver_stop_called++; return ERROR_NONE; } + +static Driver probe_driver = { + .name = "hotplug_test_probe_driver", + .compatible = (const char*[]) { "hotplug_test,probe", nullptr }, + .start_device = probe_driver_start, + .stop_device = probe_driver_stop, + .probe = probe_driver_probe, + .api = nullptr, + .device_type = nullptr, + .owner = &hotplug_test_module, + .internal = nullptr, +}; + +TEST_CASE("device_hotplug_poll_once starts/stops a probe-capable device after debounce") { + probe_present = false; + probe_driver_start_called = 0; + probe_driver_stop_called = 0; + + static Device device { + .name = "hotplug_probe_device", + .config = nullptr, + .parent = nullptr, + }; + + CHECK_EQ(driver_construct_add(&probe_driver), ERROR_NONE); + CHECK_EQ(device_construct_add(&device, "hotplug_test,probe"), ERROR_NONE); + device_hotplug_register(&device); + + // Not present: repeated polling never starts it. + CHECK_EQ(device_hotplug_poll_once(), ERROR_NONE); + CHECK_EQ(device_hotplug_poll_once(), ERROR_NONE); + CHECK_EQ(probe_driver_start_called, 0); + CHECK_FALSE(device_is_ready(&device)); + + // Present: needs DEVICE_HOTPLUG_DEBOUNCE_COUNT consecutive polls before it actually starts. + probe_present = true; + for (int i = 0; i < DEVICE_HOTPLUG_DEBOUNCE_COUNT - 1; i++) { + CHECK_EQ(device_hotplug_poll_once(), ERROR_NONE); + CHECK_FALSE(device_is_ready(&device)); + } + CHECK_EQ(device_hotplug_poll_once(), ERROR_NONE); + CHECK(device_is_ready(&device)); + CHECK_EQ(probe_driver_start_called, 1); + + // Already started: further polls while still present don't restart it. + CHECK_EQ(device_hotplug_poll_once(), ERROR_NONE); + CHECK_EQ(probe_driver_start_called, 1); + + // Absent again: needs the same debounce before it stops. + probe_present = false; + for (int i = 0; i < DEVICE_HOTPLUG_DEBOUNCE_COUNT - 1; i++) { + CHECK_EQ(device_hotplug_poll_once(), ERROR_NONE); + CHECK(device_is_ready(&device)); + } + CHECK_EQ(device_hotplug_poll_once(), ERROR_NONE); + CHECK_FALSE(device_is_ready(&device)); + CHECK_EQ(probe_driver_stop_called, 1); + + // Only started/stopped, never removed/destructed. + CHECK(device_is_added(&device)); + + device_hotplug_unregister(&device); + CHECK_EQ(device_remove(&device), ERROR_NONE); + CHECK_EQ(device_destruct(&device), ERROR_NONE); + CHECK_EQ(driver_remove_destruct(&probe_driver), ERROR_NONE); +} + +static int mandatory_driver_start_called = 0; + +static error_t mandatory_driver_start(Device*) { mandatory_driver_start_called++; return ERROR_NONE; } +static error_t mandatory_driver_stop(Device*) { return ERROR_NONE; } + +static Driver mandatory_driver = { + .name = "hotplug_test_mandatory_driver", + .compatible = (const char*[]) { "hotplug_test,mandatory", nullptr }, + .start_device = mandatory_driver_start, + .stop_device = mandatory_driver_stop, + .probe = nullptr, + .api = nullptr, + .device_type = nullptr, + .owner = &hotplug_test_module, + .internal = nullptr, +}; + +TEST_CASE("device_hotplug_poll_once starts a mandatory (probe == NULL) device unconditionally on the first call") { + mandatory_driver_start_called = 0; + + static Device device { + .name = "hotplug_mandatory_device", + .config = nullptr, + .parent = nullptr, + }; + + CHECK_EQ(driver_construct_add(&mandatory_driver), ERROR_NONE); + CHECK_EQ(device_construct_add(&device, "hotplug_test,mandatory"), ERROR_NONE); + device_hotplug_register(&device); + + CHECK_EQ(device_hotplug_poll_once(), ERROR_NONE); + CHECK(device_is_ready(&device)); + CHECK_EQ(mandatory_driver_start_called, 1); + + // Already started: further polls don't restart it. + CHECK_EQ(device_hotplug_poll_once(), ERROR_NONE); + CHECK_EQ(mandatory_driver_start_called, 1); + + device_hotplug_unregister(&device); + CHECK_EQ(device_stop(&device), ERROR_NONE); + CHECK_EQ(device_remove(&device), ERROR_NONE); + CHECK_EQ(device_destruct(&device), ERROR_NONE); + CHECK_EQ(driver_remove_destruct(&mandatory_driver), ERROR_NONE); +} diff --git a/TactilityKernel/tests/source/keyboard_test.cpp b/TactilityKernel/tests/source/keyboard_test.cpp new file mode 100644 index 000000000..64717da61 --- /dev/null +++ b/TactilityKernel/tests/source/keyboard_test.cpp @@ -0,0 +1,225 @@ +#include "doctest.h" +#include +#include +#include +#include + +static Module keyboard_test_module = { + .name = "keyboard_test_module", + .start = nullptr, + .stop = nullptr +}; + +static int read_key_call_count = 0; + +// First call reports a key press, every call after that reports nothing pending (key == 0), +// matching the convention every real driver uses (see e.g. sdl_keyboard_read_key). +static error_t fake_read_key(Device*, KeyboardKeyData* data) { + read_key_call_count++; + if (read_key_call_count == 1) { + data->key = 'a'; + data->pressed = true; + data->continue_reading = false; + } else { + data->key = 0; + data->pressed = false; + data->continue_reading = false; + } + return ERROR_NONE; +} + +static const KeyboardApi fake_keyboard_api = { + .read_key = fake_read_key, + .get_backlight = nullptr, + .is_present = nullptr, +}; + +static Driver fake_keyboard_driver = { + .name = "fake_keyboard_driver", + .compatible = (const char*[]) { "keyboard_test,fake", nullptr }, + .start_device = nullptr, + .stop_device = nullptr, + .api = &fake_keyboard_api, + .device_type = &KEYBOARD_TYPE, + .owner = &keyboard_test_module, + .internal = nullptr, +}; + +// No read_key: this driver only ever pushes events via keyboard_emit_key(). +static const KeyboardApi fake_emit_only_keyboard_api = { + .read_key = nullptr, + .get_backlight = nullptr, + .is_present = nullptr, +}; + +static Driver fake_emit_only_keyboard_driver = { + .name = "fake_emit_only_keyboard_driver", + .compatible = (const char*[]) { "keyboard_test,fake_emit_only", nullptr }, + .start_device = nullptr, + .stop_device = nullptr, + .api = &fake_emit_only_keyboard_api, + .device_type = &KEYBOARD_TYPE, + .owner = &keyboard_test_module, + .internal = nullptr, +}; + +TEST_CASE("keyboard_read_key returns ERROR_INVALID_STATE, without touching the driver, on a device that isn't started") { + read_key_call_count = 0; + + static Device fake_device { + .name = "fake_keyboard_device_not_started", + .config = nullptr, + .parent = nullptr, + }; + + CHECK_EQ(driver_construct_add(&fake_keyboard_driver), ERROR_NONE); + CHECK_EQ(device_construct_add(&fake_device, "keyboard_test,fake"), ERROR_NONE); + // Deliberately not started - a hotplug-managed keyboard can be constructed+added long before + // (or after) its driver is actually running, but LVGL's indev still polls it unconditionally. + + KeyboardKeyData out { .key = 'x' }; + CHECK_EQ(keyboard_read_key(&fake_device, &out), ERROR_INVALID_STATE); + CHECK_EQ(out.key, 0); + CHECK_EQ(read_key_call_count, 0); + + CHECK_EQ(device_remove(&fake_device), ERROR_NONE); + CHECK_EQ(device_destruct(&fake_device), ERROR_NONE); + CHECK_EQ(driver_remove_destruct(&fake_keyboard_driver), ERROR_NONE); +} + +TEST_CASE("keyboard_poll's default implementation fans out a read_key result to other subscribers") { + read_key_call_count = 0; + + static Device fake_device { + .name = "fake_keyboard_device", + .config = nullptr, + .parent = nullptr, + }; + + CHECK_EQ(driver_construct_add(&fake_keyboard_driver), ERROR_NONE); + CHECK_EQ(device_construct_add(&fake_device, "keyboard_test,fake"), ERROR_NONE); + CHECK_EQ(device_start(&fake_device), ERROR_NONE); + + KeyboardEventSubscription sub_a {}; + KeyboardEventSubscription sub_b {}; + CHECK_EQ(keyboard_subscribe(&fake_device, &sub_a), ERROR_NONE); + CHECK_EQ(keyboard_subscribe(&fake_device, &sub_b), ERROR_NONE); + + KeyboardKeyData out {}; + CHECK_EQ(keyboard_poll(&fake_device, &sub_a, &out), ERROR_NONE); + CHECK_EQ(out.key, 'a'); + CHECK_EQ(read_key_call_count, 1); + + // sub_b receives the same event without a second hardware read - it was fanned out by sub_a's poll. + CHECK_EQ(keyboard_poll(&fake_device, &sub_b, &out), ERROR_NONE); + CHECK_EQ(out.key, 'a'); + CHECK_EQ(read_key_call_count, 1); + + // sub_b's queue is now empty; polling again falls through to a real (empty) hardware read. + CHECK_EQ(keyboard_poll(&fake_device, &sub_b, &out), ERROR_TIMEOUT); + CHECK_EQ(read_key_call_count, 2); + + CHECK_EQ(keyboard_unsubscribe(&fake_device, &sub_a), ERROR_NONE); + CHECK_EQ(keyboard_unsubscribe(&fake_device, &sub_b), ERROR_NONE); + CHECK_EQ(device_stop(&fake_device), ERROR_NONE); + CHECK_EQ(device_remove(&fake_device), ERROR_NONE); + CHECK_EQ(device_destruct(&fake_device), ERROR_NONE); + CHECK_EQ(driver_remove_destruct(&fake_keyboard_driver), ERROR_NONE); +} + +TEST_CASE("keyboard_read_key fans out to subscribers even when called directly") { + read_key_call_count = 0; + + static Device fake_device { + .name = "fake_keyboard_device_read_key_fanout", + .config = nullptr, + .parent = nullptr, + }; + + CHECK_EQ(driver_construct_add(&fake_keyboard_driver), ERROR_NONE); + CHECK_EQ(device_construct_add(&fake_device, "keyboard_test,fake"), ERROR_NONE); + CHECK_EQ(device_start(&fake_device), ERROR_NONE); + + KeyboardEventSubscription sub {}; + CHECK_EQ(keyboard_subscribe(&fake_device, &sub), ERROR_NONE); + + KeyboardKeyData out {}; + CHECK_EQ(keyboard_read_key(&fake_device, &out), ERROR_NONE); + CHECK_EQ(out.key, 'a'); + + // sub never called keyboard_poll() itself - it received this via keyboard_read_key()'s fan-out. + KeyboardKeyData polled {}; + CHECK_EQ(keyboard_poll(&fake_device, &sub, &polled), ERROR_NONE); + CHECK_EQ(polled.key, 'a'); + CHECK_EQ(read_key_call_count, 1); + + CHECK_EQ(keyboard_unsubscribe(&fake_device, &sub), ERROR_NONE); + CHECK_EQ(device_stop(&fake_device), ERROR_NONE); + CHECK_EQ(device_remove(&fake_device), ERROR_NONE); + CHECK_EQ(device_destruct(&fake_device), ERROR_NONE); + CHECK_EQ(driver_remove_destruct(&fake_keyboard_driver), ERROR_NONE); +} + +TEST_CASE("keyboard_emit_key delivers to subscribers of a driver with no read_key") { + static Device fake_device { + .name = "fake_emit_only_keyboard_device", + .config = nullptr, + .parent = nullptr, + }; + + CHECK_EQ(driver_construct_add(&fake_emit_only_keyboard_driver), ERROR_NONE); + CHECK_EQ(device_construct_add(&fake_device, "keyboard_test,fake_emit_only"), ERROR_NONE); + CHECK_EQ(device_start(&fake_device), ERROR_NONE); + + KeyboardEventSubscription sub {}; + CHECK_EQ(keyboard_subscribe(&fake_device, &sub), ERROR_NONE); + + // Nothing pushed yet, and there's no read_key to fall back on. + KeyboardKeyData out {}; + CHECK_EQ(keyboard_poll(&fake_device, &sub, &out), ERROR_TIMEOUT); + + KeyboardKeyData emitted { .key = 'z', .pressed = true, .continue_reading = false }; + keyboard_emit_key(&fake_device, emitted); + + CHECK_EQ(keyboard_poll(&fake_device, &sub, &out), ERROR_NONE); + CHECK_EQ(out.key, 'z'); + + CHECK_EQ(keyboard_unsubscribe(&fake_device, &sub), ERROR_NONE); + CHECK_EQ(device_stop(&fake_device), ERROR_NONE); + CHECK_EQ(device_remove(&fake_device), ERROR_NONE); + CHECK_EQ(device_destruct(&fake_device), ERROR_NONE); + CHECK_EQ(driver_remove_destruct(&fake_emit_only_keyboard_driver), ERROR_NONE); +} + +TEST_CASE("keyboard_unsubscribe stops a subscription from receiving further fanned-out events") { + read_key_call_count = 0; + + static Device fake_device { + .name = "fake_keyboard_device_2", + .config = nullptr, + .parent = nullptr, + }; + + CHECK_EQ(driver_construct_add(&fake_keyboard_driver), ERROR_NONE); + CHECK_EQ(device_construct_add(&fake_device, "keyboard_test,fake"), ERROR_NONE); + CHECK_EQ(device_start(&fake_device), ERROR_NONE); + + KeyboardEventSubscription sub_a {}; + KeyboardEventSubscription sub_b {}; + CHECK_EQ(keyboard_subscribe(&fake_device, &sub_a), ERROR_NONE); + CHECK_EQ(keyboard_subscribe(&fake_device, &sub_b), ERROR_NONE); + CHECK_EQ(keyboard_unsubscribe(&fake_device, &sub_b), ERROR_NONE); + + KeyboardKeyData out {}; + CHECK_EQ(keyboard_poll(&fake_device, &sub_a, &out), ERROR_NONE); + CHECK_EQ(out.key, 'a'); + + // sub_b was unsubscribed before the poll, so nothing was fanned out to it. + CHECK_EQ(keyboard_poll(&fake_device, &sub_b, &out), ERROR_TIMEOUT); + + CHECK_EQ(keyboard_unsubscribe(&fake_device, &sub_a), ERROR_NONE); + CHECK_EQ(device_stop(&fake_device), ERROR_NONE); + CHECK_EQ(device_remove(&fake_device), ERROR_NONE); + CHECK_EQ(device_destruct(&fake_device), ERROR_NONE); + CHECK_EQ(driver_remove_destruct(&fake_keyboard_driver), ERROR_NONE); +}