From c43015a467253f965d1c8287a0cbf2ef89c0a699 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 9 Mar 2026 21:28:08 -1000 Subject: [PATCH 1/9] [ota,socket] Use SO_RCVTIMEO for OTA data transfer instead of polling Replace the non-blocking poll + delay(1) pattern in OTA data transfer with SO_RCVTIMEO blocking reads. The socket now wakes immediately when data arrives instead of sleeping 1ms between polls. Adds SO_RCVTIMEO support to the raw TCP socket implementation (ESP8266, RP2040) using the existing socket_delay()/socket_wake() infrastructure. The timeout is stored as a uint8_t in centiseconds, fitting in existing struct padding with zero RAM cost. Tested OTA improvements across platforms: - ESP32-S3: ~15% faster (6.96-7.76s -> 5.87-6.60s) - LibreTiny RTL: 24% faster (18.84s -> 14.33s) - LibreTiny BK72xx: 56% faster (55.52s -> 24.38s) - ESP8266: ~1% faster (compressed OTA, already efficient) --- .../components/esphome/ota/ota_esphome.cpp | 14 ++++++- esphome/components/socket/headers.h | 1 + .../components/socket/lwip_raw_tcp_impl.cpp | 42 ++++++++++++++++++- esphome/components/socket/lwip_raw_tcp_impl.h | 8 ++-- 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index a1cdf59d2b7..b84bfe67917 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -18,6 +18,7 @@ #include #include +#include namespace esphome { @@ -249,6 +250,16 @@ void ESPHomeOTAComponent::handle_data_() { size_t size_acknowledged = 0; #endif + // Switch to blocking mode with receive timeout for efficient data transfer. + // This replaces the non-blocking poll + delay(1) pattern: read() now sleeps + // until data arrives (waking immediately) instead of polling every 1ms. + // The 2-second timeout ensures the WDT is fed regularly (WDT is typically 5s). + struct timeval tv; + tv.tv_sec = 2; + tv.tv_usec = 0; + this->client_->setsockopt(SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)); + this->client_->setblocking(true); + // Acknowledge auth OK - 1 byte this->write_byte_(ota::OTA_RESPONSE_AUTH_OK); @@ -299,7 +310,8 @@ void ESPHomeOTAComponent::handle_data_() { ssize_t read = this->client_->read(buf, requested); if (read == -1) { if (this->would_block_(errno)) { - this->yield_and_feed_watchdog_(); + // read() already waited up to SO_RCVTIMEO for data, just feed WDT + App.feed_wdt(); continue; } ESP_LOGW(TAG, "Read err %d", errno); diff --git a/esphome/components/socket/headers.h b/esphome/components/socket/headers.h index 16e4d23d3ba..c3f7e1e0467 100644 --- a/esphome/components/socket/headers.h +++ b/esphome/components/socket/headers.h @@ -51,6 +51,7 @@ #define SO_REUSEADDR 0x0004 /* Allow local address reuse */ #define SO_KEEPALIVE 0x0008 /* keep connections alive */ #define SO_BROADCAST 0x0020 /* permit to send and to receive broadcast messages (see IP_SOF_BROADCAST option) */ +#define SO_RCVTIMEO 0x1006 /* receive timeout */ #define SOL_SOCKET 0xfff /* options for socket level */ diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index 445a57809d2..7995e83d245 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -5,6 +5,7 @@ #include #include +#include #include "esphome/core/helpers.h" #include "esphome/core/log.h" @@ -303,6 +304,18 @@ int LWIPRawCommon::getsockopt(int level, int optname, void *optval, socklen_t *o *optlen = 4; return 0; } + if (level == SOL_SOCKET && optname == SO_RCVTIMEO) { + if (*optlen < sizeof(struct timeval)) { + errno = EINVAL; + return -1; + } + uint32_t ms = this->recv_timeout_cs_ * 10; + auto *tv = reinterpret_cast(optval); + tv->tv_sec = ms / 1000; + tv->tv_usec = (ms % 1000) * 1000; + *optlen = sizeof(struct timeval); + return 0; + } if (level == IPPROTO_TCP && optname == TCP_NODELAY) { if (*optlen < 4) { errno = EINVAL; @@ -331,6 +344,17 @@ int LWIPRawCommon::setsockopt(int level, int optname, const void *optval, sockle // to prevent warnings return 0; } + if (level == SOL_SOCKET && optname == SO_RCVTIMEO) { + if (optlen < sizeof(struct timeval)) { + errno = EINVAL; + return -1; + } + const auto *tv = reinterpret_cast(optval); + uint32_t ms = tv->tv_sec * 1000 + tv->tv_usec / 1000; + uint32_t cs = (ms + 9) / 10; // round up to nearest centisecond + this->recv_timeout_cs_ = cs > 255 ? 255 : static_cast(cs); + return 0; + } if (level == IPPROTO_TCP && optname == TCP_NODELAY) { if (optlen != 4) { errno = EINVAL; @@ -459,8 +483,22 @@ ssize_t LWIPRawImpl::read(void *buf, size_t len) { return 0; } if (this->rx_buf_ == nullptr) { - errno = EWOULDBLOCK; - return -1; + if (this->recv_timeout_cs_ > 0) { + // Wait efficiently for data — socket_delay() sleeps and wakes + // immediately when recv_fn() fires (data arrives via socket_wake()) + socket_delay(this->recv_timeout_cs_ * 10); + // Recheck after waking — data or close may have arrived + if (this->rx_closed_ && this->rx_buf_ == nullptr) + return 0; + if (this->rx_buf_ == nullptr) { + errno = EWOULDBLOCK; + return -1; + } + // Data arrived, fall through to copy + } else { + errno = EWOULDBLOCK; + return -1; + } } size_t read = 0; diff --git a/esphome/components/socket/lwip_raw_tcp_impl.h b/esphome/components/socket/lwip_raw_tcp_impl.h index c171e0537f3..ca8ac1df17a 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.h +++ b/esphome/components/socket/lwip_raw_tcp_impl.h @@ -57,6 +57,7 @@ class LWIPRawCommon { // instead use it for determining whether to call lwip_output bool nodelay_ = false; sa_family_t family_ = 0; + uint8_t recv_timeout_cs_ = 0; // SO_RCVTIMEO in centiseconds (0 = no timeout, max 2.55s) }; /// Connected socket implementation for LWIP raw TCP. @@ -102,11 +103,8 @@ class LWIPRawImpl : public LWIPRawCommon { errno = ECONNRESET; return -1; } - if (blocking) { - // blocking operation not supported - errno = EINVAL; - return -1; - } + // Raw TCP doesn't use a blocking flag directly. Blocking behavior + // is provided by SO_RCVTIMEO which makes read() wait via socket_delay(). return 0; } int loop() { return 0; } From 5aa9c18dfc67ed7030fd2375a624258cadc5194f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 9 Mar 2026 21:49:06 -1000 Subject: [PATCH 2/9] [ota,socket] Add SO_SNDTIMEO and use delay(0) in readall_ - Add SO_SNDTIMEO to OTA socket to prevent blocking writes from stalling the WDT when the TCP send buffer is full - Add SO_SNDTIMEO as no-op in raw TCP (writes never block) - Use delay(0) instead of delay(1) in readall_() since SO_RCVTIMEO already handles the wait - Keep delay(1) in writeall_() since raw TCP writes are non-blocking and would spin on EWOULDBLOCK without it --- esphome/components/esphome/ota/ota_esphome.cpp | 7 ++++++- esphome/components/socket/headers.h | 1 + esphome/components/socket/lwip_raw_tcp_impl.cpp | 4 ++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index b84bfe67917..e955dbf1b1c 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -258,6 +258,9 @@ void ESPHomeOTAComponent::handle_data_() { tv.tv_sec = 2; tv.tv_usec = 0; this->client_->setsockopt(SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)); + // Also set send timeout to prevent blocking writes from stalling the WDT + // when the TCP send buffer is full (e.g., network congestion). + this->client_->setsockopt(SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)); this->client_->setblocking(true); // Acknowledge auth OK - 1 byte @@ -413,7 +416,9 @@ bool ESPHomeOTAComponent::readall_(uint8_t *buf, size_t len) { } else { at += read; } - this->yield_and_feed_watchdog_(); + // read() already waited via SO_RCVTIMEO, just yield without 1ms stall + App.feed_wdt(); + delay(0); } return true; diff --git a/esphome/components/socket/headers.h b/esphome/components/socket/headers.h index c3f7e1e0467..0eece6480f6 100644 --- a/esphome/components/socket/headers.h +++ b/esphome/components/socket/headers.h @@ -52,6 +52,7 @@ #define SO_KEEPALIVE 0x0008 /* keep connections alive */ #define SO_BROADCAST 0x0020 /* permit to send and to receive broadcast messages (see IP_SOF_BROADCAST option) */ #define SO_RCVTIMEO 0x1006 /* receive timeout */ +#define SO_SNDTIMEO 0x1005 /* send timeout */ #define SOL_SOCKET 0xfff /* options for socket level */ diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index 7995e83d245..8fb11c6c78b 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -355,6 +355,10 @@ int LWIPRawCommon::setsockopt(int level, int optname, const void *optval, sockle this->recv_timeout_cs_ = cs > 255 ? 255 : static_cast(cs); return 0; } + if (level == SOL_SOCKET && optname == SO_SNDTIMEO) { + // Raw TCP writes are non-blocking (tcp_write), so send timeout is a no-op. + return 0; + } if (level == IPPROTO_TCP && optname == TCP_NODELAY) { if (optlen != 4) { errno = EINVAL; From 798822215da8fe9452cecae1978e1bec4eeb86ff Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 9 Mar 2026 21:50:26 -1000 Subject: [PATCH 3/9] [ota] Add socket I/O strategy documentation table to handle_data_ --- .../components/esphome/ota/ota_esphome.cpp | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index e955dbf1b1c..688f822c485 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -239,6 +239,31 @@ void ESPHomeOTAComponent::handle_data_() { /// and reboots on success. /// /// Authentication has already been handled in the non-blocking states AUTH_SEND/AUTH_READ. + /// + /// Socket I/O strategy: + /// + /// Before this function, the handshake states use non-blocking I/O: + /// read()/write() return immediately with EWOULDBLOCK if no data + /// loop() retries on next iteration (~16ms), no delay needed + /// + /// This function switches to blocking mode with SO_RCVTIMEO/SO_SNDTIMEO: + /// + /// Path | Wait mechanism | WDT strategy + /// --------------|------------------------|--------------------------- + /// Main read | SO_RCVTIMEO (2s block) | feed_wdt() only, no delay + /// readall_() | SO_RCVTIMEO (2s block) | feed_wdt() + delay(0) + /// writeall_() | SO_SNDTIMEO (2s block) | feed_wdt() + delay(1) + /// + /// readall_() uses delay(0) because SO_RCVTIMEO already waited — just yield. + /// writeall_() uses delay(1) because on raw TCP (ESP8266, RP2040) writes + /// never block (tcp_write returns immediately), so delay(1) prevents spinning. + /// + /// Platform details: + /// BSD sockets (ESP32): setblocking(true) makes read/write block + /// lwip sockets (LT): setblocking(true) makes read/write block + /// Raw TCP (8266, RP2040): setblocking is no-op; SO_RCVTIMEO uses + /// socket_delay()/socket_wake() in read(); + /// write() always returns immediately ota::OTAResponseTypes error_code = ota::OTA_RESPONSE_ERROR_UNKNOWN; bool update_started = false; size_t total = 0; @@ -250,16 +275,11 @@ void ESPHomeOTAComponent::handle_data_() { size_t size_acknowledged = 0; #endif - // Switch to blocking mode with receive timeout for efficient data transfer. - // This replaces the non-blocking poll + delay(1) pattern: read() now sleeps - // until data arrives (waking immediately) instead of polling every 1ms. - // The 2-second timeout ensures the WDT is fed regularly (WDT is typically 5s). + // Set socket timeouts and blocking mode (see strategy table above) struct timeval tv; tv.tv_sec = 2; tv.tv_usec = 0; this->client_->setsockopt(SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)); - // Also set send timeout to prevent blocking writes from stalling the WDT - // when the TCP send buffer is full (e.g., network congestion). this->client_->setsockopt(SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)); this->client_->setblocking(true); From 753dd9e9f9e442083c8e2ff684be7bde4fbc16bc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 9 Mar 2026 21:52:19 -1000 Subject: [PATCH 4/9] [ota] Only delay(1) on EWOULDBLOCK in writeall_, feed WDT on success --- esphome/components/esphome/ota/ota_esphome.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 688f822c485..d8dbe2dee2d 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -459,10 +459,13 @@ bool ESPHomeOTAComponent::writeall_(const uint8_t *buf, size_t len) { ESP_LOGW(TAG, "Write err %zu bytes, errno %d", len, errno); return false; } + // EWOULDBLOCK: on raw TCP writes never block, delay(1) prevents spinning + this->yield_and_feed_watchdog_(); } else { at += written; + // write() may block up to SO_SNDTIMEO on BSD/lwip sockets, feed WDT + App.feed_wdt(); } - this->yield_and_feed_watchdog_(); } return true; } From 96f59a11acd38565879033bd875da4676e332cfa Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 11 Mar 2026 21:19:51 -1000 Subject: [PATCH 5/9] [socket] Release LWIP lock during SO_RCVTIMEO wait and handle spurious wakes - Extract read_locked_() to avoid holding LWIP_LOCK during socket_delay(), which would block recv_fn() on RP2040 (needs async_context lock) - Loop around socket_delay() for remaining time on spurious wakes from other sockets, ensuring SO_RCVTIMEO semantics are correct - Fix readv() to use read_locked_() directly instead of calling read(), avoiding recursive locking and unintended socket_delay() waits --- .../components/socket/lwip_raw_tcp_impl.cpp | 61 ++++++++++++------- esphome/components/socket/lwip_raw_tcp_impl.h | 1 + 2 files changed, 41 insertions(+), 21 deletions(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index b64e1ecdbf9..25dddbab9d4 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -516,36 +516,42 @@ err_t LWIPRawImpl::recv_fn(struct pbuf *pb, err_t err) { } ssize_t LWIPRawImpl::read(void *buf, size_t len) { + // If SO_RCVTIMEO is set and no data available, wait without holding lock. + // These reads are safe unlocked (atomic pointer/bool on ARM/Xtensa) — + // they're just hints; the authoritative check happens under LWIP_LOCK below. + // Lock must not be held during socket_delay() so recv_fn() can run on RP2040. + if (this->recv_timeout_cs_ > 0 && this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { + // Loop until data arrives, connection closes, or the full timeout elapses. + // socket_delay() may return early due to other sockets waking the global + // socket_wake() flag, so we re-enter for the remaining time. + uint32_t timeout_ms = this->recv_timeout_cs_ * 10; + uint32_t start = millis(); + while (this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { + uint32_t elapsed = millis() - start; + if (elapsed >= timeout_ms) + break; + socket_delay(timeout_ms - elapsed); + } + } + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = ECONNRESET; return -1; } - if (this->rx_closed_ && this->rx_buf_ == nullptr) { + if (this->rx_closed_ && this->rx_buf_ == nullptr) return 0; - } - if (len == 0) { + if (len == 0) return 0; - } if (this->rx_buf_ == nullptr) { - if (this->recv_timeout_cs_ > 0) { - // Wait efficiently for data — socket_delay() sleeps and wakes - // immediately when recv_fn() fires (data arrives via socket_wake()) - socket_delay(this->recv_timeout_cs_ * 10); - // Recheck after waking — data or close may have arrived - if (this->rx_closed_ && this->rx_buf_ == nullptr) - return 0; - if (this->rx_buf_ == nullptr) { - errno = EWOULDBLOCK; - return -1; - } - // Data arrived, fall through to copy - } else { - errno = EWOULDBLOCK; - return -1; - } + errno = EWOULDBLOCK; + return -1; } + return this->read_locked_(buf, len); +} +ssize_t LWIPRawImpl::read_locked_(void *buf, size_t len) { + // Caller must hold LWIP_LOCK and ensure rx_buf_ != nullptr size_t read = 0; uint8_t *buf8 = reinterpret_cast(buf); while (len && this->rx_buf_ != nullptr) { @@ -591,9 +597,22 @@ ssize_t LWIPRawImpl::read(void *buf, size_t len) { ssize_t LWIPRawImpl::readv(const struct iovec *iov, int iovcnt) { LWIP_LOCK(); // Hold for entire scatter-gather operation + if (this->pcb_ == nullptr) { + errno = ECONNRESET; + return -1; + } + if (this->rx_closed_ && this->rx_buf_ == nullptr) { + return 0; + } ssize_t ret = 0; for (int i = 0; i < iovcnt; i++) { - ssize_t err = this->read(reinterpret_cast(iov[i].iov_base), iov[i].iov_len); + if (this->rx_buf_ == nullptr) { + if (ret != 0) + break; + errno = EWOULDBLOCK; + return -1; + } + ssize_t err = this->read_locked_(reinterpret_cast(iov[i].iov_base), iov[i].iov_len); if (err == -1) { if (ret != 0) { // if we already read some don't return an error diff --git a/esphome/components/socket/lwip_raw_tcp_impl.h b/esphome/components/socket/lwip_raw_tcp_impl.h index 50009236ded..6e27049a7ba 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.h +++ b/esphome/components/socket/lwip_raw_tcp_impl.h @@ -120,6 +120,7 @@ class LWIPRawImpl : public LWIPRawCommon { static err_t s_recv_fn(void *arg, struct tcp_pcb *pcb, struct pbuf *pb, err_t err); protected: + ssize_t read_locked_(void *buf, size_t len); ssize_t internal_write_(const void *buf, size_t len); int internal_output_(); From ab422809f562620e3b21c9736f33a47f62176ba9 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 11 Mar 2026 21:25:20 -1000 Subject: [PATCH 6/9] [socket] Simplify SO_RCVTIMEO: extract wait_for_data_() for read/readv Replace read_locked_() approach with a simpler wait_for_data_() called at the top of both read() and readv(), keeping the original read/readv structure intact. --- .../components/socket/lwip_raw_tcp_impl.cpp | 64 +++++++++---------- esphome/components/socket/lwip_raw_tcp_impl.h | 2 +- 2 files changed, 30 insertions(+), 36 deletions(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index 25dddbab9d4..91be20ffb6d 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -515,23 +515,28 @@ err_t LWIPRawImpl::recv_fn(struct pbuf *pb, err_t err) { return ERR_OK; } +void LWIPRawImpl::wait_for_data_() { + // Wait for data without holding LWIP_LOCK so recv_fn() can run on RP2040 + // (needs async_context lock). Unlocked reads of rx_buf_/rx_closed_/pcb_ are + // safe (atomic pointer/bool on ARM/Xtensa) — they're just hints to avoid + // unnecessary sleeping; the authoritative check happens under LWIP_LOCK + // in the caller after this returns. + // Loop until data arrives, connection closes, or the full timeout elapses. + // socket_delay() may return early due to other sockets waking the global + // socket_wake() flag, so we re-enter for the remaining time. + uint32_t timeout_ms = this->recv_timeout_cs_ * 10; + uint32_t start = millis(); + while (this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { + uint32_t elapsed = millis() - start; + if (elapsed >= timeout_ms) + break; + socket_delay(timeout_ms - elapsed); + } +} + ssize_t LWIPRawImpl::read(void *buf, size_t len) { - // If SO_RCVTIMEO is set and no data available, wait without holding lock. - // These reads are safe unlocked (atomic pointer/bool on ARM/Xtensa) — - // they're just hints; the authoritative check happens under LWIP_LOCK below. - // Lock must not be held during socket_delay() so recv_fn() can run on RP2040. if (this->recv_timeout_cs_ > 0 && this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { - // Loop until data arrives, connection closes, or the full timeout elapses. - // socket_delay() may return early due to other sockets waking the global - // socket_wake() flag, so we re-enter for the remaining time. - uint32_t timeout_ms = this->recv_timeout_cs_ * 10; - uint32_t start = millis(); - while (this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { - uint32_t elapsed = millis() - start; - if (elapsed >= timeout_ms) - break; - socket_delay(timeout_ms - elapsed); - } + this->wait_for_data_(); } LWIP_LOCK(); @@ -539,19 +544,17 @@ ssize_t LWIPRawImpl::read(void *buf, size_t len) { errno = ECONNRESET; return -1; } - if (this->rx_closed_ && this->rx_buf_ == nullptr) + if (this->rx_closed_ && this->rx_buf_ == nullptr) { return 0; - if (len == 0) + } + if (len == 0) { return 0; + } if (this->rx_buf_ == nullptr) { errno = EWOULDBLOCK; return -1; } - return this->read_locked_(buf, len); -} -ssize_t LWIPRawImpl::read_locked_(void *buf, size_t len) { - // Caller must hold LWIP_LOCK and ensure rx_buf_ != nullptr size_t read = 0; uint8_t *buf8 = reinterpret_cast(buf); while (len && this->rx_buf_ != nullptr) { @@ -596,23 +599,14 @@ ssize_t LWIPRawImpl::read_locked_(void *buf, size_t len) { } ssize_t LWIPRawImpl::readv(const struct iovec *iov, int iovcnt) { + if (this->recv_timeout_cs_ > 0 && this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { + this->wait_for_data_(); + } + LWIP_LOCK(); // Hold for entire scatter-gather operation - if (this->pcb_ == nullptr) { - errno = ECONNRESET; - return -1; - } - if (this->rx_closed_ && this->rx_buf_ == nullptr) { - return 0; - } ssize_t ret = 0; for (int i = 0; i < iovcnt; i++) { - if (this->rx_buf_ == nullptr) { - if (ret != 0) - break; - errno = EWOULDBLOCK; - return -1; - } - ssize_t err = this->read_locked_(reinterpret_cast(iov[i].iov_base), iov[i].iov_len); + ssize_t err = this->read(reinterpret_cast(iov[i].iov_base), iov[i].iov_len); if (err == -1) { if (ret != 0) { // if we already read some don't return an error diff --git a/esphome/components/socket/lwip_raw_tcp_impl.h b/esphome/components/socket/lwip_raw_tcp_impl.h index 6e27049a7ba..ec0b2504b39 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.h +++ b/esphome/components/socket/lwip_raw_tcp_impl.h @@ -120,7 +120,7 @@ class LWIPRawImpl : public LWIPRawCommon { static err_t s_recv_fn(void *arg, struct tcp_pcb *pcb, struct pbuf *pb, err_t err); protected: - ssize_t read_locked_(void *buf, size_t len); + void wait_for_data_(); ssize_t internal_write_(const void *buf, size_t len); int internal_output_(); From fe576b1aa56bf8802256f95cadc0a3acdc999e83 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 11 Mar 2026 21:28:31 -1000 Subject: [PATCH 7/9] [socket] Document safety of unlocked reads in wait_for_data_/read/readv --- .../components/socket/lwip_raw_tcp_impl.cpp | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index 91be20ffb6d..f6c7cc79010 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -517,10 +517,16 @@ err_t LWIPRawImpl::recv_fn(struct pbuf *pb, err_t err) { void LWIPRawImpl::wait_for_data_() { // Wait for data without holding LWIP_LOCK so recv_fn() can run on RP2040 - // (needs async_context lock). Unlocked reads of rx_buf_/rx_closed_/pcb_ are - // safe (atomic pointer/bool on ARM/Xtensa) — they're just hints to avoid - // unnecessary sleeping; the authoritative check happens under LWIP_LOCK - // in the caller after this returns. + // (needs async_context lock). + // + // IMPORTANT: This method only null-checks rx_buf_/pcb_ and reads rx_closed_. + // It never dereferences pointers or modifies any state. All fields are only + // modified by recv_fn()/err_fn() (which set rx_buf_, rx_closed_, pcb_) and + // by the locked read path (which consumes rx_buf_). Since we haven't entered + // the locked section yet, only callbacks can change these fields, and pointer/ + // bool reads are atomic on ARM/Xtensa — so a stale value at worst causes an + // unnecessary sleep or early exit, both handled by the LWIP_LOCK recheck. + // // Loop until data arrives, connection closes, or the full timeout elapses. // socket_delay() may return early due to other sockets waking the global // socket_wake() flag, so we re-enter for the remaining time. @@ -535,6 +541,14 @@ void LWIPRawImpl::wait_for_data_() { } ssize_t LWIPRawImpl::read(void *buf, size_t len) { + // Unlocked pre-check: these fields are modified by recv_fn()/err_fn() which + // run from IRQ context on RP2040. Pointer and bool reads are atomic on + // ARM/Xtensa, so we never see a torn value — just possibly stale: + // - rx_buf_ stale null: unnecessary wait, but wait_for_data_() re-checks + // and returns immediately when data is found + // - rx_buf_ stale non-null: skip wait, locked section below handles it + // - rx_closed_/pcb_ stale: wait_for_data_() loop re-checks each iteration + // All state is authoritatively rechecked under LWIP_LOCK below. if (this->recv_timeout_cs_ > 0 && this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { this->wait_for_data_(); } @@ -599,6 +613,7 @@ ssize_t LWIPRawImpl::read(void *buf, size_t len) { } ssize_t LWIPRawImpl::readv(const struct iovec *iov, int iovcnt) { + // See read() for safety analysis of these unlocked reads. if (this->recv_timeout_cs_ > 0 && this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { this->wait_for_data_(); } From fe6ba153bc3844c371f18f50206a568850abefb8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 11 Mar 2026 21:32:11 -1000 Subject: [PATCH 8/9] [socket] Fix RP2040 socket_delay race that could miss a wake Remove the redundant s_socket_woke = false between the early-return check and the while loop. If an IRQ fires in that window (recv_fn sets s_socket_woke = true), clearing the flag would lose the wake and sleep until the timer fires. Now the while loop sees the flag immediately and exits. The flag is cleared after the loop instead. --- esphome/components/socket/lwip_raw_tcp_impl.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index f6c7cc79010..d6f54ad3282 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -82,7 +82,9 @@ void socket_delay(uint32_t ms) { s_socket_woke = false; return; } - s_socket_woke = false; + // Don't clear s_socket_woke here — if an IRQ fires between the check above + // and the while loop below, the while condition sees it immediately. Clearing + // here would lose that wake and sleep until the timer fires. s_delay_expired = false; // Set a one-shot timer to wake us after the timeout. // add_alarm_in_ms returns >0 on success, 0 if time already passed, <0 on error. @@ -100,6 +102,7 @@ void socket_delay(uint32_t ms) { // Cancel timer if we woke early (socket data arrived before timeout) if (!s_delay_expired) cancel_alarm(alarm); + s_socket_woke = false; // consume the wake for next call } // No IRAM_ATTR equivalent needed: on RP2040, CYW43 async_context runs LWIP From 235d75f830cebc212e8a69513f77bf5297093b14 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 11 Mar 2026 21:34:25 -1000 Subject: [PATCH 9/9] [socket] Extract waiting_for_data_() inline helper Deduplicate the unlocked pre-check condition used in read(), readv(), and wait_for_data_(). Safety documentation lives on the helper in the header. --- .../components/socket/lwip_raw_tcp_impl.cpp | 23 ++++--------------- esphome/components/socket/lwip_raw_tcp_impl.h | 6 +++++ 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index d6f54ad3282..566e96b2f90 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -522,20 +522,12 @@ void LWIPRawImpl::wait_for_data_() { // Wait for data without holding LWIP_LOCK so recv_fn() can run on RP2040 // (needs async_context lock). // - // IMPORTANT: This method only null-checks rx_buf_/pcb_ and reads rx_closed_. - // It never dereferences pointers or modifies any state. All fields are only - // modified by recv_fn()/err_fn() (which set rx_buf_, rx_closed_, pcb_) and - // by the locked read path (which consumes rx_buf_). Since we haven't entered - // the locked section yet, only callbacks can change these fields, and pointer/ - // bool reads are atomic on ARM/Xtensa — so a stale value at worst causes an - // unnecessary sleep or early exit, both handled by the LWIP_LOCK recheck. - // // Loop until data arrives, connection closes, or the full timeout elapses. // socket_delay() may return early due to other sockets waking the global // socket_wake() flag, so we re-enter for the remaining time. uint32_t timeout_ms = this->recv_timeout_cs_ * 10; uint32_t start = millis(); - while (this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { + while (this->waiting_for_data_()) { uint32_t elapsed = millis() - start; if (elapsed >= timeout_ms) break; @@ -544,15 +536,8 @@ void LWIPRawImpl::wait_for_data_() { } ssize_t LWIPRawImpl::read(void *buf, size_t len) { - // Unlocked pre-check: these fields are modified by recv_fn()/err_fn() which - // run from IRQ context on RP2040. Pointer and bool reads are atomic on - // ARM/Xtensa, so we never see a torn value — just possibly stale: - // - rx_buf_ stale null: unnecessary wait, but wait_for_data_() re-checks - // and returns immediately when data is found - // - rx_buf_ stale non-null: skip wait, locked section below handles it - // - rx_closed_/pcb_ stale: wait_for_data_() loop re-checks each iteration - // All state is authoritatively rechecked under LWIP_LOCK below. - if (this->recv_timeout_cs_ > 0 && this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { + // See waiting_for_data_() for safety of unlocked reads. + if (this->recv_timeout_cs_ > 0 && this->waiting_for_data_()) { this->wait_for_data_(); } @@ -617,7 +602,7 @@ ssize_t LWIPRawImpl::read(void *buf, size_t len) { ssize_t LWIPRawImpl::readv(const struct iovec *iov, int iovcnt) { // See read() for safety analysis of these unlocked reads. - if (this->recv_timeout_cs_ > 0 && this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr) { + if (this->recv_timeout_cs_ > 0 && this->waiting_for_data_()) { this->wait_for_data_(); } diff --git a/esphome/components/socket/lwip_raw_tcp_impl.h b/esphome/components/socket/lwip_raw_tcp_impl.h index ec0b2504b39..60078526920 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.h +++ b/esphome/components/socket/lwip_raw_tcp_impl.h @@ -120,6 +120,12 @@ class LWIPRawImpl : public LWIPRawCommon { static err_t s_recv_fn(void *arg, struct tcp_pcb *pcb, struct pbuf *pb, err_t err); protected: + // True when the socket could receive data but none has arrived yet. + // Safe to call without LWIP_LOCK — only null-checks pointers and reads a bool, + // all atomic on ARM/Xtensa. A stale value is harmless: the caller either does + // an unnecessary wait (stale true) or skips it (stale false), and the + // authoritative recheck happens under LWIP_LOCK afterward. + bool waiting_for_data_() const { return this->rx_buf_ == nullptr && !this->rx_closed_ && this->pcb_ != nullptr; } void wait_for_data_(); ssize_t internal_write_(const void *buf, size_t len); int internal_output_();