From 9782279ee411fe0e8d731b709dae06db2ad3d709 Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Sun, 20 Sep 2026 00:19:11 +0200 Subject: [PATCH] tab5 keyboard ported for new hot-plugging support --- .../Source/devices/devices_common.cpp | 12 +- .../Source/devices/devices_common.h | 4 + .../Source/devices/tab5_keyboard.cpp | 51 +++--- .../Source/devices/tab5_keyboard.h | 36 ++-- .../devices/tab5_keyboard_attach_detect.cpp | 173 ------------------ .../devices/tab5_keyboard_attach_detect.h | 12 -- .../devices/tab5_keyboard_lvgl_rotation.cpp | 53 ++++++ .../devices/tab5_keyboard_lvgl_rotation.h | 8 + Devices/m5stack-tab5/Source/module.cpp | 6 +- TactilityKernel/include/tactility/device.h | 7 +- TactilityKernel/include/tactility/driver.h | 8 +- .../include/tactility/drivers/keyboard.h | 27 ++- TactilityKernel/source/device.cpp | 12 +- TactilityKernel/source/driver.cpp | 2 +- TactilityKernel/source/drivers/keyboard.cpp | 67 ++++--- .../tests/source/device_hotplug_test.cpp | 2 +- .../tests/source/keyboard_test.cpp | 57 ++++++ 17 files changed, 247 insertions(+), 290 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 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..1c5dcfc50 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,13 @@ 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)); +} + +error_t tab5_keyboard_is_attached(Device* device) { + return device_is_ready(device); } // --------------------------------------------------------------------------- @@ -462,7 +467,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 +481,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 +599,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 +662,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 +673,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 +687,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 +718,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..c044df797 100644 --- a/Devices/m5stack-tab5/Source/devices/tab5_keyboard.h +++ b/Devices/m5stack-tab5/Source/devices/tab5_keyboard.h @@ -11,37 +11,27 @@ 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); +/** @brief Proxy for device_is_started(). + * @deprecated Use device_is_started() + */ +error_t tab5_keyboard_is_attached(struct Device* device); #ifdef __cplusplus } 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..12a6f36f0 --- /dev/null +++ b/Devices/m5stack-tab5/Source/devices/tab5_keyboard_lvgl_rotation.cpp @@ -0,0 +1,53 @@ +#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 && 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() { + 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/TactilityKernel/include/tactility/device.h b/TactilityKernel/include/tactility/device.h index e5c4a9c1b..8c14fd3e4 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 { @@ -395,8 +395,9 @@ 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. A device - * never registered here is never touched by the poller. + * 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. */ void device_hotplug_register(struct Device* device); diff --git a/TactilityKernel/include/tactility/driver.h b/TactilityKernel/include/tactility/driver.h index 921d1a7b9..67858330a 100644 --- a/TactilityKernel/include/tactility/driver.h +++ b/TactilityKernel/include/tactility/driver.h @@ -24,11 +24,13 @@ struct Driver { /** 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. + * 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. */ - bool (*probe)(struct Device* device); + error_t (*probe)(struct Device* device); /** Contains the driver's functions */ const void* api; /** Which type of devices this driver creates (can be NULL) */ diff --git a/TactilityKernel/include/tactility/drivers/keyboard.h b/TactilityKernel/include/tactility/drivers/keyboard.h index a5c65507a..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', @@ -137,19 +134,18 @@ 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) */ @@ -177,8 +173,7 @@ void keyboard_emit_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. - * Returns true when the driver doesn't implement KeyboardApi::is_present + * @deprecated See KeyboardApi::is_present. * @param[in] device the keyboard device */ bool keyboard_is_present(struct Device* device); diff --git a/TactilityKernel/source/device.cpp b/TactilityKernel/source/device.cpp index 26a0dde22..b5a971bc5 100644 --- a/TactilityKernel/source/device.cpp +++ b/TactilityKernel/source/device.cpp @@ -581,6 +581,9 @@ 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; + } hotplug_registered_devices.push_back(device); } @@ -592,10 +595,15 @@ void device_hotplug_unregister(Device* device) { } 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; + // Collect actions first: start_device()/stop_device() may add/remove child devices // (ledger_lock), so don't call them mid-iteration. std::vector actions; - for (Device* device : hotplug_registered_devices) { + for (Device* device : devices) { auto* internal = device->internal; auto* driver = internal->driver; if (driver == nullptr) { @@ -609,7 +617,7 @@ error_t device_hotplug_poll_once(void) { continue; } - bool present = driver->probe(device); + bool present = driver->probe(device) == ERROR_NONE; if (present == internal->hotplug.present) { internal->hotplug.consecutive = 0; continue; diff --git a/TactilityKernel/source/driver.cpp b/TactilityKernel/source/driver.cpp index fd2630693..9df0cb4fa 100644 --- a/TactilityKernel/source/driver.cpp +++ b/TactilityKernel/source/driver.cpp @@ -129,7 +129,7 @@ error_t driver_bind(Driver* driver, Device* device) { goto error; } - if (driver->probe != nullptr && !driver->probe(device)) { + if (driver->probe != nullptr && driver->probe(device) != ERROR_NONE) { error = ERROR_NOT_FOUND; goto error; } diff --git a/TactilityKernel/source/drivers/keyboard.cpp b/TactilityKernel/source/drivers/keyboard.cpp index 256d6e403..a2cd2a849 100644 --- a/TactilityKernel/source/drivers/keyboard.cpp +++ b/TactilityKernel/source/drivers/keyboard.cpp @@ -7,11 +7,7 @@ #define KEYBOARD_DRIVER_API(driver) ((struct KeyboardApi*)driver->api) -/** - * Intrusive singly-linked list of subscriptions, filtered by `device` on fan-out. Guarded by a - * single coarse-grained mutex - same rationale as app_event.cpp's subscriptions_mutex: fanning - * out here is just a struct copy, never caller code. - */ +// Subscriptions, filtered by `device` on fan-out. static KeyboardEventSubscription* subscriptions = nullptr; struct KeyboardEventMutex { @@ -20,13 +16,13 @@ struct KeyboardEventMutex { ~KeyboardEventMutex() { mutex_destruct(&handle); } }; +// One coarse mutex is fine: fan-out is just a struct copy, never caller code. static KeyboardEventMutex subscriptions_mutex; -// Pushes `data` onto every subscription for `device` except `exclude` (pass nullptr to include -// all of them). Caller must hold subscriptions_mutex. -static void fan_out_to_subscribers(Device* device, const KeyboardKeyData& data, const KeyboardEventSubscription* exclude) { +// 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 == exclude || sub->internal.device != device) { + if (sub->internal.device != device) { continue; } if (sub->internal.count < KEYBOARD_EVENT_QUEUE_CAPACITY) { @@ -37,28 +33,55 @@ static void fan_out_to_subscribers(Device* device, const KeyboardKeyData& data, } } +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); if (KEYBOARD_DRIVER_API(driver)->read_key == nullptr) { return ERROR_NOT_SUPPORTED; } - // 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. + // 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, nullptr); + fan_out_to_subscribers(device, data); mutex_unlock(&subscriptions_mutex.handle); } @@ -118,29 +141,17 @@ error_t keyboard_unsubscribe(Device*, KeyboardEventSubscription* sub) { } error_t keyboard_poll(Device* device, KeyboardEventSubscription* sub, KeyboardKeyData* out_data) { - mutex_lock(&subscriptions_mutex.handle); - - if (sub->internal.count > 0) { - *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); + if (try_pop(sub, out_data)) { return ERROR_NONE; } - // No queued event of our own - try a synchronous read (if the driver has one; a driver that - // only pushes via keyboard_emit_key() has no read_key, and there's simply nothing new yet). + // 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) { - mutex_unlock(&subscriptions_mutex.handle); return ERROR_TIMEOUT; } - fan_out_to_subscribers(device, data, sub); - - *out_data = data; - mutex_unlock(&subscriptions_mutex.handle); - return ERROR_NONE; + return try_pop(sub, out_data) ? ERROR_NONE : ERROR_TIMEOUT; } const DeviceType KEYBOARD_TYPE { diff --git a/TactilityKernel/tests/source/device_hotplug_test.cpp b/TactilityKernel/tests/source/device_hotplug_test.cpp index 3d90d4579..c23230308 100644 --- a/TactilityKernel/tests/source/device_hotplug_test.cpp +++ b/TactilityKernel/tests/source/device_hotplug_test.cpp @@ -13,7 +13,7 @@ static bool probe_present = false; static int probe_driver_start_called = 0; static int probe_driver_stop_called = 0; -static bool probe_driver_probe(Device*) { return probe_present; } +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; } diff --git a/TactilityKernel/tests/source/keyboard_test.cpp b/TactilityKernel/tests/source/keyboard_test.cpp index f1e00f055..64717da61 100644 --- a/TactilityKernel/tests/source/keyboard_test.cpp +++ b/TactilityKernel/tests/source/keyboard_test.cpp @@ -63,6 +63,30 @@ static Driver fake_emit_only_keyboard_driver = { .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; @@ -103,6 +127,39 @@ TEST_CASE("keyboard_poll's default implementation fans out a read_key result to 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",