Deduplicate the review-round additions

One ConnectBackoff shared by both engines replaces the forked hold-off fields and constants (the duration is derived from the failure count). The pending-registration scan lives once, sized by its own bound instead of the node counter. The failure-suppressing SEARCH_CMPL arm moves to the call site so the dispatcher stays void, notify-state logging is one helper on the neutral wording, and the op-check helper takes the backend's spelling.
2026-08-24 07:06:20 +00:00 · 2026-08-12 17:56:29 -05:00
parent e4e464a3b9
commit 314557f83b
5 changed files with 115 additions and 106 deletions
@@ -17,12 +17,6 @@ namespace esphome::ble_client {

 static const char *const TAG = "ble_client";

-#ifdef USE_BLE_CLIENT_GATT_NODES
-// Hold-off per consecutive materializer failure (neutral-engine parity).
-static const uint32_t GATT_FAILURE_HOLD_OFF_STEP_MS = 10000;
-static const uint8_t GATT_FAILURE_HOLD_OFF_MAX_STEPS = 6;
-#endif
-
 void BLEClient::setup() {
  BLEClientBase::setup();
  this->enabled = true;
@@ -43,8 +37,7 @@ bool BLEClient::parse_device(const espbt::ESPBTDevice &device) {
  if (!this->enabled)
    return false;
 #ifdef USE_BLE_CLIENT_GATT_NODES
-  if (this->gatt_hold_off_ms_ != 0 && millis() - this->gatt_hold_off_start_ < this->gatt_hold_off_ms_ &&
-      device.address_uint64() == this->address_)
+  if (device.address_uint64() == this->address_ && this->gatt_backoff_.holding_off())
    return false;
 #endif
  return BLEClientBase::parse_device(device);
@@ -61,8 +54,7 @@ void BLEClient::set_enabled(bool enabled) {
  }
 #ifdef USE_BLE_CLIENT_GATT_NODES
  // A re-enable clears the backoff (neutral-engine parity).
-  this->gatt_consecutive_failures_ = 0;
-  this->gatt_hold_off_ms_ = 0;
+  this->gatt_backoff_.reset();
 #endif
 }

@@ -78,12 +70,7 @@ bool BLEClient::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_t es
    if (this->pending_notify_regs_ > 0)
      this->pending_notify_regs_--;
    int err = param->reg_for_notify.status == ESP_GATT_OK ? 0 : param->reg_for_notify.status;
-    if (err != 0) {
-      ESP_LOGW(TAG, "[%s] Notify enable on handle 0x%04x failed, status=%d", this->address_str(),
-               param->reg_for_notify.handle, err);
-    }
-    for (auto *node : this->gatt_nodes_)
-      node->on_notify_state(param->reg_for_notify.handle, true, err);
+    this->notify_state_to_gatt_nodes_(param->reg_for_notify.handle, true, err);
    // A retiring last registration must still release the cache.
    this->maybe_release_services_();
    return true;
@@ -94,9 +81,15 @@ bool BLEClient::gattc_event_handler(esp_gattc_cb_event_t event, esp_gatt_if_t es

 #ifdef USE_BLE_CLIENT_GATT_NODES
  // Before the legacy fan-out so gatt nodes resolve before any trigger fires.
-  if (!this->dispatch_gatt_event_(event, param)) {
-    // Failed discovery: the on_connect trigger must not fire into the teardown.
-    return true;
+  if (!this->gatt_nodes_.empty()) {
+    if (event == ESP_GATTC_SEARCH_CMPL_EVT) {
+      // A failed discovery tears the link down; the on_connect trigger must
+      // not fire into the teardown.
+      if (!this->handle_gatt_search_cmpl_(param->search_cmpl.status))
+        return true;
+    } else {
+      this->dispatch_gatt_event_(event, param);
+    }
  }
 #endif
  for (auto *node : this->nodes_)
@@ -160,23 +153,34 @@ void BLEClient::register_gatt_node(BLEClientNode *node) {
  this->register_ble_node(node);
 }

-bool BLEClient::take_pending_gatt_reg_(uint16_t handle) {
-  // No duplicates (notify_characteristic refuses a re-push); swap-with-last.
+int BLEClient::find_pending_gatt_reg_(uint16_t handle) const {
  for (uint8_t i = 0; i < this->pending_gatt_reg_count_; i++) {
-    if (this->pending_gatt_regs_[i] == handle) {
-      this->pending_gatt_regs_[i] = this->pending_gatt_regs_[--this->pending_gatt_reg_count_];
-      return true;
-    }
+    if (this->pending_gatt_regs_[i] == handle)
+      return i;
  }
-  return false;
+  return -1;
 }

-bool BLEClient::dispatch_gatt_event_(esp_gattc_cb_event_t event, esp_ble_gattc_cb_param_t *param) {
-  if (this->gatt_nodes_.empty())
-    return true;
+bool BLEClient::take_pending_gatt_reg_(uint16_t handle) {
+  int i = this->find_pending_gatt_reg_(handle);
+  if (i < 0)
+    return false;
+  // No duplicates (notify_characteristic refuses a re-push); swap-with-last.
+  this->pending_gatt_regs_[i] = this->pending_gatt_regs_[--this->pending_gatt_reg_count_];
+  return true;
+}
+
+void BLEClient::notify_state_to_gatt_nodes_(uint16_t handle, bool enabled, int error) {
+  if (error != 0) {
+    ESP_LOGW(TAG, "[%s] Notify %s on handle 0x%04x failed, status=%d", this->address_str(),
+             enabled ? "enable" : "disable", handle, error);
+  }
+  for (auto *node : this->gatt_nodes_)
+    node->on_notify_state(handle, enabled, error);
+}
+
+void BLEClient::dispatch_gatt_event_(esp_gattc_cb_event_t event, esp_ble_gattc_cb_param_t *param) {
  switch (event) {
-    case ESP_GATTC_SEARCH_CMPL_EVT:
-      return this->handle_gatt_search_cmpl_(param->search_cmpl.status);
    case ESP_GATTC_READ_CHAR_EVT:
    case ESP_GATTC_READ_DESCR_EVT: {
      bool ok = param->read.status == ESP_GATT_OK;
@@ -209,19 +213,15 @@ bool BLEClient::dispatch_gatt_event_(esp_gattc_cb_event_t event, esp_ble_gattc_c
      break;
    case ESP_GATTC_UNREG_FOR_NOTIFY_EVT:
      // The base does no CCCD work for unregister; no interception needed.
-      for (auto *node : this->gatt_nodes_) {
-        node->on_notify_state(param->unreg_for_notify.handle, false,
-                              param->unreg_for_notify.status == ESP_GATT_OK ? 0 : param->unreg_for_notify.status);
-      }
+      this->notify_state_to_gatt_nodes_(
+          param->unreg_for_notify.handle, false,
+          param->unreg_for_notify.status == ESP_GATT_OK ? 0 : param->unreg_for_notify.status);
      break;
    default:
      break;
  }
-  return true;
 }

-// False = failed discovery: the link comes down and the caller suppresses
-// the legacy fan-out.
 bool BLEClient::handle_gatt_search_cmpl_(esp_gatt_status_t status) {
  // The base ignores the search status; the neutral contract must not.
  uint16_t service_total = 0;
@@ -234,7 +234,7 @@ bool BLEClient::handle_gatt_search_cmpl_(esp_gatt_status_t status) {
    // Distinguishes a failed search/count from a genuinely service-less peer.
    ESP_LOGW(TAG, "[%s] Service table unavailable (status=%d, services=%u); treating as failed discovery",
             this->address_str(), status, service_total);
-    this->register_gatt_failure_();
+    this->gatt_backoff_.register_failure(this->address_str());
    this->disconnect();
    return false;
  }
@@ -252,20 +252,10 @@ bool BLEClient::handle_gatt_search_cmpl_(esp_gatt_status_t status) {
  // Promote so the legacy release condition can fire.
  for (auto *node : this->gatt_nodes_)
    node->node_state = espbt::ClientState::ESTABLISHED;
-  this->gatt_consecutive_failures_ = 0;
-  this->gatt_hold_off_ms_ = 0;
+  this->gatt_backoff_.reset();
  return true;
 }

-void BLEClient::register_gatt_failure_() {
-  if (this->gatt_consecutive_failures_ < GATT_FAILURE_HOLD_OFF_MAX_STEPS)
-    this->gatt_consecutive_failures_++;
-  this->gatt_hold_off_start_ = millis();
-  this->gatt_hold_off_ms_ = this->gatt_consecutive_failures_ * GATT_FAILURE_HOLD_OFF_STEP_MS;
-  ESP_LOGW(TAG, "[%s] Holding off reconnect for %u s", this->address_str(),
-           this->gatt_consecutive_failures_ * (GATT_FAILURE_HOLD_OFF_STEP_MS / 1000));
-}
-
 void BLEClient::on_disconnect_complete(esp_err_t reason) {
  this->pending_gatt_reg_count_ = 0;
  if (!this->gatt_connected_)
@@ -275,7 +265,7 @@ void BLEClient::on_disconnect_complete(esp_err_t reason) {
    node->on_disconnected();
 }

-int BLEClient::check_gatt_op_(const char *operation, esp_err_t err) {
+int BLEClient::check_and_log_error_(const char *operation, esp_err_t err) {
  if (err != ESP_OK)
    this->log_gattc_warning_(operation, err);
  return err;
@@ -284,7 +274,7 @@ int BLEClient::check_gatt_op_(const char *operation, esp_err_t err) {
 int BLEClient::write_characteristic(uint16_t handle, const uint8_t *data, uint16_t len, bool response) {
  if (this->conn_id_ == UNSET_CONN_ID)
    return ble_device_base::GATT_ERR_NOT_CONNECTED;
-  return this->check_gatt_op_(
+  return this->check_and_log_error_(
      "esp_ble_gattc_write_char",
      esp_ble_gattc_write_char(this->gattc_if_, this->conn_id_, handle, len, const_cast<uint8_t *>(data),
                               response ? ESP_GATT_WRITE_TYPE_RSP : ESP_GATT_WRITE_TYPE_NO_RSP,
@@ -294,14 +284,14 @@ int BLEClient::write_characteristic(uint16_t handle, const uint8_t *data, uint16
 int BLEClient::read_characteristic(uint16_t handle) {
  if (this->conn_id_ == UNSET_CONN_ID)
    return ble_device_base::GATT_ERR_NOT_CONNECTED;
-  return this->check_gatt_op_("esp_ble_gattc_read_char",
-                              esp_ble_gattc_read_char(this->gattc_if_, this->conn_id_, handle, ESP_GATT_AUTH_REQ_NONE));
+  return this->check_and_log_error_("esp_ble_gattc_read_char", esp_ble_gattc_read_char(this->gattc_if_, this->conn_id_,
+                                                                                       handle, ESP_GATT_AUTH_REQ_NONE));
 }

 int BLEClient::read_descriptor(uint16_t handle) {
  if (this->conn_id_ == UNSET_CONN_ID)
    return ble_device_base::GATT_ERR_NOT_CONNECTED;
-  return this->check_gatt_op_(
+  return this->check_and_log_error_(
      "esp_ble_gattc_read_char_descr",
      esp_ble_gattc_read_char_descr(this->gattc_if_, this->conn_id_, handle, ESP_GATT_AUTH_REQ_NONE));
 }
@@ -309,7 +299,7 @@ int BLEClient::read_descriptor(uint16_t handle) {
 int BLEClient::write_descriptor(uint16_t handle, const uint8_t *data, uint16_t len) {
  if (this->conn_id_ == UNSET_CONN_ID)
    return ble_device_base::GATT_ERR_NOT_CONNECTED;
-  return this->check_gatt_op_(
+  return this->check_and_log_error_(
      "esp_ble_gattc_write_char_descr",
      esp_ble_gattc_write_char_descr(this->gattc_if_, this->conn_id_, handle, len, const_cast<uint8_t *>(data),
                                     ESP_GATT_WRITE_TYPE_RSP, ESP_GATT_AUTH_REQ_NONE));
@@ -319,14 +309,12 @@ int BLEClient::notify_characteristic(uint16_t handle, bool enable) {
  if (this->conn_id_ == UNSET_CONN_ID)
    return ble_device_base::GATT_ERR_NOT_CONNECTED;
  if (enable) {
-    for (uint8_t i = 0; i < this->pending_gatt_reg_count_; i++) {
-      if (this->pending_gatt_regs_[i] == handle) {
-        // ESP_OK: the in-flight registration's completion fans out to all nodes.
-        ESP_LOGW(TAG, "[%s] Notify registration already pending for handle 0x%04x", this->address_str(), handle);
-        return ESP_OK;
-      }
+    if (this->find_pending_gatt_reg_(handle) >= 0) {
+      // ESP_OK: the in-flight registration's completion fans out to all nodes.
+      ESP_LOGW(TAG, "[%s] Notify registration already pending for handle 0x%04x", this->address_str(), handle);
+      return ESP_OK;
    }
-    if (this->pending_gatt_reg_count_ == ESPHOME_BLE_CLIENT_MAX_NODES) {
+    if (this->pending_gatt_reg_count_ == MAX_PENDING_NOTIFY_REGS) {
      // An untracked registration would let the base's auto-CCCD through.
      ESP_LOGE(TAG, "[%s] Too many pending notify registrations", this->address_str());
      return ble_device_base::GATT_ERR_NO_MEMORY;
@@ -338,8 +326,8 @@ int BLEClient::notify_characteristic(uint16_t handle, bool enable) {
      this->pending_gatt_regs_[this->pending_gatt_reg_count_++] = handle;
    return err;
  }
-  return this->check_gatt_op_("esp_ble_gattc_unregister_for_notify",
-                              esp_ble_gattc_unregister_for_notify(this->gattc_if_, this->remote_bda_, handle));
+  return this->check_and_log_error_("esp_ble_gattc_unregister_for_notify",
+                                    esp_ble_gattc_unregister_for_notify(this->gattc_if_, this->remote_bda_, handle));
 }

 int BLEClient::unpair() { return bluetooth_connection::unpair_device(this->get_address()); }
@@ -5,6 +5,7 @@
 #ifdef USE_ESP32

 #include "ble_client_node.h"
+#include "connect_backoff.h"
 #include "esphome/components/esp32_ble_client/ble_client_base.h"
 #include "esphome/components/esp32_ble_tracker/esp32_ble_tracker.h"
 #include "esphome/core/component.h"
@@ -14,8 +15,6 @@
 #include <esp_gap_ble_api.h>
 #include <esp_gatt_common_api.h>
 #include <esp_gattc_api.h>
-#include <array>
-#include <string>
 #include <vector>

 namespace esphome::ble_client {
@@ -69,9 +68,12 @@ class BLEClient final : public BLEClientBase {
  bool all_nodes_established_();
  void maybe_release_services_();
 #ifdef USE_BLE_CLIENT_GATT_NODES
-  int check_gatt_op_(const char *operation, esp_err_t err);
-  void register_gatt_failure_();
-  bool dispatch_gatt_event_(esp_gattc_cb_event_t event, esp_ble_gattc_cb_param_t *param);
+  int check_and_log_error_(const char *operation, esp_err_t err);
+  int find_pending_gatt_reg_(uint16_t handle) const;
+  void notify_state_to_gatt_nodes_(uint16_t handle, bool enabled, int error);
+  void dispatch_gatt_event_(esp_gattc_cb_event_t event, esp_ble_gattc_cb_param_t *param);
+  // False = failed discovery: the link comes down and the caller suppresses
+  // the legacy fan-out.
  bool handle_gatt_search_cmpl_(esp_gatt_status_t status);
  bool take_pending_gatt_reg_(uint16_t handle);
  void on_disconnect_complete(esp_err_t reason) override;
@@ -81,16 +83,16 @@ class BLEClient final : public BLEClientBase {
 #ifdef USE_BLE_CLIENT_GATT_NODES
  // Nodes on the neutral surface; fed the translated callbacks and
  // auto-established after the on_connected fan-out.
+  static constexpr uint8_t MAX_PENDING_NOTIFY_REGS = 4;
+
  StaticVector<BLEClientNode *, ESPHOME_BLE_CLIENT_MAX_NODES> gatt_nodes_;
+  // Reconnect backoff after materializer failures.
+  ConnectBackoff gatt_backoff_;
  // Bridge-initiated notify registrations awaiting REG_FOR_NOTIFY_EVT.
-  uint16_t pending_gatt_regs_[ESPHOME_BLE_CLIENT_MAX_NODES];
+  uint16_t pending_gatt_regs_[MAX_PENDING_NOTIFY_REGS];
  uint8_t pending_gatt_reg_count_{0};
  // on_connected fan-out started; on_disconnected is owed at teardown.
  bool gatt_connected_{false};
-  // Reconnect backoff after materializer failures (wrap-safe start+duration).
-  uint32_t gatt_hold_off_start_{0};
-  uint32_t gatt_hold_off_ms_{0};
-  uint8_t gatt_consecutive_failures_{0};
 #endif
 };

@@ -9,11 +9,6 @@ namespace esphome::ble_client {

 static const char *const TAG = "ble_client";

-// Hold-off step per consecutive failure; capped so a flapping peer retries
-// within a minute at worst.
-static const uint32_t FAILURE_HOLD_OFF_STEP_MS = 10000;
-static const uint8_t FAILURE_HOLD_OFF_MAX_STEPS = 6;
-
 void BLEClient::register_ble_node(BLEClientNode *node) {
  node->set_ble_client_parent(this);
  if (this->nodes_.size() == ESPHOME_BLE_CLIENT_MAX_NODES) {
@@ -43,8 +38,7 @@ void BLEClient::set_enabled(bool enabled) {
  }
  // A re-enable clears the backoff; the next sighting connects (legacy
  // parity: enabling does not itself connect).
-  this->consecutive_failures_ = 0;
-  this->hold_off_ms_ = 0;
+  this->backoff_.reset();
 }

 bool BLEClient::parse_device(const ble_device_base::ESPBTDevice &device) {
@@ -55,7 +49,7 @@ bool BLEClient::parse_device(const ble_device_base::ESPBTDevice &device) {
  this->address_type_known_ = true;
  if (!this->enabled || !this->auto_connect_ || this->state_ != State::IDLE)
    return true;
-  if (this->hold_off_ms_ != 0 && millis() - this->hold_off_start_ < this->hold_off_ms_)
+  if (this->backoff_.holding_off())
    return true;
  this->attempt_connect_();
  return true;
@@ -84,7 +78,7 @@ void BLEClient::attempt_connect_() {
    // backoff, and resolve any waiting connect action through the failure
    // path so its chain terminates.
    ESP_LOGW(TAG, "[%s] Connect refused, err=%d", this->address_str_, err);
-    this->register_failure_();
+    this->backoff_.register_failure(this->address_str_);
    this->defer([this]() { this->connect_failed_callbacks_.call(); });
    return;
  }
@@ -108,15 +102,6 @@ void BLEClient::disconnect() {
  }
 }

-void BLEClient::register_failure_() {
-  if (this->consecutive_failures_ < FAILURE_HOLD_OFF_MAX_STEPS)
-    this->consecutive_failures_++;
-  this->hold_off_start_ = millis();
-  this->hold_off_ms_ = this->consecutive_failures_ * FAILURE_HOLD_OFF_STEP_MS;
-  ESP_LOGW(TAG, "[%s] Holding off reconnect for %u s", this->address_str_,
-           this->consecutive_failures_ * (FAILURE_HOLD_OFF_STEP_MS / 1000));
-}
-
 void BLEClient::on_connection_state(bool connected, uint16_t mtu, int error) {
  if (connected) {
    this->state_ = State::DISCOVERING;
@@ -124,7 +109,7 @@ void BLEClient::on_connection_state(bool connected, uint16_t mtu, int error) {
    if (discover_err != 0) {
      // Synchronous refusal: no discovery completion will follow.
      ESP_LOGW(TAG, "[%s] Service discovery refused, err=%d", this->address_str_, discover_err);
-      this->register_failure_();
+      this->backoff_.register_failure(this->address_str_);
      // Deliberate teardown: its report must not charge the backoff again.
      this->disconnect();
    }
@@ -148,7 +133,7 @@ void BLEClient::on_connection_state(bool connected, uint16_t mtu, int error) {
      ESP_LOGD(TAG, "[%s] Connect attempt cancelled, status=%d", this->address_str_, error);
    } else {
      ESP_LOGW(TAG, "[%s] Connect failed, status=%d", this->address_str_, error);
-      this->register_failure_();
+      this->backoff_.register_failure(this->address_str_);
    }
    this->defer([this]() { this->connect_failed_callbacks_.call(); });
  }
@@ -157,7 +142,7 @@ void BLEClient::on_connection_state(bool connected, uint16_t mtu, int error) {
 void BLEClient::on_service_discovery_done(int error) {
  if (error != 0) {
    ESP_LOGW(TAG, "[%s] Service discovery failed, status=%d", this->address_str_, error);
-    this->register_failure_();
+    this->backoff_.register_failure(this->address_str_);
    // The teardown is deliberate: do not charge the backoff again for its
    // connection report.
    this->disconnect();
@@ -175,7 +160,7 @@ void BLEClient::on_service_discovery_done(int error) {
      // connect_failed, never a spurious on_disconnect.
      ESP_LOGW(TAG, "[%s] Service table is empty; treating as failed discovery", this->address_str_);
      this->backend_->release_services();
-      this->register_failure_();
+      this->backoff_.register_failure(this->address_str_);
      this->disconnect();
      return;
    }
@@ -194,8 +179,7 @@ void BLEClient::on_service_discovery_done(int error) {
    }
  }
  this->backend_->release_services();
-  this->consecutive_failures_ = 0;
-  this->hold_off_ms_ = 0;
+  this->backoff_.reset();
  ESP_LOGI(TAG, "[%s] Connected", this->address_str_);
  this->defer([this]() { this->connect_callbacks_.call(); });
 }
@@ -16,6 +16,7 @@
 #if defined(USE_BLE_GATT_CLIENT) && !defined(USE_ESP32)

 #include "ble_client_node.h"
+#include "connect_backoff.h"
 #include "esphome/components/ble_device_base/ble_device.h"
 #include "esphome/components/ble_device_base/ble_gatt_client.h"
 #include "esphome/components/bluetooth_connection/bluetooth_connection.h"
@@ -110,7 +111,6 @@ class BLEClient : public Component,
  enum class State : uint8_t { IDLE, CONNECTING, DISCOVERING, CONNECTED };

  void attempt_connect_();
-  void register_failure_();

  // Group 1: pointers / containers
  ble_device_base::BLEGattConnection *backend_{nullptr};
@@ -126,11 +126,9 @@ class BLEClient : public Component,
  LazyCallbackManager<void()> connect_failed_callbacks_;

  // Group 4: 4-byte types
-  // Backoff after repeated failures so an undiscoverable database or a
-  // dead peer cannot produce a battery-draining connect loop (wrap-safe
-  // start+duration pair).
-  uint32_t hold_off_start_{0};
-  uint32_t hold_off_ms_{0};
+  // Backoff so an undiscoverable database or a dead peer cannot produce a
+  // battery-draining connect loop.
+  ConnectBackoff backoff_;

  // Group 5: arrays
  char address_str_[MAC_ADDRESS_PRETTY_BUFFER_SIZE]{};
@@ -144,7 +142,6 @@ class BLEClient : public Component,
  // A user-initiated teardown in flight; its failure report is not a
  // connect failure and must not feed the backoff.
  bool cancel_requested_{false};
-  uint8_t consecutive_failures_{0};
 };

 }  // namespace esphome::ble_client
@@ -0,0 +1,38 @@
+#pragma once
+
+#include "esphome/core/defines.h"
+
+#ifdef USE_BLE_GATT_CLIENT
+
+#include "esphome/core/hal.h"
+#include "esphome/core/log.h"
+
+#include <cstdint>
+
+namespace esphome::ble_client {
+
+/// Reconnect backoff after repeated connect/discovery failures, shared by
+/// both engines (wrap-safe start+failures pair; the duration is derived).
+class ConnectBackoff {
+ public:
+  bool holding_off() const { return this->failures_ != 0 && millis() - this->start_ < this->failures_ * STEP_MS; }
+  void register_failure(const char *address_str) {
+    if (this->failures_ < MAX_STEPS)
+      this->failures_++;
+    this->start_ = millis();
+    esph_log_w("ble_client", "[%s] Holding off reconnect for %u s", address_str, this->failures_ * (STEP_MS / 1000));
+  }
+  void reset() { this->failures_ = 0; }
+
+ private:
+  // Capped so a flapping peer retries within a minute at worst.
+  static constexpr uint32_t STEP_MS = 10000;
+  static constexpr uint8_t MAX_STEPS = 6;
+
+  uint32_t start_{0};
+  uint8_t failures_{0};
+};
+
+}  // namespace esphome::ble_client
+
+#endif  // USE_BLE_GATT_CLIENT