From c64684dd000105d3567e96b91d5fd0297f913745 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 9 Aug 2026 12:36:55 -0500 Subject: [PATCH 1/2] Tighten review-round comments to repo style --- .../bluetooth_connection_bluedroid.cpp | 56 ++++++++----------- .../bluetooth_connection_bluedroid.h | 6 +- .../bluetooth_connection_hub.cpp | 6 +- .../bluetooth_connection_hub.h | 13 ++--- 4 files changed, 32 insertions(+), 49 deletions(-) diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp index 43a52eb580..fcbb2cdf07 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp @@ -50,9 +50,8 @@ void BluedroidGattClient::setup() { void BluedroidGattClient::loop() { if (!esp32_ble::global_ble->is_active()) { - // Stack down: no CLOSE_EVT will ever come. Settle a link that was past - // IDLE so the consumer frees its slot instead of holding a phantom - // connection, then re-register the app on the next enable. + // Stack down: no CLOSE_EVT will come. Settle a live link so the consumer + // frees its slot, then re-register the app on the next enable. auto down_st = this->state(); if (down_st != ClientState::IDLE && down_st != ClientState::INIT) { this->release_services(); @@ -72,20 +71,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::DISCONNECTING || this->want_disconnect_) { - // The one teardown safety net (the wrapper has no timer): covers a lost - // CLOSE_EVT and a scheduled teardown whose OPEN_EVT never arrives. + // The one teardown safety net: a lost CLOSE_EVT, or a scheduled + // teardown whose OPEN_EVT never arrives. if (millis() - this->disconnecting_started_ > ble_device_base::GATT_DISCONNECT_TIMEOUT_MS) { ESP_LOGE(TAG, "[%d] Timeout waiting for teardown, forcing IDLE", this->connection_index_); - // Release before idling: unconditional disconnect does not release, - // and a lost completion would otherwise leak the table and the cache. + // Release before idling: a lost completion must not leak the cache. this->release_services(); this->set_idle_(); // also clears want_disconnect_ this->listener_->on_connection_state(false, 0, ESP_GATT_CONN_TIMEOUT); } } else { - // While a link exists the loop stays on watching for a stack-down (the - // settle above needs a tick to run) and flushing a pre-started search - // claimed outside an event drain; it settles only back at IDLE. + // The loop stays on while a link exists (stack-down watch, pre-started + // search flush); it settles only back at IDLE. this->deliver_pending_search_(); if (this->state() == ClientState::IDLE) { this->disable_loop(); @@ -131,9 +128,8 @@ void BluedroidGattClient::tracker_connect_() { this->services_released_ = false; this->seen_mtu_ = false; this->mtu_failed_ = false; - // Per-attempt reset: the stack-down path in loop() reaches IDLE through - // set_state() without set_idle_(), and a stale completed search would - // satisfy this connection's discovery with the previous one's result. + // 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(); @@ -174,8 +170,7 @@ int BluedroidGattClient::gatt_disconnect() { if (st == ClientState::CONNECTING || this->conn_id_ == UNSET_CONN_ID) { ESP_LOGD(TAG, "[%d] Disconnect scheduled", this->connection_index_); this->want_disconnect_ = true; - // The backend owns the whole safety window (the wrapper has no timer): - // a lost OPEN_EVT must not leak the scheduled teardown. + // Arm the safety window: a lost OPEN_EVT must not leak the teardown. this->disconnecting_started_ = millis(); this->enable_loop(); return 0; @@ -448,8 +443,8 @@ bool BluedroidGattClient::build_service_table_() { return true; }); if (!filled || char_index != char_total || desc_index != desc_total) { - // A walk error or a database that changed between the two passes; the - // consumer sees an empty table rather than a corrupt one. + // Walk error or the database changed between passes; better an empty + // table than a corrupt one. ESP_LOGW(TAG, "[%d] Service table walk mismatch, discarding", this->connection_index_); this->free_service_table_(); return false; @@ -507,8 +502,8 @@ void BluedroidGattClient::log_gattc_warning_(const char *operation, int code) { 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. + // Re-discovery moves the counts table_view_() derives offsets from; free + // the stale table. this->free_service_table_(); #endif // Step down from the fast discovery params. @@ -529,10 +524,8 @@ int BluedroidGattClient::handle_search_cmpl_() { return 0; } -// Reports a completed search once it has a claimant; a pre-started search -// stays silent until claimed. Delivery consumes the state, so a later -// discover_services() on the same connection issues a real search instead -// of re-reporting this result. +// Reports a completed search once claimed; delivery consumes the state so +// a re-discovery issues a real search. void BluedroidGattClient::deliver_pending_search_() { if (this->search_state_ != SearchState::REPORT_PENDING) return; @@ -733,8 +726,8 @@ void BluedroidGattClient::handle_open_evt_(esp_ble_gattc_cb_param_t *param) { this->search_state_ = SearchState::PRESTARTED; } if (this->mtu_failed_ && !this->seen_mtu_) { - // The MTU request was refused at CONNECT_EVT: report here with the - // default so the consumer proceeds instead of waiting forever. + // Refused MTU request: report with the default so the consumer + // proceeds. this->seen_mtu_ = true; this->listener_->on_connection_state(true, ble_device_base::DEFAULT_ATT_MTU, 0); this->deliver_pending_search_(); @@ -785,8 +778,7 @@ bool BluedroidGattClient::gattc_event_handler(esp_gattc_cb_event_t event, esp_ga auto ret = esp_ble_gattc_send_mtu_req(this->gattc_if_, param->connect.conn_id); if (ret) { this->log_gattc_warning_("esp_ble_gattc_send_mtu_req", ret); - // No CFG_MTU_EVT will follow: OPEN_EVT reports with the default MTU - // so the connection still reaches a reported state. + // No CFG_MTU_EVT will follow; OPEN_EVT reports with the default. this->mtu_failed_ = true; } break; @@ -836,9 +828,8 @@ 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_); if (this->state() == ClientState::DISCONNECTING) { - // 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). + // Teardown owns the link; the result is never delivered, skip the + // work. break; } this->search_status_ = this->handle_search_cmpl_(); @@ -894,9 +885,8 @@ void BluedroidGattClient::gap_event_handler(esp_gap_ble_cb_event_t event, esp_bl case ESP_GAP_BLE_SEC_REQ_EVT: { if (!this->check_addr_(param->ble_security.auth_cmpl.bd_addr)) break; - // Always accept a server-initiated security request. A refused - // response means no AUTH_CMPL will follow, so answer the pairing - // request with the failure instead of hanging it. + // Always accept; a refused response means no AUTH_CMPL, so answer the + // pairing request with the failure. int sec_err = this->check_and_log_error_("esp_ble_gap_security_rsp", esp_ble_gap_security_rsp(param->ble_security.ble_req.bd_addr, true)); if (sec_err != 0) { diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h index 298c7eb0d2..bed0e4aee1 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h @@ -146,10 +146,8 @@ class BluedroidGattClient final : public esp32_ble_tracker::ESPBTClient, public bool seen_mtu_ : 1 {false}; // The MTU request was refused at CONNECT_EVT; OPEN_EVT reports instead. bool mtu_failed_ : 1 {false}; - // Pre-started discovery: 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. Reset per attempt and on idle. + // Search issued at OPEN_EVT overlaps the MTU exchange; discover_services() + // completes from it. Reset per attempt and on idle. SearchState search_state_ : 4 {SearchState::NONE}; // esp_gatt_status_t of the completed search, held until claimed. uint8_t search_status_{0}; diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp b/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp index 70f22749f7..2d76040044 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp +++ b/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp @@ -46,10 +46,8 @@ void BluetoothConnection::disconnect() { } int err = this->backend_->gatt_disconnect(); if (err != 0) { - // Both backends return nonzero only when there is nothing to tear down - // (already idle): free the slot so the client is not stuck. Accepted - // teardowns always reach a terminal report - the backend owns the - // safety timer on every path. + // Nonzero means nothing to tear down (both backends): free the slot. + // Accepted teardowns always reach a terminal report. ESP_LOGW(TAG, "[%d] [%s] disconnect while backend idle, err=%d", this->connection_index_, this->address_str_, err); this->reset_connection_(err); return; diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_hub.h b/esphome/components/bluetooth_connection/bluetooth_connection_hub.h index 69113ed95e..5fb13f5e46 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_hub.h +++ b/esphome/components/bluetooth_connection/bluetooth_connection_hub.h @@ -41,9 +41,8 @@ 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. + /// Streamer abort: latch the GATT cause (first wins), park the cursor, + /// tear down. void abort_service_stream(conn_err_t err) { if (this->pending_error_ == 0) { this->pending_error_ = err; @@ -81,9 +80,8 @@ class BluetoothConnection final : public ble_device_base::GattClientListener { // an authoritative empty database). bool has_gatt_services() const { return this->services_discovered_; } - /// Stream any pending service-discovery batch. Called from the proxy's - /// loop — the wrapper has no Component loop of its own. The disconnect - /// safety timer lives in the backend, which owns every teardown path. + /// Stream any pending service-discovery batch (proxy loop; the backend + /// owns the disconnect safety timer). void process_pending_services() { if (this->send_service_ >= 0) { this->stream_pending_(this->backend_); @@ -137,8 +135,7 @@ class BluetoothConnection final : public ble_device_base::GattClientListener { // Group 4: Arrays char address_str_[MAC_ADDRESS_PRETTY_BUFFER_SIZE]{}; - // Group 5: bit-packed tail. address_ makes the object 8-aligned, so this - // group must stay within 2 bytes to keep the wrapper at 48 (dev parity). + // Group 5: bit-packed tail; within 2 bytes the 8-aligned object stays 48. ClientState state_ : 3 {ClientState::IDLE}; bool paired_ : 1 {false}; ConnectionType connection_type_ : 2 {ConnectionType::V1}; From d86bdb36cfec995bad1729ffb3e29510360a5dc6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 9 Aug 2026 12:42:17 -0500 Subject: [PATCH 2/2] Address review: honor SEARCH_CMPL status, close a late OPEN, backstops - handle_search_cmpl_ takes the event status: a failed discovery no longer reads as a clean zero-service success (the empty cached database satisfies the count calls), and the table invalidation plus param step-down still run on the failure path - A late successful OPEN_EVT on a slot the teardown net already gave up is closed instead of leaking a live controller link with no conn id - The counting pass logs its failure like the two paths below it - static_assert pins the wrapper's compile-time streamer detection; a signature drift would silently fall back to a table proxy builds compile without - find_characteristic/find_cccd range math widened to 32-bit (correct by type; the wrapped-end case was already loop-safe) --- .../ble_device_base/ble_gatt_client.h | 9 ++++--- .../bluetooth_connection_bluedroid.cpp | 25 +++++++++++++++---- .../bluetooth_connection_bluedroid.h | 2 +- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/esphome/components/ble_device_base/ble_gatt_client.h b/esphome/components/ble_device_base/ble_gatt_client.h index bb86dfa751..a1f0c4ae71 100644 --- a/esphome/components/ble_device_base/ble_gatt_client.h +++ b/esphome/components/ble_device_base/ble_gatt_client.h @@ -166,10 +166,11 @@ inline const GattService *find_service(const GattServiceTable &table, const ESPB inline const GattCharacteristic *find_characteristic(const GattServiceTable &table, const GattService &service, const ESPBTUUID &uuid) { - uint16_t end = service.first_characteristic + service.characteristic_count; + // 32-bit range math: a corrupt first/count pair cannot wrap past the check. + uint32_t end = uint32_t(service.first_characteristic) + service.characteristic_count; if (end > table.characteristic_count) return nullptr; - for (uint16_t i = service.first_characteristic; i < end; i++) { + for (uint32_t i = service.first_characteristic; i < end; i++) { if (table.characteristics[i].uuid == uuid) return &table.characteristics[i]; } @@ -179,11 +180,11 @@ inline const GattCharacteristic *find_characteristic(const GattServiceTable &tab /// Handle of the characteristic's Client Characteristic Configuration /// descriptor (0x2902), or 0 when it has none. inline uint16_t find_cccd(const GattServiceTable &table, const GattCharacteristic &characteristic) { - uint16_t end = characteristic.first_descriptor + characteristic.descriptor_count; + uint32_t end = uint32_t(characteristic.first_descriptor) + characteristic.descriptor_count; if (end > table.descriptor_count) return 0; const ESPBTUUID cccd_uuid = ESPBTUUID::from_uint16(CCCD_UUID); - for (uint16_t i = characteristic.first_descriptor; i < end; i++) { + for (uint32_t i = characteristic.first_descriptor; i < end; i++) { if (table.descriptors[i].uuid == cccd_uuid) return table.descriptors[i].handle; } diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp index fcbb2cdf07..0c4990037f 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp @@ -379,6 +379,7 @@ bool BluedroidGattClient::build_service_table_() { return true; }); if (!counted) { + ESP_LOGW(TAG, "[%d] Service table walk failed during count", this->connection_index_); return false; } @@ -500,7 +501,7 @@ void BluedroidGattClient::log_gattc_warning_(const char *operation, int code) { // ---- service streaming ---- -int BluedroidGattClient::handle_search_cmpl_() { +int BluedroidGattClient::handle_search_cmpl_(esp_gatt_status_t status) { #ifdef USE_BLE_GATT_SERVICE_TABLE // Re-discovery moves the counts table_view_() derives offsets from; free // the stale table. @@ -508,6 +509,12 @@ int BluedroidGattClient::handle_search_cmpl_() { #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"); + if (status != ESP_GATT_OK) { + // A failed discovery reads as a clean zero from the count calls below; + // honoring the event status stops it becoming an authoritative empty + // list. + return status; + } uint16_t primary = 0; uint16_t secondary = 0; auto primary_status = esp_ble_gattc_get_attr_count(this->gattc_if_, this->conn_id_, ESP_GATT_DB_PRIMARY_SERVICE, @@ -534,6 +541,11 @@ void BluedroidGattClient::deliver_pending_search_() { } #ifdef USE_BLUETOOTH_PROXY +// The wrapper's compile-time streamer detection must keep finding this +// method; a signature drift would silently fall back to the table streamer, +// which proxy builds compile without a materializer. +static_assert(requires(BluedroidGattClient c, BluetoothConnection &conn) { c.stream_service_batch(conn); }); + void BluedroidGattClient::stream_service_batch(BluetoothConnection &conn) { if (this->services_released_) { // Released under the stream: park without services-done so a partial @@ -687,9 +699,12 @@ void BluedroidGattClient::stream_service_batch(BluetoothConnection &conn) { void BluedroidGattClient::handle_open_evt_(esp_ble_gattc_cb_param_t *param) { auto st = this->state(); if (st == ClientState::IDLE) { - // IDF can deliver OPEN_EVT after esp_ble_gattc_open already returned an - // error and the slot went IDLE; do not resurrect it. - ESP_LOGD(TAG, "[%d] OPEN_EVT in IDLE state (status=%d), ignoring", this->connection_index_, param->open.status); + // Late OPEN_EVT after the slot went IDLE (open-error race, or the + // teardown net gave up): close a won link, never resurrect the slot. + ESP_LOGD(TAG, "[%d] OPEN_EVT in IDLE state (status=%d)", this->connection_index_, param->open.status); + if (param->open.status == ESP_GATT_OK || param->open.status == ESP_GATT_ALREADY_OPEN) { + esp_ble_gattc_close(this->gattc_if_, param->open.conn_id); + } return; } if (st != ClientState::CONNECTING) { @@ -832,7 +847,7 @@ bool BluedroidGattClient::gattc_event_handler(esp_gattc_cb_event_t event, esp_ga // work. break; } - this->search_status_ = this->handle_search_cmpl_(); + this->search_status_ = this->handle_search_cmpl_(static_cast(param->search_cmpl.status)); this->search_state_ = this->search_state_ == SearchState::CLAIMED ? SearchState::REPORT_PENDING : SearchState::PRESTART_DONE; this->set_state(ClientState::ESTABLISHED); diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h index bed0e4aee1..b59fa98cbc 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h @@ -93,7 +93,7 @@ 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); - int handle_search_cmpl_(); + int handle_search_cmpl_(esp_gatt_status_t status); void deliver_pending_search_(); void unconditional_disconnect_(); void set_idle_();