Tab5 ST7121 variant + fixes (#605)

Added the newest variant to the tab5, St7121.
Fixed variant detection reliability
Fixed tab5 camera WHO_AM_I failing sometimes
Fixed tab5 keyboard live rotation on boot and after stopping/starting lvgl

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

@@ -21,6 +21,34 @@ void tab5_set_variant(Tab5Variant variant) {
     detected_variant = variant;
 }
 
+// The ST7123 (V2) and ST7121 (V3) touch controllers share the same fixed I2C address, so presence
+// alone doesn't distinguish them. Both expose a firmware-version byte at register 0x0000 (a 16-bit
+// register address, per ESP_LCD_TOUCH_IO_I2C_ST7123_CONFIG's lcd_cmd_bits=16 - see the M5Tab5
+// UserDemo's bsp_detect_display_type()): fw_version 1 means ST7121/V3, fw_version 3 means
+// ST7123/V2. Returns false (leaving *out_variant untouched) if the register read fails or reports
+// an unrecognized value, so the caller's outer attempt loop can retry rather than the touch IC's
+// transient not-finished-booting state permanently misdetecting V3 hardware as V2.
+static bool probe_st7123_or_st7121_variant(Device* i2c0, TickType_t timeout, Tab5Variant* out_variant) {
+    const uint8_t reg_addr[2] = {0x00, 0x00};
+    uint8_t fw_version = 0;
+    if (i2c_controller_write_read(i2c0, ESP_LCD_TOUCH_IO_I2C_ST7123_ADDRESS, reg_addr, sizeof(reg_addr), &fw_version, 1, timeout) != ERROR_NONE) {
+        LOG_W(TAG, "display_detect: failed to read touch FW version, retrying");
+        return false;
+    }
+    if (fw_version == 1) {
+        LOG_I(TAG, "display_detect: detected ST7121 touch (FW version 1) — using variant V3");
+        *out_variant = Tab5Variant::V3;
+        return true;
+    }
+    if (fw_version == 3) {
+        LOG_I(TAG, "display_detect: detected ST7123 touch (FW version 3) — using variant V2");
+        *out_variant = Tab5Variant::V2;
+        return true;
+    }
+    LOG_W(TAG, "display_detect: touch at ST7123 address reported unknown FW version %u, retrying", fw_version);
+    return false;
+}
+
 Tab5Variant tab5_probe_variant(Device* i2c0) {
     // Allow time for the touch IC to fully boot after the reset pulse above: 100ms is enough for
     // I2C ACK (probe) but cold power-on needs ~300ms before register reads succeed reliably.
@@ -36,8 +64,12 @@ Tab5Variant tab5_probe_variant(Device* i2c0) {
         }
 
         if (i2c_controller_has_device_at_address(i2c0, ESP_LCD_TOUCH_IO_I2C_ST7123_ADDRESS, PROBE_TIMEOUT) == ERROR_NONE) {
-            LOG_I(TAG, "display_detect: detected ST7123 touch — using variant V2");
-            return Tab5Variant::V2;
+            Tab5Variant variant;
+            if (probe_st7123_or_st7121_variant(i2c0, PROBE_TIMEOUT, &variant)) {
+                return variant;
+            }
+            // FW-version read failed/unrecognized - fall through to the retry delay below instead
+            // of giving up immediately, same as an address-probe miss.
         }
 
         vTaskDelay(pdMS_TO_TICKS(100));

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

@@ -6,6 +6,7 @@ enum class Tab5Variant {
     Unknown,
     V1,  // Older variant: ILI9881C display + GT911 touch (see devices_v1.cpp)
     V2,  // Newer variant (default): ST7123 display + in-cell touch (see devices_v2.cpp)
+    V3,  // Newest variant: ST7121 display + in-cell touch (see devices_v3.cpp)
 };
 
 // Populated once the device_listener callback in display_detect.cpp has detected which

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

@@ -1,6 +1,7 @@
 #include "devices_v2.h"
 
 #include "devices_common.h"
+#include "devices_v2_v3_touch.h"
 #include "st7123_init_data.h"
 
 #include <tactility/device.h>
@@ -8,7 +9,6 @@
 #include <tactility/log.h>
 
 #include <drivers/st7123.h>
-#include <drivers/st7123_touch.h>
 
 #include <iterator>
 #include <vector>
@@ -19,43 +19,6 @@ static std::vector<uint8_t> display_init_bytes;
 static St7123Config st7123_config {};
 static Device display_device {};
 
-static St7123TouchConfig st7123_touch_config {};
-static Device st7123_touch_device {};
-
-static void create_st7123_touch(Device* i2c0) {
-    st7123_touch_device = Device {
-        .address = 0,
-        .name = "touch0",
-        .config = nullptr,
-        .parent = nullptr,
-        .internal = nullptr,
-    };
-
-    GpioPinSpec pin_interrupt = GPIO_PIN_SPEC_NONE;
-    Device* gpio0 = nullptr;
-    if (device_get_by_name("gpio0", &gpio0) == ERROR_NONE) {
-        pin_interrupt = GpioPinSpec { gpio0, 23, GPIO_FLAG_NONE };
-        device_put(gpio0);
-    } else {
-        LOG_W(TAG, "display_detect: gpio0 not found, touch interrupt pin will not be wired");
-    }
-
-    st7123_touch_config = St7123TouchConfig {
-        .address = 0x55, // fixed - see ESP_LCD_TOUCH_IO_I2C_ST7123_ADDRESS
-        .x_max = 720,
-        .y_max = 1280,
-        .swap_xy = false,
-        .mirror_x = false,
-        .mirror_y = false,
-        // Reset is pulsed via io_expander0 (detect.cpp's pulse_display_reset_pins), not a direct SoC GPIO.
-        .pin_reset = GPIO_PIN_SPEC_NONE,
-        .pin_interrupt = pin_interrupt,
-    };
-    st7123_touch_device.config = &st7123_touch_config;
-
-    construct_add_start(&st7123_touch_device, i2c0, "sitronix,st7123-touch");
-}
-
 void tab5_create_devices_v2(Device* i2c0) {
     display_device = Device {
         .address = 0,

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

@@ -0,0 +1,48 @@
+#include "devices_v2_v3_touch.h"
+
+#include "devices_common.h"
+
+#include <tactility/device.h>
+#include <tactility/drivers/gpio.h>
+#include <tactility/log.h>
+
+#include <drivers/st7123_touch.h>
+
+constexpr auto* TAG = "Tab5";
+
+static St7123TouchConfig st7123_touch_config {};
+static Device st7123_touch_device {};
+
+void create_st7123_touch(Device* i2c0) {
+    st7123_touch_device = Device {
+        .address = 0,
+        .name = "touch0",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr,
+    };
+
+    GpioPinSpec pin_interrupt = GPIO_PIN_SPEC_NONE;
+    Device* gpio0 = nullptr;
+    if (device_get_by_name("gpio0", &gpio0) == ERROR_NONE) {
+        pin_interrupt = GpioPinSpec { gpio0, 23, GPIO_FLAG_NONE };
+        device_put(gpio0);
+    } else {
+        LOG_W(TAG, "display_detect: gpio0 not found, touch interrupt pin will not be wired");
+    }
+
+    st7123_touch_config = St7123TouchConfig {
+        .address = 0x55, // fixed - see ESP_LCD_TOUCH_IO_I2C_ST7123_ADDRESS
+        .x_max = 720,
+        .y_max = 1280,
+        .swap_xy = false,
+        .mirror_x = false,
+        .mirror_y = false,
+        // Reset is pulsed via io_expander0 (detect.cpp's pulse_display_reset_pins), not a direct SoC GPIO.
+        .pin_reset = GPIO_PIN_SPEC_NONE,
+        .pin_interrupt = pin_interrupt,
+    };
+    st7123_touch_device.config = &st7123_touch_config;
+
+    construct_add_start(&st7123_touch_device, i2c0, "sitronix,st7123-touch");
+}

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

@@ -0,0 +1,8 @@
+#pragma once
+
+struct Device;
+
+// Constructs, parents, binds and starts the ST7123 in-cell touch device on i2c0. Shared by the V2
+// (ST7123 display) and V3 (ST7121 display) variants - both boards use the exact same touch
+// controller/address/wiring, only the display panel differs. Not used by V1 (ILI9881C + GT911).
+void create_st7123_touch(Device* i2c0);

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

@@ -0,0 +1,85 @@
+#include "devices_v3.h"
+
+#include "devices_common.h"
+#include "devices_v2_v3_touch.h"
+
+#include <tactility/device.h>
+#include <tactility/drivers/gpio.h>
+#include <tactility/log.h>
+
+#include <drivers/st7121.h>
+
+#define TAG "Tab5"
+
+static St7121Config st7121_config {};
+static Device display_device {};
+
+// Newest Tab5 variant: ST7121 display + the same in-cell ST7123 touch controller as V2 (see
+// devices_common.cpp's create_st7123_touch). No custom init-sequence is supplied - unlike ST7123,
+// the M5Tab5 UserDemo runs the ST7121 panel with the esp_lcd_st7121 component's own built-in
+// default bring-up sequence (see esp_lcd_st7121.c's vendor_specific_init_default), so
+// init_sequence stays null here too. Timing values (vsync_pulse_width/back_porch/front_porch) are
+// per the UserDemo's is_st7121 branch in bsp_display_new_with_handles_to_st7123() - the only
+// values that differ from V2/ST7123.
+void tab5_create_devices_v3(Device* i2c0) {
+    display_device = Device {
+        .address = 0,
+        .name = "display0",
+        .config = nullptr,
+        .parent = nullptr,
+        .internal = nullptr,
+    };
+
+    Device* backlight = nullptr;
+    if (device_get_by_name("display_backlight", &backlight) != ERROR_NONE) {
+        LOG_W(TAG, "display_detect: display_backlight not found");
+    }
+
+    st7121_config = St7121Config {
+        .horizontal_resolution = 720,
+        .vertical_resolution = 1280,
+        .bits_per_pixel = 16,
+        .bgr_order = false,
+        .invert_color = false,
+        .mirror_x = false,
+        .mirror_y = false,
+        .pin_reset = GPIO_PIN_SPEC_NONE,
+        .ldo_channel = 3,
+        .ldo_voltage_mv = 2500,
+        .dsi_bus_id = 0,
+        .num_data_lanes = 2,
+        .lane_bit_rate_mbps = 965, // ST7121 lane bitrate per M5Stack BSP (same as ST7123)
+        .dpi_clock_freq_mhz = 70,
+        .hsync_pulse_width = 2,
+        .hsync_back_porch = 40,
+        .hsync_front_porch = 40,
+        .vsync_pulse_width = 20,
+        .vsync_back_porch = 24,
+        .vsync_front_porch = 200,
+        .num_fbs = 2,
+        .use_dma2d = true,
+        .disable_lp = false,
+        .allow_tearing = true, // matches old lvgl_port_display_dsi_cfg_t.avoid_tearing = 0 (disabled = don't wait)
+        .init_sequence = nullptr, // use esp_lcd_st7121's built-in default sequence, per the UserDemo
+        .init_sequence_length = 0,
+        .backlight = backlight,
+    };
+    display_device.config = &st7121_config;
+
+    if (backlight != nullptr) {
+        device_put(backlight);
+    }
+
+    Device* root = nullptr;
+    if (device_get_by_name("/", &root) != ERROR_NONE) {
+        LOG_E(TAG, "display_detect: root device not found");
+        return;
+    }
+    bool started = construct_add_start(&display_device, root, "sitronix,st7121");
+    device_put(root);
+    if (!started) {
+        return;
+    }
+
+    create_st7123_touch(i2c0);
+}

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

@@ -0,0 +1,5 @@
+#pragma once
+
+struct Device;
+
+void tab5_create_devices_v3(Device* i2c0);

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

@@ -3,6 +3,7 @@
 #include "devices_common.h"
 #include "devices_v1.h"
 #include "devices_v2.h"
+#include "devices_v3.h"
 #include "tab5_keyboard.h"
 
 #include <tactility/device.h>
@@ -91,16 +92,18 @@ static void on_display_detect_event(Device* device, DeviceEvent event, void* con
         }
     }
 
-    if (io_expander0 != nullptr && tab5_get_variant() == Tab5Variant::Unknown) {
-        Tab5Variant variant = tab5_probe_variant(i2c0);
-        tab5_set_variant(variant);
-    }
-
-    // We need i2c0 and io_expander0 to create the display and touch devices
+    // We need i2c0 and io_expander0 to pulse the LCD/touch reset pins, probe the variant, and
+    // create the display and touch devices - all gated on the same pair, so do them together in
+    // one pass. Order matters: the reset pulse must run *before* tab5_probe_variant(), otherwise
+    // the touch IC's response depends on whatever power-on state it happened to be in rather than
+    // a deterministic post-reset state.
     if (!did_create_display && i2c0 != nullptr && io_expander0 != nullptr) {
         did_create_display = true;
         if (pulse_display_reset_pins(io_expander0)) {
-            switch (tab5_get_variant()) {
+            Tab5Variant variant = tab5_probe_variant(i2c0);
+            tab5_set_variant(variant);
+
+            switch (variant) {
                 case Tab5Variant::Unknown:
                     LOG_E(TAG, "Variant not detected yet");
                     break;
@@ -110,6 +113,9 @@ static void on_display_detect_event(Device* device, DeviceEvent event, void* con
                 case Tab5Variant::V2:
                     tab5_create_devices_v2(i2c0);
                     break;
+                case Tab5Variant::V3:
+                    tab5_create_devices_v3(i2c0);
+                    break;
             }
         } else {
             LOG_E(TAG, "display_detect: skipping display creation, failed to pulse reset pins");

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

@@ -9,7 +9,6 @@
 #include <tactility/drivers/i2c_controller.h>
 #include <tactility/drivers/keyboard.h>
 #include <tactility/log.h>
-#include <lvgl/lvgl.h>
 #include <tactility/module.h>
 
 #include <driver/gpio.h>
@@ -36,11 +35,6 @@ static constexpr uint32_t REPEAT_RATE_MS = 80;
 // REG_INT_STAT polling (when no IRQ pin) and software key-repeat ticking.
 static constexpr uint32_t POLL_INTERVAL_MS = 20;
 
-// 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 (see check_attach_state()).
-static constexpr uint32_t ATTACH_CHECK_INTERVAL_MS = 1000;
-
 // ---------------------------------------------------------------------------
 // Register addresses
 // ---------------------------------------------------------------------------
@@ -193,14 +187,10 @@ struct Tab5KeyboardInternal {
     bool irq_configured;
     gpio_num_t irq_pin;
 
-    // Poll/attach-check throttling (real-time based, since read_key() is called at whatever rate
-    // LVGL's indev timer and its own drain-loop - via continue_reading - happen to run at, unlike
-    // the old deprecated-HAL's fixed 20ms Timer)
+    // Poll throttling (real-time based, since read_key() is called at whatever rate LVGL's indev
+    // timer and its own drain-loop - via continue_reading - happen to run at, unlike the old
+    // deprecated-HAL's fixed 20ms Timer)
     uint32_t last_poll_ms;
-    uint32_t last_attach_check_ms;
-    bool was_attached;
-    bool pending_attach_state;
-    uint8_t pending_attach_confirm_count;
 
     // Software key-repeat state (tracked by position to survive modifier changes)
     uint32_t repeat_key;
@@ -208,9 +198,6 @@ struct Tab5KeyboardInternal {
     uint8_t repeat_col;
     uint32_t repeat_start_ms;
     uint32_t repeat_last_ms;
-
-    Tab5KeyboardAttachListener attach_listener;
-    void* attach_listener_context;
 };
 
 // ---------------------------------------------------------------------------
@@ -226,9 +213,20 @@ static bool write_reg(Device* device, uint8_t reg, uint8_t value) {
     return i2c_controller_write_register(parent, I2C_ADDRESS, reg, &value, 1, pdMS_TO_TICKS(50)) == ERROR_NONE;
 }
 
-static bool is_attached_raw(Device* device) {
+// Short-timeout variant used only by tab5_keyboard_reinit(), which runs on the FreeRTOS timer
+// daemon task (via tab5_keyboard_attach_detect.cpp) - a slow/absent device there blocks every
+// other software timer in the system, not just this one, so it can't afford write_reg()'s 50ms
+// per-call budget. read_reg()/write_reg() themselves stay at 50ms since they're also used from the
+// hot IRQ/poll path (drain_events()), where a too-short timeout would cause missed key events
+// under normal bus contention.
+static bool write_reg_fast(Device* device, uint8_t reg, uint8_t value) {
     auto* parent = device_get_parent(device);
-    return i2c_controller_has_device_at_address(parent, I2C_ADDRESS, pdMS_TO_TICKS(100)) == ERROR_NONE;
+    return i2c_controller_write_register(parent, I2C_ADDRESS, reg, &value, 1, pdMS_TO_TICKS(2)) == ERROR_NONE;
+}
+
+bool tab5_keyboard_is_attached(Device* device) {
+    auto* parent = device_get_parent(device);
+    return i2c_controller_has_device_at_address(parent, I2C_ADDRESS, pdMS_TO_TICKS(5)) == ERROR_NONE;
 }
 
 // ---------------------------------------------------------------------------
@@ -385,69 +383,32 @@ static void drain_events(Device* device, Tab5KeyboardInternal* internal) {
 }
 
 // ---------------------------------------------------------------------------
-// reinit_device - (re)applies the device register configuration. Used at start() and again on
-// hot-plug reattach, since the device's RGB mode and interrupt configuration are volatile and
-// reset to power-on defaults when the keyboard is unplugged and reconnected.
+// tab5_keyboard_reinit - (re)applies the device register configuration. Called from start() and
+// again by tab5_keyboard_attach_detect.cpp on confirmed hot-plug reattach, since the device's RGB
+// mode and interrupt configuration are volatile and reset to power-on defaults when the keyboard
+// is unplugged and reconnected.
 // ---------------------------------------------------------------------------
-static void reinit_device(Device* device, Tab5KeyboardInternal* internal) {
-    write_reg(device, REG_KEYBOARD_MODE, 0x00); // Normal mode
-    write_reg(device, REG_EVENT_NUM, 0x00);     // flush event queue
-    write_reg(device, REG_INT_STAT, 0x00);      // clear pending INT
-    write_reg(device, REG_RGB_MODE, 0x01);      // Custom RGB mode (manual LED control)
-    write_reg(device, REG_BRIGHTNESS, 50);      // 50% brightness
-    update_leds(device, internal);              // restore current LED state
+void tab5_keyboard_reinit(Device* device) {
+    auto* internal = static_cast<Tab5KeyboardInternal*>(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
+    write_reg_fast(device, REG_INT_STAT, 0x00);      // clear pending INT
+    write_reg_fast(device, REG_RGB_MODE, 0x01);      // Custom RGB mode (manual LED control)
+    write_reg_fast(device, REG_BRIGHTNESS, 50);      // 50% brightness
+    update_leds(device, internal);                   // restore current LED state
 
     if (internal->irq_configured) {
-        write_reg(device, REG_INT_CFG, 0x01);   // re-enable Normal-mode interrupt (bit 0)
+        write_reg_fast(device, REG_INT_CFG, 0x01);   // re-enable Normal-mode interrupt (bit 0)
     }
 }
 
-// ---------------------------------------------------------------------------
-// check_attach_state - throttled (~1s) hot-plug detection. Reapplies device register
-// configuration on reattach, and notifies the registered attach listener (if any) of confirmed
-// transitions - see Tab5KeyboardAttachListener's doc comment for the retry contract.
-// ---------------------------------------------------------------------------
-static void check_attach_state(Device* device, Tab5KeyboardInternal* internal) {
-    uint32_t now = now_ms();
-    if (now - internal->last_attach_check_ms < ATTACH_CHECK_INTERVAL_MS) {
-        return;
-    }
-    internal->last_attach_check_ms = now;
-
-    const bool attached = is_attached_raw(device);
-    if (attached == internal->was_attached) {
-        internal->pending_attach_confirm_count = 0;
-        return;
-    }
-
-    // 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 != internal->pending_attach_state || internal->pending_attach_confirm_count == 0) {
-        internal->pending_attach_state = attached;
-        internal->pending_attach_confirm_count = 1;
-        return;
-    }
-    internal->pending_attach_confirm_count = 0;
-
-    if (attached) {
-        reinit_device(device, internal);
-    }
-
-    if (internal->attach_listener != nullptr) {
-        if (!internal->attach_listener(device, attached, internal->attach_listener_context)) {
-            return; // not handled yet (e.g. LVGL lock busy) - retry on the next confirmed check
-        }
-    }
-
-    internal->was_attached = attached;
-}
-
 // ---------------------------------------------------------------------------
 // poll_if_due - the closest equivalent to the old deprecated-HAL's 20ms-Timer-driven
-// processKeyboard(): drains new key events (IRQ-gated or polled), ticks software key-repeat, and
-// checks hot-plug attach state. Called from read_key(), throttled to real elapsed time rather
-// than call count, since read_key() can be called back-to-back multiple times per LVGL indev
-// timer tick while draining an already-queued burst (continue_reading).
+// processKeyboard(): drains new key events (IRQ-gated or polled) and ticks software key-repeat.
+// Called from read_key(), throttled to real elapsed time rather than call count, since read_key()
+// can be called back-to-back multiple times per LVGL indev timer tick while draining an
+// already-queued burst (continue_reading). Hot-plug attach detection lives outside the driver -
+// see tab5_keyboard_attach_detect.cpp.
 // ---------------------------------------------------------------------------
 static void poll_if_due(Device* device, Tab5KeyboardInternal* internal) {
     uint32_t now = now_ms();
@@ -483,8 +444,6 @@ static void poll_if_due(Device* device, Tab5KeyboardInternal* internal) {
             }
         }
     }
-
-    check_attach_state(device, internal);
 }
 
 static gpio_num_t pin_or_nc(const GpioPinSpec& pin) {
@@ -517,17 +476,24 @@ static error_t start(Device* device) {
     internal->irq_pin = pin_or_nc(config->pin_interrupt);
     if (internal->irq_pin != GPIO_NUM_NC) {
         configure_irq_pin(internal); // best-effort; falls back to polling if it fails. Must
-                                     // happen before reinit_device() so REG_INT_CFG is written
-                                     // if IRQ setup succeeded.
+                                     // happen before tab5_keyboard_reinit() so REG_INT_CFG is
+                                     // written if IRQ setup succeeded.
     }
 
-    // Best-effort: if the keyboard isn't attached yet (e.g. this device is constructed
-    // speculatively at boot so it can be hot-plug-detected later), these I2C writes fail
-    // silently and reinit_device() runs again once attach is detected.
-    reinit_device(device, internal);
-    internal->was_attached = is_attached_raw(device);
-
+    // Driver data must be set before tab5_keyboard_reinit() - it looks internal back up via
+    // 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);
+    }
+
     return ERROR_NONE;
 }
 
@@ -591,76 +557,17 @@ Driver tab5_keyboard_driver = {
     .internal = nullptr
 };
 
-// region Attach listener
-
-void tab5_keyboard_add_attach_listener(Device* device, Tab5KeyboardAttachListener callback, void* context) {
-    auto* internal = static_cast<Tab5KeyboardInternal*>(device_get_driver_data(device));
-    if (internal->attach_listener != nullptr) {
-        LOG_W(TAG, "Replacing existing attach listener without it being removed first");
-    }
-    internal->attach_listener = callback;
-    internal->attach_listener_context = context;
-}
-
-void tab5_keyboard_remove_attach_listener(Device* device, Tab5KeyboardAttachListener callback) {
-    auto* internal = static_cast<Tab5KeyboardInternal*>(device_get_driver_data(device));
-    if (internal->attach_listener != callback) {
-        return;
-    }
-    internal->attach_listener = nullptr;
-    internal->attach_listener_context = nullptr;
-}
-
-// endregion
-
 // region Dynamic construction
 
-// Reacts to the Tab5 keyboard accessory's hot-plug attach state (see Tab5KeyboardAttachListener's
-// doc comment above for the retry contract). This is UI-layer behavior the driver itself can't do
-// (it has no LVGL dependency): switch to landscape while the keyboard is attached, restoring
-// whatever rotation was active before once it's removed - but only if the user hasn't manually
-// changed it since attaching, in which case their choice is respected. Ported as-is from the
-// deprecated HAL's Tab5Keyboard::applyAutoRotation().
-static bool on_keyboard_attach_changed(Device* /*device*/, bool attached, void* /*context*/) {
-    static lv_display_rotation_t saved_rotation = LV_DISPLAY_ROTATION_0;
-    static bool rotation_override_active = false;
-
-    auto* display = lv_display_get_default();
-    if (display == nullptr) {
-        return false; // LVGL not ready yet - retry on the next confirmed check
-    }
-
-    if (!lvgl_try_lock(pdMS_TO_TICKS(1000))) {
-        return false; // retry next check
-    }
-
-    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 {
-        // 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 Tab5KeyboardConfig tab5_keyboard_config {};
 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, and the driver's own read_key() polling handles hot-plug
-// internally.
+// 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.
 void tab5_create_keyboard(Device* i2c2) {
     tab5_keyboard_device = Device {
         .address = 0,
@@ -687,9 +594,7 @@ 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.
-    if (construct_add_start(&tab5_keyboard_device, i2c2, "m5stack,tab5-keyboard")) {
-        tab5_keyboard_add_attach_listener(&tab5_keyboard_device, on_keyboard_attach_changed, nullptr);
-    }
+    construct_add_start(&tab5_keyboard_device, i2c2, "m5stack,tab5-keyboard");
 }
 
 // endregion

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

@@ -20,27 +20,23 @@ struct Tab5KeyboardConfig {
     struct GpioPinSpec pin_interrupt;
 };
 
-// Called when the keyboard accessory's hot-plug attach state changes (confirmed over two
-// consecutive ~1s checks - see the driver source). Orientation changes and other LVGL-aware
-// reactions to attach state live outside the driver (it has no LVGL dependency) - module.cpp
-// registers a listener for this instead.
-// @return true once handled; false to be called again on the next confirmed check with the same
-// `attached` value (e.g. if the LVGL lock couldn't be acquired) - mirrors this driver's own
-// internal retry-until-handled pattern for reinitializing the device on reattach.
-typedef bool (*Tab5KeyboardAttachListener)(struct Device* device, bool attached, void* context);
-
-// Only one listener is supported (this board only ever has one caller - module.cpp). Registering
-// a new one before removing the previous replaces it with a warning logged.
-void tab5_keyboard_add_attach_listener(struct Device* device, Tab5KeyboardAttachListener callback, void* context);
-void tab5_keyboard_remove_attach_listener(struct Device* device, Tab5KeyboardAttachListener callback);
-
 extern struct Driver tab5_keyboard_driver;
 
-// Constructs and starts the keyboard accessory device on i2c2, then registers this project's own
-// hot-plug rotation handler as its attach listener. Called from display_detect.cpp's
+// Constructs and starts the keyboard accessory device on i2c2. Called from display_detect.cpp's
 // on_display_detect_event() once i2c2 is up.
 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);
+
 #ifdef __cplusplus
 }
 #endif

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

@@ -0,0 +1,157 @@
+#include "tab5_keyboard_attach_detect.h"
+
+#include "tab5_keyboard.h"
+
+#include <tactility/device.h>
+#include <tactility/log.h>
+
+#include <lvgl/lvgl.h>
+#include <lvgl.h>
+
+#include <freertos/FreeRTOS.h>
+#include <freertos/timers.h>
+
+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 {
+        // 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 2-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) {
+        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;
+}

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

@@ -0,0 +1,12 @@
+#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();

Devices/m5stack-tab5/Source/module.cpp

@@ -13,6 +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_power_control.h"
 #include "devices/tab_5_camera.h"
 
@@ -69,10 +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();
     return ERROR_NONE;
 }
 
 static error_t stop() {
+    tab5_keyboard_attach_detect_stop();
     tab5_headphone_detect_stop();
     device_listener_remove(on_io_expander0_started);
     tab5_detect_stop();