From 0512339c584f0c25b01440b6340528dc0f3df9d7 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 9 Aug 2026 10:05:42 -0500 Subject: [PATCH] Deduplicate the stream-abort sequence and small cleanups - abort_service_stream() on the wrapper (which owns the latch discipline) replaces five identical field-poke sequences in the streamer - The pre-start reuses check_and_log_error_ like the serialized path - discover_services() sets the requested flag where it sticks instead of set-then-rollback; deliver_pending_search_() returns void (no caller reads it); loop()'s IDLE arm folds into the settle arm - SEARCH_CMPL during a teardown skips the param/count work whose result is never delivered - frameworks_for_platforms() in config_helpers derives framework sets from platform lists; the hub.cpp filter entry is now a named map pinned by a test against the proxy's platform list --- .../bluetooth_connection/__init__.py | 38 ++++++------ .../bluetooth_connection_bluedroid.cpp | 59 +++++++------------ .../bluetooth_connection_bluedroid.h | 2 +- .../bluetooth_connection_hub.h | 11 ++++ esphome/config_helpers.py | 12 +++- .../bluetooth_proxy/test_platform_gates.py | 15 +++++ 6 files changed, 80 insertions(+), 57 deletions(-) diff --git a/esphome/components/bluetooth_connection/__init__.py b/esphome/components/bluetooth_connection/__init__.py index e1b1b842b9..4bc92ed7eb 100644 --- a/esphome/components/bluetooth_connection/__init__.py +++ b/esphome/components/bluetooth_connection/__init__.py @@ -151,21 +151,23 @@ async def new_gatt_backend(config: ConfigType) -> cg.MockObj: return backend -FILTER_SOURCE_FILES = filter_source_files_from_platform( - { - "bluetooth_connection_bluedroid.cpp": { - PlatformFramework.ESP32_ARDUINO, - PlatformFramework.ESP32_IDF, - }, - # Every hub platform the proxy admits (the file compiles empty where - # USE_BLE_GATT_CLIENT is not defined), so a platform gaining a backend - # cannot hit a missing-symbol trap here. - "bluetooth_connection_hub.cpp": { - PlatformFramework.RP2_ARDUINO, - PlatformFramework.LN882X_ARDUINO, - PlatformFramework.ESP32_ARDUINO, - PlatformFramework.ESP32_IDF, - }, - "bluetooth_connection_rp2.cpp": {PlatformFramework.RP2_ARDUINO}, - } -) +# Named so tests can pin the hub entry against bluetooth_proxy's platform +# list (this module cannot import bluetooth_proxy to derive it). +SOURCE_FILE_FRAMEWORKS: dict[str, set[PlatformFramework]] = { + "bluetooth_connection_bluedroid.cpp": { + PlatformFramework.ESP32_ARDUINO, + PlatformFramework.ESP32_IDF, + }, + # Every hub platform the proxy admits (the file compiles empty where + # USE_BLE_GATT_CLIENT is not defined), so a platform gaining a backend + # cannot hit a missing-symbol trap here. + "bluetooth_connection_hub.cpp": { + PlatformFramework.RP2_ARDUINO, + PlatformFramework.LN882X_ARDUINO, + PlatformFramework.ESP32_ARDUINO, + PlatformFramework.ESP32_IDF, + }, + "bluetooth_connection_rp2.cpp": {PlatformFramework.RP2_ARDUINO}, +} + +FILTER_SOURCE_FILES = filter_source_files_from_platform(SOURCE_FILE_FRAMEWORKS) diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp index 6b34438430..851749f845 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp @@ -63,14 +63,11 @@ void BluedroidGattClient::loop() { } // Do not wait for REG_EVT; a dropped event must not wedge the slot. this->set_state(ClientState::IDLE); - } else if (st == ClientState::IDLE) { - // The loop only drives the bootstrap, the disconnect safety timeout and - // the pre-started-search flush. - this->disable_loop(); } else if (st != ClientState::DISCONNECTING) { - // Only the pre-started-search flush can need the loop here (a consumer - // requesting the finished search outside an event drain). Settle again - - // unless delivering it just started a teardown that needs the timer. + // The loop only drives the bootstrap, the disconnect safety timeout and + // the pre-started-search flush (a consumer requesting the finished + // search outside an event drain; set_idle_() cleared any stale latch). + // Settle - unless delivering just started a teardown needing the timer. this->deliver_pending_search_(); if (this->state() != ClientState::DISCONNECTING) { this->disable_loop(); @@ -184,11 +181,11 @@ int BluedroidGattClient::discover_services() { if (this->conn_id_ == UNSET_CONN_ID) { return ble_device_base::GATT_ERR_NOT_CONNECTED; } - this->search_requested_ = true; if (this->search_prestarted_) { // Completion comes from the pre-started search: the pending SEARCH_CMPL, // or - when it already landed - the flush after the connected report // (loop() covers a request made outside that event drain). + this->search_requested_ = true; if (this->search_done_) { this->enable_loop(); } @@ -196,8 +193,8 @@ int BluedroidGattClient::discover_services() { } int err = this->check_and_log_error_("esp_ble_gattc_search_service", esp_ble_gattc_search_service(this->gattc_if_, this->conn_id_, nullptr)); - if (err != 0) { - this->search_requested_ = false; + if (err == 0) { + this->search_requested_ = true; } return err; } @@ -505,12 +502,11 @@ int BluedroidGattClient::handle_search_cmpl_() { // Reports a completed search once its consumer has asked for it; the // pre-started search must stay silent until then. -bool BluedroidGattClient::deliver_pending_search_() { +void BluedroidGattClient::deliver_pending_search_() { if (!this->search_requested_ || !this->search_done_) - return false; + return; this->search_requested_ = false; this->listener_->on_service_discovery_done(this->search_status_); - return true; } #ifdef USE_BLUETOOTH_PROXY @@ -553,10 +549,7 @@ void BluedroidGattClient::stream_service_batch(BluetoothConnection &conn) { if (svc_status != ESP_GATT_OK || svc_count == 0) { ESP_LOGE(TAG, "[%d] [%s] Service walk failed (service %d), aborting stream", conn.connection_index_, conn.address_str_, conn.send_service_); - // Latch the real cause for the disconnect report. - conn.pending_error_ = svc_status != ESP_GATT_OK ? svc_status : ESP_GATT_NOT_FOUND; - conn.send_service_ = DONE_SENDING_SERVICES; - conn.disconnect(); + conn.abort_service_stream(svc_status != ESP_GATT_OK ? svc_status : ESP_GATT_NOT_FOUND); return; } uint16_t total_char_count = 0; @@ -565,9 +558,7 @@ void BluedroidGattClient::stream_service_batch(BluetoothConnection &conn) { service_result.start_handle, service_result.end_handle, 0, &total_char_count); if (char_count_status != ESP_GATT_OK) { this->log_gattc_warning_("esp_ble_gattc_get_attr_count", char_count_status); - conn.pending_error_ = char_count_status; - conn.send_service_ = DONE_SENDING_SERVICES; - conn.disconnect(); + conn.abort_service_stream(char_count_status); return; } @@ -599,9 +590,7 @@ void BluedroidGattClient::stream_service_batch(BluetoothConnection &conn) { if (char_status != ESP_GATT_OK || cc == 0) { if (char_status != ESP_GATT_OK) { this->log_gattc_warning_("esp_ble_gattc_get_all_char", char_status); - conn.pending_error_ = char_status; - conn.send_service_ = DONE_SENDING_SERVICES; - conn.disconnect(); + conn.abort_service_stream(char_status); return; } break; @@ -620,9 +609,7 @@ void BluedroidGattClient::stream_service_batch(BluetoothConnection &conn) { // Abort rather than stream the characteristic descriptor-less: a // missing CCCD in a cached database breaks notifications for good. this->log_gattc_warning_("esp_ble_gattc_get_attr_count", desc_count_status); - conn.pending_error_ = desc_count_status; - conn.send_service_ = DONE_SENDING_SERVICES; - conn.disconnect(); + conn.abort_service_stream(desc_count_status); return; } if (total_desc_count > 0) { @@ -639,9 +626,7 @@ void BluedroidGattClient::stream_service_batch(BluetoothConnection &conn) { if (desc_status != ESP_GATT_OK || dc == 0) { if (desc_status != ESP_GATT_OK) { this->log_gattc_warning_("esp_ble_gattc_get_all_descr", desc_status); - conn.pending_error_ = desc_status; - conn.send_service_ = DONE_SENDING_SERVICES; - conn.disconnect(); + conn.abort_service_stream(desc_status); return; } break; @@ -716,11 +701,9 @@ void BluedroidGattClient::handle_open_evt_(esp_ble_gattc_cb_param_t *param) { // Discovery-bound connection: start the search now so it overlaps the // MTU exchange. On a refusal fall back to the serialized path - the // consumer's own discover_services() call retries the real search. - auto ret = esp_ble_gattc_search_service(this->gattc_if_, this->conn_id_, nullptr); - if (ret == ESP_OK) { + if (this->check_and_log_error_("esp_ble_gattc_search_service", + esp_ble_gattc_search_service(this->gattc_if_, this->conn_id_, nullptr)) == 0) { this->search_prestarted_ = true; - } else { - this->log_gattc_warning_("esp_ble_gattc_search_service", ret); } } } @@ -816,16 +799,18 @@ bool BluedroidGattClient::gattc_event_handler(esp_gattc_cb_event_t event, esp_ga return false; ESP_LOGI(TAG, "[%d] Service discovery complete", this->connection_index_); this->search_done_ = true; - this->search_status_ = this->handle_search_cmpl_(); if (this->state() == ClientState::DISCONNECTING) { - // Teardown already owns the link: keep its state and safety timer; - // the terminal connected=false report settles the consumer. + // Teardown already owns the link: keep its state and safety timer, + // and skip the param/count work - the result is never delivered (the + // terminal connected=false report settles the consumer). break; } + this->search_status_ = this->handle_search_cmpl_(); this->set_state(ClientState::ESTABLISHED); this->deliver_pending_search_(); if (this->state() != ClientState::DISCONNECTING) { - // Settled; a failed count started a teardown that needs the loop. + // Settled - unless delivering a failed status just started a + // teardown that needs the loop for its timer. this->disable_loop(); } break; diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h index a7c09833c2..294b4b627e 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h @@ -85,7 +85,7 @@ class BluedroidGattClient final : public esp32_ble_tracker::ESPBTClient, public void handle_open_evt_(esp_ble_gattc_cb_param_t *param); void handle_disconnect_evt_(esp_ble_gattc_cb_param_t *param); int handle_search_cmpl_(); - bool deliver_pending_search_(); + void deliver_pending_search_(); void unconditional_disconnect_(); void set_idle_(); void set_disconnecting_(); diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_hub.h b/esphome/components/bluetooth_connection/bluetooth_connection_hub.h index 6801e90281..9e4a8b94cf 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_hub.h +++ b/esphome/components/bluetooth_connection/bluetooth_connection_hub.h @@ -41,6 +41,17 @@ class BluetoothConnection final : public ble_device_base::GattClientListener { conn_err_t notify_characteristic(uint16_t handle, bool enable); conn_err_t update_connection_params(uint16_t min_interval, uint16_t max_interval, uint16_t latency, uint16_t timeout); + /// Streamer abort: latch the GATT cause for the disconnect report (first + /// cause wins, matching disconnect()), park the stream cursor, and tear + /// the connection down. + void abort_service_stream(conn_err_t err) { + if (this->pending_error_ == 0) { + this->pending_error_ = err; + } + this->send_service_ = DONE_SENDING_SERVICES; + this->disconnect(); + } + /// Start connecting: record the API address type (BLE_ADDR_TYPE_* code /// space) and open the connection through the backend. Failures report /// through the same reset path a failed open takes. diff --git a/esphome/config_helpers.py b/esphome/config_helpers.py index c0a3b99968..8bee3b3a52 100644 --- a/esphome/config_helpers.py +++ b/esphome/config_helpers.py @@ -1,4 +1,4 @@ -from collections.abc import Callable +from collections.abc import Callable, Collection from esphome.const import ( CONF_LEVEL, @@ -98,6 +98,16 @@ def merge_config(old, new): return new +def frameworks_for_platforms(platforms: Collection[str]) -> set[PlatformFramework]: + """All PlatformFramework members whose platform is in `platforms`. + + For FILTER_SOURCE_FILES maps that must stay in sync with a platform + registry: deriving the framework set here means a platform added to the + registry cannot validate and then fail at link on a filtered-out file. + """ + return {pf for pf in PlatformFramework if pf.value[0].value in platforms} + + def filter_source_files_from_platform( files_map: dict[str, set[PlatformFramework]], ) -> Callable[[], list[str]]: diff --git a/tests/component_tests/bluetooth_proxy/test_platform_gates.py b/tests/component_tests/bluetooth_proxy/test_platform_gates.py index 17992d25ab..1a354f116e 100644 --- a/tests/component_tests/bluetooth_proxy/test_platform_gates.py +++ b/tests/component_tests/bluetooth_proxy/test_platform_gates.py @@ -9,11 +9,13 @@ import pytest from esphome import config_validation as cv from esphome.components import ble_device_base, bluetooth_connection, bluetooth_proxy +from esphome.config_helpers import frameworks_for_platforms from esphome.const import ( CONF_ACTIVE, KEY_CORE, KEY_TARGET_FRAMEWORK, KEY_TARGET_PLATFORM, + PLATFORM_ESP32, PLATFORM_LN882X, PLATFORM_RP2, PlatformFramework, @@ -177,6 +179,19 @@ def test_rp2_rejects_esp32_only_keys_by_name( bluetooth_proxy.CONFIG_SCHEMA({"connections": [{}]}) +def test_hub_source_filter_covers_every_hub_platform() -> None: + # bluetooth_connection cannot import this module to derive the hub.cpp + # framework set, so pin it here: a platform admitted to the proxy but + # missing from the filter would validate, then fail at link. + expected = frameworks_for_platforms( + [*bluetooth_proxy._HUB_PLATFORMS, PLATFORM_ESP32] + ) + hub_frameworks = bluetooth_connection.SOURCE_FILE_FRAMEWORKS[ + "bluetooth_connection_hub.cpp" + ] + assert expected <= hub_frameworks + + def test_bluetooth_connection_auto_load_covers_its_includes() -> None: # The backend registers with its platform BLE stack (and the Bluedroid # header includes the tracker's), so that closure lives here and