diff --git a/Devices/btt-panda-touch/bigtreetech,panda-touch.dts b/Devices/btt-panda-touch/bigtreetech,panda-touch.dts index e6995484d..2bf477185 100644 --- a/Devices/btt-panda-touch/bigtreetech,panda-touch.dts +++ b/Devices/btt-panda-touch/bigtreetech,panda-touch.dts @@ -22,7 +22,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/cyd-4848s040c/cyd,4848s040c.dts b/Devices/cyd-4848s040c/cyd,4848s040c.dts index b8198b5a2..cc7379061 100644 --- a/Devices/cyd-4848s040c/cyd,4848s040c.dts +++ b/Devices/cyd-4848s040c/cyd,4848s040c.dts @@ -22,7 +22,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/cyd-8048s043c/cyd,8048s043c.dts b/Devices/cyd-8048s043c/cyd,8048s043c.dts index eecd8949b..e5712e6db 100644 --- a/Devices/cyd-8048s043c/cyd,8048s043c.dts +++ b/Devices/cyd-8048s043c/cyd,8048s043c.dts @@ -23,7 +23,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/elecrow-crowpanel-advance-28/elecrow,crowpanel-advance-28.dts b/Devices/elecrow-crowpanel-advance-28/elecrow,crowpanel-advance-28.dts index 919c72db3..bf79731b8 100644 --- a/Devices/elecrow-crowpanel-advance-28/elecrow,crowpanel-advance-28.dts +++ b/Devices/elecrow-crowpanel-advance-28/elecrow,crowpanel-advance-28.dts @@ -24,7 +24,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/elecrow-crowpanel-advance-35/elecrow,crowpanel-advance-35.dts b/Devices/elecrow-crowpanel-advance-35/elecrow,crowpanel-advance-35.dts index 810a5209d..5aa682e22 100644 --- a/Devices/elecrow-crowpanel-advance-35/elecrow,crowpanel-advance-35.dts +++ b/Devices/elecrow-crowpanel-advance-35/elecrow,crowpanel-advance-35.dts @@ -24,7 +24,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/elecrow-crowpanel-advance-50/elecrow,crowpanel-advance-50.dts b/Devices/elecrow-crowpanel-advance-50/elecrow,crowpanel-advance-50.dts index a9e168832..8291fb775 100644 --- a/Devices/elecrow-crowpanel-advance-50/elecrow,crowpanel-advance-50.dts +++ b/Devices/elecrow-crowpanel-advance-50/elecrow,crowpanel-advance-50.dts @@ -24,7 +24,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/elecrow-crowpanel-basic-50/elecrow,crowpanel-basic-50.dts b/Devices/elecrow-crowpanel-basic-50/elecrow,crowpanel-basic-50.dts index 085d8ed6c..6cb5bba2c 100644 --- a/Devices/elecrow-crowpanel-basic-50/elecrow,crowpanel-basic-50.dts +++ b/Devices/elecrow-crowpanel-basic-50/elecrow,crowpanel-basic-50.dts @@ -24,7 +24,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/es3c28p/es3c28p.dts b/Devices/es3c28p/es3c28p.dts index 6aca63899..ab0baee69 100644 --- a/Devices/es3c28p/es3c28p.dts +++ b/Devices/es3c28p/es3c28p.dts @@ -28,7 +28,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/guition-jc1060p470ciwy/guition,jc1060p470ciwy.dts b/Devices/guition-jc1060p470ciwy/guition,jc1060p470ciwy.dts index 3bd33fa25..fd1bac776 100644 --- a/Devices/guition-jc1060p470ciwy/guition,jc1060p470ciwy.dts +++ b/Devices/guition-jc1060p470ciwy/guition,jc1060p470ciwy.dts @@ -34,7 +34,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/guition-jc8048w550c/guition,jc8048w550c.dts b/Devices/guition-jc8048w550c/guition,jc8048w550c.dts index df755b847..ffa959dc3 100644 --- a/Devices/guition-jc8048w550c/guition,jc8048w550c.dts +++ b/Devices/guition-jc8048w550c/guition,jc8048w550c.dts @@ -25,7 +25,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/heltec-wifi-lora-32-v3/heltec,wifi-lora-32-v3.dts b/Devices/heltec-wifi-lora-32-v3/heltec,wifi-lora-32-v3.dts index 9d8a983e9..c989662c1 100644 --- a/Devices/heltec-wifi-lora-32-v3/heltec,wifi-lora-32-v3.dts +++ b/Devices/heltec-wifi-lora-32-v3/heltec,wifi-lora-32-v3.dts @@ -22,7 +22,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/lilygo-tdeck-plus/lilygo,tdeck-plus.dts b/Devices/lilygo-tdeck-plus/lilygo,tdeck-plus.dts index 52354d01a..fc97380b2 100644 --- a/Devices/lilygo-tdeck-plus/lilygo,tdeck-plus.dts +++ b/Devices/lilygo-tdeck-plus/lilygo,tdeck-plus.dts @@ -52,7 +52,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/lilygo-tdeck/lilygo,tdeck.dts b/Devices/lilygo-tdeck/lilygo,tdeck.dts index be3d027a1..5819ac1e2 100644 --- a/Devices/lilygo-tdeck/lilygo,tdeck.dts +++ b/Devices/lilygo-tdeck/lilygo,tdeck.dts @@ -50,7 +50,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/lilygo-tdisplay-s3/lilygo,tdisplay-s3.dts b/Devices/lilygo-tdisplay-s3/lilygo,tdisplay-s3.dts index 9f436f89b..86654e4dd 100644 --- a/Devices/lilygo-tdisplay-s3/lilygo,tdisplay-s3.dts +++ b/Devices/lilygo-tdisplay-s3/lilygo,tdisplay-s3.dts @@ -23,7 +23,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/lilygo-tdongle-s3/lilygo,tdongle-s3.dts b/Devices/lilygo-tdongle-s3/lilygo,tdongle-s3.dts index 2c7a7bcd6..02983f122 100644 --- a/Devices/lilygo-tdongle-s3/lilygo,tdongle-s3.dts +++ b/Devices/lilygo-tdongle-s3/lilygo,tdongle-s3.dts @@ -23,7 +23,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/lilygo-thmi/lilygo,thmi.dts b/Devices/lilygo-thmi/lilygo,thmi.dts index fe3d027ce..83bb6c7f8 100644 --- a/Devices/lilygo-thmi/lilygo,thmi.dts +++ b/Devices/lilygo-thmi/lilygo,thmi.dts @@ -27,7 +27,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/lilygo-tlora-pager/lilygo,tlora-pager.dts b/Devices/lilygo-tlora-pager/lilygo,tlora-pager.dts index 553213840..87a4bd8c5 100644 --- a/Devices/lilygo-tlora-pager/lilygo,tlora-pager.dts +++ b/Devices/lilygo-tlora-pager/lilygo,tlora-pager.dts @@ -31,7 +31,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/m5stack-cardputer-adv/m5stack,cardputer-adv.dts b/Devices/m5stack-cardputer-adv/m5stack,cardputer-adv.dts index 9306ff6f6..421feebf9 100644 --- a/Devices/m5stack-cardputer-adv/m5stack,cardputer-adv.dts +++ b/Devices/m5stack-cardputer-adv/m5stack,cardputer-adv.dts @@ -31,7 +31,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/m5stack-cardputer/m5stack,cardputer.dts b/Devices/m5stack-cardputer/m5stack,cardputer.dts index bf97ffe67..4f59d58b7 100644 --- a/Devices/m5stack-cardputer/m5stack,cardputer.dts +++ b/Devices/m5stack-cardputer/m5stack,cardputer.dts @@ -32,7 +32,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/m5stack-cores3/m5stack,cores3.dts b/Devices/m5stack-cores3/m5stack,cores3.dts index 0b6c098ef..225a822be 100644 --- a/Devices/m5stack-cores3/m5stack,cores3.dts +++ b/Devices/m5stack-cores3/m5stack,cores3.dts @@ -35,7 +35,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/m5stack-papers3/m5stack,papers3.dts b/Devices/m5stack-papers3/m5stack,papers3.dts index b51fa9d73..ad3d56b96 100644 --- a/Devices/m5stack-papers3/m5stack,papers3.dts +++ b/Devices/m5stack-papers3/m5stack,papers3.dts @@ -27,7 +27,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/m5stack-stackchan/m5stack,stackchan.dts b/Devices/m5stack-stackchan/m5stack,stackchan.dts index 078d491cc..bfc3a0e05 100644 --- a/Devices/m5stack-stackchan/m5stack,stackchan.dts +++ b/Devices/m5stack-stackchan/m5stack,stackchan.dts @@ -35,7 +35,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/m5stack-sticks3/m5stack,sticks3.dts b/Devices/m5stack-sticks3/m5stack,sticks3.dts index 139d12f9e..23e616f3c 100644 --- a/Devices/m5stack-sticks3/m5stack,sticks3.dts +++ b/Devices/m5stack-sticks3/m5stack,sticks3.dts @@ -28,7 +28,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/m5stack-tab5/m5stack,tab5.dts b/Devices/m5stack-tab5/m5stack,tab5.dts index 25e9c467f..0e9084272 100644 --- a/Devices/m5stack-tab5/m5stack,tab5.dts +++ b/Devices/m5stack-tab5/m5stack,tab5.dts @@ -33,7 +33,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/tulip-4r11/tulip-4r11.dts b/Devices/tulip-4r11/tulip-4r11.dts index 2781ce45c..f21e1d2d7 100644 --- a/Devices/tulip-4r11/tulip-4r11.dts +++ b/Devices/tulip-4r11/tulip-4r11.dts @@ -28,7 +28,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/unphone/unphone.dts b/Devices/unphone/unphone.dts index f98fdd8b8..9f044220f 100644 --- a/Devices/unphone/unphone.dts +++ b/Devices/unphone/unphone.dts @@ -26,7 +26,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/waveshare-esp32-s3-geek/waveshare,esp32-s3-geek.dts b/Devices/waveshare-esp32-s3-geek/waveshare,esp32-s3-geek.dts index da0a0ba5b..58ac91a0b 100644 --- a/Devices/waveshare-esp32-s3-geek/waveshare,esp32-s3-geek.dts +++ b/Devices/waveshare-esp32-s3-geek/waveshare,esp32-s3-geek.dts @@ -25,7 +25,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/waveshare-s3-lcd-13/waveshare,s3-lcd-13.dts b/Devices/waveshare-s3-lcd-13/waveshare,s3-lcd-13.dts index f59ba1bca..656a50cc5 100644 --- a/Devices/waveshare-s3-lcd-13/waveshare,s3-lcd-13.dts +++ b/Devices/waveshare-s3-lcd-13/waveshare,s3-lcd-13.dts @@ -24,7 +24,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/waveshare-s3-touch-lcd-128/waveshare,s3-touch-lcd-128.dts b/Devices/waveshare-s3-touch-lcd-128/waveshare,s3-touch-lcd-128.dts index 50dc811e6..543d5e793 100644 --- a/Devices/waveshare-s3-touch-lcd-128/waveshare,s3-touch-lcd-128.dts +++ b/Devices/waveshare-s3-touch-lcd-128/waveshare,s3-touch-lcd-128.dts @@ -25,7 +25,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/waveshare-s3-touch-lcd-147/waveshare,s3-touch-lcd-147.dts b/Devices/waveshare-s3-touch-lcd-147/waveshare,s3-touch-lcd-147.dts index 32ea31df8..728305bd9 100644 --- a/Devices/waveshare-s3-touch-lcd-147/waveshare,s3-touch-lcd-147.dts +++ b/Devices/waveshare-s3-touch-lcd-147/waveshare,s3-touch-lcd-147.dts @@ -25,7 +25,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dts b/Devices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dts index 44c66423a..f1119af52 100644 --- a/Devices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dts +++ b/Devices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dts @@ -24,7 +24,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Devices/wireless-tag-wt32-sc01-plus/wireless-tag,wt32-sc01-plus.dts b/Devices/wireless-tag-wt32-sc01-plus/wireless-tag,wt32-sc01-plus.dts index 0fb9663a4..b12d9a315 100644 --- a/Devices/wireless-tag-wt32-sc01-plus/wireless-tag,wt32-sc01-plus.dts +++ b/Devices/wireless-tag-wt32-sc01-plus/wireless-tag,wt32-sc01-plus.dts @@ -24,7 +24,6 @@ ble0 { compatible = "espressif,esp32-ble"; - status = "disabled"; }; gpio0 { diff --git a/Documentation/ideas.md b/Documentation/ideas.md index e004d7911..02aa079e6 100644 --- a/Documentation/ideas.md +++ b/Documentation/ideas.md @@ -28,7 +28,7 @@ - Improve Setup: Show "Step done" screen - Improve Setup: Add keyboard/keypad navigation explanation - 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 +- bluetooth: various getters for child devices do not change ref counting, but they should (e.g. bluetooth_hid_device_get_device()) - Improve kernel_init.cpp (and other modules): create driver_ensure_added() and driver_ensure_destructed() - Drivers/audio-codec-module is not a module. Move it somewhere else. Or make it an actual module. - LilyGO T-Dongle S3: 1 button control, stop auto-launching web server diff --git a/Platforms/platform-esp32/private/bluetooth/esp32_ble_internal.h b/Platforms/platform-esp32/private/bluetooth/esp32_ble_internal.h index 21c45f988..09520de68 100644 --- a/Platforms/platform-esp32/private/bluetooth/esp32_ble_internal.h +++ b/Platforms/platform-esp32/private/bluetooth/esp32_ble_internal.h @@ -30,17 +30,16 @@ #include #include -#define BLE_MAX_CALLBACKS 8 - -struct BleCallbackEntry { - BtEventCallback fn; - void* ctx; -}; - struct BleCtx { // Mutexes SemaphoreHandle_t radio_mutex; // guards radio state transitions - SemaphoreHandle_t cb_mutex; // guards callbacks array + // Guards GAP/GATT advertising & profile-switch calls (on_sync()'s advertising decision vs. + // hid/spp/midi's _start()/_stop()). Deliberately separate from radio_mutex: radio_mutex can + // be held for the duration of nimble_port_stop(), which blocks waiting for the NimBLE host + // task to exit its run loop - taking that same mutex from code that can run ON the host task + // (on_sync()) would deadlock disable against it. Nothing held under gap_mutex ever waits on + // host-task progress, so it's safe to take from the host task itself. + SemaphoreHandle_t gap_mutex; // Radio / scan state (atomic — read from multiple tasks) std::atomic radio_state; @@ -48,9 +47,9 @@ struct BleCtx { // Set by Tactility HID host to prevent simultaneous central connection during name resolution std::atomic hid_host_active; - // Event callbacks (guarded by cb_mutex) - BleCallbackEntry callbacks[BLE_MAX_CALLBACKS]; - size_t callback_count; + // Event subscriptions (guarded by subscriptionsMutex) + Mutex subscriptionsMutex; + BtEventSubscription* subscriptions; // Connection handles + active flags (atomic — accessed from multiple tasks) std::atomic spp_conn_handle; @@ -98,7 +97,7 @@ struct BleCtx { ble_addr_t scan_addrs[64]; size_t scan_count; - // Device reference (passed to BtEventCallback) + // Device reference (passed to subscribers via BtEvent delivery) struct Device* device; // Child devices (created by esp32_ble_start_device, destroyed by stop_device) diff --git a/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp b/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp index 60e07f92d..8a30aa86a 100644 --- a/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp +++ b/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble.cpp @@ -96,34 +96,47 @@ void ble_set_scan_active(struct Device* device, bool v) { void ble_publish_event(struct Device* device, struct BtEvent event) { BleCtx* ctx = ble_get_ctx(device); if (!ctx) return; - // Copy under mutex so callbacks can safely call add/remove_event_callback - BleCallbackEntry local[BLE_MAX_CALLBACKS]; - size_t count; - xSemaphoreTake(ctx->cb_mutex, portMAX_DELAY); - count = ctx->callback_count; - memcpy(local, ctx->callbacks, count * sizeof(BleCallbackEntry)); - xSemaphoreGive(ctx->cb_mutex); - for (size_t i = 0; i < count; i++) { - local[i].fn(ctx->device, local[i].ctx, event); + mutex_lock(&ctx->subscriptionsMutex); + for (BtEventSubscription* sub = ctx->subscriptions; sub != nullptr; sub = sub->internal.next) { + mutex_lock(&sub->internal.ring_mutex); + if (sub->internal.count < BT_EVENT_QUEUE_CAPACITY) { + uint8_t tail = (sub->internal.head + sub->internal.count) % BT_EVENT_QUEUE_CAPACITY; + sub->internal.queue[tail] = event; + sub->internal.count++; + } + mutex_unlock(&sub->internal.ring_mutex); + task_event_group_signal(sub->internal.event_group, sub->bit); } + mutex_unlock(&ctx->subscriptionsMutex); } // ---- Advertising restart helper ---- //TODO: Fix Restart name-only advertising after a rename. + +// Delay before on_sync() falls back to name-only advertising - see its call site. +constexpr uint64_t ADV_ON_SYNC_DELAY_US = 100'000; + static void adv_restart_callback(void* arg) { struct Device* device = (struct Device*)arg; BleCtx* ctx = ble_get_ctx(device); if (ctx->radio_state.load() != BT_RADIO_STATE_ON) return; + // gap_mutex (not radio_mutex - this can run on the host task and must never wait on a mutex + // disable can hold across nimble_port_stop()) serializes this against hid/spp/midi_device_ + // start()/_stop(), which touch the same GAP/GATT state from another task. + xSemaphoreTake(ctx->gap_mutex, portMAX_DELAY); uint16_t hid_conn = ble_hid_get_conn_handle(ctx->hid_device_child); - if (ctx->midi_active.load() && ctx->midi_conn_handle.load() == BLE_HS_CONN_HANDLE_NONE) { ble_start_advertising(device, &MIDI_SVC_UUID); } else if (ctx->spp_active.load() && ctx->spp_conn_handle.load() == BLE_HS_CONN_HANDLE_NONE) { ble_start_advertising(device, &NUS_SVC_UUID); } else if (ctx->hid_active.load() && hid_conn == BLE_HS_CONN_HANDLE_NONE) { ble_start_advertising_hid(device, hid_appearance); + } else if (!ctx->hid_active.load() && !ctx->spp_active.load() && !ctx->midi_active.load()) { + // Nothing profile-specific active - name-only keeps the radio discoverable/connectable. + ble_start_advertising(device, nullptr); } + xSemaphoreGive(ctx->gap_mutex); } void ble_schedule_adv_restart(struct Device* device, uint64_t delay_us) { @@ -441,17 +454,13 @@ static void on_sync() { e.radio_state = BT_RADIO_STATE_ON; ble_publish_event(ctx->device, e); - // The Tactility bridge handles auto-start (SPP/MIDI/HID) in response to - // BT_EVENT_RADIO_STATE_CHANGED(ON), dispatched to the main task above. - // On a multi-core ESP32 the main task can preempt the NimBLE host task - // between ble_publish_event() and here, call hid/spp/midi_device_start(), - // set hid/spp/midi_active, and already start profile-specific advertising. - // Only start name-only advertising if no profile has been activated yet; - // otherwise we would overwrite the correct profile advertising with name-only, - // causing Windows to connect without seeing the HID/SPP/MIDI service UUID. - if (!ctx->hid_active.load() && !ctx->spp_active.load() && !ctx->midi_active.load()) { - ble_start_advertising(ctx->device, nullptr); - } + // Deferred, not synchronous: an app reacting to the event just published above (e.g. + // starting HID) races this to set hid/spp/midi_active, and immediately starting name-only + // advertising only to tear it down again a few ms later for a profile switch was observed to + // corrupt the NimBLE host's HCI event pool (crash in os_memblock_get, called from + // lld_adv_end_ind_handler_hack). The delay gives that app time to react first; + // adv_restart_callback() re-checks the active flags, so this is a no-op if one did. + ble_schedule_adv_restart(ctx->device, ADV_ON_SYNC_DELAY_US); } static void dispatch_disable_timer_cb(void* arg) { @@ -995,38 +1004,54 @@ static error_t api_disconnect(struct Device* device, const BtAddr addr, enum BtP return ERROR_NOT_SUPPORTED; } -static error_t api_add_event_callback(struct Device* device, void* cb_ctx, BtEventCallback fn) { +static error_t api_event_subscribe(struct Device* device, BtEventSubscription* sub, TaskEventGroup* event_group) { BleCtx* ctx = (BleCtx*)device_get_driver_data(device); - if (!ctx || !fn) return ERROR_INVALID_ARGUMENT; - xSemaphoreTake(ctx->cb_mutex, portMAX_DELAY); - if (ctx->callback_count >= BLE_MAX_CALLBACKS) { - xSemaphoreGive(ctx->cb_mutex); - return ERROR_OUT_OF_MEMORY; + if (!ctx || !sub || !event_group) return ERROR_INVALID_ARGUMENT; + + uint32_t bit; + error_t claim_result = task_event_group_claim_bit(event_group, &bit); + if (claim_result != ERROR_NONE) { + return claim_result; + } + + mutex_lock(&ctx->subscriptionsMutex); + + // Avoid cyclic subscription list that would loop forever + for (BtEventSubscription* existing = ctx->subscriptions; existing != nullptr; existing = existing->internal.next) { + if (existing == sub) { + mutex_unlock(&ctx->subscriptionsMutex); + task_event_group_release_bit(event_group, bit); + return ERROR_INVALID_STATE; + } } - ctx->callbacks[ctx->callback_count].fn = fn; - ctx->callbacks[ctx->callback_count].ctx = cb_ctx; - ctx->callback_count++; - xSemaphoreGive(ctx->cb_mutex); + + sub->internal.event_group = event_group; + sub->bit = bit; + sub->internal.next = ctx->subscriptions; + ctx->subscriptions = sub; + mutex_unlock(&ctx->subscriptionsMutex); return ERROR_NONE; } -static error_t api_remove_event_callback(struct Device* device, BtEventCallback fn) { +static error_t api_event_unsubscribe(struct Device* device, BtEventSubscription* sub) { BleCtx* ctx = (BleCtx*)device_get_driver_data(device); - if (!ctx || !fn) return ERROR_INVALID_ARGUMENT; - xSemaphoreTake(ctx->cb_mutex, portMAX_DELAY); - for (size_t i = 0; i < ctx->callback_count; i++) { - if (ctx->callbacks[i].fn == fn) { - // Shift remaining entries down - for (size_t j = i; j + 1 < ctx->callback_count; j++) { - ctx->callbacks[j] = ctx->callbacks[j + 1]; - } - ctx->callback_count--; - xSemaphoreGive(ctx->cb_mutex); - return ERROR_NONE; + if (!ctx || !sub) return ERROR_INVALID_ARGUMENT; + + error_t result = ERROR_NOT_FOUND; + mutex_lock(&ctx->subscriptionsMutex); + for (BtEventSubscription** link = &ctx->subscriptions; *link != nullptr; link = &(*link)->internal.next) { + if (*link == sub) { + *link = sub->internal.next; + result = ERROR_NONE; + break; } } - xSemaphoreGive(ctx->cb_mutex); - return ERROR_NOT_FOUND; + mutex_unlock(&ctx->subscriptionsMutex); + + if (result == ERROR_NONE) { + task_event_group_release_bit(sub->internal.event_group, sub->bit); + } + return result; } static error_t api_set_device_name(struct Device* device, const char* name) { @@ -1079,8 +1104,8 @@ const BluetoothApi nimble_bluetooth_api = { .get_paired_peers = api_get_paired_peers, .connect = api_connect, .disconnect = api_disconnect, - .add_event_callback = api_add_event_callback, - .remove_event_callback = api_remove_event_callback, + .event_subscribe = api_event_subscribe, + .event_unsubscribe = api_event_unsubscribe, .set_device_name = api_set_device_name, .get_device_name = api_get_device_name, .set_hid_host_active = api_set_hid_host_active, @@ -1116,11 +1141,12 @@ static void destroy_child_device(struct Device*& child) { static error_t esp32_ble_start_device(struct Device* device) { BleCtx* ctx = new BleCtx(); ctx->radio_mutex = xSemaphoreCreateMutex(); - ctx->cb_mutex = xSemaphoreCreateMutex(); + ctx->gap_mutex = xSemaphoreCreateMutex(); + mutex_construct(&ctx->subscriptionsMutex); + ctx->subscriptions = nullptr; ctx->radio_state.store(BT_RADIO_STATE_OFF); ctx->scan_active.store(false); ctx->hid_host_active.store(false); - ctx->callback_count = 0; ctx->spp_conn_handle.store(BLE_HS_CONN_HANDLE_NONE); ctx->spp_active.store(false); ctx->midi_conn_handle.store(BLE_HS_CONN_HANDLE_NONE); @@ -1206,6 +1232,29 @@ static error_t esp32_ble_stop_device(struct Device* device) { ctx->disable_timer = nullptr; } + if (ctx->gap_mutex != nullptr) { + vSemaphoreDelete(ctx->gap_mutex); + ctx->gap_mutex = nullptr; + } + + // Release any subscribers that never unsubscribed: device_stop() doesn't wait for apps still + // using this device, so a later bluetooth_event_unsubscribe() would find no ctx and skip + // releasing the bit. + mutex_lock(&ctx->subscriptionsMutex); + for (BtEventSubscription* sub = ctx->subscriptions; sub != nullptr;) { + BtEventSubscription* next = sub->internal.next; + // Force-close, don't destruct: a concurrent bluetooth_event_poll() may hold/await this + // same lock (see BtEventSubscription::internal::closed). + mutex_lock(&sub->internal.ring_mutex); + sub->internal.closed = true; + mutex_unlock(&sub->internal.ring_mutex); + task_event_group_release_bit(sub->internal.event_group, sub->bit); + sub = next; + } + ctx->subscriptions = nullptr; + mutex_unlock(&ctx->subscriptionsMutex); + mutex_destruct(&ctx->subscriptionsMutex); + s_ctx = nullptr; device_set_driver_data(device, nullptr); delete ctx; diff --git a/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cpp b/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cpp index 15d7b8604..aa4f1ac6d 100644 --- a/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cpp +++ b/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_hid.cpp @@ -388,17 +388,27 @@ static error_t hid_device_start(struct Device* device, enum BtHidDeviceMode mode } hid_appearance = appearance; - if (!ble_hid_switch_profile(device, profile)) { + // gap_mutex (not radio_mutex - see BleCtx::gap_mutex's comment) serializes this GAP/GATT + // sequence against on_sync()'s own advertising decision and spp/midi's start/stop. + BleCtx* root_ctx = ble_get_ctx(device); + xSemaphoreTake(root_ctx->gap_mutex, portMAX_DELAY); + bool switched = ble_hid_switch_profile(device, profile); + if (switched) { + ble_hid_set_active(device, true); + ble_start_advertising_hid(device, hid_appearance); + } + xSemaphoreGive(root_ctx->gap_mutex); + if (!switched) { delete (BleHidDeviceCtx*)device_get_driver_data(device); device_set_driver_data(device, nullptr); return ERROR_INVALID_STATE; } - ble_hid_set_active(device, true); - ble_start_advertising_hid(device, hid_appearance); return ERROR_NONE; } static error_t hid_device_stop(struct Device* device) { + BleCtx* root_ctx = ble_get_ctx(device); + xSemaphoreTake(root_ctx->gap_mutex, portMAX_DELAY); ble_hid_set_active(device, false); ble_gap_adv_stop(); BleHidDeviceCtx* hid_ctx = (BleHidDeviceCtx*)device_get_driver_data(device); @@ -421,6 +431,7 @@ static error_t hid_device_stop(struct Device* device) { delete hid_ctx; device_set_driver_data(device, nullptr); } + xSemaphoreGive(root_ctx->gap_mutex); return ERROR_NONE; } diff --git a/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_midi.cpp b/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_midi.cpp index 59dfbd8af..67cfcd4a7 100644 --- a/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_midi.cpp +++ b/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_midi.cpp @@ -167,13 +167,19 @@ static void midi_keepalive_cb(void* arg) { // All functions receive the midi child Device*. static error_t midi_start(struct Device* device) { + // gap_mutex (not radio_mutex - see BleCtx::gap_mutex's comment) serializes this GAP sequence + // against on_sync()'s own advertising decision and hid/spp's start/stop. + BleCtx* root_ctx = ble_get_ctx(device); + xSemaphoreTake(root_ctx->gap_mutex, portMAX_DELAY); ble_midi_set_active(device, true); // Create 2-second periodic Active Sensing timer to prevent Windows BLE MIDI // driver from declaring the connection idle and disconnecting (~8-10 s timeout). error_t rc = ble_midi_ensure_keepalive(device, midi_keepalive_cb, 2'000'000); - if (rc != ERROR_NONE) return rc; - ble_start_advertising(device, &MIDI_SVC_UUID); - return ERROR_NONE; + if (rc == ERROR_NONE) { + ble_start_advertising(device, &MIDI_SVC_UUID); + } + xSemaphoreGive(root_ctx->gap_mutex); + return rc; } error_t ble_midi_start_internal(struct Device* midi_child) { @@ -181,6 +187,8 @@ error_t ble_midi_start_internal(struct Device* midi_child) { } static error_t midi_stop(struct Device* device) { + BleCtx* root_ctx = ble_get_ctx(device); + xSemaphoreTake(root_ctx->gap_mutex, portMAX_DELAY); ble_midi_set_active(device, false); ble_midi_stop_keepalive(device); uint16_t conn = ble_midi_get_conn_handle(device); @@ -191,6 +199,7 @@ static error_t midi_stop(struct Device* device) { if (!ble_spp_get_active(device) && !ble_hid_get_active(device)) { ble_gap_adv_stop(); } + xSemaphoreGive(root_ctx->gap_mutex); return ERROR_NONE; } diff --git a/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_spp.cpp b/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_spp.cpp index ce948b3ef..a3ee4fcd2 100644 --- a/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_spp.cpp +++ b/Platforms/platform-esp32/source/drivers/bluetooth/esp32_ble_spp.cpp @@ -126,8 +126,13 @@ void ble_spp_set_conn_handle(struct Device* device, uint16_t h) { // All functions receive the serial child Device*. static error_t spp_start(struct Device* device) { + // gap_mutex (not radio_mutex - see BleCtx::gap_mutex's comment) serializes this GAP sequence + // against on_sync()'s own advertising decision and hid/midi's start/stop. + BleCtx* root_ctx = ble_get_ctx(device); + xSemaphoreTake(root_ctx->gap_mutex, portMAX_DELAY); ble_spp_set_active(device, true); ble_start_advertising(device, &NUS_SVC_UUID); + xSemaphoreGive(root_ctx->gap_mutex); return ERROR_NONE; } @@ -136,6 +141,8 @@ error_t ble_spp_start_internal(struct Device* serial_child) { } static error_t spp_stop(struct Device* device) { + BleCtx* root_ctx = ble_get_ctx(device); + xSemaphoreTake(root_ctx->gap_mutex, portMAX_DELAY); ble_spp_set_active(device, false); uint16_t conn = ble_spp_get_conn_handle(device); if (conn != BLE_HS_CONN_HANDLE_NONE) { @@ -147,6 +154,7 @@ static error_t spp_stop(struct Device* device) { if (!ble_midi_get_active(device) && !ble_hid_get_active(device)) { ble_gap_adv_stop(); } + xSemaphoreGive(root_ctx->gap_mutex); return ERROR_NONE; } diff --git a/Platforms/platform-esp32/source/drivers/esp32_wifi.cpp b/Platforms/platform-esp32/source/drivers/esp32_wifi.cpp index afae6d75a..248bfa69a 100644 --- a/Platforms/platform-esp32/source/drivers/esp32_wifi.cpp +++ b/Platforms/platform-esp32/source/drivers/esp32_wifi.cpp @@ -542,10 +542,12 @@ error_t api_event_subscribe(Device* device, WifiEventSubscription* sub, TaskEven mutex_lock(&ctx->subscriptionsMutex); // Avoid cyclic subscription list that would loop forever - if (ctx->subscriptions == sub) { - mutex_unlock(&ctx->subscriptionsMutex); - task_event_group_release_bit(event_group, bit); - return ERROR_INVALID_STATE; + for (WifiEventSubscription* existing = ctx->subscriptions; existing != nullptr; existing = existing->internal.next) { + if (existing == sub) { + mutex_unlock(&ctx->subscriptionsMutex); + task_event_group_release_bit(event_group, bit); + return ERROR_INVALID_STATE; + } } sub->internal.event_group = event_group; @@ -639,12 +641,16 @@ error_t stop_device(Device* device) { // Release any subscribers that never unsubscribed: device_stop() doesn't wait for apps still // using this device, so a later wifi_event_unsubscribe() would find no ctx and skip releasing - // the bit and destructing sub->internal.ring_mutex. + // the bit. mutex_lock(&ctx->subscriptionsMutex); for (WifiEventSubscription* sub = ctx->subscriptions; sub != nullptr;) { WifiEventSubscription* next = sub->internal.next; + // Force-close, don't destruct: a concurrent wifi_event_poll() may hold/await this same + // lock (see WifiEventSubscription::internal::closed). + mutex_lock(&sub->internal.ring_mutex); + sub->internal.closed = true; + mutex_unlock(&sub->internal.ring_mutex); task_event_group_release_bit(sub->internal.event_group, sub->bit); - mutex_destruct(&sub->internal.ring_mutex); sub = next; } ctx->subscriptions = nullptr; diff --git a/Platforms/platform-posix/source/drivers/mock_wifi.cpp b/Platforms/platform-posix/source/drivers/mock_wifi.cpp index 42c85e4dc..92e40589b 100644 --- a/Platforms/platform-posix/source/drivers/mock_wifi.cpp +++ b/Platforms/platform-posix/source/drivers/mock_wifi.cpp @@ -302,10 +302,12 @@ error_t apiEventSubscribe(Device* device, WifiEventSubscription* sub, TaskEventG mutex_lock(&ctx->subscriptionsMutex); // Avoid cyclic subscription list that would loop forever - if (ctx->subscriptions == sub) { - mutex_unlock(&ctx->subscriptionsMutex); - task_event_group_release_bit(event_group, bit); - return ERROR_INVALID_STATE; + for (WifiEventSubscription* existing = ctx->subscriptions; existing != nullptr; existing = existing->internal.next) { + if (existing == sub) { + mutex_unlock(&ctx->subscriptionsMutex); + task_event_group_release_bit(event_group, bit); + return ERROR_INVALID_STATE; + } } sub->internal.event_group = event_group; @@ -382,12 +384,16 @@ error_t stopDevice(Device* device) { // Release any subscribers that never unsubscribed: device_stop() doesn't wait for apps still // using this device, so a later wifi_event_unsubscribe() would find no ctx and skip releasing - // the bit and destructing sub->internal.ring_mutex. + // the bit. mutex_lock(&ctx->subscriptionsMutex); for (WifiEventSubscription* sub = ctx->subscriptions; sub != nullptr;) { WifiEventSubscription* next = sub->internal.next; + // Force-close, don't destruct: a concurrent wifi_event_poll() may hold/await this same + // lock (see WifiEventSubscription::internal::closed). + mutex_lock(&sub->internal.ring_mutex); + sub->internal.closed = true; + mutex_unlock(&sub->internal.ring_mutex); task_event_group_release_bit(sub->internal.event_group, sub->bit); - mutex_destruct(&sub->internal.ring_mutex); sub = next; } ctx->subscriptions = nullptr; diff --git a/Tactility/Include/Tactility/bluetooth/Bluetooth.h b/Tactility/Include/Tactility/bluetooth/Bluetooth.h index 4d01357d7..e68d85807 100644 --- a/Tactility/Include/Tactility/bluetooth/Bluetooth.h +++ b/Tactility/Include/Tactility/bluetooth/Bluetooth.h @@ -26,10 +26,10 @@ struct PeerRecord { int profileId = 0; }; -// Wrapper around device start & radio on +// Turns the BT radio on (dev is already started - see ble0's devicetree status) bool start(Device* dev); -// Wrapper around device stop & radio off +// Turns the BT radio off (dev stays started) bool stop(Device* dev); bool isRadioOnOrPending(Device* dev); diff --git a/Tactility/Private/Tactility/app/btmanage/BtManagePrivate.h b/Tactility/Private/Tactility/app/btmanage/BtManagePrivate.h index 8bf01be2f..12937c3e3 100644 --- a/Tactility/Private/Tactility/app/btmanage/BtManagePrivate.h +++ b/Tactility/Private/Tactility/app/btmanage/BtManagePrivate.h @@ -7,9 +7,6 @@ #include #include -#include -#include - namespace tt::app::btmanage { struct Context { @@ -19,13 +16,7 @@ struct Context { State state; View view = View(&bindings, &state); Device* btDevice = nullptr; - bool callbackRegistered = false; - - // Bumped right before the BT event callback is unregistered at the end of appMain(), to - // invalidate any BT event already dispatched to the main task for this instance. Kept in - // its own heap allocation, independent of Context's (stack-local) lifetime, so a dispatched - // callback can check it without touching a possibly already-destroyed Context. - std::shared_ptr> generation = std::make_shared>(0); + BtEventSubscription btEventSub {}; void lock() { mutex.lock(); } void unlock() { mutex.unlock(); } @@ -34,15 +25,4 @@ struct Context { void onBtEvent(Context* ctx, const struct BtEvent& event); void requestViewUpdate(Context* ctx); -// Re-attempts registering the device event callback. Needed because the BLE driver only -// allocates its callback list while the device is started/on: a registration attempted while -// the radio is off silently no-ops, so this must be called again right after a successful -// bluetooth::start(). Idempotent: no-ops if already registered for this device. -void registerDeviceCallback(Context* ctx, Device* dev); - -// Call after bluetooth::stop(): the driver frees its callback list on stop, so the -// registration state must be cleared here too, without touching the (now-dangling) driver-side -// list. -void forgetCallbackRegistration(Context* ctx); - } // namespace tt::app::btmanage diff --git a/Tactility/Source/app/btmanage/BtManage.cpp b/Tactility/Source/app/btmanage/BtManage.cpp index 8aae722db..6e5dc5ae6 100644 --- a/Tactility/Source/app/btmanage/BtManage.cpp +++ b/Tactility/Source/app/btmanage/BtManage.cpp @@ -3,8 +3,6 @@ #include #include -#include - #include #include #include @@ -13,6 +11,7 @@ #include #include +#include #include namespace tt::app::btmanage { @@ -22,26 +21,17 @@ constexpr auto* TAG = "BtManage"; extern const ::AppManifest manifest; -static void onBtToggled(void* context, bool requestOn) { +static void onBtToggled(void* /*context*/, bool requestOn) { #if defined(CONFIG_BT_NIMBLE_ENABLED) - auto* ctx = static_cast(context); 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"); - if (bluetooth::start(dev)) { - // The driver only allocates its callback list once the device is started, - // so the registration attempted at startup (while radio was off) was a - // no-op. Register again now that the device is actually up. - registerDeviceCallback(ctx, dev); - } + bluetooth::start(dev); } else if (!requestOn && radio_on) { LOG_I(TAG, "Turning off"); - if (bluetooth::stop(dev)) { - // A completed stop frees the driver's callback list. - forgetCallbackRegistration(ctx); - } + bluetooth::stop(dev); } device_put(dev); } else { @@ -85,8 +75,6 @@ static void onForgetPeer(const std::array& addr) { bluetooth::unpair(addr); } -static void onKernelBtEvent(Device* /*device*/, void* context, BtEvent event); - void requestViewUpdate(Context* ctx) { // Lock order must match appMain()'s setup/teardown: both run under the LVGL lock // and then take `ctx->mutex` internally. Taking `mutex` before lvgl_lock() here would @@ -143,50 +131,6 @@ void onBtEvent(Context* ctx, const BtEvent& event) { requestViewUpdate(ctx); } -static void onKernelBtEvent(Device* /*device*/, void* context, BtEvent event) { - // BT event callbacks can fire from the NimBLE host task (e.g. DISCONNECT during - // nimble_port_stop shutdown). Calling onBtEvent() synchronously from the NimBLE - // task would block it on the LVGL mutex (held by the LVGL task waiting in - // nimble_port_stop), creating a permanent deadlock. Dispatch to the main task so - // the NimBLE host task is never blocked by BtManage's state updates or LVGL lock. - auto* ctx = static_cast(context); - // Captured while `ctx` is still guaranteed valid (the callback is only invoked while - // registered, i.e. before appMain()'s cleanup removes it). Comparing this later - - // without dereferencing `ctx` - lets the dispatched lambda detect a stale event from an - // instance that has since closed (and had its Context destroyed) without a UAF: the - // generation bump in appMain()'s cleanup always happens before window_manager_remove() - // destroys ctx's widgets, and this dispatched lambda always re-reads the live generation - // at run time (not at dispatch time), so a bump landing anywhere before this lambda - // actually runs is enough to make it skip touching ctx. - auto generation = ctx->generation; - int expectedGeneration = generation->load(); - getMainDispatcher().dispatch([ctx, generation, expectedGeneration, event] { - if (generation->load() != expectedGeneration) { - return; - } - onBtEvent(ctx, event); - }); -} - -void registerDeviceCallback(Context* ctx, Device* dev) { - ctx->lock(); - if (ctx->btDevice == dev && !ctx->callbackRegistered) { - // Only latch the flag on success: while the radio is off the driver has no - // callback list yet, so this add is a silent no-op and must be retried once - // bluetooth::start() actually brings the device up. - if (bluetooth_add_event_callback(dev, ctx, onKernelBtEvent) == ERROR_NONE) { - ctx->callbackRegistered = true; - } - } - ctx->unlock(); -} - -void forgetCallbackRegistration(Context* ctx) { - ctx->lock(); - ctx->callbackRegistered = false; - ctx->unlock(); -} - void onBackPressed(lv_event_t* event) { auto* ctx = static_cast(lv_event_get_user_data(event)); AppEvent closeEvent { .type = APP_EVENT_CLOSE, .timestamp = 0, .result = {} }; @@ -229,13 +173,18 @@ int32_t appMain(int argc, char* argv[]) { AppEventSubscription sub {}; check(app_event_subscribe(&sub, &event_group) == ERROR_NONE); - WindowId window = window_manager_create(appInstanceId, createWidgets, &ctx); - - ctx.btDevice = dev; - if (ctx.btDevice) { - registerDeviceCallback(&ctx, ctx.btDevice); + // dev is started for the process lifetime once ble0 is enabled in the devicetree - + // subscribe once here rather than resubscribing on every bluetooth::start()/stop() toggle. + if (dev != nullptr) { + if (bluetooth_event_subscribe(dev, &ctx.btEventSub, &event_group) == ERROR_NONE) { + ctx.btDevice = dev; + } else { + LOG_W(TAG, "Failed to subscribe to BT events"); + } } + WindowId window = window_manager_create(appInstanceId, createWidgets, &ctx); + auto radio_state = bluetooth::getRadioState(); bool can_scan = radio_state == bluetooth::RadioState::On; LOG_I(TAG, "Radio: %s, Scanning: %d, Can scan: %d", @@ -261,21 +210,24 @@ int32_t appMain(int argc, char* argv[]) { } if (shouldClose) break; } - } - // Invalidate any BT event dispatched-but-not-yet-run for this instance before doing - // anything else, so it can't race the teardown below (see onKernelBtEvent()). - ctx.generation->fetch_add(1); + if (ctx.btDevice != nullptr) { + BtEvent bt_event {}; + while (bluetooth_event_poll(&ctx.btEventSub, &bt_event) == ERROR_NONE) { + onBtEvent(&ctx, bt_event); + } + } + } if (ctx.btDevice) { - if (ctx.callbackRegistered) { - bluetooth_remove_event_callback(ctx.btDevice, onKernelBtEvent); - ctx.callbackRegistered = false; - } - device_put(ctx.btDevice); + bluetooth_event_unsubscribe(ctx.btDevice, &ctx.btEventSub); ctx.btDevice = nullptr; } + if (dev != nullptr) { + device_put(dev); + } + window_manager_remove(window); check(app_event_unsubscribe(&sub) == ERROR_NONE); task_event_group_destruct(&event_group); diff --git a/Tactility/Source/app/btpeersettings/BtPeerSettings.cpp b/Tactility/Source/app/btpeersettings/BtPeerSettings.cpp index 75b8a3bb8..9fbc3dda6 100644 --- a/Tactility/Source/app/btpeersettings/BtPeerSettings.cpp +++ b/Tactility/Source/app/btpeersettings/BtPeerSettings.cpp @@ -58,8 +58,7 @@ void updateViews(const Context* ctx) { } } -void onKernelBtEvent(struct Device* /*device*/, void* context, struct BtEvent /*event*/) { - auto* ctx = static_cast(context); +void onBtEvent(Context* ctx) { lvgl_lock(); updateViews(ctx); lvgl_unlock(); @@ -196,18 +195,21 @@ int32_t appMain(int argc, char* argv[]) { } - Device* btDevice = nullptr; - if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &btDevice) == ERROR_NONE) { - bluetooth_add_event_callback(btDevice, &ctx, onKernelBtEvent); - device_put(btDevice); - } - TaskEventGroup event_group {}; task_event_group_construct(&event_group); AppEventSubscription sub {}; check(app_event_subscribe(&sub, &event_group) == ERROR_NONE); + Device* btDevice = nullptr; + BtEventSubscription btSub {}; + if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &btDevice) == ERROR_NONE) { + if (bluetooth_event_subscribe(btDevice, &btSub, &event_group) != ERROR_NONE) { + device_put(btDevice); + btDevice = nullptr; + } + } + WindowId window = window_manager_create(appInstanceId, createWidgets, &ctx); bool shouldClose = false; @@ -239,10 +241,17 @@ int32_t appMain(int argc, char* argv[]) { } if (shouldClose) break; } + + if (btDevice != nullptr) { + BtEvent bt_event {}; + while (bluetooth_event_poll(&btSub, &bt_event) == ERROR_NONE) { + onBtEvent(&ctx); + } + } } - if (device_get_first_active_by_type(&BLUETOOTH_TYPE, &btDevice) == ERROR_NONE) { - bluetooth_remove_event_callback(btDevice, onKernelBtEvent); + if (btDevice != nullptr) { + bluetooth_event_unsubscribe(btDevice, &btSub); device_put(btDevice); } diff --git a/Tactility/Source/bluetooth/Bluetooth.cpp b/Tactility/Source/bluetooth/Bluetooth.cpp index b5811c042..d710e0725 100644 --- a/Tactility/Source/bluetooth/Bluetooth.cpp +++ b/Tactility/Source/bluetooth/Bluetooth.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -20,6 +21,7 @@ #include #include +#include #include #include @@ -103,12 +105,17 @@ static void cachePeerRecord(const BtPeerRecord& krecord) { scan_results_cache.push_back(std::move(rec)); } -// ---- Bridge callback (registered with kernel driver) ---- -// This callback listens to platform driver events to perform auto-start logic -// and settings management. Consumers should register their own callbacks via -// bluetooth_add_event_callback() to receive events directly. +// ---- Bridge thread (subscribed to the kernel driver) ---- +// This thread listens to platform driver events to perform auto-start logic and settings +// management. Consumers should subscribe directly via bluetooth_event_subscribe() to receive +// events themselves. -static void bt_event_bridge(Device*, void* /*context*/, BtEvent event) { +TaskEventGroup btEventGroup {}; +BtEventSubscription btEventSub {}; +Thread* btEventThread = nullptr; +std::atomic btEventThreadRunning {false}; + +static void bt_event_bridge(BtEvent event) { switch (event.type) { case BT_EVENT_RADIO_STATE_CHANGED: switch (event.radio_state) { @@ -250,6 +257,57 @@ static void bt_event_bridge(Device*, void* /*context*/, BtEvent event) { } } +// ---- Bridge thread lifecycle ---- +// Runs bt_event_bridge() on its own stack instead of whichever thread published the event, by +// blocking in task_event_group_wait_any() rather than being called back directly. + +constexpr configSTACK_DEPTH_TYPE BT_EVENT_THREAD_STACK_SIZE = 4096; + +Device* btEventDevice = nullptr; + +int32_t btEventThreadMain() { + while (btEventThreadRunning.load()) { + task_event_group_wait_any(&btEventGroup, nullptr, pdMS_TO_TICKS(250)); + + BtEvent event {}; + while (bluetooth_event_poll(&btEventSub, &event) == ERROR_NONE) { + bt_event_bridge(event); + } + } + return 0; +} + +bool startBtEventThread(Device* dev) { + if (btEventThread != nullptr) { + return true; // already running + } + + task_event_group_construct(&btEventGroup); + if (bluetooth_event_subscribe(dev, &btEventSub, &btEventGroup) != ERROR_NONE) { + task_event_group_destruct(&btEventGroup); + return false; + } + + btEventDevice = dev; + btEventThreadRunning = true; + btEventThread = new Thread("bt-events", BT_EVENT_THREAD_STACK_SIZE, [] { return btEventThreadMain(); }); + btEventThread->start(); + return true; +} + +void stopBtEventThread() { + if (btEventThread == nullptr) return; + + btEventThreadRunning = false; + btEventThread->join(); + delete btEventThread; + btEventThread = nullptr; + + bluetooth_event_unsubscribe(btEventDevice, &btEventSub); + task_event_group_destruct(&btEventGroup); + btEventDevice = nullptr; +} + // ---- systemStart ---- void systemStart() { @@ -271,26 +329,20 @@ bool isRadioOnOrPending(Device* dev) { return state == BT_RADIO_STATE_ON || state == BT_RADIO_STATE_ON_PENDING; } +// dev is started (device_start()) once, at kernel_init (see ble0's devicetree status) and never +// stopped for the process lifetime - this only toggles the radio itself, so callers subscribed +// directly to the driver (e.g. BtManage) stay subscribed across on/off toggles instead of +// having to resubscribe. bool start(Device* dev) { - LOG_I(TAG, "Auto-enabling BLE on boot"); - if (!device_is_ready(dev)) { - LOG_I(TAG, "Starting BLE device"); - if (device_start(dev) != ERROR_NONE) { - LOG_E(TAG, "Failed to start BLE device"); - return false; - } - } - - // TODO: Fix bug where repeatedly calling start would add this callback multiple times - if (bluetooth_add_event_callback(dev, nullptr, bt_event_bridge) != ERROR_NONE) { - LOG_E(TAG, "Failed to set BLE callback"); + // TODO: Fix bug where repeatedly calling start would try to subscribe the bridge thread twice + if (!startBtEventThread(dev)) { + LOG_E(TAG, "Failed to subscribe to BLE events"); } LOG_I(TAG, "Enabling BT radio"); if (bluetooth_set_radio_enabled(dev, true) != ERROR_NONE) { LOG_E(TAG, "Failed to enable BLE radio"); - // Add bridge again - bluetooth_remove_event_callback(dev, bt_event_bridge); + stopBtEventThread(); return false; } @@ -308,19 +360,12 @@ bool stop(Device* dev) { return true; } - if (bluetooth_remove_event_callback(dev, bt_event_bridge) != ERROR_NONE) { - LOG_E(TAG, "Failed to remove BLE callback"); - } + stopBtEventThread(); if (bluetooth_set_radio_enabled(dev, false) != ERROR_NONE) { LOG_E(TAG, "Failed to disable BT radio"); - // Re-register bridge - bluetooth_add_event_callback(dev, nullptr, bt_event_bridge); - return false; - } - - if (device_stop(dev) != ERROR_NONE) { - LOG_E(TAG, "Failed to stop BT device"); + // Re-subscribe bridge + startBtEventThread(dev); return false; } diff --git a/TactilityKernel/include/tactility/drivers/bluetooth.h b/TactilityKernel/include/tactility/drivers/bluetooth.h index 89762eb93..e777f3707 100644 --- a/TactilityKernel/include/tactility/drivers/bluetooth.h +++ b/TactilityKernel/include/tactility/drivers/bluetooth.h @@ -4,6 +4,8 @@ #include #include +#include +#include #include #ifdef __cplusplus @@ -134,7 +136,45 @@ struct BtEvent { }; }; -typedef void (*BtEventCallback)(struct Device* device, void* context, struct BtEvent event); +/** Number of events a BtEventSubscription can hold before bluetooth_fire_event() starts dropping + * the newest event for it (still delivered to any other matching subscription). Generous: a + * device fires these serially, one at a time, not in genuinely concurrent bursts. */ +#define BT_EVENT_QUEUE_CAPACITY 4 + +/** + * Caller-owned subscription node, registered with bluetooth_event_subscribe() and polled with + * bluetooth_event_poll(). Like wifi_event, queues events by value (FIFO) rather than coalescing + * to the latest one. + * @warning Fields other than `bit` are for internal use only; do not read or write them + * directly. + */ +struct BtEventSubscription { + /** Set by bluetooth_event_subscribe(). Read-only for the caller: OR it into a + * task_event_group_wait() mask (alongside other subscriptions sharing the same + * `event_group`) to block on this subscription and other event sources with one call. */ + uint32_t bit; + + struct { + /** Caller-owned, borrowed; set by bluetooth_event_subscribe(). */ + struct TaskEventGroup* event_group; + /** Guards `queue`/`head`/`count`/`closed` between bluetooth_fire_event() (driver thread) + * and bluetooth_event_poll() (caller's thread) - the two live in different translation + * units with no other shared lock. */ + struct Mutex ring_mutex; + struct BtEvent queue[BT_EVENT_QUEUE_CAPACITY]; + uint8_t head; + uint8_t count; + /** Set under `ring_mutex` when the device is torn down while still subscribed (see + * esp32_ble_stop_device()). `ring_mutex` stays constructed - only the subscription's own + * bluetooth_event_unsubscribe() destructs it, since that can't race its own polling. */ + bool closed; + /** True while `ring_mutex` is constructed. Read without locking `ring_mutex`: it's what + * decides whether locking it is safe. */ + bool constructed; + + struct BtEventSubscription* next; + } internal; +}; // ---- Top-level Bluetooth API ---- @@ -219,21 +259,30 @@ struct BluetoothApi { error_t (*disconnect)(struct Device* device, const BtAddr addr, enum BtProfileId profile); /** - * Add an event callback. + * Register a subscription for this device's BtEvents. + * @warning Does not work in ISR context. * @param[in] device the bluetooth device - * @param[in] context context pointer passed to the callback - * @param[in] callback the callback function - * @return ERROR_NONE on success + * @param[in,out] sub subscription to register; owns the storage, must stay alive (and + * stationary) until unsubscribed + * @param[in] event_group caller-owned group to wait on; must outlive @a sub (i.e. be + * destructed only after event_unsubscribe()). To block for an event, call + * task_event_group_wait()/task_event_group_wait_any() on this group (OR sub->bit into the + * mask, or use _wait_any() to include every subscription sharing it), then drain with + * bluetooth_event_poll(). + * @retval ERROR_NONE on success + * @retval ERROR_RESOURCE @a event_group has no free bits left to claim; @a sub was not registered + * @retval ERROR_INVALID_STATE @a sub is already registered */ - error_t (*add_event_callback)(struct Device* device, void* context, BtEventCallback callback); + error_t (*event_subscribe)(struct Device* device, struct BtEventSubscription* sub, struct TaskEventGroup* event_group); /** - * Remove a previously added event callback. + * Remove a previously registered subscription. + * @warning Does not work in ISR context. * @param[in] device the bluetooth device - * @param[in] callback the callback to remove - * @return ERROR_NONE on success + * @param[in] sub subscription to remove, as passed to event_subscribe() + * @return ERROR_NONE on success, ERROR_NOT_FOUND if no matching subscription exists */ - error_t (*remove_event_callback)(struct Device* device, BtEventCallback callback); + error_t (*event_unsubscribe)(struct Device* device, struct BtEventSubscription* sub); /** * Set the BLE device name used in advertising and the GAP service. @@ -264,7 +313,7 @@ struct BluetoothApi { void (*set_hid_host_active)(struct Device* device, bool active); /** - * Fire an event through all registered event callbacks. + * Fire an event to all registered subscriptions. * Used by the Tactility HID host module to inject profile-state events that * originate outside the platform driver (e.g. HID host connect/disconnect). */ @@ -295,13 +344,44 @@ error_t bluetooth_unpair(struct Device* device, const BtAddr addr); error_t bluetooth_get_paired_peers(struct Device* device, struct BtPeerRecord* out, size_t* count); error_t bluetooth_connect(struct Device* device, const BtAddr addr, enum BtProfileId profile); error_t bluetooth_disconnect(struct Device* device, const BtAddr addr, enum BtProfileId profile); -error_t bluetooth_add_event_callback(struct Device* device, void* context, BtEventCallback callback); -error_t bluetooth_remove_event_callback(struct Device* device, BtEventCallback callback); error_t bluetooth_set_device_name(struct Device* device, const char* name); error_t bluetooth_get_device_name(struct Device* device, char* buf, size_t buf_len); void bluetooth_set_hid_host_active(struct Device* device, bool active); void bluetooth_fire_event(struct Device* device, struct BtEvent event); +/** + * Register a subscription for @a device's BtEvents. + * @warning Does not work in ISR context. + * @param[in] device the bluetooth device + * @param[in,out] sub subscription to register; owns the storage, must stay alive (and + * stationary) until unsubscribed + * @param[in] event_group caller-owned group to wait on; must outlive @a sub. To block for an + * event, call task_event_group_wait()/task_event_group_wait_any() on this group (OR sub->bit + * into the mask, or use _wait_any() to include every subscription sharing it), then drain with + * bluetooth_event_poll(). + * @retval ERROR_NONE on success + * @retval ERROR_RESOURCE @a event_group has no free bits left to claim; @a sub was not registered + * @retval ERROR_INVALID_STATE @a sub is already registered + */ +error_t bluetooth_event_subscribe(struct Device* device, struct BtEventSubscription* sub, struct TaskEventGroup* event_group); + +/** + * Remove a previously registered subscription. + * @warning Does not work in ISR context. + * @return ERROR_NONE on success, ERROR_NOT_FOUND if no matching subscription exists + */ +error_t bluetooth_event_unsubscribe(struct Device* device, struct BtEventSubscription* sub); + +/** + * Non-blocking: pop the next event for @a sub if one is already queued. + * @warning Never blocks. To wait for an event, block in task_event_group_wait()/ + * task_event_group_wait_any() on @a sub's event group first (see bluetooth_event_subscribe()), + * then drain with this in a loop. + * @retval ERROR_NONE @a out_event was filled + * @retval ERROR_TIMEOUT nothing queued right now + */ +error_t bluetooth_event_poll(struct BtEventSubscription* sub, struct BtEvent* out_event); + #ifdef __cplusplus } #endif diff --git a/TactilityKernel/include/tactility/drivers/wifi.h b/TactilityKernel/include/tactility/drivers/wifi.h index 78c7cff6a..114b4a494 100644 --- a/TactilityKernel/include/tactility/drivers/wifi.h +++ b/TactilityKernel/include/tactility/drivers/wifi.h @@ -116,13 +116,20 @@ struct WifiEventSubscription { struct { /** Caller-owned, borrowed; set by wifi_event_subscribe(). */ struct TaskEventGroup* event_group; - /** Guards `queue`/`head`/`count` between wifi_event_emit() (driver thread) and + /** Guards `queue`/`head`/`count`/`closed` between wifi_event_emit() (driver thread) and * wifi_event_poll() (caller's thread) - the two live in different translation units with * no other shared lock. */ struct Mutex ring_mutex; struct WifiEvent queue[WIFI_EVENT_QUEUE_CAPACITY]; uint8_t head; uint8_t count; + /** Set under `ring_mutex` when the device is torn down while still subscribed (see + * esp32_wifi's stop_device()). `ring_mutex` stays constructed - only the subscription's + * own wifi_event_unsubscribe() destructs it, since that can't race its own polling. */ + bool closed; + /** True while `ring_mutex` is constructed. Read without locking `ring_mutex`: it's what + * decides whether locking it is safe. */ + bool constructed; struct WifiEventSubscription* next; } internal; diff --git a/TactilityKernel/source/drivers/bluetooth.cpp b/TactilityKernel/source/drivers/bluetooth.cpp index 4ed5ebdc9..73843f9c4 100644 --- a/TactilityKernel/source/drivers/bluetooth.cpp +++ b/TactilityKernel/source/drivers/bluetooth.cpp @@ -66,14 +66,55 @@ error_t bluetooth_disconnect(struct Device* device, const BtAddr addr, enum BtPr return BT_API(device)->disconnect(device, addr, profile); } -// ---- Event callbacks ---- - -error_t bluetooth_add_event_callback(struct Device* device, void* context, BtEventCallback callback) { - return BT_API(device)->add_event_callback(device, context, callback); -} - -error_t bluetooth_remove_event_callback(struct Device* device, BtEventCallback callback) { - return BT_API(device)->remove_event_callback(device, callback); +// ---- Event subscription ---- + +error_t bluetooth_event_subscribe(struct Device* device, struct BtEventSubscription* sub, struct TaskEventGroup* event_group) { + mutex_construct(&sub->internal.ring_mutex); + sub->internal.head = 0; + sub->internal.count = 0; + sub->internal.closed = false; + sub->internal.constructed = true; + + error_t result = BT_API(device)->event_subscribe(device, sub, event_group); + if (result != ERROR_NONE) { + sub->internal.constructed = false; + mutex_destruct(&sub->internal.ring_mutex); + } + return result; +} + +error_t bluetooth_event_unsubscribe(struct Device* device, struct BtEventSubscription* sub) { + error_t result = BT_API(device)->event_unsubscribe(device, sub); + // sub was never subscribed, or was already unsubscribed. ring_mutex was never constructed, + // unsafe to lock or destruct. + if (!sub->internal.constructed) { + return result; + } + mutex_lock(&sub->internal.ring_mutex); + bool was_closed = sub->internal.closed; + mutex_unlock(&sub->internal.ring_mutex); + // Destruct on success or force-close. + if (result == ERROR_NONE || was_closed) { + sub->internal.constructed = false; + mutex_destruct(&sub->internal.ring_mutex); + } + return result; +} + +error_t bluetooth_event_poll(struct BtEventSubscription* sub, struct BtEvent* out_event) { + mutex_lock(&sub->internal.ring_mutex); + if (sub->internal.closed) { + mutex_unlock(&sub->internal.ring_mutex); + return ERROR_TIMEOUT; + } + bool has_event = sub->internal.count > 0; + if (has_event) { + *out_event = sub->internal.queue[sub->internal.head]; + sub->internal.head = (sub->internal.head + 1) % BT_EVENT_QUEUE_CAPACITY; + sub->internal.count--; + } + mutex_unlock(&sub->internal.ring_mutex); + return has_event ? ERROR_NONE : ERROR_TIMEOUT; } error_t bluetooth_set_device_name(struct Device* device, const char* name) { diff --git a/TactilityKernel/source/drivers/wifi.cpp b/TactilityKernel/source/drivers/wifi.cpp index 5c6c917ec..0a2e9b268 100644 --- a/TactilityKernel/source/drivers/wifi.cpp +++ b/TactilityKernel/source/drivers/wifi.cpp @@ -63,9 +63,12 @@ error_t wifi_event_subscribe(struct Device* device, struct WifiEventSubscription mutex_construct(&sub->internal.ring_mutex); sub->internal.head = 0; sub->internal.count = 0; + sub->internal.closed = false; + sub->internal.constructed = true; error_t result = WIFI_API(device)->event_subscribe(device, sub, event_group); if (result != ERROR_NONE) { + sub->internal.constructed = false; mutex_destruct(&sub->internal.ring_mutex); } return result; @@ -73,7 +76,17 @@ error_t wifi_event_subscribe(struct Device* device, struct WifiEventSubscription error_t wifi_event_unsubscribe(struct Device* device, struct WifiEventSubscription* sub) { error_t result = WIFI_API(device)->event_unsubscribe(device, sub); - if (result == ERROR_NONE) { + // sub was never subscribed, or was already unsubscribed. ring_mutex was never constructed, + // unsafe to lock or destruct. + if (!sub->internal.constructed) { + return result; + } + mutex_lock(&sub->internal.ring_mutex); + bool was_closed = sub->internal.closed; + mutex_unlock(&sub->internal.ring_mutex); + // Destruct on success or force-close. + if (result == ERROR_NONE || was_closed) { + sub->internal.constructed = false; mutex_destruct(&sub->internal.ring_mutex); } return result; @@ -81,6 +94,10 @@ error_t wifi_event_unsubscribe(struct Device* device, struct WifiEventSubscripti error_t wifi_event_poll(struct WifiEventSubscription* sub, struct WifiEvent* out_event) { mutex_lock(&sub->internal.ring_mutex); + if (sub->internal.closed) { + mutex_unlock(&sub->internal.ring_mutex); + return ERROR_TIMEOUT; + } bool has_event = sub->internal.count > 0; if (has_event) { *out_event = sub->internal.queue[sub->internal.head]; diff --git a/TactilityKernel/source/symbols.c b/TactilityKernel/source/symbols.c index accedeada..d478fdc6b 100644 --- a/TactilityKernel/source/symbols.c +++ b/TactilityKernel/source/symbols.c @@ -337,8 +337,9 @@ const struct ModuleSymbol KERNEL_SYMBOLS[] = { DEFINE_MODULE_SYMBOL(bluetooth_get_paired_peers), DEFINE_MODULE_SYMBOL(bluetooth_connect), DEFINE_MODULE_SYMBOL(bluetooth_disconnect), - DEFINE_MODULE_SYMBOL(bluetooth_add_event_callback), - DEFINE_MODULE_SYMBOL(bluetooth_remove_event_callback), + DEFINE_MODULE_SYMBOL(bluetooth_event_subscribe), + DEFINE_MODULE_SYMBOL(bluetooth_event_unsubscribe), + DEFINE_MODULE_SYMBOL(bluetooth_event_poll), DEFINE_MODULE_SYMBOL(bluetooth_set_device_name), DEFINE_MODULE_SYMBOL(bluetooth_get_device_name), DEFINE_MODULE_SYMBOL(bluetooth_set_hid_host_active),