From cf5faa454c7208a0bf8dd95d5524e9ce1b7efbe9 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 9 Aug 2026 12:47:17 -0500 Subject: [PATCH] Address review: rejection logs, honest service_table docstring, terse sweep - connect()/disconnect() log a rejected user action like the legacy engine instead of silently ignoring it - The service_table docstring states the flag is Bluedroid-only forward scaffolding and that rp2's proxy hub must keep its materializer regardless (it streams through get_service_table()) - Review-round comments tightened to repo style --- .../components/ble_client/ble_client_gatt.cpp | 23 ++++++++++--------- .../components/ble_client/ble_client_gatt.h | 7 +++--- .../bluetooth_connection/__init__.py | 8 ++++--- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/esphome/components/ble_client/ble_client_gatt.cpp b/esphome/components/ble_client/ble_client_gatt.cpp index 809768fde2..23a1f37c8b 100644 --- a/esphome/components/ble_client/ble_client_gatt.cpp +++ b/esphome/components/ble_client/ble_client_gatt.cpp @@ -30,9 +30,8 @@ void BLEClient::set_enabled(bool enabled) { this->disconnect(); return; } - // A re-enable is an explicit "try again": clear the backoff so the next - // sighting connects promptly. Enabling does not itself connect (legacy - // parity). + // 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; } @@ -52,8 +51,10 @@ bool BLEClient::parse_device(const ble_device_base::ESPBTDevice &device) { } void BLEClient::connect() { - if (this->state_ != State::IDLE) + if (this->state_ != State::IDLE) { + ESP_LOGD(TAG, "[%s] Connect requested while busy, ignoring", this->address_str_); return; + } // An absent peer can inhibit scanning for the backend's full connect // timeout, so this is worth a breadcrumb - but it is a supported action. ESP_LOGI(TAG, "[%s] Connecting on request", this->address_str_); @@ -81,15 +82,16 @@ void BLEClient::attempt_connect_() { } void BLEClient::disconnect() { - if (this->state_ == State::IDLE) + if (this->state_ == State::IDLE) { + ESP_LOGD(TAG, "[%s] Disconnect requested while idle, ignoring", this->address_str_); return; + } // A deliberate teardown's failure report must not feed the backoff. this->cancel_requested_ = true; int err = this->backend_->gatt_disconnect(); if (err != 0) { - // Refused synchronously: the backend and the client disagreed about the - // link state. Worth a warning of its own - the settle below then runs - // the same deliberate-cancel path as a clean teardown. + // Refused synchronously: backend and client disagree about the link + // state. Warn, then settle through the deliberate-cancel path. ESP_LOGW(TAG, "[%s] Disconnect refused, err=%d; settling locally", this->address_str_, err); this->on_connection_state(false, 0, err); } @@ -157,9 +159,8 @@ void BLEClient::on_service_discovery_done(int error) { for (auto *node : this->nodes_) { node->on_connected(table); if (this->state_ != State::CONNECTED) { - // A node tore the link down mid-fan-out; the teardown settled. Release - // the borrowed table here since the normal release below is skipped - // (idempotent when the backend released on teardown already). + // A node tore the link down mid-fan-out; the normal release below is + // skipped (release is idempotent). this->backend_->release_services(); return; } diff --git a/esphome/components/ble_client/ble_client_gatt.h b/esphome/components/ble_client/ble_client_gatt.h index 4b0d72e545..50cd4bca23 100644 --- a/esphome/components/ble_client/ble_client_gatt.h +++ b/esphome/components/ble_client/ble_client_gatt.h @@ -84,10 +84,9 @@ class BLEClient : public Component, void run_later(std::function &&f) { this->defer(std::move(f)); } // NOLINT // Backend ops for nodes and actions - the frozen node-facing surface. - // Only write_characteristic has an in-tree caller today; the rest exist so - // the first migrated node codes against a complete interface (subscribing - // means notify_characteristic plus a write_descriptor on the CCCD - the - // backend contract keeps the CCCD write the caller's responsibility). + // Only write_characteristic has an in-tree caller; subscribing means + // notify_characteristic plus a CCCD write_descriptor (the caller's job + // per the contract). int write_characteristic(uint16_t handle, const uint8_t *data, uint16_t len, bool response) { return this->backend_->write_characteristic(handle, data, len, response); } diff --git a/esphome/components/bluetooth_connection/__init__.py b/esphome/components/bluetooth_connection/__init__.py index 62ac8be41a..fab908674e 100644 --- a/esphome/components/bluetooth_connection/__init__.py +++ b/esphome/components/bluetooth_connection/__init__.py @@ -212,9 +212,11 @@ async def new_gatt_backend( it with its platform stack. The connection slot is claimed at validation (the consume_gatt_slot validators), not here. - service_table compiles the on-demand service-table materializer into the - backend; direct consumers need it, the streaming proxy does not, so - proxy-only builds keep the smaller footprint. + service_table is honored by the Bluedroid backend only: forward + scaffolding for the first esp32 direct consumer, load-bearing on no + current build (rp2 ignores the define and always materializes - its + proxy hub streams through get_service_table(), so it must keep the + materializer regardless of the flag). """ from esphome.components import ble_device_base