From 2f71ec16d77cd896f07ebaacf4d93fd6d71a6ccd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 9 Aug 2026 11:19:25 -0500 Subject: [PATCH] Close the silent-failure paths from review - A refused MTU request no longer wedges the connection: OPEN_EVT reports with the default MTU so the consumer proceeds (previously no CFG_MTU_EVT meant no connected report and a slot that never freed) - Disabling the BLE stack settles a live link with a connected=false report before forgetting it, so the consumer frees its slot instead of holding a phantom connection - A refused security response answers the pairing request with the failure instead of hanging it - The service-table walk mismatch logs its reason before discarding - The shared streamer's two bounds-check aborts use abort_service_stream with an ATT Unlikely Error cause (new shared GATT_ERR_UNLIKELY) --- .../ble_device_base/ble_client_state.h | 3 ++ .../bluetooth_connection_bluedroid.cpp | 34 +++++++++++++++++-- .../bluetooth_connection_bluedroid.h | 2 ++ .../bluetooth_connection_hub.cpp | 6 ++-- 4 files changed, 38 insertions(+), 7 deletions(-) diff --git a/esphome/components/ble_device_base/ble_client_state.h b/esphome/components/ble_device_base/ble_client_state.h index a8909d643d..92754b70b4 100644 --- a/esphome/components/ble_device_base/ble_client_state.h +++ b/esphome/components/ble_device_base/ble_client_state.h @@ -17,6 +17,9 @@ namespace esphome::ble_device_base { /// client backend. static constexpr int GATT_ERR_NOT_CONNECTED = -1; static constexpr int GATT_ERR_NO_MEMORY = -2; +/// ATT "Unlikely Error" (spec 0x0E): a client-side internal inconsistency, +/// e.g. a service table failing its own bounds checks. +static constexpr int GATT_ERR_UNLIKELY = 0x0E; /// Safety net shared by every GATT backend: force IDLE when the stack never /// delivers its disconnect completion. diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp index abb3f4efe5..2d095cb3ef 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.cpp @@ -51,7 +51,15 @@ void BluedroidGattClient::setup() { void BluedroidGattClient::loop() { if (!esp32_ble::global_ble->is_active()) { - // Stack down: re-register the app on the next enable. + // 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. + auto down_st = this->state(); + if (down_st != ClientState::IDLE && down_st != ClientState::INIT) { + this->release_services(); + this->set_idle_(); + this->listener_->on_connection_state(false, 0, ble_device_base::GATT_ERR_NOT_CONNECTED); + } this->set_state(ClientState::INIT); return; } @@ -120,6 +128,7 @@ void BluedroidGattClient::tracker_connect_() { ESP_LOGI(TAG, "[%d] 0x%02x Connecting", this->connection_index_, this->remote_addr_type_); 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. @@ -433,6 +442,9 @@ 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. + ESP_LOGW(TAG, "[%d] Service table walk mismatch, discarding", this->connection_index_); this->free_service_table_(); return false; } @@ -718,6 +730,13 @@ void BluedroidGattClient::handle_open_evt_(esp_ble_gattc_cb_param_t *param) { esp_ble_gattc_search_service(this->gattc_if_, this->conn_id_, nullptr)) == 0) { 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. + this->seen_mtu_ = true; + this->listener_->on_connection_state(true, ble_device_base::DEFAULT_ATT_MTU, 0); + this->deliver_pending_search_(); + } } } @@ -764,6 +783,9 @@ 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. + this->mtu_failed_ = true; } break; } @@ -875,8 +897,14 @@ 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. - esp_ble_gap_security_rsp(param->ble_security.ble_req.bd_addr, true); + // 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. + 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) { + this->listener_->on_pairing_result(sec_err); + } break; } case ESP_GAP_BLE_AUTH_CMPL_EVT: { diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h index 799847f32e..6ec61cb729 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h +++ b/esphome/components/bluetooth_connection/bluetooth_connection_bluedroid.h @@ -146,6 +146,8 @@ class BluedroidGattClient final : public esp32_ble_tracker::ESPBTClient, public // The connected report waits for the MTU exchange; OPEN_EVT alone would // hand HA the default 23. 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 diff --git a/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp b/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp index 755b534463..062e32b474 100644 --- a/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp +++ b/esphome/components/bluetooth_connection/bluetooth_connection_hub.cpp @@ -382,8 +382,7 @@ void BluetoothConnection::send_service_for_discovery_() { if (char_count != 0 && service.first_characteristic + char_count > table.characteristic_count) { ESP_LOGE(TAG, "[%d] [%s] Characteristic range out of bounds (service %d), aborting stream", this->connection_index_, this->address_str_, this->send_service_); - this->send_service_ = DONE_SENDING_SERVICES; - this->disconnect(); + this->abort_service_stream(ble_device_base::GATT_ERR_UNLIKELY); return; } if (char_count > 0) { @@ -399,8 +398,7 @@ void BluetoothConnection::send_service_for_discovery_() { if (desc_count != 0 && chr.first_descriptor + desc_count > table.descriptor_count) { ESP_LOGE(TAG, "[%d] [%s] Descriptor range out of bounds (service %d), aborting stream", this->connection_index_, this->address_str_, this->send_service_); - this->send_service_ = DONE_SENDING_SERVICES; - this->disconnect(); + this->abort_service_stream(ble_device_base::GATT_ERR_UNLIKELY); return; } if (desc_count == 0) {