From f75d6ab88d1dd3d1be67aab789d69520dd8bb090 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 10 Apr 2026 17:39:37 -1000 Subject: [PATCH 1/3] [esphome.ota] Update docstrings to match post-filter behavior Two doc-drift fixes flagged by copilot review: 1. esphome_fast_select_set_ota_listener_sock() header comment claimed passing NULL 'clears the filter' so wake fires on every RCVPLUS. The actual code stores NULL and the conn == s_ota_listener_conn check never matches, so NULL means 'no wakes' not 'all wakes'. Rewrite to describe the actual semantics (install a listener to enable filtered wakes; NULL disables OTA wakes entirely). 2. ESPHomeOTAComponent::loop() docstring still claimed false wakes from unrelated monitored sockets are expected. Post-filter that's no longer true on fast-select (filtered to OTA listener netconn) or raw TCP (per-pcb accept_fn_). Rewrite to describe the current behavior: loop() runs ~once per real incoming OTA connection, with the idle self-disable retained as a safety net for the few narrow cases where a wake can land with no pending work (queued-during-session, filter not yet installed, host select fallback). --- .../components/esphome/ota/ota_esphome.cpp | 24 ++++++++++++------- esphome/core/lwip_fast_select.h | 10 +++++--- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 1999b88ef4..408bde14ac 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -99,15 +99,23 @@ void ESPHomeOTAComponent::dump_config() { void ESPHomeOTAComponent::loop() { // Self-disabling idle loop. On the first tick after setup() (and after every session - // cleanup and every false wake), if there's no client and the listener has nothing - // queued, we disable ourselves and go back to sleep. Socket-wake paths (LwIP fast - // select, raw TCP accept, host select) mark us pending-enable via - // App.wake_ota_component_any_context() when a monitored socket signals activity, and - // enable_pending_loops_() reactivates us. + // ends), if there's no client and the listener has nothing queued, we disable + // ourselves and go back to sleep. Wake paths (LwIP fast select with listener filter, + // raw TCP accept_fn_, host select) mark us pending-enable via + // App.wake_ota_component_any_context() when an incoming connection arrives on the OTA + // listen socket; enable_pending_loops_() reactivates us. // - // False wakes from unrelated monitored sockets are expected — the event callbacks - // fire on every RCVPLUS across all monitored sockets, not just OTA's listener — and - // they land here with no pending work. + // On fast-select platforms (ESP32 / LibreTiny), the callback is filtered to OTA's + // listener netconn, so unrelated monitored sockets (API client data, mDNS, etc.) do + // NOT wake OTA through that path. Raw TCP (ESP8266 / RP2040) is inherently filtered: + // accept_fn_ is registered per listener pcb and only fires on completed handshakes + // to that specific listener. So in normal steady state this loop() runs exactly + // once per real incoming OTA connection. + // + // We can still land here with no pending work in a few narrow cases — a second + // connection queued on the listener while an OTA session was active, a listener + // filter that hasn't been installed yet, or the host select path (no filter) — which + // is why the idle-check and self-disable are retained as a safety net. // // cleanup_connection_() deliberately does NOT call disable_loop() — letting loop() // run one more iteration after a session ends guarantees we re-read server_->ready() diff --git a/esphome/core/lwip_fast_select.h b/esphome/core/lwip_fast_select.h index 9e1438ecdd..2572a9103d 100644 --- a/esphome/core/lwip_fast_select.h +++ b/esphome/core/lwip_fast_select.h @@ -53,12 +53,16 @@ static inline bool esphome_lwip_socket_has_data(struct lwip_sock *sock) { /// The sock pointer must have been obtained from esphome_lwip_get_sock(). void esphome_lwip_hook_socket(struct lwip_sock *sock); -/// Filter the inline OTA wake hook in the fast-select callback so it only fires for -/// RCVPLUS events on this specific listener's netconn. Without this, every monitored +/// Install an OTA listener netconn as the wake-filter target for the fast-select +/// callback. After this is called, the OTA wake hook only fires for RCVPLUS events +/// whose `conn` argument matches this listener's netconn — i.e. only on actual +/// incoming connections to the OTA listen socket. Without this, every monitored /// socket's RCVPLUS (API client data, web server, mDNS, etc.) would mark OTA /// pending-enable and force its loop to run a wake-up tick only to re-disable itself. /// Captured at OTA setup(); stays pointing at the listener for the device's lifetime. -/// Pass NULL to clear the filter (wake fires on every RCVPLUS, pre-filter behavior). +/// Passing NULL disables OTA wake notifications entirely (no RCVPLUS event will match +/// a null listener), which is the correct behavior before the listener is installed +/// and after it's torn down. void esphome_fast_select_set_ota_listener_sock(struct lwip_sock *sock); /// Set or clear TCP_NODELAY on a socket's tcp_pcb directly. From f8a1c54690fa58eed69ba462229a93091e1463e4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 10 Apr 2026 17:44:18 -1000 Subject: [PATCH 2/3] [esphome.ota] Document NULL-fallback degraded mode for listener filter Per review feedback: if esphome_lwip_get_sock() ever returned nullptr (shouldn't after successful listen(), but defensively), the listener filter compare would never match and no fast-select wakes would fire. loop()'s self-disable safety net + the first-tick-after-setup window cover that degraded mode correctly. Spell it out in the comment so a future reader doesn't treat the nullptr path as a silent bug. --- esphome/components/esphome/ota/ota_esphome.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 408bde14ac..2e2c0bc73b 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -76,10 +76,15 @@ void ESPHomeOTAComponent::setup() { // Install the listener filter so the fast-select RCVPLUS wake hook only fires for // events on this listener's netconn (i.e. new incoming connections). Without this, // every RCVPLUS across all monitored sockets (API client data, mDNS, etc.) would - // pay the inline hook's two volatile stores + memw barriers to mark OTA - // pending-enable, even though OTA would just re-disable itself on the next tick. - // Uses the existing public esphome_lwip_get_sock() lookup instead of adding a - // dedicated accessor on Socket — the lookup happens once at setup, not per event. + // pay the wake hook cost to mark OTA pending-enable, even though OTA would just + // re-disable itself on the next tick. Uses the existing public esphome_lwip_get_sock() + // lookup instead of adding a dedicated accessor on Socket — the lookup happens once + // at setup, not per event. If the lookup ever returns nullptr (shouldn't after a + // successful listen()), the filter is installed with nullptr and the callback + // compare never matches, so no wakes fire — loop() falls back to the self-disable + // safety net. That degraded mode is the correct behavior: OTA will still work on + // the first loop tick (which runs during setup before disable), and any subsequent + // wakes go through the wake_loop_any_context() path that fires unconditionally. esphome_fast_select_set_ota_listener_sock(esphome_lwip_get_sock(this->server_->get_fd())); #endif } From 82699a7ce3c175a1efe71fa48438c28243193572 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 10 Apr 2026 17:47:36 -1000 Subject: [PATCH 3/3] [esphome.ota] Trim verbose comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per CLAUDE.md: comments should explain the non-obvious why, not narrate the design journey. Remove the reasoning-aloud, before/after framing, redundant restatements, and cross-file cross-references — the commit messages and PR description already carry that context. Behavior unchanged. --- .../components/esphome/ota/ota_esphome.cpp | 55 ++++--------------- esphome/components/ota/__init__.py | 7 +-- .../components/socket/lwip_raw_tcp_impl.cpp | 11 +--- esphome/core/application.cpp | 11 +--- esphome/core/application.h | 14 +---- esphome/core/lwip_fast_select.c | 25 ++------- esphome/core/lwip_fast_select.h | 14 ++--- 7 files changed, 29 insertions(+), 108 deletions(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 2e2c0bc73b..041aa4bbf9 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -69,22 +69,11 @@ void ESPHomeOTAComponent::setup() { return; } - // Register for socket wake notifications. loop() disables itself on its first - // idle tick — no need to disable_loop() here explicitly. + // loop() self-disables on its first idle tick; no explicit disable_loop() needed here. App.set_ota_wake_component(this); #ifdef USE_LWIP_FAST_SELECT - // Install the listener filter so the fast-select RCVPLUS wake hook only fires for - // events on this listener's netconn (i.e. new incoming connections). Without this, - // every RCVPLUS across all monitored sockets (API client data, mDNS, etc.) would - // pay the wake hook cost to mark OTA pending-enable, even though OTA would just - // re-disable itself on the next tick. Uses the existing public esphome_lwip_get_sock() - // lookup instead of adding a dedicated accessor on Socket — the lookup happens once - // at setup, not per event. If the lookup ever returns nullptr (shouldn't after a - // successful listen()), the filter is installed with nullptr and the callback - // compare never matches, so no wakes fire — loop() falls back to the self-disable - // safety net. That degraded mode is the correct behavior: OTA will still work on - // the first loop tick (which runs during setup before disable), and any subsequent - // wakes go through the wake_loop_any_context() path that fires unconditionally. + // Filter fast-select wakes to this listener only. If the sock lookup returns nullptr, + // no wakes fire and loop() falls back to the self-disable safety net. esphome_fast_select_set_ota_listener_sock(esphome_lwip_get_sock(this->server_->get_fd())); #endif } @@ -103,31 +92,10 @@ void ESPHomeOTAComponent::dump_config() { } void ESPHomeOTAComponent::loop() { - // Self-disabling idle loop. On the first tick after setup() (and after every session - // ends), if there's no client and the listener has nothing queued, we disable - // ourselves and go back to sleep. Wake paths (LwIP fast select with listener filter, - // raw TCP accept_fn_, host select) mark us pending-enable via - // App.wake_ota_component_any_context() when an incoming connection arrives on the OTA - // listen socket; enable_pending_loops_() reactivates us. - // - // On fast-select platforms (ESP32 / LibreTiny), the callback is filtered to OTA's - // listener netconn, so unrelated monitored sockets (API client data, mDNS, etc.) do - // NOT wake OTA through that path. Raw TCP (ESP8266 / RP2040) is inherently filtered: - // accept_fn_ is registered per listener pcb and only fires on completed handshakes - // to that specific listener. So in normal steady state this loop() runs exactly - // once per real incoming OTA connection. - // - // We can still land here with no pending work in a few narrow cases — a second - // connection queued on the listener while an OTA session was active, a listener - // filter that hasn't been installed yet, or the host select path (no filter) — which - // is why the idle-check and self-disable are retained as a safety net. - // - // cleanup_connection_() deliberately does NOT call disable_loop() — letting loop() - // run one more iteration after a session ends guarantees we re-read server_->ready() - // and either accept a client that queued during the session or disable cleanly here. - // - // Note: No need to check server_ for null — setup() marks the component failed if - // server_ creation fails. + // Self-disabling idle loop. Runs when a wake path marks us pending-enable (fast-select + // listener filter, raw-TCP accept_fn_, or host select), finds no work, and goes back + // to sleep. cleanup_connection_() deliberately leaves the loop enabled for one more + // iteration so a connection queued mid-session is still caught here. if (this->client_ == nullptr && !this->server_->ready()) { this->disable_loop(); return; @@ -611,12 +579,9 @@ void ESPHomeOTAComponent::cleanup_connection_() { #ifdef USE_OTA_PASSWORD this->cleanup_auth_(); #endif - // Do not disable_loop() here. loop() itself disables when idle. If a second - // connection was queued on the listener while we were busy, the wake flag was - // set while this component was in LOOP state — enable_pending_loops_() only - // scans the inactive section and would never clear it. Letting loop() run one - // more iteration guarantees we re-check server_->ready() and either accept the - // queued client or disable ourselves cleanly. + // Intentionally no disable_loop() — letting loop() run one more iteration catches + // any connection that queued on the listener mid-session (otherwise the wake flag, + // set while we were in LOOP state, would be lost to enable_pending_loops_()). } void ESPHomeOTAComponent::yield_and_feed_watchdog_() { diff --git a/esphome/components/ota/__init__.py b/esphome/components/ota/__init__.py index 3abfd2c69e..e0de31f239 100644 --- a/esphome/components/ota/__init__.py +++ b/esphome/components/ota/__init__.py @@ -103,11 +103,8 @@ BASE_OTA_SCHEMA = cv.Schema( @coroutine_with_priority(CoroPriority.OTA_UPDATES) async def to_code(config): cg.add_define("USE_OTA") - # Separate compiler -D flag using an ESPHOME_-prefixed name so .c translation units - # (which cannot include defines.h because macros.h → Arduino.h breaks the C compile - # under Arduino builds) can still tell OTA is compiled in. Needed by the fast-select - # OTA wake hook in lwip_fast_select.c. A distinct name avoids the "USE_OTA redefined" - # warning that would fire if we also emitted -DUSE_OTA — defines.h already has it. + # Separate -D flag so .c files can see it — .c can't include defines.h (macros.h + # pulls in Arduino.h). Different name avoids USE_OTA redefinition warnings. cg.add_build_flag("-DESPHOME_USE_OTA") CORE.add_job(final_step) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index 08de3f73a8..04605f1e55 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -11,7 +11,7 @@ #include "esphome/core/wake.h" #include "esphome/core/log.h" #ifdef USE_OTA -#include "esphome/core/application.h" // for App.wake_ota_component_any_context() +#include "esphome/core/application.h" #endif #ifdef USE_ESP8266 @@ -858,13 +858,8 @@ err_t LWIPRawListenImpl::accept_fn_(struct tcp_pcb *newpcb, err_t err) { tcp_recv(newpcb, LWIPRawListenImpl::s_queued_recv_fn); LWIP_LOG("Accepted connection, queue size: %d", this->accepted_socket_count_); #ifdef USE_OTA - // Mark the OTA component loop to be re-enabled if it disabled itself while idle. - // This MUST happen before wake_loop_any_context() below — otherwise the main loop - // could wake, run a full iteration, and finish before we set the pending-enable - // flags, losing the wake event. accept_fn_ only fires for the specific listener - // pcb it was registered on, so there's implicit filtering here (no "false wakes" - // from unrelated sockets, unlike the fast-select path). Safe from RP2040's - // low-priority user IRQ context — only writes a volatile bool, no locks. + // Mark OTA pending-enable before the wake below — flags must be visible before the + // main task runs. accept_fn_ is per-pcb, so filtering is implicit here. esphome::App.wake_ota_component_any_context(); #endif // Wake the main loop immediately so it can accept the new connection. diff --git a/esphome/core/application.cpp b/esphome/core/application.cpp index ffb374b321..c847c173f1 100644 --- a/esphome/core/application.cpp +++ b/esphome/core/application.cpp @@ -450,10 +450,7 @@ void Application::enable_pending_loops_() { } #if defined(USE_OTA) && defined(USE_LWIP_FAST_SELECT) -// Extern-C trampoline for the lwip fast-select callback to reach the C++ wake method. -// Invoked only when the listener filter in lwip_fast_select.c matches — i.e. only on -// actual incoming connections to the OTA listen socket — so the function-call overhead -// here is paid at most once per real OTA attempt, not on every monitored-socket event. +// C trampoline called from lwip_fast_select.c when the listener filter matches. extern "C" void esphome_wake_ota_component_any_context() { App.wake_ota_component_any_context(); } #endif @@ -563,11 +560,7 @@ void Application::yield_with_select_(uint32_t delay_ms) { // ret == 0: timeout occurred - normal and expected if (ret >= 0) [[likely]] { #ifdef USE_OTA - // Dead code today — host does not currently support the esphome OTA platform, - // so ota_wake_component_ is always null and this is a no-op. Kept so the wake - // path works out of the box if host ever gains OTA support. Host has no listener - // filter (that lives in lwip_fast_select.c, ESP32/LibreTiny only), so any - // ret > 0 fires this — harmless when the pointer is null. + // No-op today — host has no esphome OTA platform, so ota_wake_component_ is null. if (ret > 0) { this->wake_ota_component_any_context(); } diff --git a/esphome/core/application.h b/esphome/core/application.h index 7a3595ac49..8c9789774b 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -560,17 +560,9 @@ class Application { static void IRAM_ATTR wake_loop_any_context() { esphome::wake_loop_any_context(); } #ifdef USE_OTA - /// Register the OTA component so socket-wake paths can enable its loop when a new - /// connection arrives on the listening socket. OTA calls this once from setup(); - /// the component self-disables its loop on its first idle tick and is re-enabled - /// via wake_ota_component_any_context() when the fast-select filter in - /// lwip_fast_select.c matches an incoming connection on the OTA listener. void set_ota_wake_component(Component *component) { this->ota_wake_component_ = component; } - /// Mark the registered OTA component pending loop-enable. Called from the LwIP - /// TCP/IP task (fast-select callback), raw-TCP accept callback, and host select - /// return path — all task / user-IRQ context, not a real ISR. Does NOT wake the - /// main task itself: every caller already does so separately. Application is a - /// friend of Component, so we set pending_enable_loop_ directly. + // Marks OTA pending-enable from socket wake paths. Does NOT wake the main task — + // every caller already does. Callable from LwIP TCP/IP task and user-IRQ context. void wake_ota_component_any_context() { if (this->ota_wake_component_ != nullptr) { this->ota_wake_component_->pending_enable_loop_ = true; @@ -655,7 +647,7 @@ class Application { // Pointer-sized members first Component *current_component_{nullptr}; #ifdef USE_OTA - Component *ota_wake_component_{nullptr}; // Set by ESPHomeOTAComponent to receive socket-wake notifications + Component *ota_wake_component_{nullptr}; #endif // std::vector (3 pointers each: begin, end, capacity) diff --git a/esphome/core/lwip_fast_select.c b/esphome/core/lwip_fast_select.c index 9f23401aba..f5cfe7f0fc 100644 --- a/esphome/core/lwip_fast_select.c +++ b/esphome/core/lwip_fast_select.c @@ -158,20 +158,10 @@ _Static_assert(offsetof(struct lwip_sock, rcvevent) == ESPHOME_LWIP_SOCK_RCVEVEN static netconn_callback s_original_callback = NULL; #ifdef ESPHOME_USE_OTA -// OTA listener netconn, captured via esphome_fast_select_set_ota_listener_sock() at OTA -// setup(). The wake hook only fires when the callback's `conn` argument matches this -// pointer — i.e. only for RCVPLUS events on the OTA listen socket (new accepts). Avoids -// paying the wake-hook function call on every API client data packet, mDNS query, etc. +// ESPHOME_USE_OTA (not USE_OTA): this .c file can't include defines.h — macros.h → +// Arduino.h would break the C compile. ota/__init__.py emits -DESPHOME_USE_OTA. static struct netconn *s_ota_listener_conn = NULL; - -// Extern-C trampoline defined in application.cpp — calls App.wake_ota_component_any_context() -// to mark the OTA component pending loop-enable. Out-of-line (not inlined) because with the -// listener filter above, this only fires on actual OTA connection attempts — the function-call -// cost is paid at most once per real wake, not on every monitored-socket RCVPLUS. -// ESPHOME_USE_OTA (not USE_OTA) because USE_OTA only lives in defines.h, and this .c file -// cannot include defines.h — macros.h → Arduino.h would break the C compile under Arduino -// builds. ota/__init__.py emits -DESPHOME_USE_OTA as a build flag so this file can see it. -extern void esphome_wake_ota_component_any_context(void); +extern void esphome_wake_ota_component_any_context(void); // trampoline in application.cpp void esphome_fast_select_set_ota_listener_sock(struct lwip_sock *sock) { s_ota_listener_conn = (sock != NULL) ? sock->conn : NULL; @@ -195,13 +185,8 @@ static void esphome_socket_event_callback(struct netconn *conn, enum netconn_evt // already wake the main loop through the RCVPLUS path. if (evt == NETCONN_EVT_RCVPLUS) { #ifdef ESPHOME_USE_OTA - // Filter: only mark OTA pending-enable when the event is for OTA's listen socket. - // Without this, every RCVPLUS (API client data, mDNS, etc.) would pay the inline - // wake hook's two volatile stores + memw barriers. The setter that installs - // s_ota_listener_conn is called from OTA setup(); until then the pointer is NULL - // and the filter skips the wake work entirely, which is the correct idle behavior. - // MUST happen before xTaskNotifyGive below — the flags have to be visible before - // the main task wakes, or the main loop could run a full iteration and miss them. + // Mark OTA pending-enable only for events on its listen socket. MUST happen + // before xTaskNotifyGive so the flags are visible when the main task wakes. if (conn == s_ota_listener_conn) { esphome_wake_ota_component_any_context(); } diff --git a/esphome/core/lwip_fast_select.h b/esphome/core/lwip_fast_select.h index 2572a9103d..3b5e449148 100644 --- a/esphome/core/lwip_fast_select.h +++ b/esphome/core/lwip_fast_select.h @@ -53,16 +53,10 @@ static inline bool esphome_lwip_socket_has_data(struct lwip_sock *sock) { /// The sock pointer must have been obtained from esphome_lwip_get_sock(). void esphome_lwip_hook_socket(struct lwip_sock *sock); -/// Install an OTA listener netconn as the wake-filter target for the fast-select -/// callback. After this is called, the OTA wake hook only fires for RCVPLUS events -/// whose `conn` argument matches this listener's netconn — i.e. only on actual -/// incoming connections to the OTA listen socket. Without this, every monitored -/// socket's RCVPLUS (API client data, web server, mDNS, etc.) would mark OTA -/// pending-enable and force its loop to run a wake-up tick only to re-disable itself. -/// Captured at OTA setup(); stays pointing at the listener for the device's lifetime. -/// Passing NULL disables OTA wake notifications entirely (no RCVPLUS event will match -/// a null listener), which is the correct behavior before the listener is installed -/// and after it's torn down. +/// Set the listener netconn that the fast-select callback filters OTA wakes against. +/// After this is called, the OTA wake hook only fires for RCVPLUS events whose `conn` +/// matches this listener. Passing NULL disables OTA wakes (no event matches a NULL +/// listener) — correct behavior before install and after teardown. void esphome_fast_select_set_ota_listener_sock(struct lwip_sock *sock); /// Set or clear TCP_NODELAY on a socket's tcp_pcb directly.