[socket] Address review feedback on UDP implementation

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;
 }
 

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

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 ----

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<UDPRxPacket, UDP_RX_QUEUE_SIZE> rx_queue_{};
+  uint16_t rx_dropped_{0};
   uint8_t rx_read_idx_{0};
   uint8_t rx_count_{0};
 };

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 {