diff --git a/esphome/components/captive_portal/__init__.py b/esphome/components/captive_portal/__init__.py index 703ae983926..d62c7180978 100644 --- a/esphome/components/captive_portal/__init__.py +++ b/esphome/components/captive_portal/__init__.py @@ -1,7 +1,7 @@ import logging import esphome.codegen as cg -from esphome.components import web_server_base +from esphome.components import web_server_base, wifi from esphome.components.web_server_base import CONF_WEB_SERVER_BASE_ID from esphome.config_helpers import filter_source_files_from_platform import esphome.config_validation as cv @@ -101,6 +101,9 @@ async def to_code(config): var = cg.new_Pvariable(config[CONF_ID], paren) await cg.register_component(var, config) cg.add_define("USE_CAPTIVE_PORTAL") + # The portal reads wifi scan results from the web server task; this makes the + # wifi component guard them with a lock on multi-threaded platforms. + wifi.request_wifi_scan_results_lock() if config[CONF_COMPRESSION] == "gzip": cg.add_define("USE_CAPTIVE_PORTAL_GZIP") diff --git a/esphome/components/captive_portal/captive_portal.cpp b/esphome/components/captive_portal/captive_portal.cpp index 365e5f64db5..228fdf79340 100644 --- a/esphome/components/captive_portal/captive_portal.cpp +++ b/esphome/components/captive_portal/captive_portal.cpp @@ -24,23 +24,28 @@ void CaptivePortal::handle_config(AsyncWebServerRequest *request) { stream->printf(R"({"mac":"%s","name":"%s","aps":[{})", mac_str, App.get_name().c_str()); #endif - for (auto &scan : wifi::global_wifi_component->get_scan_result()) { - if (scan.get_is_hidden()) - continue; + { + // Invariant: only bounded in-memory work under the lock; the network send + // happens later in request->send() + wifi::ScanResultsLock lock(wifi::global_wifi_component); + for (const auto &scan : wifi::global_wifi_component->get_scan_result()) { + if (scan.get_is_hidden()) + continue; - // Assumes no " in ssid, possible unicode isses? + // Assumes no " in ssid, possible unicode issues? #ifdef USE_ESP8266 - stream->print(ESPHOME_F(",{\"ssid\":\"")); - stream->print(scan.get_ssid().c_str()); - stream->print(ESPHOME_F("\",\"rssi\":")); - stream->print(scan.get_rssi()); - stream->print(ESPHOME_F(",\"lock\":")); - stream->print(scan.get_with_auth()); - stream->print(ESPHOME_F("}")); + stream->print(ESPHOME_F(",{\"ssid\":\"")); + stream->print(scan.get_ssid().c_str()); + stream->print(ESPHOME_F("\",\"rssi\":")); + stream->print(scan.get_rssi()); + stream->print(ESPHOME_F(",\"lock\":")); + stream->print(scan.get_with_auth()); + stream->print(ESPHOME_F("}")); #else - stream->printf(R"(,{"ssid":"%s","rssi":%d,"lock":%d})", scan.get_ssid().c_str(), scan.get_rssi(), - scan.get_with_auth()); + stream->printf(R"(,{"ssid":"%s","rssi":%d,"lock":%d})", scan.get_ssid().c_str(), scan.get_rssi(), + scan.get_with_auth()); #endif + } } stream->print(ESPHOME_F("]}")); request->send(stream); diff --git a/esphome/components/wifi/__init__.py b/esphome/components/wifi/__init__.py index 1810a62155e..4bb6629da11 100644 --- a/esphome/components/wifi/__init__.py +++ b/esphome/components/wifi/__init__.py @@ -823,6 +823,7 @@ IP_STATE_LISTENERS_KEY = "wifi_ip_state_listeners" SCAN_RESULTS_LISTENERS_KEY = "wifi_scan_results_listeners" CONNECT_STATE_LISTENERS_KEY = "wifi_connect_state_listeners" POWER_SAVE_LISTENERS_KEY = "wifi_power_save_listeners" +SCAN_RESULTS_LOCK_KEY = "wifi_scan_results_lock" def request_wifi_scan_results(): @@ -835,6 +836,19 @@ def request_wifi_scan_results(): CORE.data[KEEP_SCAN_RESULTS_KEY] = True +def request_wifi_scan_results_lock() -> None: + """Request that scan results be guarded by a lock for cross-task readers. + + Components that read WiFi scan results from a task other than the main loop + (for example a web server handler) must call this function during their code + generation, and their C++ code must hold a wifi::ScanResultsLock while + iterating get_scan_result(). On multi-threaded platforms this compiles in a + lock that scan result writers hold; on single-threaded platforms it compiles + to nothing. + """ + CORE.data[SCAN_RESULTS_LOCK_KEY] = True + + def enable_runtime_power_save_control(): """Enable runtime WiFi power save control. @@ -896,6 +910,8 @@ async def final_step(): cg.add_define("USE_WIFI_RUNTIME_POWER_SAVE") if CORE.data.get(RUNTIME_ROAMING_SUPPRESSION_KEY, False): cg.add_define("USE_WIFI_RUNTIME_ROAMING_SUPPRESSION") + if CORE.data.get(SCAN_RESULTS_LOCK_KEY): + cg.add_define("USE_WIFI_SCAN_RESULTS_LOCK") # Generate listener defines - each listener type has its own #ifdef ip_state_count = CORE.data.get(IP_STATE_LISTENERS_KEY, 0) diff --git a/esphome/components/wifi/wifi_component.cpp b/esphome/components/wifi/wifi_component.cpp index 44e3cb6af91..650b06cae1c 100644 --- a/esphome/components/wifi/wifi_component.cpp +++ b/esphome/components/wifi/wifi_component.cpp @@ -1483,23 +1483,26 @@ void WiFiComponent::check_scanning_finished() { } ESP_LOGD(TAG, "Found networks:"); - for (auto &res : this->scan_result_) { - for (auto &ap : this->sta_) { - if (res.matches(ap)) { - res.set_matches(true); - // Cache priority lookup - do single search instead of 2 separate searches - const bssid_t &bssid = res.get_bssid(); - if (!this->has_sta_priority(bssid)) { - this->set_sta_priority(bssid, ap.get_priority()); + { + ScanResultsLock lock(this); + for (auto &res : this->scan_result_) { + for (auto &ap : this->sta_) { + if (res.matches(ap)) { + res.set_matches(true); + // Cache priority lookup - do single search instead of 2 separate searches + const bssid_t &bssid = res.get_bssid(); + if (!this->has_sta_priority(bssid)) { + this->set_sta_priority(bssid, ap.get_priority()); + } + res.set_priority(this->get_sta_priority(bssid)); + break; } - res.set_priority(this->get_sta_priority(bssid)); - break; } } - } - // Sort scan results using insertion sort for better memory efficiency - insertion_sort_scan_results(this->scan_result_); + // Sort scan results using insertion sort for better memory efficiency + insertion_sort_scan_results(this->scan_result_); + } // Log matching networks (non-matching already logged at VERBOSE in scan callback) for (auto &res : this->scan_result_) { @@ -1885,11 +1888,13 @@ bool WiFiComponent::transition_to_phase_(WiFiRetryPhase new_phase) { // Phase-specific setup switch (new_phase) { #ifdef USE_WIFI_FAST_CONNECT - case WiFiRetryPhase::FAST_CONNECT_CYCLING_APS: + case WiFiRetryPhase::FAST_CONNECT_CYCLING_APS: { // Move to next configured AP - clear old scan data so new AP is tried with config only this->selected_sta_index_++; + ScanResultsLock lock(this); this->scan_result_.clear(); break; + } #endif case WiFiRetryPhase::EXPLICIT_HIDDEN: @@ -2404,6 +2409,7 @@ void WiFiComponent::clear_roaming_state_() { void WiFiComponent::release_scan_results_() { if (!this->keep_scan_results_) { + ScanResultsLock lock(this); #if defined(USE_RP2) || defined(USE_ESP32) // std::vector - use swap trick since shrink_to_fit is non-binding decltype(this->scan_result_)().swap(this->scan_result_); diff --git a/esphome/components/wifi/wifi_component.h b/esphome/components/wifi/wifi_component.h index 6faabc223c0..43e44a135f2 100644 --- a/esphome/components/wifi/wifi_component.h +++ b/esphome/components/wifi/wifi_component.h @@ -187,6 +187,13 @@ template using wifi_scan_vector_t = std::vector; template using wifi_scan_vector_t = FixedVector; #endif +// A consumer component (e.g. the captive portal) reads scan results from another +// task; guard them with a real lock only on platforms that actually run multiple +// threads. See ScanResultsLock below the WiFiComponent class. +#if defined(USE_WIFI_SCAN_RESULTS_LOCK) && !defined(ESPHOME_THREAD_SINGLE) +#define WIFI_SCAN_RESULTS_LOCK_ENABLED +#endif + /// 20-byte string: 18 chars inline + null, heap for longer. Always null-terminated. /// Used internally for WiFi SSID/password storage to reduce heap fragmentation. class CompactString { @@ -506,6 +513,9 @@ class WiFiComponent final : public Component { const char *get_use_address() const { return this->use_address_; } void set_use_address(const char *use_address) { this->use_address_ = use_address; } + /// Main-loop callers may read this directly. Callers on any other task must + /// hold a ScanResultsLock for the whole iteration and must call + /// wifi.request_wifi_scan_results_lock() from their code generation. const wifi_scan_vector_t &get_scan_result() const { return scan_result_; } network::IPAddress wifi_soft_ap_ip(); @@ -817,6 +827,8 @@ class WiFiComponent final : public Component { friend void event_handler(void *arg, esp_event_base_t event_base, int32_t event_id, void *event_data); #endif + friend class ScanResultsLock; + #ifdef USE_RP2 static int s_wifi_scan_result(void *env, const cyw43_ev_scan_result_t *result); void wifi_scan_result_(void *env, const cyw43_ev_scan_result_t *result); @@ -831,7 +843,11 @@ class WiFiComponent final : public Component { // Large/pointer-aligned members first FixedVector sta_; std::vector sta_priorities_; + // Guarded by ScanResultsLock (see below this class) wifi_scan_vector_t scan_result_; +#ifdef WIFI_SCAN_RESULTS_LOCK_ENABLED + Mutex scan_result_lock_; +#endif #ifdef USE_WIFI_AP WiFiAP ap_; #endif @@ -1003,5 +1019,25 @@ class WiFiComponent final : public Component { extern WiFiComponent *global_wifi_component; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) +/// Guards WiFiComponent::scan_result_. Invariant: every mutation and every read +/// from outside the main loop holds this lock, and holders only do bounded work +/// (never unbounded waits or network sends). On every platform where the lock is +/// enabled (ESP32, LibreTiny) scan-done events are drained from the event queue +/// on the main loop, so all writers are main-loop there and main-loop reads take +/// no lock. Single-threaded platforms write from driver context and the lock is +/// a no-op. Compiles to nothing unless a cross-task reader is in the build and +/// the platform is multi-threaded (WIFI_SCAN_RESULTS_LOCK_ENABLED). +class ScanResultsLock { + public: +#ifdef WIFI_SCAN_RESULTS_LOCK_ENABLED + ScanResultsLock(WiFiComponent *parent) : guard_(parent->scan_result_lock_) {} + + private: + LockGuard guard_; +#else + ScanResultsLock(WiFiComponent *) {} +#endif +}; + } // namespace esphome::wifi #endif diff --git a/esphome/components/wifi/wifi_component_esp8266.cpp b/esphome/components/wifi/wifi_component_esp8266.cpp index 84b864c0c5d..e082b2c8c18 100644 --- a/esphome/components/wifi/wifi_component_esp8266.cpp +++ b/esphome/components/wifi/wifi_component_esp8266.cpp @@ -733,6 +733,8 @@ void WiFiComponent::s_wifi_scan_done_callback(void *arg, STATUS status) { } void WiFiComponent::wifi_scan_done_callback_(void *arg, STATUS status) { + // Compiles to nothing here; kept so every scan_result_ mutation holds the lock + ScanResultsLock lock(this); this->scan_result_.clear(); if (status != OK) { diff --git a/esphome/components/wifi/wifi_component_esp_idf.cpp b/esphome/components/wifi/wifi_component_esp_idf.cpp index 2ade015a255..d78cd213807 100644 --- a/esphome/components/wifi/wifi_component_esp_idf.cpp +++ b/esphome/components/wifi/wifi_component_esp_idf.cpp @@ -891,65 +891,65 @@ void WiFiComponent::wifi_process_event_(IDFWiFiEvent *data) { const auto &it = data->data.sta_scan_done; ESP_LOGV(TAG, "Scan done: status=%" PRIu32 " number=%u scan_id=%u", it.status, it.number, it.scan_id); - scan_result_.clear(); - this->scan_done_ = true; - if (it.status != 0) { - // scan error - return; - } - - if (it.number == 0) { - // no results - return; - } - uint16_t number = it.number; bool needs_full = this->needs_full_scan_results_(); + { + // Mutate in place under the lock; blocking a portal request is fine and + // avoids scratch buffers + ScanResultsLock lock(this); + this->scan_result_.clear(); + this->scan_done_ = true; + if (it.status != 0) { + // scan error + return; + } - // Smart reserve: full capacity if needed, small reserve otherwise - if (needs_full) { - this->scan_result_.reserve(number); - } else { - this->scan_result_.reserve(WIFI_SCAN_RESULT_FILTERED_RESERVE); - } + if (number == 0) { + // no results + return; + } + + // Smart reserve: full capacity if needed, small reserve otherwise + this->scan_result_.reserve(needs_full ? number : WIFI_SCAN_RESULT_FILTERED_RESERVE); #ifdef USE_ESP32_HOSTED - // getting records one at a time fails on P4 with hosted esp32 WiFi coprocessor - // Presumably an upstream bug, work-around by getting all records at once - // Use stack buffer (3904 bytes / ~80 bytes per record = ~48 records) with heap fallback - static constexpr size_t SCAN_RECORD_STACK_COUNT = 3904 / sizeof(wifi_ap_record_t); - SmallBufferWithHeapFallback records(number); - err = esp_wifi_scan_get_ap_records(&number, records.get()); - if (err != ESP_OK) { - esp_wifi_clear_ap_list(); - ESP_LOGW(TAG, "esp_wifi_scan_get_ap_records failed: %s", esp_err_to_name(err)); - return; - } - for (uint16_t i = 0; i < number; i++) { - wifi_ap_record_t &record = records.get()[i]; -#else - // Process one record at a time to avoid large buffer allocation - for (uint16_t i = 0; i < number; i++) { - wifi_ap_record_t record; - err = esp_wifi_scan_get_ap_record(&record); + // getting records one at a time fails on P4 with hosted esp32 WiFi coprocessor + // Presumably an upstream bug, work-around by getting all records at once + // Use stack buffer (3904 bytes / ~80 bytes per record = ~48 records) with heap fallback + static constexpr size_t SCAN_RECORD_STACK_COUNT = 3904 / sizeof(wifi_ap_record_t); + SmallBufferWithHeapFallback records(number); + err = esp_wifi_scan_get_ap_records(&number, records.get()); if (err != ESP_OK) { - ESP_LOGW(TAG, "esp_wifi_scan_get_ap_record failed: %s", esp_err_to_name(err)); - esp_wifi_clear_ap_list(); // Free remaining records not yet retrieved - break; + esp_wifi_clear_ap_list(); + ESP_LOGW(TAG, "esp_wifi_scan_get_ap_records failed: %s", esp_err_to_name(err)); + return; } + for (uint16_t i = 0; i < number; i++) { + wifi_ap_record_t &record = records.get()[i]; +#else + // Process one record at a time to avoid large buffer allocation + for (uint16_t i = 0; i < number; i++) { + wifi_ap_record_t record; + err = esp_wifi_scan_get_ap_record(&record); + if (err != ESP_OK) { + ESP_LOGW(TAG, "esp_wifi_scan_get_ap_record failed: %s", esp_err_to_name(err)); + esp_wifi_clear_ap_list(); // Free remaining records not yet retrieved + break; + } #endif // USE_ESP32_HOSTED - // Check C string first - avoid std::string construction for non-matching networks - const char *ssid_cstr = reinterpret_cast(record.ssid); + // Check C string first - avoid std::string construction for non-matching networks + const char *ssid_cstr = reinterpret_cast(record.ssid); - // Only construct std::string and store if needed - if (needs_full || this->matches_configured_network_(ssid_cstr, record.bssid)) { - bssid_t bssid; - std::copy(record.bssid, record.bssid + 6, bssid.begin()); - this->scan_result_.emplace_back(bssid, ssid_cstr, strlen(ssid_cstr), record.primary, record.rssi, - record.authmode != WIFI_AUTH_OPEN, ssid_cstr[0] == '\0'); - } else { - this->log_discarded_scan_result_(ssid_cstr, record.bssid, record.rssi, record.primary); + // Only construct std::string and store if needed + if (needs_full || this->matches_configured_network_(ssid_cstr, record.bssid)) { + bssid_t bssid; + std::copy(record.bssid, record.bssid + 6, bssid.begin()); + this->scan_result_.emplace_back(bssid, ssid_cstr, strlen(ssid_cstr), record.primary, record.rssi, + record.authmode != WIFI_AUTH_OPEN, ssid_cstr[0] == '\0'); + } else { + this->log_discarded_scan_result_(ssid_cstr, record.bssid, record.rssi, record.primary); + } } } ESP_LOGV(TAG, "Scan complete: %u found, %zu stored%s", number, this->scan_result_.size(), diff --git a/esphome/components/wifi/wifi_component_libretiny.cpp b/esphome/components/wifi/wifi_component_libretiny.cpp index 59efa4f8425..ce9c4eb6ceb 100644 --- a/esphome/components/wifi/wifi_component_libretiny.cpp +++ b/esphome/components/wifi/wifi_component_libretiny.cpp @@ -657,44 +657,48 @@ bool WiFiComponent::wifi_scan_start_(bool passive) { return true; } void WiFiComponent::wifi_scan_done_callback_() { - this->scan_result_.clear(); - this->scan_done_ = true; - int16_t num = WiFi.scanComplete(); - if (num < 0) - return; - bool needs_full = this->needs_full_scan_results_(); + { + // Mutate in place under the lock; blocking a portal request is fine and + // avoids scratch buffers + ScanResultsLock lock(this); + this->scan_result_.clear(); + this->scan_done_ = true; - // Access scan results directly via WiFi.scan struct to avoid Arduino String allocations - // WiFi.scan is public in LibreTiny for WiFiEvents & WiFiScan static handlers - auto *scan = WiFi.scan; + if (num < 0) + return; - // First pass: count matching networks - size_t count = 0; - for (int i = 0; i < num; i++) { - const char *ssid_cstr = scan->ap[i].ssid; - if (needs_full || this->matches_configured_network_(ssid_cstr, scan->ap[i].bssid.addr)) { - count++; + // Access scan results directly via WiFi.scan struct to avoid Arduino String allocations + // WiFi.scan is public in LibreTiny for WiFiEvents & WiFiScan static handlers + auto *scan = WiFi.scan; + + // First pass: count matching networks + size_t count = 0; + for (int i = 0; i < num; i++) { + const char *ssid_cstr = scan->ap[i].ssid; + if (needs_full || this->matches_configured_network_(ssid_cstr, scan->ap[i].bssid.addr)) { + count++; + } + } + + this->scan_result_.init(count); // Exact allocation + + // Second pass: store matching networks + for (int i = 0; i < num; i++) { + const char *ssid_cstr = scan->ap[i].ssid; + auto &ap = scan->ap[i]; + if (needs_full || this->matches_configured_network_(ssid_cstr, ap.bssid.addr)) { + this->scan_result_.emplace_back(bssid_t{ap.bssid.addr[0], ap.bssid.addr[1], ap.bssid.addr[2], ap.bssid.addr[3], + ap.bssid.addr[4], ap.bssid.addr[5]}, + ssid_cstr, strlen(ssid_cstr), ap.channel, ap.rssi, ap.auth != WIFI_AUTH_OPEN, + ssid_cstr[0] == '\0'); + } else { + this->log_discarded_scan_result_(ssid_cstr, ap.bssid.addr, ap.rssi, ap.channel); + } } } - this->scan_result_.init(count); // Exact allocation - - // Second pass: store matching networks - for (int i = 0; i < num; i++) { - const char *ssid_cstr = scan->ap[i].ssid; - if (needs_full || this->matches_configured_network_(ssid_cstr, scan->ap[i].bssid.addr)) { - auto &ap = scan->ap[i]; - this->scan_result_.emplace_back(bssid_t{ap.bssid.addr[0], ap.bssid.addr[1], ap.bssid.addr[2], ap.bssid.addr[3], - ap.bssid.addr[4], ap.bssid.addr[5]}, - ssid_cstr, strlen(ssid_cstr), ap.channel, ap.rssi, ap.auth != WIFI_AUTH_OPEN, - ssid_cstr[0] == '\0'); - } else { - auto &ap = scan->ap[i]; - this->log_discarded_scan_result_(ssid_cstr, ap.bssid.addr, ap.rssi, ap.channel); - } - } ESP_LOGV(TAG, "Scan complete: %d found, %zu stored%s", num, this->scan_result_.size(), needs_full ? "" : " (filtered)"); WiFi.scanDelete(); diff --git a/esphome/components/wifi/wifi_component_pico_w.cpp b/esphome/components/wifi/wifi_component_pico_w.cpp index 69ac90822f6..69af9e9a4e5 100644 --- a/esphome/components/wifi/wifi_component_pico_w.cpp +++ b/esphome/components/wifi/wifi_component_pico_w.cpp @@ -190,12 +190,16 @@ void WiFiComponent::wifi_scan_result_(void *env, const cyw43_ev_scan_result_t *r std::copy(result->bssid, result->bssid + 6, bssid.begin()); WiFiScanResult res(bssid, ssid_buf, len, result->channel, result->rssi, result->auth_mode != CYW43_AUTH_OPEN, len == 0); + // Compiles to nothing here; kept so every scan_result_ mutation holds the lock + ScanResultsLock lock(this); if (std::find(this->scan_result_.begin(), this->scan_result_.end(), res) == this->scan_result_.end()) { this->scan_result_.push_back(res); } } bool WiFiComponent::wifi_scan_start_(bool passive) { + // Compiles to nothing here; kept so every scan_result_ mutation holds the lock + ScanResultsLock lock(this); this->scan_result_.clear(); this->scan_done_ = false; s_scan_result_count = 0; diff --git a/esphome/core/defines.h b/esphome/core/defines.h index 03175bf2cc2..b794254e12b 100644 --- a/esphome/core/defines.h +++ b/esphome/core/defines.h @@ -257,6 +257,7 @@ #define BLUETOOTH_PROXY_MAX_CONNECTIONS 3 #define BLUETOOTH_PROXY_ADVERTISEMENT_BATCH_SIZE 16 #define USE_CAPTIVE_PORTAL +#define USE_WIFI_SCAN_RESULTS_LOCK #define USE_ESP32_BLE #define USE_ESP32_BLE_MAX_CONNECTIONS 3 #define USE_ESP32_BLE_CLIENT @@ -393,6 +394,7 @@ #define USE_ESP8266_CRASH_HANDLER #define USE_ARDUINO_VERSION_CODE VERSION_CODE(3, 1, 2) #define USE_CAPTIVE_PORTAL +#define USE_WIFI_SCAN_RESULTS_LOCK #define USE_ESP8266_LOGGER_SERIAL #define USE_ESP8266_LOGGER_SERIAL1 #define USE_ESP8266_PREFERENCES_FLASH @@ -448,6 +450,7 @@ #ifdef USE_LIBRETINY #define USE_BK72XX_BLE #define USE_CAPTIVE_PORTAL +#define USE_WIFI_SCAN_RESULTS_LOCK #define USE_SOCKET_IMPL_LWIP_SOCKETS #define USE_LWIP_FAST_SELECT #define USE_WEBSERVER