From ce436ff8b40235485184d2d016f14b35398b18f8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 9 Aug 2026 01:54:16 -0500 Subject: [PATCH] Apply review: refusal resolves waiters, cancel skips backoff, wrap-safe hold-off --- esphome/components/ble_client/__init__.py | 15 +--- .../components/ble_client/automation_gatt.h | 10 ++- .../components/ble_client/ble_client_gatt.cpp | 69 ++++++------------- .../components/ble_client/ble_client_gatt.h | 31 +++------ .../components/ble_client/output/__init__.py | 6 +- .../components/ble_client/sensor/__init__.py | 6 +- .../components/ble_client/switch/__init__.py | 6 +- .../ble_client/text_sensor/__init__.py | 6 +- 8 files changed, 46 insertions(+), 103 deletions(-) diff --git a/esphome/components/ble_client/__init__.py b/esphome/components/ble_client/__init__.py index fe088a9104..46045ba990 100644 --- a/esphome/components/ble_client/__init__.py +++ b/esphome/components/ble_client/__init__.py @@ -263,15 +263,6 @@ async def register_ble_node(var, config): cg.add(parent.register_ble_node(var)) -def _esp32_only_action(name: str): - def validator(config: ConfigType) -> ConfigType: - if not CORE.is_esp32: - raise cv.Invalid(f"{name} is only supported on esp32") - return config - - return validator - - BLE_WRITE_ACTION_SCHEMA = cv.Schema( { cv.GenerateID(CONF_ID): cv.use_id(BLEClient), @@ -288,33 +279,33 @@ BLE_CONNECT_ACTION_SCHEMA = maybe_simple_id( ) BLE_NUMERIC_COMPARISON_REPLY_ACTION_SCHEMA = cv.All( + cv.only_on_esp32, cv.Schema( { cv.GenerateID(CONF_ID): cv.use_id(BLEClient), cv.Required(CONF_ACCEPT): cv.templatable(cv.boolean), } ), - _esp32_only_action("ble_client.numeric_comparison_reply"), ) BLE_PASSKEY_REPLY_ACTION_SCHEMA = cv.All( + cv.only_on_esp32, cv.Schema( { cv.GenerateID(CONF_ID): cv.use_id(BLEClient), cv.Required(CONF_PASSKEY): cv.templatable(cv.int_range(min=0, max=999999)), } ), - _esp32_only_action("ble_client.passkey_reply"), ) BLE_REMOVE_BOND_ACTION_SCHEMA = cv.All( + cv.only_on_esp32, cv.Schema( { cv.GenerateID(CONF_ID): cv.use_id(BLEClient), } ), - _esp32_only_action("ble_client.remove_bond"), ) diff --git a/esphome/components/ble_client/automation_gatt.h b/esphome/components/ble_client/automation_gatt.h index d7fefa373d..74826c7843 100644 --- a/esphome/components/ble_client/automation_gatt.h +++ b/esphome/components/ble_client/automation_gatt.h @@ -20,6 +20,9 @@ #include "esphome/core/helpers.h" #include "esphome/core/log.h" +// Maximum bytes to log in hex format for BLE writes (many logging buffers are 256 chars) +static constexpr size_t BLE_WRITE_MAX_LOG_BYTES = 64; + namespace esphome::ble_client { // placeholder class for static TAG (shared with automation.cpp). @@ -98,6 +101,10 @@ template class BLEClientWriteAction final : public Action esph_log_w(Automation::TAG, "Cannot write to BLE characteristic - not connected"); return false; } +#if ESPHOME_LOG_LEVEL >= ESPHOME_LOG_LEVEL_VERY_VERBOSE + char hex_buf[format_hex_pretty_size(BLE_WRITE_MAX_LOG_BYTES)]; + esph_log_vv(Automation::TAG, "Will write %d bytes: %s", len, format_hex_pretty_to(hex_buf, data, len)); +#endif int err = this->ble_client_->write_characteristic(this->char_handle_, data, len, this->write_response_); if (err != 0) { esph_log_e(Automation::TAG, "Error writing to characteristic: %d!", err); @@ -135,12 +142,13 @@ template class BLEClientWriteAction final : public Action void on_disconnected() override { this->resolved_ = false; + this->char_handle_ = 0; if (this->num_running_ != 0) this->stop_complex(); } void on_write_result(uint16_t handle, int error) override { - if (handle == this->char_handle_ && this->num_running_ != 0) { + if (this->resolved_ && handle == this->char_handle_ && this->num_running_ != 0) { this->ble_client_->run_later([this]() { this->play_next_tuple_(this->var_); }); } } diff --git a/esphome/components/ble_client/ble_client_gatt.cpp b/esphome/components/ble_client/ble_client_gatt.cpp index 7415b829ae..0f72da12c9 100644 --- a/esphome/components/ble_client/ble_client_gatt.cpp +++ b/esphome/components/ble_client/ble_client_gatt.cpp @@ -39,9 +39,9 @@ bool BLEClient::parse_device(const ble_device_base::ESPBTDevice &device) { this->address_type_ = device.get_address_type(); if (!this->enabled || !this->auto_connect_ || this->state_ != State::IDLE) return true; - if (this->hold_off_until_ != 0 && millis() < this->hold_off_until_) + if (this->hold_off_ms_ != 0 && millis() - this->hold_off_start_ < this->hold_off_ms_) return true; - this->attempt_connect_(false); + this->attempt_connect_(); return true; } @@ -51,15 +51,18 @@ void BLEClient::connect() { // Without a fresh sighting each attempt can inhibit scanning for the // backend's full connect timeout (20 s on rp2) if the peer is absent. ESP_LOGW(TAG, "[%s] Connecting without a recent advertisement", this->address_str_); - this->attempt_connect_(true); + this->attempt_connect_(); } -void BLEClient::attempt_connect_(bool from_action) { +void BLEClient::attempt_connect_() { int err = this->backend_->connect(this->address_, this->address_type_); if (err != 0) { - // A refused connect never produces a callback; stay idle and let the - // next sighting (or action) retry. + // A refused connect never produces a callback: stay idle, charge the + // 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->defer([this]() { this->connect_failed_callbacks_.call(); }); return; } ESP_LOGD(TAG, "[%s] Connecting", this->address_str_); @@ -69,38 +72,16 @@ void BLEClient::attempt_connect_(bool from_action) { void BLEClient::disconnect() { if (this->state_ == State::IDLE) return; + // A deliberate teardown's failure report must not feed the backoff. + this->cancel_requested_ = true; this->backend_->disconnect(); } -void BLEClient::request_notify(uint16_t char_handle, uint16_t cccd_handle, uint8_t properties) { - if (this->cccd_count_ >= BLE_CLIENT_MAX_NOTIFY_REQUESTS) { - ESP_LOGW(TAG, "[%s] Too many notify requests, dropping handle 0x%04x", this->address_str_, char_handle); - return; - } - // Local registration is free (no operation slot); the CCCD write is - // serialized through the client's queue because the backend accepts one - // outstanding operation at a time. - this->backend_->notify_characteristic(char_handle, true); - uint8_t value = (properties & ble_device_base::GATT_CHAR_PROP_NOTIFY) ? 1 : 2; - this->pending_cccd_[this->cccd_count_++] = {cccd_handle, value}; -} - -void BLEClient::issue_next_cccd_() { - while (this->cccd_head_ < this->cccd_count_) { - const auto &pending = this->pending_cccd_[this->cccd_head_]; - const uint8_t value[2] = {pending.value, 0x00}; - if (this->backend_->write_descriptor(pending.handle, value, sizeof(value)) == 0) { - return; // advance on the matching on_write_result - } - ESP_LOGW(TAG, "[%s] CCCD write failed for handle 0x%04x", this->address_str_, pending.handle); - this->cccd_head_++; - } -} - void BLEClient::register_failure_() { if (this->consecutive_failures_ < FAILURE_HOLD_OFF_MAX_STEPS) this->consecutive_failures_++; - this->hold_off_until_ = millis() + this->consecutive_failures_ * FAILURE_HOLD_OFF_STEP_MS; + 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)); } @@ -115,8 +96,9 @@ void BLEClient::on_connection_state(bool connected, uint16_t mtu, int error) { return; } bool was_connected = this->state_ == State::CONNECTED; + bool cancelled = this->cancel_requested_; + this->cancel_requested_ = false; this->state_ = State::IDLE; - this->cccd_head_ = this->cccd_count_ = 0; if (was_connected) { ESP_LOGI(TAG, "[%s] Disconnected, status=%d", this->address_str_, error); for (auto *node : this->nodes_) { @@ -125,8 +107,12 @@ void BLEClient::on_connection_state(bool connected, uint16_t mtu, int error) { // Continuations leave the backend's event-drain stack first. this->defer([this]() { this->disconnect_callbacks_.call(); }); } else { - ESP_LOGW(TAG, "[%s] Connect failed, status=%d", this->address_str_, error); - this->register_failure_(); + if (cancelled) { + ESP_LOGD(TAG, "[%s] Connect attempt cancelled", this->address_str_); + } else { + ESP_LOGW(TAG, "[%s] Connect failed, status=%d", this->address_str_, error); + this->register_failure_(); + } this->defer([this]() { this->connect_failed_callbacks_.call(); }); } } @@ -145,23 +131,12 @@ void BLEClient::on_service_discovery_done(int error) { this->backend_->release_services(); this->state_ = State::CONNECTED; this->consecutive_failures_ = 0; - this->hold_off_until_ = 0; + this->hold_off_ms_ = 0; ESP_LOGI(TAG, "[%s] Connected", this->address_str_); this->defer([this]() { this->connect_callbacks_.call(); }); - this->issue_next_cccd_(); } void BLEClient::on_write_result(uint16_t handle, int error) { - // The client's CCCD queue claims its own completion before the node - // fan-out sees it. - if (this->cccd_head_ < this->cccd_count_ && this->pending_cccd_[this->cccd_head_].handle == handle) { - if (error != 0) { - ESP_LOGW(TAG, "[%s] CCCD write error %d on handle 0x%04x", this->address_str_, error, handle); - } - this->cccd_head_++; - this->issue_next_cccd_(); - return; - } for (auto *node : this->nodes_) { node->on_write_result(handle, error); } diff --git a/esphome/components/ble_client/ble_client_gatt.h b/esphome/components/ble_client/ble_client_gatt.h index 16bfb7a814..258b0b82c2 100644 --- a/esphome/components/ble_client/ble_client_gatt.h +++ b/esphome/components/ble_client/ble_client_gatt.h @@ -22,7 +22,6 @@ #include "esphome/core/component.h" #include "esphome/core/helpers.h" -#include #include #include #include @@ -31,10 +30,6 @@ namespace esphome::ble_client { class BLEClient; -// Concurrent notify subscriptions per client (matches the smallest backend's -// own cap; a node requesting more logs and is dropped). -static constexpr uint8_t BLE_CLIENT_MAX_NOTIFY_REQUESTS = 16; - /// The neutral node interface. Nodes resolve their handles from the service /// table during on_connected() and MUST copy what they need: the table is /// borrowed backend storage, valid only for the duration of that call. @@ -57,7 +52,6 @@ class BLEClient : public Component, public ble_device_base::ESPBTDeviceListener, public ble_device_base::GattClientListener { public: - void setup() override { this->enabled = true; } void dump_config() override; // Public field for legacy parity (the switch platform republishes it). @@ -90,11 +84,6 @@ class BLEClient : public Component, /// continuations must leave that stack first. void run_later(std::function &&f) { this->defer(std::move(f)); } // NOLINT - /// Called by nodes during on_connected(): registers the notification with - /// the backend and queues the CCCD write (the backend accepts one - /// outstanding operation, so the client serializes them). - void request_notify(uint16_t char_handle, uint16_t cccd_handle, uint8_t properties); - // Backend ops for nodes and actions. int write_characteristic(uint16_t handle, const uint8_t *data, uint16_t len, bool response) { return this->backend_->write_characteristic(handle, data, len, response); @@ -130,13 +119,7 @@ class BLEClient : public Component, protected: enum class State : uint8_t { IDLE, CONNECTING, DISCOVERING, CONNECTED }; - struct PendingCccd { - uint16_t handle; - uint8_t value; // 1 = notify, 2 = indicate - }; - - void attempt_connect_(bool from_action); - void issue_next_cccd_(); + void attempt_connect_(); void register_failure_(); // Group 1: pointers / containers @@ -153,19 +136,21 @@ class BLEClient : public Component, // Group 4: 4-byte types // Backoff after repeated failures so an undiscoverable database or a - // dead peer cannot produce a battery-draining connect loop. - uint32_t hold_off_until_{0}; + // 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}; // Group 5: arrays char address_str_[MAC_ADDRESS_PRETTY_BUFFER_SIZE]{}; - std::array pending_cccd_{}; // Group 6: 1-byte types State state_{State::IDLE}; uint8_t address_type_{0}; // BLE_ADDR_TYPE_*, captured from the sighting bool auto_connect_{true}; - uint8_t cccd_head_{0}; - uint8_t cccd_count_{0}; + // 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}; }; diff --git a/esphome/components/ble_client/output/__init__.py b/esphome/components/ble_client/output/__init__.py index c2405c04fa..22a6b29442 100644 --- a/esphome/components/ble_client/output/__init__.py +++ b/esphome/components/ble_client/output/__init__.py @@ -13,7 +13,7 @@ BLEBinaryOutput = ble_client_ns.class_( "BLEBinaryOutput", output.BinaryOutput, ble_client.BLEClientNode, cg.Component ) -_CONFIG_SCHEMA = cv.All( +CONFIG_SCHEMA = cv.All( output.BINARY_OUTPUT_SCHEMA.extend( { cv.Required(CONF_ID): cv.declare_id(BLEBinaryOutput), @@ -66,7 +66,3 @@ async def to_code(config): await output.register_output(var, config) await ble_client.register_ble_node(var, config) await cg.register_component(var, config) - - -# Raw-gattc node platform: not yet migrated to the neutral ble_client engine. -CONFIG_SCHEMA = cv.All(cv.only_on_esp32, _CONFIG_SCHEMA) diff --git a/esphome/components/ble_client/sensor/__init__.py b/esphome/components/ble_client/sensor/__init__.py index 0d6e7b5397..7764955d89 100644 --- a/esphome/components/ble_client/sensor/__init__.py +++ b/esphome/components/ble_client/sensor/__init__.py @@ -50,7 +50,7 @@ def checkType(value): return value -_CONFIG_SCHEMA = cv.All( +CONFIG_SCHEMA = cv.All( checkType, cv.typed_schema( { @@ -164,7 +164,3 @@ async def to_code(config): await rssi_sensor_to_code(config) elif config[CONF_TYPE] == TYPE_CHARACTERISTIC: await characteristic_sensor_to_code(config) - - -# Raw-gattc node platform: not yet migrated to the neutral ble_client engine. -CONFIG_SCHEMA = cv.All(cv.only_on_esp32, _CONFIG_SCHEMA) diff --git a/esphome/components/ble_client/switch/__init__.py b/esphome/components/ble_client/switch/__init__.py index d180518090..70314e8f30 100644 --- a/esphome/components/ble_client/switch/__init__.py +++ b/esphome/components/ble_client/switch/__init__.py @@ -9,7 +9,7 @@ BLEClientSwitch = ble_client_ns.class_( "BLEClientSwitch", switch.Switch, cg.Component, ble_client.BLEClientNode ) -_CONFIG_SCHEMA = ( +CONFIG_SCHEMA = ( switch.switch_schema(BLEClientSwitch, icon=ICON_BLUETOOTH, block_inverted=True) .extend(ble_client.BLE_CLIENT_SCHEMA) .extend(cv.COMPONENT_SCHEMA) @@ -20,7 +20,3 @@ async def to_code(config): var = await switch.new_switch(config) await cg.register_component(var, config) await ble_client.register_ble_node(var, config) - - -# Raw-gattc node platform: not yet migrated to the neutral ble_client engine. -CONFIG_SCHEMA = cv.All(cv.only_on_esp32, _CONFIG_SCHEMA) diff --git a/esphome/components/ble_client/text_sensor/__init__.py b/esphome/components/ble_client/text_sensor/__init__.py index ed2e05107c..820f60845d 100644 --- a/esphome/components/ble_client/text_sensor/__init__.py +++ b/esphome/components/ble_client/text_sensor/__init__.py @@ -33,7 +33,7 @@ BLETextSensorNotifyTrigger = ble_client_ns.class_( "BLETextSensorNotifyTrigger", automation.Trigger.template(cg.std_string) ) -_CONFIG_SCHEMA = cv.All( +CONFIG_SCHEMA = cv.All( text_sensor.text_sensor_schema(BLETextSensor) .extend( { @@ -109,7 +109,3 @@ async def to_code(config): trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var) await ble_client.register_ble_node(trigger, config) await automation.build_automation(trigger, [(cg.std_string, "x")], conf) - - -# Raw-gattc node platform: not yet migrated to the neutral ble_client engine. -CONFIG_SCHEMA = cv.All(cv.only_on_esp32, _CONFIG_SCHEMA)