diff --git a/Devices/m5stack-tab5/Source/devices/tab5_headphone_detect.cpp b/Devices/m5stack-tab5/Source/devices/tab5_headphone_detect.cpp index a9355d4d5..d04a23f5a 100644 --- a/Devices/m5stack-tab5/Source/devices/tab5_headphone_detect.cpp +++ b/Devices/m5stack-tab5/Source/devices/tab5_headphone_detect.cpp @@ -1,16 +1,18 @@ #include "tab5_headphone_detect.h" +#include #include #include #include #include +#include #include #include #include -#define TAG "Tab5" +constexpr auto* TAG = "Tab5"; // PI4IOE5V6408-0 (0x43) bit 1 constexpr auto GPIO_EXP0_PIN_SPEAKER_ENABLE = 1; @@ -20,18 +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 }; -static void headphone_detect_callback(TimerHandle_t /*timer*/) { - Device* cached = io_expander0_cached.load(std::memory_order_acquire); - if (!cached) { - cached = device_find_by_name("io_expander0"); - io_expander0_cached.store(cached, std::memory_order_release); +// 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 + } + } } - auto* io_expander0 = cached; if (!io_expander0) { return; // Not ready yet, will retry on next tick } @@ -39,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; } @@ -48,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; } @@ -57,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() { @@ -80,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); } } @@ -97,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"); } @@ -106,5 +193,6 @@ void tab5_headphone_detect_stop() { // Always clear the handle — stale non-null handle is worse than a resource leak, as it would // cause tab5_headphone_detect_start() to silently skip re-creating the timer. hp_detect_timer = nullptr; - io_expander0_cached.store(nullptr, std::memory_order_release); + + cache.setIoExpander0(nullptr); } diff --git a/Documentation/ideas.md b/Documentation/ideas.md index afe5b753f..9aafe7368 100644 --- a/Documentation/ideas.md +++ b/Documentation/ideas.md @@ -13,6 +13,9 @@ ## 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() - Remove and migrate `Include/Tactility/kernel/Kernel.h` into `tactility/delay.h` - Drivers/audio-codec-module is not a module. Move it somewhere else. Or make it an actual module. diff --git a/Drivers/lilygo-module/source/trackball.cpp b/Drivers/lilygo-module/source/trackball.cpp index 268874bb3..4f6a0484a 100644 --- a/Drivers/lilygo-module/source/trackball.cpp +++ b/Drivers/lilygo-module/source/trackball.cpp @@ -76,8 +76,7 @@ lv_indev_t* init() { return g_indev; } - g_device = device_find_first_active_by_type(&TDECK_TRACKBALL_TYPE); - if (g_device == nullptr) { + if (device_get_first_active_by_type(&TDECK_TRACKBALL_TYPE, &g_device) != ERROR_NONE) { LOG_E(TAG, "tdeck_trackball kernel device not found or not started"); return nullptr; } @@ -88,6 +87,7 @@ 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; } @@ -129,6 +129,8 @@ void deinit() { lv_indev_delete(g_indev); g_indev = nullptr; + + device_put(g_device); g_device = nullptr; g_mode = Mode::Encoder; diff --git a/Tactility/Private/Tactility/app/btmanage/BtManagePrivate.h b/Tactility/Private/Tactility/app/btmanage/BtManagePrivate.h index 2e03793ce..c22fe7d0e 100644 --- a/Tactility/Private/Tactility/app/btmanage/BtManagePrivate.h +++ b/Tactility/Private/Tactility/app/btmanage/BtManagePrivate.h @@ -17,7 +17,7 @@ class BtManage final : public App { State state; View view = View(&bindings, &state); bool isViewEnabled = false; - struct Device* btDevice = nullptr; + Device* btDevice = nullptr; public: diff --git a/Tactility/Source/app/boot/Boot.cpp b/Tactility/Source/app/boot/Boot.cpp index c7612b147..a713d6861 100644 --- a/Tactility/Source/app/boot/Boot.cpp +++ b/Tactility/Source/app/boot/Boot.cpp @@ -54,13 +54,8 @@ class BootApp : public App { ); static void setupDisplay() { - auto* display = device_find_first_by_type(&DISPLAY_TYPE); - // Boards not yet migrated to the kernel display driver register a placeholder device (so - // the devicetree node resolves) with a NULL api - nothing for this function to act on. - if (display != nullptr && device_get_driver(display)->api == nullptr) { - display = nullptr; - } - if (display != nullptr) { + Device* display = nullptr; + if (device_get_first_by_type(&DISPLAY_TYPE, &display) == ERROR_NONE) { Device* backlight; if (display_get_backlight(display, &backlight) == ERROR_NONE) { if (!device_is_ready(backlight)) { @@ -83,6 +78,7 @@ class BootApp : public App { } else { LOG_I(TAG, "No backlight for %s", display->name); } + device_put(display); } else { LOG_I(TAG, "No kernel display"); } diff --git a/Tactility/Source/app/btmanage/BtManage.cpp b/Tactility/Source/app/btmanage/BtManage.cpp index 4b4e152c7..33fc08452 100644 --- a/Tactility/Source/app/btmanage/BtManage.cpp +++ b/Tactility/Source/app/btmanage/BtManage.cpp @@ -18,25 +18,38 @@ extern const AppManifest manifest; static void onBtToggled(bool requestOn) { #if defined(CONFIG_BT_NIMBLE_ENABLED) - Device* dev = device_find_first_by_type(&BLUETOOTH_TYPE); - if (!dev) return; - bool radio_on = bluetooth::isRadioOnOrPending(dev); - if (requestOn && !radio_on) { - bluetooth::start(dev); - } else if (!requestOn && radio_on) { - bluetooth::stop(dev); + Device* dev; + if (device_get_first_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { + bool radio_on = bluetooth::isRadioOnOrPending(dev); + if (requestOn && !radio_on) { + LOG_I(TAG, "Turning on"); + bluetooth::start(dev); + } else if (!requestOn && radio_on) { + LOG_I(TAG, "Turning off"); + bluetooth::stop(dev); + } + device_put(dev); + } else { + LOG_W(TAG, "Toggle: No bluetooth device found"); } + #endif } static void onScanToggled(bool enabled) { - Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE); - if (!dev) return; + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) != ERROR_NONE) { + LOG_W(TAG, "Scan: No bluetooth device found"); + return; + } + if (enabled) { bluetooth_scan_start(dev); } else { bluetooth_scan_stop(dev); } + + device_put(dev); } static void onConnectPeer(const std::array& addr, int profileId) { @@ -86,7 +99,7 @@ void BtManage::requestViewUpdate() { unlock(); } -void BtManage::onBtEvent(const struct BtEvent& event) { +void BtManage::onBtEvent(const BtEvent& event) { auto radio_state = bluetooth::getRadioState(); LOG_I(TAG, "Update with state %s", bluetooth::radioStateToString(radio_state)); getState().setRadioState(radio_state); @@ -112,10 +125,13 @@ void BtManage::onBtEvent(const struct BtEvent& event) { case BT_EVENT_RADIO_STATE_CHANGED: if (event.radio_state == BT_RADIO_STATE_ON) { getState().updatePairedPeers(); - Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE); - if (dev && !bluetooth_is_scanning(dev)) { + Device* dev = nullptr; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE && !bluetooth_is_scanning(dev)) { bluetooth_scan_start(dev); } + if (dev) { + device_put(dev); + } } break; default: @@ -141,7 +157,9 @@ void BtManage::onShow(AppContext& app, lv_obj_t* parent) { // Initialise state and view before subscribing to avoid incoming events // racing with state initialisation. state.setRadioState(bluetooth::getRadioState()); - Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE); + Device* dev = nullptr; + device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev); + state.setScanning(dev ? bluetooth_is_scanning(dev) : false); state.updateScanResults(); state.updatePairedPeers(); @@ -152,6 +170,11 @@ void BtManage::onShow(AppContext& app, lv_obj_t* parent) { view.update(); unlock(); + if (btDevice) { + // Decrease refcount before re-ssignment + device_put(btDevice); + } + btDevice = dev; if (btDevice) { bluetooth_add_event_callback(btDevice, this, onKernelBtEvent); @@ -172,6 +195,7 @@ void BtManage::onHide(AppContext& app) { lock(); if (btDevice) { bluetooth_remove_event_callback(btDevice, onKernelBtEvent); + device_put(btDevice); btDevice = nullptr; } isViewEnabled = false; diff --git a/Tactility/Source/app/btmanage/View.cpp b/Tactility/Source/app/btmanage/View.cpp index c0d356fc9..caf997789 100644 --- a/Tactility/Source/app/btmanage/View.cpp +++ b/Tactility/Source/app/btmanage/View.cpp @@ -46,8 +46,12 @@ static void onEnableOnBootParentClicked(lv_event_t* event) { static void onScanButtonClicked(lv_event_t* event) { auto bt = std::static_pointer_cast(getCurrentApp()); - Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE); + 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); + } bt->getBindings().onScanToggled(!scanning); } diff --git a/Tactility/Source/app/btpeersettings/BtPeerSettings.cpp b/Tactility/Source/app/btpeersettings/BtPeerSettings.cpp index 2e769acf2..c78cf7b80 100644 --- a/Tactility/Source/app/btpeersettings/BtPeerSettings.cpp +++ b/Tactility/Source/app/btpeersettings/BtPeerSettings.cpp @@ -121,8 +121,12 @@ public: } void onShow(AppContext& app, lv_obj_t* parent) override { - if (Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE)) { - bluetooth_add_event_callback(dev, this, onKernelBtEvent); + { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { + bluetooth_add_event_callback(dev, this, onKernelBtEvent); + device_put(dev); + } } // Load stored settings (name, autoConnect) @@ -189,8 +193,10 @@ public: } void onHide(AppContext& app) override { - if (Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE)) { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { bluetooth_remove_event_callback(dev, onKernelBtEvent); + device_put(dev); } viewEnabled = false; } diff --git a/Tactility/Source/app/files/View.cpp b/Tactility/Source/app/files/View.cpp index 4beef3a13..eaf6e9ac7 100644 --- a/Tactility/Source/app/files/View.cpp +++ b/Tactility/Source/app/files/View.cpp @@ -436,12 +436,16 @@ void View::onEjectPressed() { std::string mount_path = state->getSelectedChildPath(); LOG_I(TAG, "Ejecting %s", mount_path.c_str()); - struct Device* msc_dev = device_find_first_active_by_type(&USB_HOST_MSC_TYPE); - if (!msc_dev || !usb_msc_eject(msc_dev, mount_path.c_str())) { + 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) + "\"."); } + if (msc_dev) { + device_put(msc_dev); + } + onNavigate(); state->setEntriesForPath(state->getCurrentPath()); update(); diff --git a/Tactility/Source/app/kerneldisplay/KernelDisplay.cpp b/Tactility/Source/app/kerneldisplay/KernelDisplay.cpp index d37ffef37..ff6eca909 100644 --- a/Tactility/Source/app/kerneldisplay/KernelDisplay.cpp +++ b/Tactility/Source/app/kerneldisplay/KernelDisplay.cpp @@ -25,15 +25,18 @@ namespace tt::app::kerneldisplay { constexpr auto* TAG = "KernelDisplay"; static Device* getBacklightDevice() { - Device* display = device_find_first_by_type(&DISPLAY_TYPE); - check(display); + Device* display; + check(device_get_first_by_type(&DISPLAY_TYPE, &display) == ERROR_NONE); // Boards not yet migrated to the kernel display driver register a placeholder device (so the // devicetree node resolves) with a NULL api - nothing for display_get_backlight() to act on. if (device_get_driver(display)->api == nullptr) { + device_put(display); return nullptr; } Device* backlight = nullptr; - return display_get_backlight(display, &backlight) == ERROR_NONE ? backlight : nullptr; + display_get_backlight(display, &backlight); + device_put(display); + return backlight; } class KernelDisplayApp final : public App { diff --git a/Tactility/Source/app/keyboard/KeyboardSettings.cpp b/Tactility/Source/app/keyboard/KeyboardSettings.cpp index 073c4aa83..111341e16 100644 --- a/Tactility/Source/app/keyboard/KeyboardSettings.cpp +++ b/Tactility/Source/app/keyboard/KeyboardSettings.cpp @@ -28,9 +28,10 @@ static uint32_t timeoutMsToIndex(uint32_t ms) { static void applyKeyboardBacklight(bool enabled, uint8_t brightness) { // TODO: Get keyboard backlight from (optional) keyboard child device - Device* backlight = device_find_by_name("keyboard_backlight"); - if (backlight != nullptr) { + Device* backlight; + if (device_get_by_name("keyboard_backlight", &backlight) == ERROR_NONE) { backlight_set_brightness(backlight, enabled ? brightness : 0); + device_put(backlight); } } diff --git a/Tactility/Source/bluetooth/Bluetooth.cpp b/Tactility/Source/bluetooth/Bluetooth.cpp index b3a17c923..b5811c042 100644 --- a/Tactility/Source/bluetooth/Bluetooth.cpp +++ b/Tactility/Source/bluetooth/Bluetooth.cpp @@ -124,8 +124,10 @@ static void bt_event_bridge(Device*, void* /*context*/, BtEvent event) { } if (has_hid_host_auto) { LOG_I(TAG, "HID host auto-connect peer found — starting scan"); - if (Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE)) { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { bluetooth_scan_start(dev); + device_put(dev); } } else if (has_hid_device_auto) { LOG_I(TAG, "HID device auto-start (bonded peer found)"); @@ -231,10 +233,12 @@ static void bt_event_bridge(Device*, void* /*context*/, BtEvent event) { } } if (has_auto) { - if (Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE)) { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { if (!bluetooth_is_scanning(dev)) { bluetooth_scan_start(dev); } + device_put(dev); } } }); @@ -337,10 +341,19 @@ const char* radioStateToString(RadioState state) { } RadioState getRadioState() { - Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE); - if (dev == nullptr) return RadioState::Off; BtRadioState state = BT_RADIO_STATE_OFF; - bluetooth_get_radio_state(dev, &state); + + // Scoped to safeguard dev usage + { + Device* dev = nullptr; + device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev); + if (dev == nullptr) { + return RadioState::Off; + } + bluetooth_get_radio_state(dev, &state); + device_put(dev); + } + switch (state) { case BT_RADIO_STATE_OFF: return RadioState::Off; case BT_RADIO_STATE_ON_PENDING: return RadioState::OnPending; @@ -405,9 +418,10 @@ void pair(const std::array& /*addr*/) { } void unpair(const std::array& addr) { - Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE); - if (dev != nullptr) { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { bluetooth_unpair(dev, addr.data()); + device_put(dev); } settings::remove(settings::addrToHex(addr)); } @@ -442,9 +456,11 @@ void disconnect(const std::array& addr, int profileId) { bluetooth_hid_device_stop(dev); } } else { - Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE); - if (dev == nullptr) return; - bluetooth_disconnect(dev, addr.data(), (BtProfileId)profileId); + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { + bluetooth_disconnect(dev, addr.data(), (BtProfileId)profileId); + device_put(dev); + } } } diff --git a/Tactility/Source/bluetooth/BluetoothHidHost.cpp b/Tactility/Source/bluetooth/BluetoothHidHost.cpp index 07e7eef5b..f900ed6f7 100644 --- a/Tactility/Source/bluetooth/BluetoothHidHost.cpp +++ b/Tactility/Source/bluetooth/BluetoothHidHost.cpp @@ -500,12 +500,14 @@ static void hidHostSubscribeNext(HidHostCtx& ctx) { } device.name = name; settings::save(device); - if (Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE)) { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { BtEvent e = {}; e.type = BT_EVENT_PROFILE_STATE_CHANGED; e.profile_state.state = BT_PROFILE_STATE_CONNECTED; e.profile_state.profile = BT_PROFILE_HID_HOST; bluetooth_fire_event(dev, e); + device_put(dev); } }); return; @@ -660,13 +662,15 @@ static int hidHostGapCb(struct ble_gap_event* event, void* /*arg*/) { } else { LOG_W(TAG, "Connect failed status=%d", event->connect.status); hid_host_ctx.reset(); - if (Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE)) { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { bluetooth_set_hid_host_active(dev, false); - struct BtEvent e = {}; + BtEvent e = {}; e.type = BT_EVENT_PROFILE_STATE_CHANGED; e.profile_state.state = BT_PROFILE_STATE_IDLE; e.profile_state.profile = BT_PROFILE_HID_HOST; bluetooth_fire_event(dev, e); + device_put(dev); } } break; @@ -685,13 +689,15 @@ static int hidHostGapCb(struct ble_gap_event* event, void* /*arg*/) { hid_host_mouse_btn.store(false); hid_host_mouse_active.store(false); - if (Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE)) { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { bluetooth_set_hid_host_active(dev, false); struct BtEvent e = {}; e.type = BT_EVENT_PROFILE_STATE_CHANGED; e.profile_state.state = BT_PROFILE_STATE_IDLE; e.profile_state.profile = BT_PROFILE_HID_HOST; bluetooth_fire_event(dev, e); + device_put(dev); } getMainDispatcher().dispatch([saved_kb, saved_mouse, saved_cursor, saved_queue] { @@ -793,7 +799,13 @@ void hidHostConnect(const std::array& addr) { } // Notify driver that a HID host central connection is starting. - if (Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE)) bluetooth_set_hid_host_active(dev, true); + { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { + bluetooth_set_hid_host_active(dev, true); + device_put(dev); + } + } // Look up the addr_type from the cached scan results. ble_addr_t ble_addr = {}; @@ -813,7 +825,8 @@ void hidHostConnect(const std::array& addr) { if (rc != 0) { LOG_W(TAG, "ble_gap_connect failed rc=%d", rc); hid_host_ctx.reset(); - if (Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE)) { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { bluetooth_set_hid_host_active(dev, false); // Fire IDLE so bt_event_bridge can start a new scan and retry. BtEvent e = {}; @@ -821,6 +834,7 @@ void hidHostConnect(const std::array& addr) { e.profile_state.state = BT_PROFILE_STATE_IDLE; e.profile_state.profile = BT_PROFILE_HID_HOST; bluetooth_fire_event(dev, e); + device_put(dev); } } else { LOG_I(TAG, "Connecting..."); @@ -867,11 +881,13 @@ void autoConnectHidHost() { auto peers = settings::loadAll(); for (const auto& peer : peers) { if (peer.autoConnect && peer.profileId == BT_PROFILE_HID_HOST) { - if (Device* dev = device_find_first_active_by_type(&BLUETOOTH_TYPE)) { + Device* dev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &dev) == ERROR_NONE) { if (!bluetooth_is_scanning(dev)) { LOG_I(TAG, "Auto-connect HID host: device not in scan, retrying scan"); bluetooth_scan_start(dev); } + device_put(dev); } break; } diff --git a/Tactility/Source/lvgl/UsbHidInput.cpp b/Tactility/Source/lvgl/UsbHidInput.cpp index 5f2b6f6d8..3f9bb09f7 100644 --- a/Tactility/Source/lvgl/UsbHidInput.cpp +++ b/Tactility/Source/lvgl/UsbHidInput.cpp @@ -179,8 +179,11 @@ static void usbHidInputTask(void* arg) { UsbHidEvent hid_evt; if (xQueueReceive(ctx->hid_queue, &hid_evt, pdMS_TO_TICKS(100)) != pdTRUE) { if (!ctx->subscribed) { - struct Device* hid_dev = device_find_first_active_by_type(&USB_HOST_HID_TYPE); - if (hid_dev) ctx->subscribed = usb_host_hid_subscribe(hid_dev, ctx->hid_queue); + 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); + } } continue; } @@ -300,14 +303,23 @@ void startUsbHidInput() { return; } - struct Device* hid_dev = device_find_first_active_by_type(&USB_HOST_HID_TYPE); - if (hid_dev) ctx->subscribed = usb_host_hid_subscribe(hid_dev, ctx->hid_queue); + Device* hid_dev = nullptr; + 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); + } ctx->running = true; 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 (hid_dev) usb_host_hid_unsubscribe(hid_dev, ctx->hid_queue); + 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); + } + } vQueueDelete(ctx->hid_queue); vQueueDelete(ctx->key_queue); vSemaphoreDelete(ctx->task_done); @@ -345,8 +357,11 @@ void stopUsbHidInput() { ctx->task = nullptr; if (ctx->subscribed) { - struct Device* hid_dev = device_find_first_active_by_type(&USB_HOST_HID_TYPE); - if (hid_dev) usb_host_hid_unsubscribe(hid_dev, ctx->hid_queue); + Device* hid_dev; + if (device_get_first_active_by_type(&USB_HOST_HID_TYPE, &hid_dev) == ERROR_NONE) { + usb_host_hid_unsubscribe(hid_dev, ctx->hid_queue); + device_put(hid_dev); + } } vQueueDelete(ctx->hid_queue); vQueueDelete(ctx->key_queue); diff --git a/Tactility/Source/service/rtctime/RtcTimeService.cpp b/Tactility/Source/service/rtctime/RtcTimeService.cpp index 554a47e83..44a026ddf 100644 --- a/Tactility/Source/service/rtctime/RtcTimeService.cpp +++ b/Tactility/Source/service/rtctime/RtcTimeService.cpp @@ -19,7 +19,7 @@ constexpr auto* TAG = "RtcTime"; Device* RtcTimeService::findRtcDevice() { if (!rtcDevice) { - rtcDevice = device_find_first_active_by_type(&RTC_TYPE); + device_get_first_active_by_type(&RTC_TYPE, &rtcDevice); } return rtcDevice; } @@ -130,6 +130,11 @@ void RtcTimeService::onStop(ServiceContext& serviceContext) { kernel::unsubscribeSystemEvent(timeEventSubscription); timeEventSubscription = 0; } + + if (rtcDevice) { + device_put(rtcDevice); + rtcDevice = nullptr; + } } extern const ServiceManifest manifest = { diff --git a/Tactility/Source/service/statusbar/Statusbar.cpp b/Tactility/Source/service/statusbar/Statusbar.cpp index ac0f3fa4f..3bbce017a 100644 --- a/Tactility/Source/service/statusbar/Statusbar.cpp +++ b/Tactility/Source/service/statusbar/Statusbar.cpp @@ -166,8 +166,18 @@ class StatusbarService final : public Service { void updateBluetoothIcon() { auto radio_state = bluetooth::getRadioState(); - Device* btdev = device_find_first_active_by_type(&BLUETOOTH_TYPE); - bool scanning = btdev ? bluetooth_is_scanning(btdev) : false; + bool scanning; + + { + Device* btdev; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &btdev) == ERROR_NONE) { + scanning = bluetooth_is_scanning(btdev); + device_put(btdev); + } else { + scanning = false; + } + } + Device* serial_dev = bluetooth_serial_get_device(); Device* midi_dev = bluetooth_midi_get_device(); bool connected = (serial_dev && bluetooth_serial_is_connected(serial_dev)) || @@ -211,11 +221,27 @@ class StatusbarService final : public Service { } } - void updateUsbIcon() { - Device* hid_dev = device_find_first_active_by_type(&USB_HOST_HID_TYPE); - Device* midi_dev = device_find_first_active_by_type(&USB_HOST_MIDI_TYPE); + static bool isHidOrMidiConnected() { + Device* hid_dev = nullptr; + device_get_first_active_by_type(&USB_HOST_HID_TYPE, &hid_dev); + Device* midi_dev = nullptr; + device_get_first_active_by_type(&USB_HOST_MIDI_TYPE, &midi_dev); + bool connected = (hid_dev && usb_host_hid_is_connected(hid_dev)) || (midi_dev && usb_midi_is_connected(midi_dev)); + + if (hid_dev) { + device_put(hid_dev); + } + if (midi_dev) { + device_put(midi_dev); + } + + return connected; + } + + void updateUsbIcon() { + bool connected = isHidOrMidiConnected(); if (!connected) { // MSC: scan filesystems for any mounted /usb* path file_system_for_each(&connected, [](struct FileSystem* fs, void* ctx) -> bool { diff --git a/TactilityKernel/include/tactility/device.h b/TactilityKernel/include/tactility/device.h index ee46d7313..d919d9f23 100644 --- a/TactilityKernel/include/tactility/device.h +++ b/TactilityKernel/include/tactility/device.h @@ -334,45 +334,11 @@ void device_for_each_of_type(const struct DeviceType* type, void* callback_conte bool device_exists_of_type(const struct DeviceType* type) ; /** - * Find a device by its name. - * - * @param[in] name non-null device name to look up - * @return the device pointer if found, or NULL if not found - */ -struct Device* device_find_by_name(const char* name) __attribute__((deprecated("Use device_get_by_name() and device_put()"))); - -/** - * Find the first started device of the given type. - * - * @param[in] type non-null device type pointer - * @return the first started device of the given type, or NULL if none found - */ -struct Device* device_find_first_active_by_type(const struct DeviceType* type) __attribute__((deprecated("Use device_get_first_active_by_type() and device_put()"))); - -/** - * Find the first device of the given type. - * - * @param[in] type non-null device type pointer - * @return the first device of the given type, or NULL if none found - */ -struct Device* device_find_first_by_type(const struct DeviceType* type) __attribute__((deprecated("Use device_get_first_by_type() and device_put()"))); - -/** - * Find the first device whose driver matches the given compatible string. - * - * @param[in] compatible non-null compatible string to match - * @return the first matching device, or NULL if none found - */ -struct Device* device_find_first_by_compatible(const char* compatible) __attribute__((deprecated("Use device_get_first_by_compatible() and device_put()"))); - -/** - * Find a device by name and atomically take a reference on it (equivalent to a device_find_by_name() + * Find a device by name and atomically take a reference on it. * immediately followed by a successful device_get(), but race-free: the lookup and the reference * are taken under the same lock, so a device that gets torn down concurrently either isn't found * or is safely referenced - there is no gap where a caller could be handed a pointer that's about - * to become invalid). Prefer this over device_find_by_name() + device_get() for any device that - * might be dynamically constructed/destructed at runtime (e.g. a hot-pluggable child device), - * rather than a static devicetree-defined one. + * to become invalid). * * @param[in] name non-null device name to look up * @param[out] out_device receives the found device on success; untouched on failure @@ -383,9 +349,7 @@ struct Device* device_find_first_by_compatible(const char* compatible) __attribu error_t device_get_by_name(const char* name, struct Device** out_device); /** - * Find the first device of the given type and atomically take a reference on it. See - * device_get_by_name() for why this is preferred over device_find_first_by_type() + device_get() - * for dynamically constructed/destructed devices. + * Find the first device of the given type and atomically take a reference on it. * * @param[in] type non-null device type pointer * @param[out] out_device receives the found device on success; untouched on failure @@ -396,9 +360,7 @@ error_t device_get_by_name(const char* name, struct Device** out_device); error_t device_get_first_by_type(const struct DeviceType* type, struct Device** out_device); /** - * Find the first started device of the given type and atomically take a reference on it. See - * device_get_by_name() for why this is preferred over device_find_first_active_by_type() + - * device_get() for dynamically constructed/destructed devices. + * Find the first started device of the given type and atomically take a reference on it. * * @param[in] type non-null device type pointer * @param[out] out_device receives the found device on success; untouched on failure @@ -417,9 +379,7 @@ error_t device_get_first_active_by_type(const struct DeviceType* type, struct De bool device_has_active_by_type(const struct DeviceType* type); /** - * Find the first device whose driver matches the given compatible string and atomically take a - * reference on it. See device_get_by_name() for why this is preferred over - * device_find_first_by_compatible() + device_get() for dynamically constructed/destructed devices. + * Find the first device whose driver matches the given compatible string and atomically take a reference on it. * * @param[in] compatible non-null compatible string to match * @param[out] out_device receives the found device on success; untouched on failure diff --git a/TactilityKernel/source/device.cpp b/TactilityKernel/source/device.cpp index eaa6c67af..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; }; @@ -94,6 +92,10 @@ error_t device_destruct(Device* device) { auto* internal = device->internal; + if (internal->ref_count > 0) { + unlock_internal(device->internal); + return ERROR_RESOURCE_BUSY; + } if (internal->state.started || internal->state.added) { unlock_internal(device->internal); return ERROR_INVALID_STATE; @@ -102,13 +104,6 @@ error_t device_destruct(Device* device) { unlock_internal(device->internal); return ERROR_INVALID_STATE; } - // Callers are expected to sequence teardown correctly (device_stop() already refuses to - // clear `started` while ref_count > 0, so by the time !started holds above, ref_count is - // already 0) - this is a cheap defense-in-depth check, not a substitute for that discipline. - if (internal->ref_count > 0) { - unlock_internal(device->internal); - return ERROR_RESOURCE_BUSY; - } LOG_D(TAG, "destruct %s", device->name); device->internal = nullptr; @@ -253,11 +248,6 @@ error_t device_stop(Device* device) { return ERROR_NONE; } - if (internal->ref_count > 0) { - unlock_internal(internal); - return ERROR_RESOURCE_BUSY; - } - // Already stopping on another thread if (internal->state.stopping) { unlock_internal(internal); @@ -266,10 +256,6 @@ error_t device_stop(Device* device) { internal->state.stopping = true; unlock_internal(internal); - // driver_unbind() runs the driver's stop_device callback, which may remove/destruct child - // devices (device_remove() takes ledger_lock) - `mutex` must stay released across this call, - // same reasoning as device_start(). state.stopping keeps device_get() from handing out a new - // ref while ref_count is meant to stay at 0 during the unbind. error_t unbind_error = driver_unbind(internal->driver, device); lock_internal(internal); @@ -393,11 +379,10 @@ bool device_is_constructed(const Device* device) { error_t device_get(Device* device) { auto* internal = device->internal; - lock_internal(internal); - if (!internal->state.started || internal->state.stopping) { - unlock_internal(internal); + if (!internal) { return ERROR_INVALID_STATE; } + lock_internal(internal); internal->ref_count++; unlock_internal(internal); return ERROR_NONE; @@ -466,54 +451,6 @@ bool device_exists_of_type(const DeviceType* type) { return found; } -Device* device_find_by_name(const char* name) { - Device* found = nullptr; - ledger_lock(); - for (auto* device : ledger.devices) { - if (device->name != nullptr && std::strcmp(device->name, name) == 0) { - found = device; - break; - } - } - ledger_unlock(); - return found; -} - -Device* device_find_first_active_by_type(const DeviceType* type) { - Device* found = nullptr; - device_for_each_of_type(type, &found, [](Device* dev, void* ctx) -> bool { - if (device_is_ready(dev)) { - *static_cast(ctx) = dev; - return false; - } - return true; - }); - return found; -} - -Device* device_find_first_by_type(const DeviceType* type) { - Device* found = nullptr; - device_for_each_of_type(type, &found, [](Device* dev, void* ctx) -> bool { - *static_cast(ctx) = dev; - return false; - }); - return found; -} - -Device* device_find_first_by_compatible(const char* compatible) { - struct Ctx { Device* found; const char* compatible; }; - Ctx ctx = { nullptr, compatible }; - device_for_each(&ctx, [](Device* dev, void* raw_ctx) -> bool { - auto* c = static_cast(raw_ctx); - if (device_is_compatible(dev, c->compatible)) { - c->found = dev; - return false; - } - return true; - }); - return ctx.found; -} - error_t device_get_by_name(const char* name, Device** out_device) { ledger_lock(); Device* found = nullptr; diff --git a/TactilityKernel/source/symbols.c b/TactilityKernel/source/symbols.c index 22248b3ca..47875d6ca 100644 --- a/TactilityKernel/source/symbols.c +++ b/TactilityKernel/source/symbols.c @@ -81,10 +81,6 @@ const struct ModuleSymbol KERNEL_SYMBOLS[] = { DEFINE_MODULE_SYMBOL(device_for_each_child), DEFINE_MODULE_SYMBOL(device_for_each_of_type), DEFINE_MODULE_SYMBOL(device_exists_of_type), - DEFINE_MODULE_SYMBOL(device_find_by_name), - DEFINE_MODULE_SYMBOL(device_find_first_active_by_type), - DEFINE_MODULE_SYMBOL(device_find_first_by_type), - DEFINE_MODULE_SYMBOL(device_find_first_by_compatible), DEFINE_MODULE_SYMBOL(device_get), DEFINE_MODULE_SYMBOL(device_put), DEFINE_MODULE_SYMBOL(device_get_by_name), 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);