diff --git a/Devices/m5stack-tab5/Source/devices/tab5_headphone_detect.cpp b/Devices/m5stack-tab5/Source/devices/tab5_headphone_detect.cpp index f50bf7d72..d04a23f5a 100644 --- a/Devices/m5stack-tab5/Source/devices/tab5_headphone_detect.cpp +++ b/Devices/m5stack-tab5/Source/devices/tab5_headphone_detect.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -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 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); } diff --git a/Documentation/ideas.md b/Documentation/ideas.md index c12417b63..9aafe7368 100644 --- a/Documentation/ideas.md +++ b/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() diff --git a/Tactility/Source/app/btmanage/View.cpp b/Tactility/Source/app/btmanage/View.cpp index d54469575..caf997789 100644 --- a/Tactility/Source/app/btmanage/View.cpp +++ b/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); } diff --git a/Tactility/Source/app/files/View.cpp b/Tactility/Source/app/files/View.cpp index 658f67df4..eaf6e9ac7 100644 --- a/Tactility/Source/app/files/View.cpp +++ b/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) + "\"."); diff --git a/TactilityKernel/source/device.cpp b/TactilityKernel/source/device.cpp index 9de3a11e4..baa4af77d 100644 --- a/TactilityKernel/source/device.cpp +++ b/TactilityKernel/source/device.cpp @@ -43,11 +43,9 @@ struct DeviceInternal { } state; /** Attached child devices */ std::vector 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); diff --git a/Tests/TactilityKernel/Source/DeviceGetPutTest.cpp b/Tests/TactilityKernel/Source/DeviceGetPutTest.cpp index 8fa0e1a0a..a837fb61c 100644 --- a/Tests/TactilityKernel/Source/DeviceGetPutTest.cpp +++ b/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 acquired { false }; static std::atomic 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);