From c71a599f5a3a012370e47d83f7c89944ff30303e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 28 Aug 2026 10:08:58 -0500 Subject: [PATCH] [socket] Address review feedback on UDP implementation --- .../socket/lwip_raw_common_impl.cpp | 1 + esphome/components/socket/lwip_raw_tcp_impl.h | 2 - .../components/socket/lwip_raw_udp_impl.cpp | 42 +++++++++---------- esphome/components/socket/lwip_raw_udp_impl.h | 29 +++++++------ esphome/components/socket/socket.h | 1 + 5 files changed, 40 insertions(+), 35 deletions(-) diff --git a/esphome/components/socket/lwip_raw_common_impl.cpp b/esphome/components/socket/lwip_raw_common_impl.cpp index 9bb5339d65..e6fbf6bf2c 100644 --- a/esphome/components/socket/lwip_raw_common_impl.cpp +++ b/esphome/components/socket/lwip_raw_common_impl.cpp @@ -43,6 +43,7 @@ int lwip_ip_to_sockaddr(sa_family_t family, const ip_addr_t *ip, uint16_t port_h return 0; } #endif + errno = EAFNOSUPPORT; return -1; } diff --git a/esphome/components/socket/lwip_raw_tcp_impl.h b/esphome/components/socket/lwip_raw_tcp_impl.h index a2bbc49f7c..917b5b2f7a 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.h +++ b/esphome/components/socket/lwip_raw_tcp_impl.h @@ -16,8 +16,6 @@ #include "lwip/opt.h" #include "lwip/tcp.h" -#include "lwip_raw_udp_impl.h" - namespace esphome::socket { // Forward declaration diff --git a/esphome/components/socket/lwip_raw_udp_impl.cpp b/esphome/components/socket/lwip_raw_udp_impl.cpp index 40ed6d6863..e5dc8f50e7 100644 --- a/esphome/components/socket/lwip_raw_udp_impl.cpp +++ b/esphome/components/socket/lwip_raw_udp_impl.cpp @@ -22,8 +22,6 @@ namespace esphome::socket { // On RP2040, it acquires cyw43_arch_lwip_begin/end. On ESP8266, it's a no-op. #define LWIP_LOCK() esphome::LwIPLock lwip_lock_guard // NOLINT -static const char *const TAG = "socket.lwip_udp"; - // ---- LWIPRawUDPSendImpl (send-only) methods ---- LWIPRawUDPSendImpl::LWIPRawUDPSendImpl(sa_family_t family) : family_(family) { @@ -234,6 +232,14 @@ int LWIPRawUDPSendImpl::setblocking(bool blocking) { // ---- LWIPRawUDPImpl methods ---- +LWIPRawUDPImpl::LWIPRawUDPImpl(sa_family_t family) : LWIPRawUDPSendImpl(family) { + // Register recv here (not in bind) so unbound client sockets can receive replies + if (this->pcb_ != nullptr) { + LWIP_LOCK(); + udp_recv(this->pcb_, LWIPRawUDPImpl::s_recv_fn, this); + } +} + LWIPRawUDPImpl::~LWIPRawUDPImpl() { // Flush rx queue and unregister callback before base destructor removes pcb if (this->pcb_ != nullptr) @@ -260,16 +266,6 @@ int LWIPRawUDPImpl::close() { return this->close_internal_locked_(); } -int LWIPRawUDPImpl::bind(const struct sockaddr *name, socklen_t addrlen) { - LWIP_LOCK(); - int ret = this->bind_internal_locked_(name, addrlen); - if (ret != 0) - return ret; - // Register recv callback now that we're bound and ready to receive - udp_recv(this->pcb_, LWIPRawUDPImpl::s_recv_fn, this); - return 0; -} - ssize_t LWIPRawUDPImpl::read(void *buf, size_t len) { return this->recvfrom(buf, len, nullptr, nullptr); } ssize_t LWIPRawUDPImpl::recvfrom(void *buf, size_t len, struct sockaddr *src_addr, socklen_t *addrlen) { @@ -292,17 +288,20 @@ ssize_t LWIPRawUDPImpl::recvfrom(void *buf, size_t len, struct sockaddr *src_add size_t copy_len = std::min(len, pkt_len); // Fill in source address if requested. - // If ip2sockaddr_ fails (e.g., addrlen too small), fail the entire recvfrom - // rather than silently returning data without a source address. + // If ip2sockaddr_ fails (e.g., addrlen too small), fail the recvfrom but + // still consume the packet — the failure is deterministic (depends only on + // family_ and *addrlen), so keeping the packet would wedge the queue forever. + bool addr_ok = true; if (src_addr != nullptr && addrlen != nullptr && this->ip2sockaddr_(&pkt.src_addr, pkt.src_port, src_addr, addrlen) != 0) { - // Don't consume the packet or modify the caller buffer on address conversion failure - return -1; + addr_ok = false; } // Copy data from pbuf chain — done after validation so caller buffer is // not modified on error paths. - pbuf_copy_partial(pkt.pb, buf, copy_len, 0); + if (addr_ok) { + pbuf_copy_partial(pkt.pb, buf, copy_len, 0); + } // Free the pbuf and advance the read pointer pbuf_free(pkt.pb); @@ -310,7 +309,7 @@ ssize_t LWIPRawUDPImpl::recvfrom(void *buf, size_t len, struct sockaddr *src_add this->rx_read_idx_ = (this->rx_read_idx_ + 1) & UDP_RX_MASK; this->rx_count_--; - return (ssize_t) copy_len; + return addr_ok ? (ssize_t) copy_len : -1; } void LWIPRawUDPImpl::s_recv_fn(void *arg, struct udp_pcb *pcb, struct pbuf *p, const ip_addr_t *addr, u16_t port) { @@ -327,7 +326,10 @@ void LWIPRawUDPImpl::recv_fn_(struct pbuf *p, const ip_addr_t *addr, u16_t port) // Check if queue is full if (this->rx_count_ >= UDP_RX_QUEUE_SIZE) { - // Drop packet — queue full + // Drop packet — queue full. Can't log from IRQ context, so count it + // (saturating) for consumers to surface via get_rx_dropped(). + if (this->rx_dropped_ != UINT16_MAX) + this->rx_dropped_++; pbuf_free(p); return; } @@ -340,9 +342,7 @@ void LWIPRawUDPImpl::recv_fn_(struct pbuf *p, const ip_addr_t *addr, u16_t port) slot.src_port = port; this->rx_count_++; -#if defined(USE_ESP8266) || defined(USE_RP2040) esphome::wake_loop_any_context(); -#endif } // ---- UDP Factory functions ---- diff --git a/esphome/components/socket/lwip_raw_udp_impl.h b/esphome/components/socket/lwip_raw_udp_impl.h index 8afe52625e..2d28670e03 100644 --- a/esphome/components/socket/lwip_raw_udp_impl.h +++ b/esphome/components/socket/lwip_raw_udp_impl.h @@ -55,23 +55,24 @@ class LWIPRawUDPSendImpl { /// UDP socket with receive support for LWIP raw API. /// Extends LWIPRawUDPSendImpl with a fixed-size ring buffer for incoming packets. -/// The recv callback is registered on bind(). -/// -/// Note: close() and bind() intentionally hide the base class methods to add -/// recv callback registration/cleanup. This is safe because these classes are -/// never used polymorphically (no virtual dispatch) — callers always use the -/// concrete LWIPRawUDPImpl type via the UDPSocket alias. -class LWIPRawUDPImpl : public LWIPRawUDPSendImpl { +/// Inheritance is private (base dtor is non-virtual; converting to a base +/// pointer would leak queued pbufs on destruction). +class LWIPRawUDPImpl : private LWIPRawUDPSendImpl { public: - using LWIPRawUDPSendImpl::LWIPRawUDPSendImpl; + LWIPRawUDPImpl(sa_family_t family); ~LWIPRawUDPImpl(); + using LWIPRawUDPSendImpl::bind; + using LWIPRawUDPSendImpl::get_fd; + using LWIPRawUDPSendImpl::getsockopt; + using LWIPRawUDPSendImpl::is_valid; + using LWIPRawUDPSendImpl::sendto; + using LWIPRawUDPSendImpl::setblocking; + using LWIPRawUDPSendImpl::setsockopt; + /// Close the socket, flushing any queued rx packets first. int close(); - /// Bind and register the recv callback for incoming packets. - int bind(const struct sockaddr *name, socklen_t addrlen); - /// Read the next queued packet, discarding source address info. /// If buf is smaller than the packet, data is silently truncated (returns bytes copied). /// Note: unlike POSIX MSG_TRUNC, this does not return the original packet length on truncation. @@ -85,6 +86,9 @@ class LWIPRawUDPImpl : public LWIPRawUDPSendImpl { /// Intentionally unlocked — same rationale as LWIPRawImpl::ready(). bool ready() const { return this->rx_count_ > 0; } + /// Number of packets dropped because the rx queue was full (saturating). + uint16_t get_rx_dropped() const { return this->rx_dropped_; } + protected: static void s_recv_fn(void *arg, struct udp_pcb *pcb, struct pbuf *p, const ip_addr_t *addr, u16_t port); void recv_fn_(struct pbuf *p, const ip_addr_t *addr, u16_t port); @@ -98,11 +102,12 @@ class LWIPRawUDPImpl : public LWIPRawUDPSendImpl { static constexpr uint8_t UDP_RX_MASK = UDP_RX_QUEUE_SIZE - 1; static_assert((UDP_RX_QUEUE_SIZE & UDP_RX_MASK) == 0, "UDP_RX_QUEUE_SIZE must be power of 2"); struct UDPRxPacket { - struct pbuf *pb{nullptr}; ip_addr_t src_addr{}; + struct pbuf *pb{nullptr}; uint16_t src_port{0}; }; std::array rx_queue_{}; + uint16_t rx_dropped_{0}; uint8_t rx_read_idx_{0}; uint8_t rx_count_{0}; }; diff --git a/esphome/components/socket/socket.h b/esphome/components/socket/socket.h index 2638af3a1d..8a136b81be 100644 --- a/esphome/components/socket/socket.h +++ b/esphome/components/socket/socket.h @@ -20,6 +20,7 @@ #include "lwip_sockets_impl.h" #elif defined(USE_SOCKET_IMPL_LWIP_TCP) #include "lwip_raw_tcp_impl.h" +#include "lwip_raw_udp_impl.h" #endif namespace esphome::socket {