Fixes

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

@@ -5,6 +5,7 @@
 #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>
@@ -21,19 +22,88 @@ 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*/) {
-    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& 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
+            }
         }
     }
-    auto* io_expander0 = cached;
     if (!io_expander0) {
         return; // Not ready yet, will retry on next tick
     }
@@ -41,6 +111,7 @@ 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;
     }
 
@@ -50,6 +121,7 @@ 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;
     }
 
@@ -59,18 +131,22 @@ 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() {
@@ -82,15 +158,20 @@ 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);
     }
 }
 
@@ -99,6 +180,10 @@ 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");
     }
@@ -109,9 +194,5 @@ void tab5_headphone_detect_stop() {
     // cause tab5_headphone_detect_start() to silently skip re-creating the timer.
     hp_detect_timer = 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);
-    }
+    cache.setIoExpander0(nullptr);
 }

Documentation/ideas.md

@@ -13,6 +13,7 @@
 
 ## 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()

Tactility/Source/app/btmanage/View.cpp

@@ -49,7 +49,9 @@ 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;
-    device_put(dev);
+    if (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;
+    Device* msc_dev = nullptr;
     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) + "\".");

TactilityKernel/source/device.cpp

@@ -43,11 +43,9 @@ struct DeviceInternal {
     } state;
     /** Attached child devices */
     std::vector<Device*> children {};
-    // 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.
+    // 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.
     int32_t ref_count = 0;
 };
 
@@ -382,7 +380,6 @@ 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 fail with ERROR_INVALID_STATE when the device is not started") {
+TEST_CASE("device_get should succeed even 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,13 +40,30 @@ TEST_CASE("device_get should fail with ERROR_INVALID_STATE when the device is no
     device_set_driver(&device, &test_driver);
     CHECK_EQ(device_add(&device), ERROR_NONE);
 
-    CHECK_EQ(device_get(&device), ERROR_INVALID_STATE);
+    // 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(driver_remove_destruct(&test_driver), ERROR_NONE);
+}
+
 TEST_CASE("device_get should succeed once started, and device_put should release it") {
     Device device = { .name = "get_started", .config = nullptr, .parent = nullptr };
 
@@ -65,7 +82,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 fail with ERROR_RESOURCE_BUSY while a reference is held, from a concurrent thread") {
+TEST_CASE("device_stop should succeed while a reference is held, but device_destruct should fail with ERROR_RESOURCE_BUSY until it is released") {
     static Device device = { .name = "get_put_concurrent", .config = nullptr, .parent = nullptr };
     static std::atomic<bool> acquired { false };
     static std::atomic<bool> release { false };
@@ -103,21 +120,22 @@ TEST_CASE("device_stop should fail with ERROR_RESOURCE_BUSY while a reference is
         delay_millis(1);
     }
 
-    // Held by the worker thread right now - device_stop() must fail fast, not block.
-    CHECK_EQ(device_stop(&device), ERROR_RESOURCE_BUSY);
+    // 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);
 
     release = true;
     CHECK_EQ(thread_join(thread, 200, 1), ERROR_NONE);
     thread_free(thread);
 
-    // Reference released - device_stop() now succeeds.
-    CHECK_EQ(device_stop(&device), ERROR_NONE);
-    CHECK_EQ(device_remove(&device), ERROR_NONE);
+    // Reference released - device_destruct() now succeeds.
     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 a started device, or fail if not found/not started") {
+TEST_CASE("device_get_by_name should find and reference an added device regardless of started state, or fail if not found") {
     Device device = { .name = "get_by_name_device", .config = nullptr, .parent = nullptr };
 
     CHECK_EQ(driver_construct_add(&test_driver), ERROR_NONE);
@@ -127,7 +145,11 @@ TEST_CASE("device_get_by_name should find and reference a started device, or fai
 
     Device* out = nullptr;
     CHECK_EQ(device_get_by_name("does_not_exist", &out), ERROR_NOT_FOUND);
-    CHECK_EQ(device_get_by_name("get_by_name_device", &out), ERROR_INVALID_STATE);
+
+    // 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_start(&device), ERROR_NONE);
     CHECK_EQ(device_get_by_name("get_by_name_device", &out), ERROR_NONE);