From 7e369f332ce18011414ddbeedb410baf604c0cf0 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 9 Aug 2026 14:37:57 -0500 Subject: [PATCH] Dry-pass round 1: one door to IDLE, complete teardown guards, residue - set_idle_() is the single door back to IDLE (bootstrap, open-fail and the DISCOVERED park went through bare set_state), so the per-attempt reset list holds only per-attempt latches - CFG_MTU joins OPEN_EVT and SEARCH_CMPL in suppressing reports while a teardown owns the link - one spelling of the guard across all three events, and the wrapper's race arm becomes defense instead of the only cover - The connections-free retry drain compiles on every proxy build (the advertisement-only arm sends the message too; the latch already did) - latch_pending_error_ makes first-cause-wins the mechanism at all three latch sites; the dead freed-slot refused branch and its retired vocabulary go; the initiate_connection/start_connect_ pair collapses - UNSET_CONN_ID hoisted next to the field it initializes; count-status shadow renamed; stale busy-error rationale replaced with the real one (a repeat call would re-arm the teardown timer) - Fixture states the batch-grouping caveat like its rp2 sibling; the get_service_table stub carries a greppable direct-consumer warning --- .../bluetooth_connection_bluedroid.cpp | 25 +++++++++---------- .../bluetooth_connection_bluedroid.h | 8 ++++-- .../bluetooth_connection_hub.cpp | 20 +++++++-------- .../bluetooth_connection_hub.h | 16 ++++++------ .../bluetooth_proxy/bluetooth_proxy.cpp | 6 +++-- .../test-passive.esp32-c6-idf.yaml | 2 ++ 6 files changed, 42 insertions(+), 35 deletions(-) diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp index f4283f7b72..f78c02d744 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp @@ -31,7 +31,6 @@ using ble_device_base::MEDIUM_MIN_CONN_INTERVAL; using esp32_ble_tracker::ClientState; using esp32_ble_tracker::ConnectionType; -static constexpr uint16_t UNSET_CONN_ID = 0xFFFF; // ---- tracker surface ---- void BluedroidGattClient::connect() { this->tracker_connect_(); } @@ -65,7 +64,7 @@ void BluedroidGattClient::loop() { this->mark_failed(); } // Do not wait for REG_EVT; a dropped event must not wedge the slot. - this->set_state(ClientState::IDLE); + this->set_idle_(); } else if (st == ClientState::DISCONNECTING || this->want_disconnect_) { // The one teardown safety net: a lost CLOSE_EVT, or a scheduled // teardown whose OPEN_EVT never arrives. @@ -121,13 +120,11 @@ void BluedroidGattClient::tracker_connect_() { return; } ESP_LOGI(TAG, "[%d] 0x%02x Connecting", this->connection_index_, this->remote_addr_type_); + // Per-attempt latches; the search machine is reset by set_idle_(), the + // one door back to IDLE. this->services_released_ = false; this->seen_mtu_ = false; this->mtu_failed_ = false; - // Per-attempt reset: the stack-down path reaches IDLE without set_idle_(), - // and a stale result must not satisfy this attempt's discovery. - this->search_state_ = SearchState::NONE; - this->search_status_ = 0; this->enable_loop(); this->set_state(ClientState::CONNECTING); if (this->connection_type_ == ConnectionType::V3_WITHOUT_CACHE) { @@ -142,8 +139,8 @@ void BluedroidGattClient::tracker_connect_() { static_cast(this->remote_addr_type_), true); if (ret) { this->log_gattc_warning_("esp_ble_gattc_open", ret); - // CONNECT_EVT never fired, so conn_id_ is legitimately unset: plain IDLE. - this->set_state(ClientState::IDLE); + // CONNECT_EVT never fired; nothing to close. + this->set_idle_(); this->listener_->on_connection_state(false, 0, ret); } } @@ -160,7 +157,7 @@ int BluedroidGattClient::gatt_disconnect() { } if (st == ClientState::DISCOVERED) { // Parked for the tracker promote loop, never opened. - this->set_state(ClientState::IDLE); + this->set_idle_(); return ble_device_base::GATT_ERR_NOT_CONNECTED; } if (st == ClientState::CONNECTING || this->conn_id_ == UNSET_CONN_ID) { @@ -357,9 +354,9 @@ int BluedroidGattClient::handle_search_cmpl_(esp_gatt_status_t status) { 0x0001, 0xFFFF, 0, &secondary); if (primary_status != ESP_GATT_OK || secondary_status != ESP_GATT_OK) { // A failed count must not become an authoritative empty database. - auto status = primary_status != ESP_GATT_OK ? primary_status : secondary_status; - this->log_gattc_warning_("esp_ble_gattc_get_attr_count", status); - return status; + auto count_status = primary_status != ESP_GATT_OK ? primary_status : secondary_status; + this->log_gattc_warning_("esp_ble_gattc_get_attr_count", count_status); + return count_status; } this->service_total_ = primary + secondary; return 0; @@ -635,7 +632,9 @@ bool BluedroidGattClient::gattc_event_handler(esp_gattc_cb_event_t event, esp_ga // Warn only; a disconnect will follow if the link is dead. this->log_gattc_warning_("MTU exchange", param->cfg_mtu.status); } - if (!this->seen_mtu_) { + if (!this->seen_mtu_ && !this->disconnect_pending() && this->state() != ClientState::DISCONNECTING) { + // Teardown owns the link: suppress the connected report here like + // OPEN_EVT and SEARCH_CMPL do; the terminal report settles it. this->seen_mtu_ = true; // The connected report waited for the MTU; forwarded, not stored. this->listener_->on_connection_state( diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h index 2dd3cb239a..2f0336f2ea 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h @@ -31,6 +31,8 @@ class BluetoothConnection; // void disconnect() cannot overload with an int-returning twin. class BluedroidGattClient final : public esp32_ble_tracker::ESPBTClient, public Component { public: + static constexpr uint16_t UNSET_CONN_ID = 0xFFFF; + // Lifecycle of one connection attempt's service search. enum class SearchState : uint8_t { NONE, // no search this attempt @@ -70,7 +72,9 @@ class BluedroidGattClient final : public esp32_ble_tracker::ESPBTClient, public int pair(); int update_connection_params(uint16_t min_interval, uint16_t max_interval, uint16_t latency, uint16_t timeout); // Contract stub: the proxy streams in place; the on-demand materializer - // for direct consumers lands with #18205. + // for direct consumers lands with #18205. NOTE: a direct consumer reaching + // this stub gets an empty table indistinguishable from a service-less + // peer - do not ship one against this backend before the materializer. ble_device_base::GattServiceTable get_service_table() { return {}; } void release_services(); @@ -107,7 +111,7 @@ class BluedroidGattClient final : public esp32_ble_tracker::ESPBTClient, public esp_bd_addr_t remote_bda_{}; // Group 4: 2-byte types - uint16_t conn_id_{0xFFFF}; + uint16_t conn_id_{UNSET_CONN_ID}; uint16_t service_total_{0}; // Group 5: 1-byte types diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp b/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp index 2d76040044..f972760d14 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp +++ b/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp @@ -26,7 +26,7 @@ void BluetoothConnection::set_address(uint64_t address) { format_mac_addr_upper(mac, this->address_str_); } -void BluetoothConnection::start_connect_(uint8_t address_type) { +void BluetoothConnection::initiate_connection(uint8_t address_type) { // No connect timeout here: the API client's own timeout or // the api-gone sweep drives disconnect(). this->state_ = ClientState::CONNECTING; @@ -38,9 +38,9 @@ void BluetoothConnection::start_connect_(uint8_t address_type) { } void BluetoothConnection::disconnect() { - // Idempotent: the proxy's teardown loop calls this - // every 100 ms while the API subscriber is gone, and a repeat call must not - // reach the backend (whose busy error would free the slot mid-teardown). + // Idempotent: the proxy's teardown loop calls this every 100 ms while the + // API subscriber is gone, and a repeat call reaching the backend would + // re-arm its teardown timer so the safety timeout never fires. if (this->state_ == ClientState::IDLE || this->state_ == ClientState::DISCONNECTING) { return; } @@ -83,11 +83,9 @@ void BluetoothConnection::on_connection_state(bool connected, uint16_t mtu, int if (connected && this->address_ == 0) { // Late completion for a slot that was already freed: nothing to report, // and the api-gone sweep or a new reservation owns the slot now. - int err = this->backend_->gatt_disconnect(); - if (err != 0 && err != GATT_NOT_CONNECTED) { - // Log only: re-arming a freed slot could clobber a new reservation. - ESP_LOGW(TAG, "[%d] freed-slot disconnect refused, err=%d", this->connection_index_, err); - } + // Return ignored: nonzero just means the backend was already idle, and + // re-arming a freed slot could clobber a new reservation. + this->backend_->gatt_disconnect(); return; } if (connected && this->state_ == ClientState::DISCONNECTING) { @@ -129,7 +127,7 @@ void BluetoothConnection::on_connection_state(bool connected, uint16_t mtu, int if (err != 0) { ESP_LOGW(TAG, "[%d] [%s] discover_services failed, err=%d", this->connection_index_, this->address_str_, err); // Latch the real cause for the disconnect report. - this->pending_error_ = err; + this->latch_pending_error_(err); this->disconnect(); } return; @@ -148,7 +146,7 @@ void BluetoothConnection::on_service_discovery_done(int error) { ESP_LOGW(TAG, "[%d] [%s] Service discovery failed, err=%d", this->connection_index_, this->address_str_, error); // Carry the GATT error into the disconnection report so the client sees // the real cause instead of a generic HCI reason. - this->pending_error_ = error; + this->latch_pending_error_(error); this->disconnect(); return; } diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_hub.h b/esphome/components/bluetooth_connection/bluetooth_connection_hub.h index 5fb13f5e46..1bf1f8eef6 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_hub.h +++ b/esphome/components/bluetooth_connection/bluetooth_connection_hub.h @@ -41,12 +41,9 @@ 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 (first wins), park the cursor, - /// tear down. + /// Streamer abort: latch the GATT cause, park the cursor, tear down. void abort_service_stream(conn_err_t err) { - if (this->pending_error_ == 0) { - this->pending_error_ = err; - } + this->latch_pending_error_(err); this->send_service_ = DONE_SENDING_SERVICES; this->disconnect(); } @@ -54,7 +51,7 @@ class BluetoothConnection final : public ble_device_base::GattClientListener { /// Start connecting with the API address type (BLE_ADDR_TYPE_* code /// space). Failures report through the same reset path a failed open /// takes. - void initiate_connection(uint8_t address_type) { this->start_connect_(address_type); } + void initiate_connection(uint8_t address_type); void disconnect(); bool is_paired() const { return this->paired_; } void set_unpaired() { this->paired_ = false; } @@ -102,7 +99,12 @@ class BluetoothConnection final : public ble_device_base::GattClientListener { // The Bluedroid backend streams services in place from its stack cache. friend class BluedroidGattClient; - void start_connect_(uint8_t address_type); + /// First cause wins: a later, less specific error must not overwrite it. + void latch_pending_error_(conn_err_t err) { + if (this->pending_error_ == 0) { + this->pending_error_ = err; + } + } // A backend providing its own streamer (see the contract doc) builds the // response in place from its stack cache; the rest use the table streamer. // Template so the discarded branch is not odr-checked against backends diff --git a/esphome/components/bluetooth_proxy/bluetooth_proxy.cpp b/esphome/components/bluetooth_proxy/bluetooth_proxy.cpp index c27538fc43..c792088952 100644 --- a/esphome/components/bluetooth_proxy/bluetooth_proxy.cpp +++ b/esphome/components/bluetooth_proxy/bluetooth_proxy.cpp @@ -475,12 +475,14 @@ void BluetoothProxy::loop() { for (uint8_t i = 0; i < this->connection_count_; i++) { this->connections_[i]->process_pending_services(); } +#endif if (this->connections_free_pending_ && this->api_connection_ != nullptr) { - // Resend a dropped slot-state update once the TCP buffer drains. + // Resend a dropped slot-state update once the TCP buffer drains; the + // advertisement-only arm answers DISCONNECT requests with this message + // too, so the drain compiles on every proxy build. this->connections_free_pending_ = false; this->send_connections_free(this->api_connection_); } -#endif // Run advertisement flush / scanner-state poll every 100ms uint32_t now = App.get_loop_component_start_time(); diff --git a/tests/components/bluetooth_proxy/test-passive.esp32-c6-idf.yaml b/tests/components/bluetooth_proxy/test-passive.esp32-c6-idf.yaml index 06ce74dbad..b3445f16c8 100644 --- a/tests/components/bluetooth_proxy/test-passive.esp32-c6-idf.yaml +++ b/tests/components/bluetooth_proxy/test-passive.esp32-c6-idf.yaml @@ -1,6 +1,8 @@ # Advertisement-only proxy on esp32 by explicit choice: no GATT backend is # compiled (USE_BLE_GATT_CLIENT unset), which pins the HAS_GATT gating and the # address-scoped maintenance path that a connections build never exercises. +# Under batch grouping the active default build is what runs; the standalone +# compile of this fixture is what exercises the passive gating. packages: common: !include common.yaml