From 4a9e5f5da99f0326f1dc9d904ab281c67615f037 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 9 Aug 2026 09:44:23 -0500 Subject: [PATCH 1/2] Require the proxy in the wrapper's compile gate The hub wrapper serves the proxy's API surface, but its gate keyed on the backend define alone while the backend guarded its own proxy pieces on USE_BLUETOOTH_PROXY. Narrow BLUETOOTH_CONNECTION_HAS_GATT to require both so a future backend-only consumer build agrees with the backend's guards. No change to any build this PR can produce (only the proxy emits USE_BLE_GATT_CLIENT here). --- .../bluetooth_connection/bluetooth_connection.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/esphome/components/bluetooth_connection/bluetooth_connection.h b/esphome/components/bluetooth_connection/bluetooth_connection.h index cdf7076ab7..5052e7eca1 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection.h +++ b/esphome/components/bluetooth_connection/bluetooth_connection.h @@ -19,10 +19,12 @@ // The connection-aware API request handlers are compiled: a GATT backend is // wired by codegen (one slot per connection). This is the single spelling of // that predicate - the hub wrapper and the API request handlers gate on it. -// Advertisement-only builds get the clean-error handlers; address-scoped -// maintenance (unpair, cache clear) still works there through the -// per-platform free functions below. -#ifdef USE_BLE_GATT_CLIENT +// The wrapper serves the proxy's API surface, so it compiles only when a +// backend AND the proxy are present; advertisement-only and backend-only +// builds get the clean-error handlers instead. Address-scoped maintenance +// (unpair, cache clear) still works there through the per-platform free +// functions below. +#if defined(USE_BLE_GATT_CLIENT) && defined(USE_BLUETOOTH_PROXY) #define BLUETOOTH_CONNECTION_HAS_GATT #endif From 084ff9d6e3f757f60b51dd4994474e4eec3af469 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 9 Aug 2026 09:52:58 -0500 Subject: [PATCH 2/2] Restore the discovery/MTU overlap and latch streamer abort causes The split serialized service discovery behind the MTU exchange: the backend reported connected only at CFG_MTU_EVT and the wrapper started discovery on that report, costing one ATT round trip per uncached connection. The backend now pre-starts the search at OPEN_EVT (it knows the connection type) and completes the consumer's discover_services() from it: SEARCH_CMPL latches silently until requested, the flush after the connected report delivers in the same event drain, and a refused pre-start falls back to the serialized path. Net RAM cost is zero (the flags pack into one bitfield byte plus a status byte, replacing the two existing bools). Also from review: SEARCH_CMPL no longer clobbers a teardown in progress with ESTABLISHED, the five streamer abort sites latch pending_error_ so HA sees the real Bluedroid status instead of a generic HCI reason, and a completed re-discovery frees a materialized service table before the counts its offsets derive from move. --- .../bluetooth_connection_bluedroid.cpp | 92 ++++++++++++++++--- .../bluetooth_connection_bluedroid.h | 17 +++- 2 files changed, 93 insertions(+), 16 deletions(-) diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp index d8e1654b90..6b34438430 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp @@ -64,10 +64,18 @@ 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 and the disconnect safety timeout. + // The loop only drives the bootstrap, the disconnect safety timeout and + // the pre-started-search flush. this->disable_loop(); - } else if (st == ClientState::DISCONNECTING && - millis() - this->disconnecting_started_ > ble_device_base::GATT_DISCONNECT_TIMEOUT_MS) { + } 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. + this->deliver_pending_search_(); + if (this->state() != ClientState::DISCONNECTING) { + this->disable_loop(); + } + } else if (millis() - this->disconnecting_started_ > ble_device_base::GATT_DISCONNECT_TIMEOUT_MS) { ESP_LOGE(TAG, "[%d] Timeout waiting for CLOSE_EVT, forcing IDLE", this->connection_index_); // Release before idling: unconditional disconnect does not release, and a // lost CLOSE/DISCONNECT would otherwise leak the table and the cache. @@ -176,8 +184,22 @@ int BluedroidGattClient::discover_services() { if (this->conn_id_ == UNSET_CONN_ID) { return ble_device_base::GATT_ERR_NOT_CONNECTED; } - return this->check_and_log_error_("esp_ble_gattc_search_service", - esp_ble_gattc_search_service(this->gattc_if_, this->conn_id_, nullptr)); + 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). + if (this->search_done_) { + this->enable_loop(); + } + return 0; + } + 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; + } + return err; } int BluedroidGattClient::read_characteristic(uint16_t handle) { @@ -419,6 +441,10 @@ bool BluedroidGattClient::check_addr_(const esp_bd_addr_t &addr) const { void BluedroidGattClient::set_idle_() { this->set_state(ClientState::IDLE); this->conn_id_ = UNSET_CONN_ID; + this->search_prestarted_ = false; + this->search_done_ = false; + this->search_requested_ = false; + this->search_status_ = 0; } void BluedroidGattClient::set_disconnecting_() { @@ -453,7 +479,12 @@ void BluedroidGattClient::log_gattc_warning_(const char *operation, int code) { // ---- service streaming ---- -void BluedroidGattClient::handle_search_cmpl_() { +int BluedroidGattClient::handle_search_cmpl_() { +#ifdef USE_BLE_GATT_SERVICE_TABLE + // A completed re-discovery moves the counts table_view_() derives its + // offsets from; a table built from the old ones must not survive it. + this->free_service_table_(); +#endif // Step down from the fast discovery params. this->update_conn_params_(MEDIUM_MIN_CONN_INTERVAL, MEDIUM_MAX_CONN_INTERVAL, 0, MEDIUM_CONN_TIMEOUT, "medium"); uint16_t primary = 0; @@ -466,11 +497,20 @@ void BluedroidGattClient::handle_search_cmpl_() { // 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); - this->listener_->on_service_discovery_done(status); - return; + return status; } this->service_total_ = primary + secondary; - this->listener_->on_service_discovery_done(0); + return 0; +} + +// 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_() { + if (!this->search_requested_ || !this->search_done_) + return false; + this->search_requested_ = false; + this->listener_->on_service_discovery_done(this->search_status_); + return true; } #ifdef USE_BLUETOOTH_PROXY @@ -508,11 +548,13 @@ void BluedroidGattClient::stream_service_batch(BluetoothConnection &conn) { while (conn.send_service_ < this->service_total_) { esp_gattc_service_elem_t service_result; uint16_t svc_count = 1; - if (esp_ble_gattc_get_service(this->gattc_if_, this->conn_id_, nullptr, &service_result, &svc_count, - conn.send_service_) != ESP_GATT_OK || - svc_count == 0) { + esp_gatt_status_t svc_status = esp_ble_gattc_get_service(this->gattc_if_, this->conn_id_, nullptr, &service_result, + &svc_count, conn.send_service_); + 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(); return; @@ -523,6 +565,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(); return; @@ -556,6 +599,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(); return; @@ -576,6 +620,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(); return; @@ -594,6 +639,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(); return; @@ -666,6 +712,16 @@ void BluedroidGattClient::handle_open_evt_(esp_ble_gattc_cb_param_t *param) { // Settled; set_disconnecting_() re-enables the loop for the net. this->disable_loop(); } + } else { + // 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) { + this->search_prestarted_ = true; + } else { + this->log_gattc_warning_("esp_ble_gattc_search_service", ret); + } } } @@ -733,6 +789,9 @@ bool BluedroidGattClient::gattc_event_handler(esp_gattc_cb_event_t event, esp_ga // The connected report waited for the MTU; forwarded, not stored. this->listener_->on_connection_state( true, param->cfg_mtu.status == ESP_GATT_OK ? param->cfg_mtu.mtu : ble_device_base::DEFAULT_ATT_MTU, 0); + // The consumer requests discovery from inside that report; when the + // pre-started search already finished, complete it in the same drain. + this->deliver_pending_search_(); } break; } @@ -756,8 +815,15 @@ bool BluedroidGattClient::gattc_event_handler(esp_gattc_cb_event_t event, esp_ga if (this->conn_id_ != param->search_cmpl.conn_id) 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. + break; + } this->set_state(ClientState::ESTABLISHED); - this->handle_search_cmpl_(); + this->deliver_pending_search_(); if (this->state() != ClientState::DISCONNECTING) { // Settled; a failed count started a teardown that needs the loop. this->disable_loop(); diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h index 58e9e49f77..a7c09833c2 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h @@ -84,7 +84,8 @@ class BluedroidGattClient final : public esp32_ble_tracker::ESPBTClient, public void tracker_connect_(); void handle_open_evt_(esp_ble_gattc_cb_param_t *param); void handle_disconnect_evt_(esp_ble_gattc_cb_param_t *param); - void handle_search_cmpl_(); + int handle_search_cmpl_(); + bool deliver_pending_search_(); void unconditional_disconnect_(); void set_idle_(); void set_disconnecting_(); @@ -130,10 +131,20 @@ class BluedroidGattClient final : public esp32_ble_tracker::ESPBTClient, public uint8_t connection_index_; // Terminates an in-flight stream (never send a partial list as authoritative) // and marks a cleaned cache unsafe to walk (Bluedroid asserts). - bool services_released_{false}; + bool services_released_ : 1 {false}; // The connected report waits for the MTU exchange; OPEN_EVT alone would // hand HA the default 23. - bool seen_mtu_{false}; + bool seen_mtu_ : 1 {false}; + // Pre-started discovery latch: the search is issued at OPEN_EVT so it + // overlaps the MTU exchange (the replaced class's timing) and the + // consumer's discover_services() completes from it instead of paying a + // serialized ATT round trip. requested/done meet in + // deliver_pending_search_(). + bool search_prestarted_ : 1 {false}; + bool search_done_ : 1 {false}; + bool search_requested_ : 1 {false}; + // esp_gatt_status_t of the completed search, held until requested. + uint8_t search_status_{0}; }; } // namespace esphome::bluetooth_connection