diff --git a/esphome/components/ble_client/ble_client.cpp b/esphome/components/ble_client/ble_client.cpp index d41fb17961..25001c8f74 100644 --- a/esphome/components/ble_client/ble_client.cpp +++ b/esphome/components/ble_client/ble_client.cpp @@ -51,7 +51,9 @@ bool BLEClient::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_t es for (auto *node : this->nodes_) node->gattc_event_handler(event, esp_gattc_if, param); - if (!this->services_.empty() && this->all_nodes_established_()) { + // The release frees the GATT cache that BLEClientBase's CCCD lookup still needs. + // The last REG_FOR_NOTIFY event clears the counter before node dispatch, so the release still runs here. + if (!this->services_.empty() && !this->notify_registration_pending() && this->all_nodes_established_()) { this->release_services(); ESP_LOGD(TAG, "All clients established, services released"); } diff --git a/esphome/components/ble_client/ble_client.h b/esphome/components/ble_client/ble_client.h index f27bef332b..f20df31816 100644 --- a/esphome/components/ble_client/ble_client.h +++ b/esphome/components/ble_client/ble_client.h @@ -34,6 +34,11 @@ class BLEClientNode { // This should be transitioned to Established once the node no longer needs // the services/descriptors/characteristics of the parent client. This will // allow some memory to be freed. + // The parent frees the peer's GATT cache once every node reports Established. + // Never report Established while an operation that reads that cache is outstanding. + // - esp_ble_gattc_register_for_notify() completes asynchronously. + // - Register from ESP_GATTC_SEARCH_CMPL_EVT, then set this from ESP_GATTC_REG_FOR_NOTIFY_EVT. + // - BLEClientBase::register_for_notify() holds the release until the registration completes. espbt::ClientState node_state; BLEClient *parent() { return this->parent_; } diff --git a/esphome/components/ble_client/sensor/ble_sensor.cpp b/esphome/components/ble_client/sensor/ble_sensor.cpp index 4bd871dc81..60992f282e 100644 --- a/esphome/components/ble_client/sensor/ble_sensor.cpp +++ b/esphome/components/ble_client/sensor/ble_sensor.cpp @@ -77,8 +77,7 @@ void BLESensor::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_t ga this->handle = descr->handle; } if (this->notify_) { - auto status = esp_ble_gattc_register_for_notify(this->parent()->get_gattc_if(), - this->parent()->get_remote_bda(), chr->handle); + auto status = this->parent()->register_for_notify(chr->handle); if (status) { ESP_LOGW(TAG, "esp_ble_gattc_register_for_notify failed, status=%d", status); } diff --git a/esphome/components/ble_client/text_sensor/ble_text_sensor.cpp b/esphome/components/ble_client/text_sensor/ble_text_sensor.cpp index 7eaa6af076..6f09281922 100644 --- a/esphome/components/ble_client/text_sensor/ble_text_sensor.cpp +++ b/esphome/components/ble_client/text_sensor/ble_text_sensor.cpp @@ -77,8 +77,7 @@ void BLETextSensor::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_ this->handle = descr->handle; } if (this->notify_) { - auto status = esp_ble_gattc_register_for_notify(this->parent()->get_gattc_if(), - this->parent()->get_remote_bda(), chr->handle); + auto status = this->parent()->register_for_notify(chr->handle); if (status) { ESP_LOGW(TAG, "esp_ble_gattc_register_for_notify failed, status=%d", status); } diff --git a/esphome/components/esp32_ble_client/ble_client_base.cpp b/esphome/components/esp32_ble_client/ble_client_base.cpp index 3fb9632e9a..bd80f71a49 100644 --- a/esphome/components/esp32_ble_client/ble_client_base.cpp +++ b/esphome/components/esp32_ble_client/ble_client_base.cpp @@ -125,6 +125,9 @@ void BLEClientBase::connect() { } ESP_LOGI(TAG, "[%d] [%s] 0x%02x Connecting", this->connection_index_, this->address_str_, this->remote_addr_type_); this->paired_ = false; + // A registration whose event never arrived must not block this connection's release. + this->services_released_ = false; + this->pending_notify_regs_ = 0; // Enable loop for state processing this->enable_loop(); // Immediately transition to CONNECTING to prevent duplicate connection attempts @@ -200,10 +203,26 @@ void BLEClientBase::release_services() { this->services_.clear(); #endif #ifndef CONFIG_BT_GATTC_CACHE_NVS_FLASH + // Only the cache clean makes the stack's database unsafe to walk. + this->services_released_ = true; esp_ble_gattc_cache_clean(this->remote_bda_); #endif } +esp_err_t BLEClientBase::register_for_notify(uint16_t char_handle) { + esp_err_t err = esp_ble_gattc_register_for_notify(this->gattc_if_, this->remote_bda_, char_handle); + if (err != ESP_OK) + return err; + if (this->pending_notify_regs_ == UINT8_MAX) { + // Saturating undercounts, so the release can run before the last registration completes. + // Wrapping to zero would undercount by the full range instead, which is worse. + this->log_warning_("Too many outstanding notify registrations to track"); + return err; + } + this->pending_notify_regs_++; + return err; +} + void BLEClientBase::log_event_(const char *name) { ESP_LOGD(TAG, "[%d] [%s] %s", this->connection_index_, this->address_str_, name); } @@ -498,12 +517,20 @@ bool BLEClientBase::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_ } case ESP_GATTC_REG_FOR_NOTIFY_EVT: { this->log_gattc_data_event_("REG_FOR_NOTIFY"); + // The event carries no conn_id, so this is the only place the request can be retired. + if (this->pending_notify_regs_ > 0) + this->pending_notify_regs_--; if (this->connection_type_ == espbt::ConnectionType::V3_WITH_CACHE || this->connection_type_ == espbt::ConnectionType::V3_WITHOUT_CACHE) { // Client is responsible for flipping the descriptor value // when using the cache break; } + if (this->services_released_) { + // The lookup below walks the freed GATT cache, and Bluedroid asserts on it rather than erroring. + this->log_warning_("REG_FOR_NOTIFY after services released, notifications not enabled"); + break; + } esp_gattc_descr_elem_t desc_result; uint16_t count = 1; esp_gatt_status_t descr_status = esp_ble_gattc_get_descr_by_char_handle( diff --git a/esphome/components/esp32_ble_client/ble_client_base.h b/esphome/components/esp32_ble_client/ble_client_base.h index 0291a4b993..0902aad924 100644 --- a/esphome/components/esp32_ble_client/ble_client_base.h +++ b/esphome/components/esp32_ble_client/ble_client_base.h @@ -44,6 +44,12 @@ class BLEClientBase : public espbt::ESPBTClient, public Component { void unconditional_disconnect(); void release_services(); + /// Register for notifications, holding the service release until the registration completes. + esp_err_t register_for_notify(uint16_t char_handle); + + /// True while a register_for_notify() request has not completed. + bool notify_registration_pending() const { return this->pending_notify_regs_ > 0; } + bool connected() { return this->state() == espbt::ClientState::ESTABLISHED; } void set_auto_connect(bool auto_connect) { this->auto_connect_ = auto_connect; } @@ -125,9 +131,15 @@ class BLEClientBase : public espbt::ESPBTClient, public Component { espbt::ConnectionType connection_type_{espbt::ConnectionType::V1}; uint8_t connection_index_; uint8_t service_count_{0}; // ESP32 has max handles < 255, typical devices have < 50 services + // Outstanding register_for_notify() requests + // A count, not per-request state, so a raw esp_ble_gattc_register_for_notify() on the same client can retire one + // services_released_ is the backstop if that ever lets the release run early + uint8_t pending_notify_regs_{0}; bool auto_connect_{false}; bool paired_{false}; - // 6 bytes used, 2 bytes padding + // Set only when release_services() cleans the stack's GATT cache, which no API may then walk + bool services_released_{false}; + // 8 bytes used, no padding void log_event_(const char *name); void log_gattc_lifecycle_event_(const char *name);