From 4630c7ad945c68bc5e3e1da9027a5817f7acb901 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 10 Apr 2026 16:28:30 -1000 Subject: [PATCH] [ota] Emit USE_OTA as build flag + define so .c files can see it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the wake failure: ota/__init__.py only called cg.add_define("USE_OTA"), which writes to the generated defines.h. That's invisible to .c translation units that can't include defines.h — notably lwip_fast_select.c, which can't include defines.h because macros.h → Arduino.h under Arduino builds would break the C compile. So the #ifdef USE_OTA gate inside lwip_fast_select.c's RCVPLUS callback was always false, and esphome_wake_ota_component_any_context() was never called. The listener callback fired (total rcvplus counter incremented from the device), but OTA's pending-enable flag was never set, so the main task woke and ran a loop iteration without touching the (disabled) OTA component. Fix: emit both cg.add_define (keeps defines.h in sync for static analyzers / IDEs) and cg.add_build_flag("-DUSE_OTA") (passes it as a compiler -D flag, visible to every .c TU). Also reverts all the diagnostic scaffolding (per-loop-tick logs, debug counters, runtime function pointer hook) that I added while chasing this. --- .../components/esphome/ota/ota_esphome.cpp | 74 +++++-------------- esphome/components/ota/__init__.py | 6 ++ esphome/core/application.cpp | 16 ++-- esphome/core/application.h | 7 +- esphome/core/lwip_fast_select.c | 19 ++--- esphome/core/lwip_fast_select.h | 7 -- 6 files changed, 37 insertions(+), 92 deletions(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 3367696ddd8..e2b12e1208a 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -15,9 +15,6 @@ #include "esphome/core/helpers.h" #include "esphome/core/log.h" #include "esphome/core/util.h" -#ifdef USE_LWIP_FAST_SELECT -#include "esphome/core/lwip_fast_select.h" -#endif #include #include @@ -37,13 +34,6 @@ void ESPHomeOTAComponent::setup() { this->server_failed_(LOG_STR("creation")); return; } - // DEBUG: immediately after socket creation, ready() on an idle monitored socket - // MUST return false. If it returns true, loop_monitored_ is false — meaning - // App.register_socket() failed (likely because esphome_lwip_get_sock returned null - // because the socket fd is outside the lwip socket table range), and our wake hook - // was never installed on this socket. In that case the whole disable_loop+wake - // approach silently degrades — ready() stays true forever and we poll every tick. - ESP_LOGD(TAG, "setup: server_->ready() immediately after socket creation = %d (expect 0)", this->server_->ready()); int enable = 1; int err = this->server_->setsockopt(SOL_SOCKET, SO_REUSEADDR, &enable, sizeof(int)); if (err != 0) { @@ -79,7 +69,6 @@ void ESPHomeOTAComponent::setup() { // Register for socket wake notifications. loop() disables itself on its first // idle tick — no need to disable_loop() here explicitly. App.set_ota_wake_component(this); - ESP_LOGD(TAG, "setup complete: registered wake component, listener fd ready"); } void ESPHomeOTAComponent::dump_config() { @@ -96,49 +85,26 @@ void ESPHomeOTAComponent::dump_config() { } void ESPHomeOTAComponent::loop() { - // Self-disabling idle loop. On the first tick after setup() (and after every - // session cleanup and after 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. + // 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. // - // 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. + // 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. // - // 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. + // 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. - // DEBUG: self-disable removed; always poll so we can see ready/wake state. - const uint32_t wake_count = App.ota_wake_count_debug(); -#ifdef USE_LWIP_FAST_SELECT - const uint32_t total_rcvplus = esphome_fast_select_rcvplus_total_debug; - const uint32_t shim_count = esphome_ota_shim_call_count_debug; -#else - const uint32_t total_rcvplus = 0; - const uint32_t shim_count = 0; -#endif - const bool ready = this->server_->ready(); - static uint32_t last_wake_count = 0; - static uint32_t last_total_rcvplus = 0; - static uint32_t last_shim_count = 0; - static bool last_ready = false; - if (wake_count != last_wake_count || total_rcvplus != last_total_rcvplus || shim_count != last_shim_count || - ready != last_ready || this->client_ != nullptr) { - ESP_LOGD(TAG, "loop tick: client=%p ready=%d wakes=%u shim=%u total_rcvplus=%u", (void *) this->client_.get(), - ready, wake_count, shim_count, total_rcvplus); - last_wake_count = wake_count; - last_total_rcvplus = total_rcvplus; - last_shim_count = shim_count; - last_ready = ready; - } - if (this->client_ == nullptr && !ready) { - return; // stay in LOOP state so we can poll every tick for diagnosis + // Note: No need to check server_ for null — setup() marks the component failed if + // server_ creation fails. + if (this->client_ == nullptr && !this->server_->ready()) { + this->disable_loop(); + return; } this->handle_handshake_(); } @@ -159,13 +125,9 @@ void ESPHomeOTAComponent::handle_handshake_() { socklen_t addr_len = sizeof(source_addr); int enable = 1; - ESP_LOGD(TAG, "handle_handshake_: attempting accept"); this->client_ = this->server_->accept_loop_monitored((struct sockaddr *) &source_addr, &addr_len); - if (this->client_ == nullptr) { - ESP_LOGD(TAG, "handle_handshake_: accept returned null (would-block)"); + if (this->client_ == nullptr) return; - } - ESP_LOGD(TAG, "handle_handshake_: accept ok, client=%p", (void *) this->client_.get()); int err = this->client_->setsockopt(IPPROTO_TCP, TCP_NODELAY, &enable, sizeof(int)); if (err != 0) { this->log_socket_error_(LOG_STR("nodelay")); diff --git a/esphome/components/ota/__init__.py b/esphome/components/ota/__init__.py index 8f31eb5cdd3..e7a362ca108 100644 --- a/esphome/components/ota/__init__.py +++ b/esphome/components/ota/__init__.py @@ -102,7 +102,13 @@ BASE_OTA_SCHEMA = cv.Schema( @coroutine_with_priority(CoroPriority.OTA_UPDATES) async def to_code(config): + # Both: add_define keeps defines.h in sync for static analyzers / IDEs that read it, + # while add_build_flag passes -DUSE_OTA as a compiler flag so USE_OTA is visible in .c + # translation units that cannot include defines.h (lwip_fast_select.c in particular — + # including defines.h would drag in macros.h → Arduino.h under Arduino builds and + # break the C compile). Needed for the fast-select OTA wake hook. cg.add_define("USE_OTA") + cg.add_build_flag("-DUSE_OTA") CORE.add_job(final_step) if CORE.is_rp2040 and CORE.using_arduino: diff --git a/esphome/core/application.cpp b/esphome/core/application.cpp index 46f8922bc68..d99b7f28d95 100644 --- a/esphome/core/application.cpp +++ b/esphome/core/application.cpp @@ -450,19 +450,13 @@ void Application::enable_pending_loops_() { } #if defined(USE_OTA) && defined(USE_LWIP_FAST_SELECT) -// DEBUG: directly-incremented C counter so we can tell whether the shim is being called at all, -// independent of whether the App.wake_ota_component_any_context() call inside it is working. -extern "C" { -volatile uint32_t esphome_ota_shim_call_count_debug = 0; -} // Called from the LwIP TCP/IP task via esphome_socket_event_callback() on NETCONN_EVT_RCVPLUS, -// BEFORE the callback calls xTaskNotifyGive() — the flag-set must happen before the wake, -// otherwise the main task could wake, run a full iteration, and miss the pending-enable. +// BEFORE that callback calls xTaskNotifyGive() — pending-enable flags must be visible before +// the main task wakes, or the main loop can run a full iteration without seeing the request. // Only marks the OTA component as pending loop-enable; does not itself wake the main task. -extern "C" void esphome_wake_ota_component_any_context() { - esphome_ota_shim_call_count_debug++; - esphome::App.wake_ota_component_any_context(); -} +// OTA's __init__.py emits USE_OTA as both a cg.add_define (for static analyzers) AND a +// cg.add_build_flag (so the .c fast-select file also sees it). +extern "C" void esphome_wake_ota_component_any_context() { App.wake_ota_component_any_context(); } #endif #ifdef USE_LWIP_FAST_SELECT diff --git a/esphome/core/application.h b/esphome/core/application.h index 185991a37af..67ef1d666f3 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -572,15 +572,11 @@ class Application { /// Not IRAM_ATTR: all callers run in task / user-IRQ context, not a real ISR, /// and the LwIP event callbacks that invoke this are not IRAM-resident either. void wake_ota_component_any_context() { - this->ota_wake_count_debug_++; if (this->ota_wake_component_ != nullptr) { this->ota_wake_component_->pending_enable_loop_ = true; this->has_pending_enable_loop_requests_ = true; } } - /// DEBUG: monotonically increasing count of wake calls, regardless of whether - /// ota_wake_component_ was set. Read from OTA::loop() to verify the hook fires. - uint32_t ota_wake_count_debug() const { return this->ota_wake_count_debug_; } #endif protected: @@ -659,8 +655,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 - volatile uint32_t ota_wake_count_debug_{0}; // DEBUG: incremented by wake_ota_component_any_context + Component *ota_wake_component_{nullptr}; // Set by ESPHomeOTAComponent to receive socket-wake notifications #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 a3d9898757d..ab20bdd29e4 100644 --- a/esphome/core/lwip_fast_select.c +++ b/esphome/core/lwip_fast_select.c @@ -157,17 +157,15 @@ _Static_assert(offsetof(struct lwip_sock, rcvevent) == ESPHOME_LWIP_SOCK_RCVEVEN // Saved original event_callback pointer — written once in first hook_socket(), read from TCP/IP task. static netconn_callback s_original_callback = NULL; -// DEBUG: counts every RCVPLUS the wrapper callback observes, for any monitored socket. -// Read by OTA's debug logging to distinguish "callback not firing at all" from -// "callback fires for other sockets but not OTA's listener". -volatile uint32_t esphome_fast_select_rcvplus_total_debug = 0; - #ifdef USE_OTA // Extern wake hook for the OTA component (implemented in application.cpp). Called from the // TCP/IP task on every NETCONN_EVT_RCVPLUS — not just OTA's listener, so this can be a false // wake from an unrelated monitored socket. OTA::loop() handles that by disabling itself again // when there is no pending work. The hook only marks the OTA component as pending loop-enable; // it does not itself wake the main task (the caller below already does that). +// NOTE: USE_OTA reaches this file only because ota/__init__.py adds it as a build flag +// (not a cg.add_define). defines.h cannot be included from this .c file (it pulls in +// macros.h → Arduino.h under Arduino builds). extern void esphome_wake_ota_component_any_context(void); #endif @@ -185,14 +183,11 @@ static void esphome_socket_event_callback(struct netconn *conn, enum netconn_evt // (rcvevent++ with a NULL pbuf or error in recvmbox), so error conditions // already wake the main loop through the RCVPLUS path. if (evt == NETCONN_EVT_RCVPLUS) { - esphome_fast_select_rcvplus_total_debug++; // DEBUG -#ifdef USE_OTA - // Mark the OTA component loop to be re-enabled if it disabled itself while idle. - // This MUST happen before xTaskNotifyGive below — otherwise the main task could - // wake, run a full iteration, and finish before we set the pending-enable flags, - // causing the wake event to be lost until the next unrelated socket activity. + // Invoke the OTA wake hook BEFORE xTaskNotifyGive — if OTA is compiled in, this marks + // its component pending-enable, and those flags must be visible before we wake the + // main task. Otherwise the main loop could run a full iteration without seeing the + // pending-enable request. When OTA is not compiled in, this function's body is empty. esphome_wake_ota_component_any_context(); -#endif TaskHandle_t task = esphome_main_task_handle; if (task != NULL) { xTaskNotifyGive(task); diff --git a/esphome/core/lwip_fast_select.h b/esphome/core/lwip_fast_select.h index f152106f0ef..20ac191673f 100644 --- a/esphome/core/lwip_fast_select.h +++ b/esphome/core/lwip_fast_select.h @@ -53,13 +53,6 @@ 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); -// DEBUG counter: total RCVPLUS events the wrapper callback observed across all monitored sockets. -extern volatile uint32_t esphome_fast_select_rcvplus_total_debug; - -// DEBUG counter: number of times esphome_wake_ota_component_any_context() C shim was entered. -// Independent of whether App.wake_ota_component_any_context() inside it actually ran. -extern volatile uint32_t esphome_ota_shim_call_count_debug; - /// Set or clear TCP_NODELAY on a socket's tcp_pcb directly. /// Must be called with the TCPIP core lock held (LwIPLock in C++). /// This bypasses lwip_setsockopt() overhead (socket lookups, switch cascade,