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

@@ -5,7 +5,6 @@
 #include <tactility/drivers/gpio.h>
 #include <tactility/drivers/gpio_controller.h>
 #include <tactility/log.h>
-#include <tactility/concurrent/mutex.h>
 
 #include <freertos/FreeRTOS.h>
 #include <freertos/timers.h>
@@ -22,88 +21,19 @@ constexpr auto GPIO_EXP0_PIN_HEADPHONE_DETECT = 7;
 constexpr auto HP_DETECT_POLL_MS = 1000;
 
 static TimerHandle_t hp_detect_timer = nullptr;
+static std::atomic<Device*> io_expander0_cached { nullptr };
 // Flags are written by the timer daemon task
 static std::atomic hp_detect_last { false };
 static std::atomic hp_detect_initialized { false };
 
-// Owns the cached io_expander0 reference.
-// Takes care of refcounting and concurrency.
-struct HeadphoneDetectCache {
-    Mutex mutex {};
-    Device* io_expander0 = nullptr;
-    bool active = false;
-
-    HeadphoneDetectCache() {
-        mutex_construct(&mutex);
-    }
-
-    bool isActive() {
-        mutex_lock(&mutex);
-        bool result = active;
-        mutex_unlock(&mutex);
-        return result;
-    }
-
-    void setActive(bool value) {
-        mutex_lock(&mutex);
-        active = value;
-        mutex_unlock(&mutex);
-    }
-
-    Device* getIoExpander0() {
-        mutex_lock(&mutex);
-        Device* dev = io_expander0;
-        if (dev) {
-            device_get(dev);
-        }
-        mutex_unlock(&mutex);
-        return dev;
-    }
-
-    // Pass nullptr to clear/release the current entry - that always succeeds, regardless of
-    // `active`, since it's what stop() uses to tear the cache down.
-    bool setIoExpander0(Device* dev) {
-        mutex_lock(&mutex);
-        if (dev && !active) {
-            mutex_unlock(&mutex);
-            return false;
-        }
-        Device* old = io_expander0;
-        if (dev) {
-            device_get(dev);
-        }
-        io_expander0 = dev;
-        mutex_unlock(&mutex);
-        if (old) {
-            device_put(old);
-        }
-        return true;
-    }
-};
-
-static HeadphoneDetectCache& headphoneDetectCache() {
-    static HeadphoneDetectCache instance;
-    return instance;
-}
-
 static void headphone_detect_callback(TimerHandle_t /*timer*/) {
-    auto& cache = headphoneDetectCache();
-    if (!cache.isActive()) {
-        return; // Teardown is in progress or done - don't acquire/publish a new reference
-    }
-
-    Device* io_expander0 = cache.getIoExpander0();
-    if (!io_expander0) {
-        Device* dev = nullptr;
-        if (device_get_by_name("io_expander0", &dev) == ERROR_NONE) {
-            if (cache.setIoExpander0(dev)) {
-                device_put(dev); // Cache now holds its own reference
-                io_expander0 = cache.getIoExpander0();
-            } else {
-                io_expander0 = dev; // Deactivated concurrently - use our own reference just this once
-            }
+    Device* cached = io_expander0_cached.load(std::memory_order_acquire);
+    if (!cached) {
+        if (device_get_by_name("io_expander0", &cached) == ERROR_NONE) {
+            io_expander0_cached.store(cached, std::memory_order_release);
         }
     }
+    auto* io_expander0 = cached;
     if (!io_expander0) {
         return; // Not ready yet, will retry on next tick
     }
@@ -111,7 +41,6 @@ static void headphone_detect_callback(TimerHandle_t /*timer*/) {
     auto* hp_pin = gpio_descriptor_acquire(io_expander0, GPIO_EXP0_PIN_HEADPHONE_DETECT, GPIO_FLAG_DIRECTION_INPUT, GPIO_OWNER_GPIO);
     if (!hp_pin) {
         LOG_W(TAG, "hp_detect: HP_DET pin busy");
-        device_put(io_expander0);
         return;
     }
 
@@ -121,7 +50,6 @@ static void headphone_detect_callback(TimerHandle_t /*timer*/) {
 
     if (err != ERROR_NONE) {
         LOG_W(TAG, "hp_detect: HP_DET read error: %s", error_to_string(err));
-        device_put(io_expander0);
         return;
     }
 
@@ -131,22 +59,18 @@ static void headphone_detect_callback(TimerHandle_t /*timer*/) {
         auto* spk_pin = gpio_descriptor_acquire(io_expander0, GPIO_EXP0_PIN_SPEAKER_ENABLE, GPIO_FLAG_DIRECTION_OUTPUT, GPIO_OWNER_GPIO);
         if (!spk_pin) {
             LOG_W(TAG, "hp_detect: SPK_EN pin busy, will retry");
-            device_put(io_expander0);
             return;
         }
         error_t spk_err = gpio_descriptor_set_level(spk_pin, !hp);
         gpio_descriptor_release(spk_pin);
         if (spk_err != ERROR_NONE) {
             LOG_W(TAG, "hp_detect: SPK_EN set error: %s, will retry", error_to_string(spk_err));
-            device_put(io_expander0);
             return;
         }
         hp_detect_last = hp;
         hp_detect_initialized = true;
         LOG_I(TAG, "Headphones %s, speaker %s", hp ? "detected" : "removed", hp ? "disabled" : "enabled");
     }
-
-    device_put(io_expander0);
 }
 
 void tab5_headphone_detect_start() {
@@ -158,20 +82,15 @@ void tab5_headphone_detect_start() {
     hp_detect_initialized = false;
     hp_detect_last = false;
 
-    auto& cache = headphoneDetectCache();
-    cache.setActive(true);
-
     hp_detect_timer = xTimerCreate("hp_detect", pdMS_TO_TICKS(HP_DETECT_POLL_MS), pdTRUE, nullptr, headphone_detect_callback);
     if (!hp_detect_timer) {
         LOG_E(TAG, "Failed to create hp_detect timer");
-        cache.setActive(false);
         return;
     }
     if (xTimerStart(hp_detect_timer, pdMS_TO_TICKS(100)) != pdPASS) {
         LOG_E(TAG, "Failed to start hp_detect timer");
         xTimerDelete(hp_detect_timer, pdMS_TO_TICKS(100));
         hp_detect_timer = nullptr;
-        cache.setActive(false);
     }
 }
 
@@ -180,10 +99,6 @@ void tab5_headphone_detect_stop() {
         return;
     }
 
-    auto& cache = headphoneDetectCache();
-    // Block any callback invocation from this point on from installing a new reference.
-    cache.setActive(false);
-
     if (xTimerStop(hp_detect_timer, pdMS_TO_TICKS(100)) != pdPASS) {
         LOG_W(TAG, "Failed to stop hp_detect timer");
     }
@@ -194,5 +109,9 @@ void tab5_headphone_detect_stop() {
     // cause tab5_headphone_detect_start() to silently skip re-creating the timer.
     hp_detect_timer = nullptr;
 
-    cache.setIoExpander0(nullptr);
+    Device* cached = io_expander0_cached.load(std::memory_order_acquire);
+    if (cached) {
+        io_expander0_cached.store(nullptr, std::memory_order_release);
+        device_put(cached);
+    }
 }

Documentation/ideas.md

@@ -13,7 +13,6 @@
 
 ## Higher Priority
 
-- Bluetooth app: when toggling BT on, it doesn't update the UI with discovered devices. It only works after re-opening the app.
 - display.h API: get_backlight does not change ref counting, but it should
 - bluetooth: various getters for child devices do not change ref counting, but they should
 - Improve kernel_init.cpp (and other modules): create driver_ensure_added() and driver_ensure_destructed()

Drivers/lilygo-module/source/trackball.cpp

@@ -87,7 +87,6 @@ lv_indev_t* init() {
     g_indev = lv_indev_create();
     if (g_indev == nullptr) {
         LOG_E(TAG, "Failed to register LVGL input device");
-        device_put(g_device);
         g_device = nullptr;
         return nullptr;
     }

Tactility/Source/app/btmanage/View.cpp

@@ -49,9 +49,7 @@ static void onScanButtonClicked(lv_event_t* event) {
     Device* dev = nullptr;
     device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev);
     bool scanning = dev ? bluetooth_is_scanning(dev) : false;
-    if (dev) {
-        device_put(dev);
-    }
+    device_put(dev);
     bt->getBindings().onScanToggled(!scanning);
 }
 

Tactility/Source/app/files/View.cpp

@@ -436,7 +436,7 @@ void View::onEjectPressed() {
     std::string mount_path = state->getSelectedChildPath();
     LOG_I(TAG, "Ejecting %s", mount_path.c_str());
 
-    Device* msc_dev = nullptr;
+    Device* msc_dev;
     if (device_get_first_active_by_type(&USB_HOST_MSC_TYPE, &msc_dev) != ERROR_NONE || !usb_msc_eject(msc_dev, mount_path.c_str())) {
         LOG_W(TAG, "usb_msc_eject: %s not found", mount_path.c_str());
         alertdialog::start("Eject failed", "Could not eject \"" + file::getLastPathSegment(mount_path) + "\".");

Tactility/Source/lvgl/UsbHidInput.cpp

@@ -303,7 +303,7 @@ void startUsbHidInput() {
         return;
     }
 
-    Device* hid_dev = nullptr;
+    Device* hid_dev;
     if (device_get_first_active_by_type(&USB_HOST_HID_TYPE, &hid_dev) == ERROR_NONE) {
         ctx->subscribed = usb_host_hid_subscribe(hid_dev, ctx->hid_queue);
         device_put(hid_dev);
@@ -313,13 +313,7 @@ void startUsbHidInput() {
     if (xTaskCreate(usbHidInputTask, "usb_hid_inp", TASK_STACK, ctx, TASK_PRIORITY, &ctx->task) != pdPASS) {
         LOG_E(TAG, "failed to create task");
         ctx->running = false;
-        if (ctx->subscribed) {
-            Device* cleanup_dev = nullptr;
-            if (device_get_first_active_by_type(&USB_HOST_HID_TYPE, &cleanup_dev) == ERROR_NONE) {
-                usb_host_hid_unsubscribe(cleanup_dev, ctx->hid_queue);
-                device_put(cleanup_dev);
-            }
-        }
+        if (hid_dev) usb_host_hid_unsubscribe(hid_dev, ctx->hid_queue);
         vQueueDelete(ctx->hid_queue);
         vQueueDelete(ctx->key_queue);
         vSemaphoreDelete(ctx->task_done);

TactilityKernel/source/device.cpp

@@ -43,9 +43,11 @@ struct DeviceInternal {
     } state;
     /** Attached child devices */
     std::vector<Device*> children {};
-    // 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.
+    // Outstanding device_get() holders. Guarded by `mutex`. device_get() refuses new refs once
+    // state.stopping is set, and device_stop() refuses to set state.stopping while this is > 0 -
+    // together that guarantees ref_count > 0 implies state.started == true, so by the time
+    // device_remove()/device_destruct() run (both already require !started), this is always
+    // already 0.
     int32_t ref_count = 0;
 };
 
@@ -380,6 +382,7 @@ bool device_is_constructed(const Device* device) {
 error_t device_get(Device* device) {
     auto* internal = device->internal;
     if (!internal) {
+        unlock_internal(internal);
         return ERROR_INVALID_STATE;
     }
     lock_internal(internal);

Tests/TactilityKernel/Source/DeviceGetPutTest.cpp

@@ -32,7 +32,7 @@ Driver test_driver = {
 
 } // namespace
 
-TEST_CASE("device_get should succeed even when the device is not started") {
+TEST_CASE("device_get should fail with ERROR_INVALID_STATE when the device is not started") {
     Device device = { .name = "get_not_started", .config = nullptr, .parent = nullptr };
 
     CHECK_EQ(driver_construct_add(&test_driver), ERROR_NONE);
@@ -40,27 +40,10 @@ TEST_CASE("device_get should succeed even when the device is not started") {
     device_set_driver(&device, &test_driver);
     CHECK_EQ(device_add(&device), ERROR_NONE);
 
-    // Ref-counting brackets construct/destruct, not start/stop.
-    CHECK_EQ(device_get(&device), ERROR_NONE);
-    device_put(&device);
-
-    CHECK_EQ(device_remove(&device), ERROR_NONE);
-    CHECK_EQ(device_destruct(&device), ERROR_NONE);
-    CHECK_EQ(driver_remove_destruct(&test_driver), ERROR_NONE);
-}
-
-TEST_CASE("device_get should fail with ERROR_INVALID_STATE once the device has been destructed") {
-    Device device = { .name = "get_after_destruct", .config = nullptr, .parent = nullptr };
-
-    CHECK_EQ(driver_construct_add(&test_driver), ERROR_NONE);
-    CHECK_EQ(device_construct(&device), ERROR_NONE);
-    device_set_driver(&device, &test_driver);
-    CHECK_EQ(device_add(&device), ERROR_NONE);
-    CHECK_EQ(device_remove(&device), ERROR_NONE);
-    CHECK_EQ(device_destruct(&device), ERROR_NONE);
-
     CHECK_EQ(device_get(&device), ERROR_INVALID_STATE);
 
+    CHECK_EQ(device_remove(&device), ERROR_NONE);
+    CHECK_EQ(device_destruct(&device), ERROR_NONE);
     CHECK_EQ(driver_remove_destruct(&test_driver), ERROR_NONE);
 }
 
@@ -82,7 +65,7 @@ TEST_CASE("device_get should succeed once started, and device_put should release
     CHECK_EQ(driver_remove_destruct(&test_driver), ERROR_NONE);
 }
 
-TEST_CASE("device_stop should succeed while a reference is held, but device_destruct should fail with ERROR_RESOURCE_BUSY until it is released") {
+TEST_CASE("device_stop should fail with ERROR_RESOURCE_BUSY while a reference is held, from a concurrent thread") {
     static Device device = { .name = "get_put_concurrent", .config = nullptr, .parent = nullptr };
     static std::atomic<bool> acquired { false };
     static std::atomic<bool> release { false };
@@ -120,22 +103,21 @@ TEST_CASE("device_stop should succeed while a reference is held, but device_dest
         delay_millis(1);
     }
 
-    // Held by the worker thread right now - device_stop() is independent of ref-counting, so it
-    // still succeeds; only device_destruct() gates on outstanding refs.
-    CHECK_EQ(device_stop(&device), ERROR_NONE);
-    CHECK_EQ(device_remove(&device), ERROR_NONE);
-    CHECK_EQ(device_destruct(&device), ERROR_RESOURCE_BUSY);
+    // Held by the worker thread right now - device_stop() must fail fast, not block.
+    CHECK_EQ(device_stop(&device), ERROR_RESOURCE_BUSY);
 
     release = true;
     CHECK_EQ(thread_join(thread, 200, 1), ERROR_NONE);
     thread_free(thread);
 
-    // Reference released - device_destruct() now succeeds.
+    // Reference released - device_stop() now succeeds.
+    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(&test_driver), ERROR_NONE);
 }
 
-TEST_CASE("device_get_by_name should find and reference an added device regardless of started state, or fail if not found") {
+TEST_CASE("device_get_by_name should find and reference a started device, or fail if not found/not started") {
     Device device = { .name = "get_by_name_device", .config = nullptr, .parent = nullptr };
 
     CHECK_EQ(driver_construct_add(&test_driver), ERROR_NONE);
@@ -145,11 +127,7 @@ TEST_CASE("device_get_by_name should find and reference an added device regardle
 
     Device* out = nullptr;
     CHECK_EQ(device_get_by_name("does_not_exist", &out), ERROR_NOT_FOUND);
-
-    // Not started yet - lookup still succeeds, since it only requires the device to be added.
-    CHECK_EQ(device_get_by_name("get_by_name_device", &out), ERROR_NONE);
-    CHECK_EQ(out, &device);
-    device_put(out);
+    CHECK_EQ(device_get_by_name("get_by_name_device", &out), ERROR_INVALID_STATE);
 
     CHECK_EQ(device_start(&device), ERROR_NONE);
     CHECK_EQ(device_get_by_name("get_by_name_device", &out), ERROR_NONE);