From eca0c219669ae48bd64328dde15ab88e10c1f5fc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 25 Feb 2025 20:13:30 +0000 Subject: [PATCH] Fix bluetooth race when disconnect called while still connecting (#8297) --- .../esp32_ble_client/ble_client_base.cpp | 70 ++++++++++++++++++- .../esp32_ble_client/ble_client_base.h | 5 +- .../esp32_ble_tracker/esp32_ble_tracker.h | 12 +++- 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/esphome/components/esp32_ble_client/ble_client_base.cpp b/esphome/components/esp32_ble_client/ble_client_base.cpp index 53c430350c..2a1757524f 100644 --- a/esphome/components/esp32_ble_client/ble_client_base.cpp +++ b/esphome/components/esp32_ble_client/ble_client_base.cpp @@ -123,12 +123,49 @@ void BLEClientBase::connect() { esp_err_t BLEClientBase::pair() { return esp_ble_set_encryption(this->remote_bda_, ESP_BLE_SEC_ENCRYPT); } void BLEClientBase::disconnect() { - if (this->state_ == espbt::ClientState::IDLE || this->state_ == espbt::ClientState::DISCONNECTING) + if (this->state_ == espbt::ClientState::IDLE) { + ESP_LOGI(TAG, "[%d] [%s] Disconnect requested, but already idle.", this->connection_index_, + this->address_str_.c_str()); return; - ESP_LOGI(TAG, "[%d] [%s] Disconnecting.", this->connection_index_, this->address_str_.c_str()); + } + if (this->state_ == espbt::ClientState::DISCONNECTING) { + ESP_LOGI(TAG, "[%d] [%s] Disconnect requested, but already disconnecting.", this->connection_index_, + this->address_str_.c_str()); + return; + } + if (this->state_ == espbt::ClientState::CONNECTING || this->conn_id_ == UNSET_CONN_ID) { + ESP_LOGW(TAG, "[%d] [%s] Disconnecting before connected, disconnect scheduled.", this->connection_index_, + this->address_str_.c_str()); + this->want_disconnect_ = true; + return; + } + this->unconditional_disconnect(); +} + +void BLEClientBase::unconditional_disconnect() { + // Disconnect without checking the state. + ESP_LOGI(TAG, "[%d] [%s] Disconnecting (conn_id: %d).", this->connection_index_, this->address_str_.c_str(), + this->conn_id_); + if (this->state_ == espbt::ClientState::DISCONNECTING) { + ESP_LOGE(TAG, "[%d] [%s] Tried to disconnect while already disconnecting.", this->connection_index_, + this->address_str_.c_str()); + return; + } + if (this->conn_id_ == UNSET_CONN_ID) { + ESP_LOGE(TAG, "[%d] [%s] No connection ID set, cannot disconnect.", this->connection_index_, + this->address_str_.c_str()); + return; + } auto err = esp_ble_gattc_close(this->gattc_if_, this->conn_id_); if (err != ESP_OK) { - ESP_LOGW(TAG, "[%d] [%s] esp_ble_gattc_close error, err=%d", this->connection_index_, this->address_str_.c_str(), + // + // This is a fatal error, but we can't do anything about it + // and it likely means the BLE stack is in a bad state. + // + // In the future we might consider App.reboot() here since + // the BLE stack is in an indeterminate state. + // + ESP_LOGE(TAG, "[%d] [%s] esp_ble_gattc_close error, err=%d", this->connection_index_, this->address_str_.c_str(), err); } @@ -184,12 +221,38 @@ bool BLEClientBase::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_ this->log_event_("ESP_GATTC_OPEN_EVT"); this->conn_id_ = param->open.conn_id; this->service_count_ = 0; + if (this->state_ != espbt::ClientState::CONNECTING) { + // This should not happen but lets log it in case it does + // because it means we have a bad assumption about how the + // ESP BT stack works. + if (this->state_ == espbt::ClientState::CONNECTED) { + ESP_LOGE(TAG, "[%d] [%s] Got ESP_GATTC_OPEN_EVT while already connected, status=%d", this->connection_index_, + this->address_str_.c_str(), param->open.status); + } else if (this->state_ == espbt::ClientState::ESTABLISHED) { + ESP_LOGE(TAG, "[%d] [%s] Got ESP_GATTC_OPEN_EVT while already established, status=%d", + this->connection_index_, this->address_str_.c_str(), param->open.status); + } else if (this->state_ == espbt::ClientState::DISCONNECTING) { + ESP_LOGE(TAG, "[%d] [%s] Got ESP_GATTC_OPEN_EVT while disconnecting, status=%d", this->connection_index_, + this->address_str_.c_str(), param->open.status); + } else { + ESP_LOGE(TAG, "[%d] [%s] Got ESP_GATTC_OPEN_EVT while not in connecting state, status=%d", + this->connection_index_, this->address_str_.c_str(), param->open.status); + } + } if (param->open.status != ESP_GATT_OK && param->open.status != ESP_GATT_ALREADY_OPEN) { ESP_LOGW(TAG, "[%d] [%s] Connection failed, status=%d", this->connection_index_, this->address_str_.c_str(), param->open.status); this->set_state(espbt::ClientState::IDLE); break; } + if (this->want_disconnect_) { + // Disconnect was requested after connecting started, + // but before the connection was established. Now that we have + // this->conn_id_ set, we can disconnect it. + this->unconditional_disconnect(); + this->conn_id_ = UNSET_CONN_ID; + break; + } auto ret = esp_ble_gattc_send_mtu_req(this->gattc_if_, param->open.conn_id); if (ret) { ESP_LOGW(TAG, "[%d] [%s] esp_ble_gattc_send_mtu_req failed, status=%x", this->connection_index_, @@ -241,6 +304,7 @@ bool BLEClientBase::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_ this->log_event_("ESP_GATTC_CLOSE_EVT"); this->release_services(); this->set_state(espbt::ClientState::IDLE); + this->conn_id_ = UNSET_CONN_ID; break; } case ESP_GATTC_SEARCH_RES_EVT: { diff --git a/esphome/components/esp32_ble_client/ble_client_base.h b/esphome/components/esp32_ble_client/ble_client_base.h index 84c35c4633..89ac04e38c 100644 --- a/esphome/components/esp32_ble_client/ble_client_base.h +++ b/esphome/components/esp32_ble_client/ble_client_base.h @@ -21,6 +21,8 @@ namespace esp32_ble_client { namespace espbt = esphome::esp32_ble_tracker; +static const int UNSET_CONN_ID = 0xFFFF; + class BLEClientBase : public espbt::ESPBTClient, public Component { public: void setup() override; @@ -37,6 +39,7 @@ class BLEClientBase : public espbt::ESPBTClient, public Component { void connect() override; esp_err_t pair(); void disconnect() override; + void unconditional_disconnect(); void release_services(); bool connected() { return this->state_ == espbt::ClientState::ESTABLISHED; } @@ -94,7 +97,7 @@ class BLEClientBase : public espbt::ESPBTClient, public Component { int gattc_if_; esp_bd_addr_t remote_bda_; esp_ble_addr_type_t remote_addr_type_{BLE_ADDR_TYPE_PUBLIC}; - uint16_t conn_id_{0xFFFF}; + uint16_t conn_id_{UNSET_CONN_ID}; uint64_t address_{0}; bool auto_connect_{false}; std::string address_str_{}; diff --git a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h index 52b091619e..99126f9173 100644 --- a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h +++ b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h @@ -173,12 +173,22 @@ class ESPBTClient : public ESPBTDeviceListener { virtual void gap_event_handler(esp_gap_ble_cb_event_t event, esp_ble_gap_cb_param_t *param) = 0; virtual void connect() = 0; virtual void disconnect() = 0; - virtual void set_state(ClientState st) { this->state_ = st; } + virtual void set_state(ClientState st) { + this->state_ = st; + if (st == ClientState::IDLE) { + this->want_disconnect_ = false; + } + } ClientState state() const { return state_; } int app_id; protected: ClientState state_{ClientState::INIT}; + // want_disconnect_ is set to true when a disconnect is requested + // while the client is connecting. This is used to disconnect the + // client as soon as we get the connection id (conn_id_) from the + // ESP_GATTC_OPEN_EVT event. + bool want_disconnect_{false}; }; class ESP32BLETracker : public Component,