From 314557f83b6e839ade46e4e86d6c7b79b67d911d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 12 Aug 2026 17:56:29 -0500 Subject: [PATCH] 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. --- esphome/components/ble_client/ble_client.cpp | 118 ++++++++---------- esphome/components/ble_client/ble_client.h | 22 ++-- .../components/ble_client/ble_client_gatt.cpp | 32 ++--- .../components/ble_client/ble_client_gatt.h | 11 +- .../components/ble_client/connect_backoff.h | 38 ++++++ 5 files changed, 115 insertions(+), 106 deletions(-) create mode 100644 esphome/components/ble_client/connect_backoff.h diff --git a/esphome/components/ble_client/ble_client.cpp b/esphome/components/ble_client/ble_client.cpp index db786b9110..0f5f363bb1 100644 --- a/esphome/components/ble_client/ble_client.cpp +++ b/esphome/components/ble_client/ble_client.cpp @@ -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(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(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()); } diff --git a/esphome/components/ble_client/ble_client.h b/esphome/components/ble_client/ble_client.h index 8312530f0d..d5019622da 100644 --- a/esphome/components/ble_client/ble_client.h +++ b/esphome/components/ble_client/ble_client.h @@ -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 #include #include -#include -#include #include 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 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 }; diff --git a/esphome/components/ble_client/ble_client_gatt.cpp b/esphome/components/ble_client/ble_client_gatt.cpp index 2365190819..fc8ef7338a 100644 --- a/esphome/components/ble_client/ble_client_gatt.cpp +++ b/esphome/components/ble_client/ble_client_gatt.cpp @@ -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(); }); } diff --git a/esphome/components/ble_client/ble_client_gatt.h b/esphome/components/ble_client/ble_client_gatt.h index a7a547f522..66e6e16204 100644 --- a/esphome/components/ble_client/ble_client_gatt.h +++ b/esphome/components/ble_client/ble_client_gatt.h @@ -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 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 diff --git a/esphome/components/ble_client/connect_backoff.h b/esphome/components/ble_client/connect_backoff.h new file mode 100644 index 0000000000..6acd74e4f7 --- /dev/null +++ b/esphome/components/ble_client/connect_backoff.h @@ -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 + +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