From 8886ff5ba61482c25fb717daa832eb5f8e264b49 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 21 Aug 2026 18:34:54 -0500 Subject: [PATCH] Derive the restart from the programmed window; share the scan-end sweep; fold validation loop; parametrize codegen tests --- .../components/ble_device_base/__init__.py | 18 ++--- .../esp32_ble_tracker/esp32_ble_tracker.cpp | 80 ++++++++----------- .../esp32_ble_tracker/esp32_ble_tracker.h | 11 ++- .../test_scan_window_default.py | 59 +++++--------- 4 files changed, 69 insertions(+), 99 deletions(-) diff --git a/esphome/components/ble_device_base/__init__.py b/esphome/components/ble_device_base/__init__.py index 2f4b1013bc..43ec736727 100644 --- a/esphome/components/ble_device_base/__init__.py +++ b/esphome/components/ble_device_base/__init__.py @@ -206,21 +206,17 @@ def validate_scan_parameters(config: ConfigType) -> ConfigType: interval = config[CONF_INTERVAL] window = config[CONF_WINDOW] - if window > interval: - raise cv.Invalid( - f"Scan window ({window}) needs to be smaller than scan interval ({interval})" - ) - - # Labels name the YAML key the user typed so the errors are greppable. + # Labels are reused in every error below; the optional one names its key. windows = [("Scan window", window)] if (connection_window := config.get(CONF_CONNECTION_SCAN_WINDOW)) is not None: - if connection_window > interval: - raise cv.Invalid( - f"{CONF_CONNECTION_SCAN_WINDOW} ({connection_window}) needs to be " - f"smaller than scan interval ({interval})" - ) windows.append((CONF_CONNECTION_SCAN_WINDOW, connection_window)) + for name, value in windows: + if value > interval: + raise cv.Invalid( + f"{name} ({value}) needs to be smaller than scan interval ({interval})" + ) + # BLE scan interval/window are programmed in 0.625 ms units as a 16-bit value; the # controller only accepts 2.5 ms .. 10240 ms (0x0004 .. 0x4000). Reject out-of-range # values here instead of letting the unit conversion silently overflow. diff --git a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp index 9d31219ab5..363dc5a949 100644 --- a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp +++ b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp @@ -122,8 +122,8 @@ void ESP32BLETracker::loop() { // - start_scan_(): scanner_state_ becomes IDLE via set_scanner_state_() in cleanup_scan_state_() // - try_promote_discovered_clients_(): client enters DISCOVERED via set_state(), or // connecting client finishes (state change), or scanner reaches RUNNING/IDLE - // - connection-window restart: using_connection_window_ is only written in - // start_scan_() (which changes scanner state via set_scanner_state_()), and + // - connection-window restart: scan_params_ is only written in start_scan_() + // (which changes scanner state via set_scanner_state_()), and // counts.active/disconnecting only change on client state changes // // All conditions that affect the logic below are tied to state changes that increment @@ -149,13 +149,14 @@ void ESP32BLETracker::loop() { } #ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT - // Last connection dropped: restart so the configured window returns now + // The programmed window no longer matches the connection state (typically + // the last connection dropped): restart so the right window applies now // instead of at the end of the scan period. Continuous only (a user-started // scan would not restart); !disconnecting matches the restart gate below. - if (this->using_connection_window_ && !counts.active && !counts.disconnecting && this->scan_continuous_ && - this->scanner_state_ == ScannerState::RUNNING) { - // Same logical scan period continues: cleanup_scan_state_ and start_scan_ - // both skip their on_scan_end sweep. Only armed when the stop was issued. + if (this->scanner_state_ == ScannerState::RUNNING && this->scan_continuous_ && !counts.disconnecting && + this->scan_params_.scan_window != this->desired_scan_window_(counts.active)) { + // Same logical scan period continues: no on_scan_end sweeps for this + // restart. Only armed when the stop was issued. this->skip_next_scan_end_ = this->stop_scan_(); } #endif @@ -250,24 +251,11 @@ void ESP32BLETracker::start_scan_(bool first) { } this->set_scanner_state_(ScannerState::STARTING); ESP_LOGV(TAG, "Starting scan, set scanner state to STARTING."); - bool notify_scan_end = !first; + if (!first) + this->notify_scan_end_(); #ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT - if (this->skip_next_scan_end_) { - // Window-change restart continues the same scan period. - this->skip_next_scan_end_ = false; - notify_scan_end = false; - } + this->skip_next_scan_end_ = false; #endif - if (notify_scan_end) { -#ifdef ESPHOME_ESP32_BLE_TRACKER_LISTENER_COUNT - for (auto *listener : this->listeners_) - listener->on_scan_end(); -#endif -#ifdef ESPHOME_BLE_DEVICE_BASE_LISTENER_COUNT - for (auto *listener : this->neutral_listeners_) - listener->on_scan_end(); -#endif - } #ifdef USE_ESP32_BLE_DEVICE this->discovered_log_.clear(); #endif @@ -277,15 +265,11 @@ void ESP32BLETracker::start_scan_(bool first) { this->scan_params_.scan_interval = this->scan_interval_; uint32_t window = this->scan_window_; #ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT - // Count fresh: an automation can start a scan before loop() refreshes the - // counts. An equal window changes nothing and must not arm the restart. - this->using_connection_window_ = this->connection_scan_window_ != 0 && - this->connection_scan_window_ != this->scan_window_ && - this->count_client_states_().active > 0; - if (this->using_connection_window_) { + // Count fresh: an automation can start a scan before loop() refreshes the counts. + window = this->desired_scan_window_(this->count_client_states_().active); + if (window != this->scan_window_) { // Guarantee the connection airtime instead of scanning wall to wall. - ESP_LOGV(TAG, "Connection active, using %" PRIu32 " unit scan window", this->connection_scan_window_); - window = this->connection_scan_window_; + ESP_LOGV(TAG, "Connection active, using %" PRIu32 " unit scan window", window); } #endif this->scan_params_.scan_window = window; @@ -533,26 +517,28 @@ void ESP32BLETracker::cleanup_scan_state_(bool is_stop_complete) { // Reset timeout state machine instead of cancelling scheduler timeout this->scan_timeout_state_ = ScanTimeoutState::INACTIVE; - bool notify_scan_end = true; -#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT - // Window-change restart continues the scan period: no on_scan_end here. - // The flag stays set so start_scan_ skips its sweep too. - notify_scan_end = !this->skip_next_scan_end_; -#endif - if (notify_scan_end) { -#ifdef ESPHOME_ESP32_BLE_TRACKER_LISTENER_COUNT - for (auto *listener : this->listeners_) - listener->on_scan_end(); -#endif -#ifdef ESPHOME_BLE_DEVICE_BASE_LISTENER_COUNT - for (auto *listener : this->neutral_listeners_) - listener->on_scan_end(); -#endif - } + this->notify_scan_end_(); this->set_scanner_state_(ScannerState::IDLE); } +void ESP32BLETracker::notify_scan_end_() { +#ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT + // Window-change restart continues the same scan period; the flag stays set + // across the stop and is cleared by the restart in start_scan_. + if (this->skip_next_scan_end_) + return; +#endif +#ifdef ESPHOME_ESP32_BLE_TRACKER_LISTENER_COUNT + for (auto *listener : this->listeners_) + listener->on_scan_end(); +#endif +#ifdef ESPHOME_BLE_DEVICE_BASE_LISTENER_COUNT + for (auto *listener : this->neutral_listeners_) + listener->on_scan_end(); +#endif +} + void ESP32BLETracker::handle_scanner_failure_() { this->stop_scan_(); if (this->scan_start_fail_count_ == std::numeric_limits::max()) { diff --git a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h index 6aa69c6bb4..618444e626 100644 --- a/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h +++ b/esphome/components/esp32_ble_tracker/esp32_ble_tracker.h @@ -231,6 +231,8 @@ class ESP32BLETracker final : public Component, protected: /// Returns true when a stop was issued to the controller. bool stop_scan_(); + /// Fire on_scan_end on every listener unless a window-change restart suppressed it. + void notify_scan_end_(); /// Start a single scan by setting up the parameters and doing some esp-idf calls. void start_scan_(bool first); /// Called when a `ESP_GAP_BLE_SCAN_RESULT_EVT` event is received. @@ -321,6 +323,10 @@ class ESP32BLETracker final : public Component, /// Window used while a GATT connection is active; set by the user, or /// defaulted when the window was raised to full duty (0 = no fallback). uint32_t connection_scan_window_{0}; + /// The window to scan at for the given number of active GATT connections. + uint32_t desired_scan_window_(uint8_t active) const { + return (this->connection_scan_window_ != 0 && active > 0) ? this->connection_scan_window_ : this->scan_window_; + } #endif esp_bt_status_t scan_start_failed_{ESP_BT_STATUS_SUCCESS}; esp_bt_status_t scan_set_param_failed_{ESP_BT_STATUS_SUCCESS}; @@ -348,9 +354,6 @@ class ESP32BLETracker final : public Component, bool ble_was_disabled_ : 1 {true}; bool parse_advertisements_ : 1 {false}; #ifdef ESPHOME_ESP32_BLE_TRACKER_CLIENT_COUNT - /// The running scan was started with connection_scan_window_; lets loop() - /// restart the scan at the configured window when the last connection drops. - bool using_connection_window_ : 1 {false}; /// Suppress the window-change restart's on_scan_end sweeps (stop and start). bool skip_next_scan_end_ : 1 {false}; #endif @@ -363,10 +366,10 @@ class ESP32BLETracker final : public Component, MONITORING, // Actively monitoring for timeout EXCEEDED_WAIT, // Timeout exceeded, waiting one loop before reboot }; + ScanTimeoutState scan_timeout_state_{ScanTimeoutState::INACTIVE}; uint32_t scan_start_time_{0}; /// Precomputed timeout value: scan_duration_ * 2000 uint32_t scan_timeout_ms_{0}; - ScanTimeoutState scan_timeout_state_{ScanTimeoutState::INACTIVE}; }; // NOLINTNEXTLINE diff --git a/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py b/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py index bd577248fb..8612ac6732 100644 --- a/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py +++ b/tests/component_tests/esp32_ble_tracker/test_scan_window_default.py @@ -195,44 +195,29 @@ def test_connection_scan_window_truncation_collapse_rejected( ) -def test_codegen_connection_window_when_raised( - generate_main: Callable[[str | Path], str], - component_config_path: Callable[[str], Path], -) -> None: - main_cpp = generate_main(component_config_path("scan_window_raised.yaml")) - assert "set_scan_window(512)" in main_cpp - assert "set_connection_scan_window(48)" in main_cpp - - -def test_codegen_no_connection_window_for_explicit_window( - generate_main: Callable[[str | Path], str], - component_config_path: Callable[[str], Path], -) -> None: - main_cpp = generate_main(component_config_path("scan_window_explicit.yaml")) - assert "set_scan_window(48)" in main_cpp - assert "set_connection_scan_window" not in main_cpp - - -def test_codegen_no_connection_window_without_gatt_clients( - generate_main: Callable[[str | Path], str], - component_config_path: Callable[[str], Path], -) -> None: - """Scan-only builds compile the connection-window path out, so the raised - default must not emit a call to the guarded setter.""" - main_cpp = generate_main(component_config_path("scan_window_scan_only.yaml")) - assert "set_scan_window(512)" in main_cpp - assert "set_connection_scan_window" not in main_cpp - - -def test_user_set_connection_window_warns_without_gatt_clients( +@pytest.mark.parametrize( + ("config_file", "window_call", "connection_call", "warns"), + [ + # Raised window with GATT clients: the injected fallback is emitted. + ("scan_window_raised.yaml", "set_scan_window(512)", True, False), + # Explicit window: nothing injected. + ("scan_window_explicit.yaml", "set_scan_window(48)", False, False), + # Scan-only build compiles the path out: the injected default is + # dropped silently, a user-set value warns. + ("scan_window_scan_only.yaml", "set_scan_window(512)", False, False), + ("scan_window_user_set_scan_only.yaml", "set_scan_window(512)", False, True), + ], +) +def test_connection_scan_window_codegen( generate_main: Callable[[str | Path], str], component_config_path: Callable[[str], Path], caplog: pytest.LogCaptureFixture, + config_file: str, + window_call: str, + connection_call: bool, + warns: bool, ) -> None: - """A user-set value that cannot take effect warns; the injected default - (previous test) is dropped silently.""" - main_cpp = generate_main( - component_config_path("scan_window_user_set_scan_only.yaml") - ) - assert "set_connection_scan_window" not in main_cpp - assert "'connection_scan_window' has no effect" in caplog.text + main_cpp = generate_main(component_config_path(config_file)) + assert window_call in main_cpp + assert ("set_connection_scan_window(48)" in main_cpp) == connection_call + assert ("'connection_scan_window' has no effect" in caplog.text) == warns