Address review: de-vacuous the reject test, loud node overflow, walker logs

- The rejection test registers the hub and matches the extra-keys error,
  so the missing-tracker error can no longer satisfy it vacuously
- register_ble_node logs and drops loudly at capacity: a push_back past
  a StaticVector's bound is a silent no-op, and an undersized slot count
  must show at boot, not as an unresolvable node
- Every walker failure names the failing call and status; the descriptor
  cap gets its own message so a >64-descriptor characteristic is
  diagnosable instead of a permanent silent backoff loop
This commit is contained in:
J. Nick Koston
2026-08-09 19:37:32 -05:00
parent 64034b5b23
commit 1933126ad7
4 changed files with 31 additions and 11 deletions
@@ -14,6 +14,17 @@ static const char *const TAG = "ble_client";
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) {
// push_back past capacity is a silent no-op; an undersized slot count
// must be loud at boot, not an unresolvable node at runtime.
ESP_LOGE(TAG, "[%s] Node capacity exceeded; node dropped", this->address_str_);
return;
}
this->nodes_.push_back(node);
}
void BLEClient::set_address(uint64_t address) {
this->address_ = address;
uint8_t mac[6];
@@ -65,10 +65,7 @@ class BLEClient : public Component,
void set_enabled(bool enabled);
const char *address_str() const { return this->address_str_; }
void register_ble_node(BLEClientNode *node) {
node->set_ble_client_parent(this);
this->nodes_.push_back(node);
}
void register_ble_node(BLEClientNode *node);
bool connected() const { return this->state_ == State::CONNECTED; }
bool idle() const { return this->state_ == State::IDLE; }
@@ -365,16 +365,19 @@ bool BluedroidGattClient::walk_database_(ServiceFn &&on_service, CharFn &&on_cha
for (uint16_t s = 0; s < this->service_total_; s++) {
esp_gattc_service_elem_t svc;
uint16_t svc_count = 1;
if (esp_ble_gattc_get_service(this->gattc_if_, this->conn_id_, nullptr, &svc, &svc_count, s) != ESP_GATT_OK ||
svc_count == 0) {
auto svc_status = esp_ble_gattc_get_service(this->gattc_if_, this->conn_id_, nullptr, &svc, &svc_count, s);
if (svc_status != ESP_GATT_OK || svc_count == 0) {
this->log_gattc_warning_("esp_ble_gattc_get_service", svc_status);
return false;
}
if (!on_service(s, svc)) {
return false;
}
uint16_t svc_chars = 0;
if (esp_ble_gattc_get_attr_count(this->gattc_if_, this->conn_id_, ESP_GATT_DB_CHARACTERISTIC, svc.start_handle,
svc.end_handle, 0, &svc_chars) != ESP_GATT_OK) {
auto count_status = esp_ble_gattc_get_attr_count(this->gattc_if_, this->conn_id_, ESP_GATT_DB_CHARACTERISTIC,
svc.start_handle, svc.end_handle, 0, &svc_chars);
if (count_status != ESP_GATT_OK) {
this->log_gattc_warning_("esp_ble_gattc_get_attr_count", count_status);
return false;
}
for (uint16_t c = 0; c < svc_chars; c++) {
@@ -385,6 +388,7 @@ bool BluedroidGattClient::walk_database_(ServiceFn &&on_service, CharFn &&on_cha
if (status != ESP_GATT_OK || char_count == 0) {
// An early terminator contradicts svc_chars from the same cache;
// never build a silently truncated table.
this->log_gattc_warning_("esp_ble_gattc_get_all_char", status);
return false;
}
if (!on_char(svc, chr)) {
@@ -394,6 +398,8 @@ bool BluedroidGattClient::walk_database_(ServiceFn &&on_service, CharFn &&on_cha
if (d == MAX_DESCRIPTORS_PER_CHARACTERISTIC) {
// A stack that never reports end-of-range; fail like every other
// inconsistency instead of truncating the table silently.
ESP_LOGW(TAG, "[%d] Descriptor walk exceeded %u entries", this->connection_index_,
MAX_DESCRIPTORS_PER_CHARACTERISTIC);
return false;
}
esp_gattc_descr_elem_t desc;
@@ -404,6 +410,7 @@ bool BluedroidGattClient::walk_database_(ServiceFn &&on_service, CharFn &&on_cha
break;
}
if (desc_status != ESP_GATT_OK || desc_count == 0) {
this->log_gattc_warning_("esp_ble_gattc_get_all_descr", desc_status);
return false;
}
if (!on_desc(chr, desc)) {
@@ -21,6 +21,7 @@ from esphome.const import (
CONF_TYPE,
PlatformFramework,
)
from esphome.core import CORE
from esphome.types import ConfigType
from ..types import SetCoreConfigCallable
@@ -115,10 +116,14 @@ def test_neutral_arm_rejects_esp32_only_keys(
set_core_config: SetCoreConfigCallable,
) -> None:
# Pins the schema split's rejection side: the legacy-only keys must not
# leak into the neutral arm.
from esphome.components import ble_client
# leak into the neutral arm. The hub is registered so the extra key is
# the only error - without it the missing-tracker error would satisfy
# the raises vacuously.
from esphome.components import ble_client, ble_device_base
set_core_config(PlatformFramework.RP2_ARDUINO)
ble_device_base.register_hub_provider("rp2_ble_tracker")
CORE.loaded_integrations.add("rp2_ble_tracker")
for key in ("name", "on_passkey_request", "on_passkey_notification"):
with pytest.raises(cv.Invalid):
with pytest.raises(cv.Invalid, match="extra keys not allowed"):
ble_client.CONFIG_SCHEMA({"mac_address": "AA:BB:CC:DD:EE:FF", key: "x"})