Disambiguates the verb-vs-noun parse of the original name. The new
form reads as 'hook this fd for the fast-select path', matching the
function's actual job.
- cached_sock_ comment in both impl headers: drop the 'iff' wording
since the pointer can also be null if esphome_lwip_get_sock() fails
on a fd that was requested to be monitored. Document all three null
cases explicitly, plus the close()-path nulling for UAF protection.
- application.h register_socket_fd / unregister_socket_fd comment
block: move inside the #ifdef USE_HOST so the generic
'register/unregister a socket' wording no longer implies these APIs
exist on fast-select builds. Add a forward reference to
fast_select_hook_fd for readers wondering where the ESP32/LibreTiny
equivalent went.
Restore the cached_sock_ = nullptr assignment inside close() on the
fast-select path. The lwip slot can be recycled for a new connection
as soon as the underlying close() returns, so any dereference of
cached_sock_ afterwards would touch an unrelated socket's pcb.
No current caller does this — setsockopt(TCP_NODELAY) and ready() are
the only consumers and neither is invoked post-close today — but
leaving the pointer dangling is a footgun for future changes. The
fd_ = -1 sentinel alone would catch the ready() path via closed
semantics, but setsockopt() reaches cached_sock_ directly and would
not be protected. Null the pointer so the protection is by
construction rather than by caller discipline.
Replace the separate closed_ bool with fd_ < 0 as the 'not open'
sentinel. close() now sets fd_ = -1 after the underlying close call,
so the destructor and double-close paths just check fd_ < 0. As a
side benefit, get_fd() on a closed socket now returns -1, making
use-after-close visible to callers instead of returning a stale
descriptor.
Drop loop_monitored_ on the USE_LWIP_FAST_SELECT path — the pointer
cached_sock_ already encodes monitoring state (non-null iff
monitored). On USE_HOST the bool is still needed because there is no
cached pointer to derive from.
Combined effect on the fast-select path:
Before: fd_(4) + cached_sock_(4) + closed_(1) + loop_monitored_(1)
+ pad(2) = 12 bytes per socket
After: fd_(4) + cached_sock_(4)
= 8 bytes per socket (aligned, no tail padding)
Saves 4 bytes per Socket instance on ESP32/LibreTiny. With typical
workloads running 5-10 sockets (API listen + clients + mDNS) that's
20-40 bytes of RAM.
Lift the lwip_sock resolve + event-callback hook sequence into
socket::fast_select_hook_fd() in socket.h so the USE_LWIP_FAST_SELECT
constructor blocks in lwip_sockets_impl.cpp and bsd_sockets_impl.cpp
stop drifting in lockstep. Both impls now collapse to a two-line call
site.
- Drop redundant cached_sock_ = nullptr assignment in close(). After
closed_ = true the socket is a corpse and no ready() or other member
access is valid, so the nulling is not load-bearing. The comment now
explains why on both impl variants.
- Reword the yield_with_select_ comment so the wake-source sentence
reads as a complete list rather than a trailing fragment.
The pre-sleep scan in Application::yield_with_select_() walks
monitored_sockets_ on every loop iteration, issuing a volatile
cross-thread read on each socket's lwip_sock::rcvevent to preserve
select() semantics when the FreeRTOS task notification counter had
been consumed but a socket still had unread data.
That scenario only existed because of a Socket::ready() contract
violation: callers could stop reading with rcvevent > 0, leaving
data behind with no pending notification. That contract is now
documented and enforced (#15590), and #15589 (the first failure
that reverted the earlier removal attempt #14475) has been fixed.
With the contract honoured, every rcvevent > 0 is paired with a
pending xTaskNotifyGive from the lwip event_callback wrapper (see
lwip_fast_select.c). ulTaskNotifyTake either returns immediately
(counter non-zero) or wakes the moment the notify lands — the scan
has nothing left to rescue.
Evidence: https://github.com/esphome/esphome/pull/15638 — an
instrumentation PR ran across 5 devices (ESP32 rev1/rev3.1/C3 on
Ethernet and WiFi, plus LibreTiny RTL8720CF) through Home Assistant
disconnect/reconnect cycles, multi-client API logger bursts, and
BLE GATT connect storms. Across ~275,000 scans and 4 observed
load-bearing candidates, every hit was in the 2–14µs range — the
instruction-level window between the lwip callback writing
rcvevent and calling xTaskNotifyGive a few instructions later.
Zero hits exceeded 100µs. No hit came anywhere near loop_interval
(16ms), which is the latency scale the scan was added to prevent.
In addition to being unused, the scan is actively harmful on the
hot path: N volatile 16-bit loads against cache-cold cross-thread
lwip_sock structures on every main-loop iteration, just to
reproduce a microsecond-scale ordering artifact the notification
path is already handling authoritatively.
This also removes the now-unused monitored_sockets_ vector and
Application::{register,unregister}_socket() on the fast-select
path. Socket implementations now call esphome_lwip_hook_socket()
directly to install the netconn event callback wrapper.
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.
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.
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).
With the listener filter from the previous commit, the wake hook only fires
on actual OTA connection attempts — no more spurious wakes from API client
data packets or other monitored-socket traffic. That removes the motivation
for inlining the hook at three call sites.
Collapse back to the simpler shape:
- Application gains Component *ota_wake_component_ (one pointer, 4 bytes,
gated on USE_OTA) and a wake_ota_component_any_context() inline method.
- lwip_fast_select.c reaches the method via an extern-C trampoline
(esphome_wake_ota_component_any_context) defined in application.cpp.
One call_n instruction per actual wake, which now happens at most once
per real OTA upload attempt.
- Raw-TCP (LWIPRawListenImpl::accept_fn_) and host select paths call
App.wake_ota_component_any_context() directly — both are .cpp files.
- wake.h reverts to its pre-PR state (no C-compatible section, no extern
pointer globals, no inline OTA hook). All wake-related state still lives
in Application.
Net effect versus the inline approach:
- RAM: -4 bytes (one pointer vs two)
- Flash: ~-60 bytes (no 3x duplication of the inlined hook body)
- CPU: function-call overhead (~10 cycles) paid only on actual OTA wakes,
which happen ~0 times/sec in steady state. Inline was premature once
filtering reduced the fire rate to "rare intentional events."
get_cached_sock() was a new public method that only OTA's fast-select wake
filter would ever call. Drop it. The existing public C API already covers
this: esphome_lwip_get_sock(fd) looks up a lwip_sock* from a file descriptor
(it's exactly what BSDSocketImpl's own constructor calls to populate
cached_sock_). OTA uses the public get_fd() + esphome_lwip_get_sock() chain
instead — no new Socket accessor, no friend declarations, no layering
concerns. The one-time lookup at setup is negligible.
The inline OTA wake hook was firing on every NETCONN_EVT_RCVPLUS across every
monitored socket (API client data packets, mDNS queries, web server, etc.).
Each false fire paid two volatile stores + memw barriers to mark OTA
pending-enable, only for OTA::loop() to run a wake-up tick and re-disable
itself because there was no actual listener activity.
Add a compare-against-listener filter in esphome_socket_event_callback so the
wake hook only fires when `conn` matches the OTA listen socket's netconn.
Non-match sockets now cost only a pointer load + one branch (~3 instructions)
instead of the full ~10-instruction hook body.
Plumbing:
- lwip_fast_select.[ch]: new s_ota_listener_conn global +
esphome_fast_select_set_ota_listener_sock() setter, used in the callback.
- BSDSocketImpl / LwIPSocketImpl: new public get_cached_sock() accessor (only
under USE_LWIP_FAST_SELECT) mirroring the existing get_fd() pattern.
- ESPHomeOTAComponent::setup(): after registering the wake component,
install the listener filter with this->server_->get_cached_sock().
Raw TCP (ESP8266/RP2040) is unaffected — that path wakes from
LWIPRawListenImpl::accept_fn_, which only fires for the specific listener
pcb it was registered on, so the filtering is implicit there.
All wake_* state lives in one place now. wake.h gains a C-compatible section
at the top (the inline esphome_wake_ota_component_any_context() + its two
extern 'volatile bool *' globals) guarded outside any C++ namespace, with
the existing C++ platform wake primitives moved behind an outer
#ifdef __cplusplus. lwip_fast_select.c includes wake.h directly for the
inline; .cpp files continue to see the C++ side as before.
Deletes the ephemeral esphome/core/ota_wake_hook.h — same code, better home.
The extern C shim esphome_wake_ota_component_any_context() was an out-of-line
call from lwip_fast_select.c into application.cpp: save registers, call,
prologue, two stores, epilogue, ret. Per-RCVPLUS, that's ~10-15 Xtensa cycles
of pure call overhead on top of the two volatile bool stores the shim actually
does.
Move the body into a new C-compatible header (esphome/core/ota_wake_hook.h)
as a static inline, backed by two extern C 'volatile bool *' globals that
point at Component::pending_enable_loop_ and
Application::has_pending_enable_loop_requests_. set_ota_wake_component()
captures the addresses once at registration time; the fast-select callback
then inlines a null-check + two volatile stores with zero call overhead.
The main loop sees the same two flags it already checks every iteration
(has_pending_enable_loop_requests_ gates enable_pending_loops_, which
iterates the inactive section looking for components with
pending_enable_loop_ set). Zero new main-loop work — the inline hook writes
exactly the state enable_loop_soon_any_context() would have written.
RAM change: -4 bytes on Application (ota_wake_component_ field removed) plus
+8 bytes in BSS for the two extern pointers. Net +4 bytes RAM.
Raw-TCP and HOST paths switched from App.wake_ota_component_any_context() to
the inline hook too.
Emitting -DUSE_OTA alongside the defines.h #define USE_OTA entry caused
'USE_OTA redefined' warnings in every TU that includes defines.h. Use a
distinct ESPHOME_USE_OTA name for the compiler -D flag — only the .c
files that cannot include defines.h (lwip_fast_select.c) reference the
ESPHOME_-prefixed name, and everyone else continues to use USE_OTA via
defines.h.
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.
The wake-hook call (esphome_wake_ota_component_any_context / App.wake_ota_component_any_context)
was placed AFTER xTaskNotifyGive()/wake_loop_any_context() in both the fast-select callback and
the raw-TCP accept callback. That opened a race: the main task could wake, run a full iteration
(draining has_pending_enable_loop_requests_), and finish before the flag-set ran — losing the
pending-enable request until the next unrelated socket event happened to re-trigger the path.
Swap the order so the pending-enable flags are set first, then the main task is woken. The
main-loop iteration triggered by the wake is now guaranteed to see the pending request.
Note: host's yield_with_select_ path already sets and consumes the flag on the main thread
with no cross-task wake in between, so it has no race and is unchanged.
The wake hook is called only from:
- esphome_socket_event_callback (lwip fast select, LwIP TCP/IP task context)
- LWIPRawListenImpl::accept_fn_ (raw TCP accept callback)
- Application::yield_with_select_ (host select fallback, main thread)
None of those contexts require IRAM-resident code. The LwIP fast-select
event callback itself is not IRAM_ATTR; the raw-TCP accept callback runs
from a low-priority user IRQ on RP2040 where IRAM_ATTR is a no-op anyway
(it's an ESP32-specific section attribute for code that must run while
flash cache is disabled). This is not a real ISR path the way
enable_loop_soon_any_context() is (which is called from GPIO ISRs and
genuinely does need IRAM).
Removing IRAM_ATTR frees scarce IRAM on ESP32.
Addresses copilot review on #15636.
1. Fix cleanup_connection_() race with queued listener events.
While an OTA session was active, a second incoming connection would
fire esphome_socket_event_callback → esphome_wake_ota_component_any_context,
which sets pending_enable_loop_ on the (still-active) OTA component.
enable_pending_loops_() only scans the inactive section, so that flag
goes invisible. When cleanup_connection_() then called disable_loop(),
the component dropped to LOOP_DONE with a stale pending flag and
nothing to re-trigger the scan — the queued client sat forever until
some unrelated socket activity woke the main loop.
Fix: don't call disable_loop() from cleanup_connection_(). loop() has
the idempotent idle check at its top; one more dispatch after cleanup
is cheap and guarantees we re-read server_->ready() and either accept
the queued client or disable cleanly.
2. Tighten the multi-port error message. Merging is fine — the constraint
is single-port. Reworded: "Only a single port is supported for 'ota'
'platform: esphome'. Got ports [...]. Consolidate onto a single port;
configs sharing a port are merged automatically."
3. Comment drift: three call sites and the fast-select extern declaration
still referred to enable_loop_soon_any_context() and implied the hook
wakes the main loop. Updated to reflect the current mechanism (sets
pending-enable flags only; callers have already woken the main loop).
Also clarified that esphome_wake_ota_component_any_context fires on
every RCVPLUS event across all monitored sockets, so false wakes are
expected and OTA::loop() disables itself again when idle.
4. Added tests/component_tests/ota/test_esphome_ota.py covering
ota_esphome_final_validate: single instance accepted, same-port
configs merge, different-port configs rejected with cv.Invalid,
non-esphome platforms unaffected.
Address copilot review on #15636.
1. Enforce single ESPHome OTA instance (BREAKING CHANGE).
The `ota_esphome_final_validate` hook has always merged multiple
`ota: - platform: esphome` configs by port so a user config and a
remote package that both define OTA would merge rather than break.
That merge behavior is preserved. But if two configs survive on
*different* ports they produce two independent listening sockets,
which is not a sane deployment: it creates ambiguity for safe_mode
coordination and for the socket wake hook added in this PR.
Raise cv.Invalid when more than one port remains after merging.
2. Drop redundant main-loop wake in Application::wake_ota_component_any_context.
Every caller (lwip fast-select callback already calls xTaskNotifyGive,
raw-tcp accept callback already calls wake_loop_any_context(), host
path is already running post-select in the main loop) has woken the
main loop by the time we reach this hook. Calling
enable_loop_soon_any_context() would re-wake it. Application is a
friend of Component, so set pending_enable_loop_ and
has_pending_enable_loop_requests_ directly instead.
ESPHomeOTAComponent::loop() previously ran every main-loop tick just to
check `client_ != nullptr || server_->ready()` — a wasted dispatch on
every device, since OTA is idle the vast majority of the time.
OTA now disables its own loop after setup() and after cleanup_connection_().
A single 4-byte Component* slot in Application (only compiled in under
USE_OTA) lets the existing socket-wake paths call
enable_loop_soon_any_context() on the registered OTA component:
- ESP32 / LibreTiny (lwip fast select): hooked in
esphome_socket_event_callback on NETCONN_EVT_RCVPLUS.
- ESP8266 / RP2040 (raw TCP): hooked in LWIPRawListenImpl::accept_fn_
right after the existing wake_loop_any_context() call.
- Host (select fallback): called after select() returns ready in
Application::yield_with_select_.
False wakes (e.g. an API-socket event firing the fast-select callback)
land in ESPHomeOTAComponent::loop(), which re-disables itself immediately
when idle. Net cost is still far below running every tick.
This is deliberately an OTA-only hook: OTA is the only component that
benefits, and a single global slot avoids adding per-socket Component*
storage, wake-callback lists, or any new API churn to the socket layer.
J. Nick Koston