[ble_client] Do not walk the GATT cache after releasing services (#17919)

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");
   }

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_; }

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(),
+        auto status = this->parent()->register_for_notify(chr->handle);
-                                                        this->parent()->get_remote_bda(), chr->handle);
         if (status) {
           ESP_LOGW(TAG, "esp_ble_gattc_register_for_notify failed, status=%d", status);
         }

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(),
+        auto status = this->parent()->register_for_notify(chr->handle);
-                                                        this->parent()->get_remote_bda(), chr->handle);
         if (status) {
           ESP_LOGW(TAG, "esp_ble_gattc_register_for_notify failed, status=%d", status);
         }

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(

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);