From 2cf6ed2af729bed731326b2eac6342e19010b8c5 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 18 Dec 2025 09:07:35 -0700 Subject: [PATCH] [socket] Refactor socket implementations for memory efficiency and code quality (#12550) --- .../components/socket/bsd_sockets_impl.cpp | 106 ++++++++++-------- .../components/socket/lwip_raw_tcp_impl.cpp | 6 +- .../components/socket/lwip_sockets_impl.cpp | 106 ++++++++++-------- esphome/components/socket/socket.cpp | 28 +---- esphome/components/socket/socket.h | 13 +-- 5 files changed, 122 insertions(+), 137 deletions(-) diff --git a/esphome/components/socket/bsd_sockets_impl.cpp b/esphome/components/socket/bsd_sockets_impl.cpp index c7cca62027..09cd81752a 100644 --- a/esphome/components/socket/bsd_sockets_impl.cpp +++ b/esphome/components/socket/bsd_sockets_impl.cpp @@ -12,8 +12,7 @@ #include #endif -namespace esphome { -namespace socket { +namespace esphome::socket { std::string format_sockaddr(const struct sockaddr_storage &storage) { if (storage.ss_family == AF_INET) { @@ -44,11 +43,11 @@ class BSDSocketImpl : public Socket { BSDSocketImpl(int fd, bool monitor_loop = false) : fd_(fd) { #ifdef USE_SOCKET_SELECT_SUPPORT // Register new socket with the application for select() if monitoring requested - if (monitor_loop && fd_ >= 0) { + if (monitor_loop && this->fd_ >= 0) { // Only set loop_monitored_ to true if registration succeeds - loop_monitored_ = App.register_socket_fd(fd_); + this->loop_monitored_ = App.register_socket_fd(this->fd_); } else { - loop_monitored_ = false; + this->loop_monitored_ = false; } #else // Without select support, ignore monitor_loop parameter @@ -56,70 +55,69 @@ class BSDSocketImpl : public Socket { #endif } ~BSDSocketImpl() override { - if (!closed_) { - close(); // NOLINT(clang-analyzer-optin.cplusplus.VirtualCall) + if (!this->closed_) { + this->close(); // NOLINT(clang-analyzer-optin.cplusplus.VirtualCall) } } - int connect(const struct sockaddr *addr, socklen_t addrlen) override { return ::connect(fd_, addr, addrlen); } + int connect(const struct sockaddr *addr, socklen_t addrlen) override { return ::connect(this->fd_, addr, addrlen); } std::unique_ptr accept(struct sockaddr *addr, socklen_t *addrlen) override { - return accept_impl_(addr, addrlen, false); - } - std::unique_ptr accept_loop_monitored(struct sockaddr *addr, socklen_t *addrlen) override { - return accept_impl_(addr, addrlen, true); - } - - private: - std::unique_ptr accept_impl_(struct sockaddr *addr, socklen_t *addrlen, bool loop_monitored) { - int fd = ::accept(fd_, addr, addrlen); + int fd = ::accept(this->fd_, addr, addrlen); if (fd == -1) return {}; - return make_unique(fd, loop_monitored); + return make_unique(fd, false); + } + std::unique_ptr accept_loop_monitored(struct sockaddr *addr, socklen_t *addrlen) override { + int fd = ::accept(this->fd_, addr, addrlen); + if (fd == -1) + return {}; + return make_unique(fd, true); } - public: - int bind(const struct sockaddr *addr, socklen_t addrlen) override { return ::bind(fd_, addr, addrlen); } + int bind(const struct sockaddr *addr, socklen_t addrlen) override { return ::bind(this->fd_, addr, addrlen); } int close() override { - if (!closed_) { + if (!this->closed_) { #ifdef USE_SOCKET_SELECT_SUPPORT // Unregister from select() before closing if monitored - if (loop_monitored_) { - App.unregister_socket_fd(fd_); + if (this->loop_monitored_) { + App.unregister_socket_fd(this->fd_); } #endif - int ret = ::close(fd_); - closed_ = true; + int ret = ::close(this->fd_); + this->closed_ = true; return ret; } return 0; } - int shutdown(int how) override { return ::shutdown(fd_, how); } + int shutdown(int how) override { return ::shutdown(this->fd_, how); } - int getpeername(struct sockaddr *addr, socklen_t *addrlen) override { return ::getpeername(fd_, addr, addrlen); } + int getpeername(struct sockaddr *addr, socklen_t *addrlen) override { + return ::getpeername(this->fd_, addr, addrlen); + } std::string getpeername() override { struct sockaddr_storage storage; socklen_t len = sizeof(storage); - int err = this->getpeername((struct sockaddr *) &storage, &len); - if (err != 0) + if (::getpeername(this->fd_, (struct sockaddr *) &storage, &len) != 0) return {}; return format_sockaddr(storage); } - int getsockname(struct sockaddr *addr, socklen_t *addrlen) override { return ::getsockname(fd_, addr, addrlen); } + int getsockname(struct sockaddr *addr, socklen_t *addrlen) override { + return ::getsockname(this->fd_, addr, addrlen); + } std::string getsockname() override { struct sockaddr_storage storage; socklen_t len = sizeof(storage); - int err = this->getsockname((struct sockaddr *) &storage, &len); - if (err != 0) + if (::getsockname(this->fd_, (struct sockaddr *) &storage, &len) != 0) return {}; return format_sockaddr(storage); } int getsockopt(int level, int optname, void *optval, socklen_t *optlen) override { - return ::getsockopt(fd_, level, optname, optval, optlen); + return ::getsockopt(this->fd_, level, optname, optval, optlen); } int setsockopt(int level, int optname, const void *optval, socklen_t optlen) override { - return ::setsockopt(fd_, level, optname, optval, optlen); + return ::setsockopt(this->fd_, level, optname, optval, optlen); } - int listen(int backlog) override { return ::listen(fd_, backlog); } - ssize_t read(void *buf, size_t len) override { return ::read(fd_, buf, len); } + int listen(int backlog) override { return ::listen(this->fd_, backlog); } + ssize_t read(void *buf, size_t len) override { return ::read(this->fd_, buf, len); } ssize_t recvfrom(void *buf, size_t len, sockaddr *addr, socklen_t *addr_len) override { #if defined(USE_ESP32) || defined(USE_HOST) return ::recvfrom(this->fd_, buf, len, 0, addr, addr_len); @@ -129,41 +127,52 @@ class BSDSocketImpl : public Socket { } ssize_t readv(const struct iovec *iov, int iovcnt) override { #if defined(USE_ESP32) - return ::lwip_readv(fd_, iov, iovcnt); + return ::lwip_readv(this->fd_, iov, iovcnt); #else - return ::readv(fd_, iov, iovcnt); + return ::readv(this->fd_, iov, iovcnt); #endif } - ssize_t write(const void *buf, size_t len) override { return ::write(fd_, buf, len); } - ssize_t send(void *buf, size_t len, int flags) { return ::send(fd_, buf, len, flags); } + ssize_t write(const void *buf, size_t len) override { return ::write(this->fd_, buf, len); } + ssize_t send(void *buf, size_t len, int flags) { return ::send(this->fd_, buf, len, flags); } ssize_t writev(const struct iovec *iov, int iovcnt) override { #if defined(USE_ESP32) - return ::lwip_writev(fd_, iov, iovcnt); + return ::lwip_writev(this->fd_, iov, iovcnt); #else - return ::writev(fd_, iov, iovcnt); + return ::writev(this->fd_, iov, iovcnt); #endif } ssize_t sendto(const void *buf, size_t len, int flags, const struct sockaddr *to, socklen_t tolen) override { - return ::sendto(fd_, buf, len, flags, to, tolen); // NOLINT(readability-suspicious-call-argument) + return ::sendto(this->fd_, buf, len, flags, to, tolen); // NOLINT(readability-suspicious-call-argument) } int setblocking(bool blocking) override { - int fl = ::fcntl(fd_, F_GETFL, 0); + int fl = ::fcntl(this->fd_, F_GETFL, 0); if (blocking) { fl &= ~O_NONBLOCK; } else { fl |= O_NONBLOCK; } - ::fcntl(fd_, F_SETFL, fl); + ::fcntl(this->fd_, F_SETFL, fl); return 0; } - int get_fd() const override { return fd_; } + int get_fd() const override { return this->fd_; } + +#ifdef USE_SOCKET_SELECT_SUPPORT + bool ready() const override { + if (!this->loop_monitored_) + return true; + return App.is_socket_ready(this->fd_); + } +#endif protected: int fd_; - bool closed_ = false; + bool closed_{false}; +#ifdef USE_SOCKET_SELECT_SUPPORT + bool loop_monitored_{false}; +#endif }; // Helper to create a socket with optional monitoring @@ -182,7 +191,6 @@ std::unique_ptr socket_loop_monitored(int domain, int type, int protocol return create_socket(domain, type, protocol, true); } -} // namespace socket -} // namespace esphome +} // namespace esphome::socket #endif // USE_SOCKET_IMPL_BSD_SOCKETS diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index 328df24bdd..cb5d17d5af 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -18,8 +18,7 @@ #include // For esp_schedule() #endif -namespace esphome { -namespace socket { +namespace esphome::socket { #ifdef USE_ESP8266 // Flag to signal socket activity - checked by socket_delay() to exit early @@ -711,7 +710,6 @@ std::unique_ptr socket_loop_monitored(int domain, int type, int protocol return socket(domain, type, protocol); } -} // namespace socket -} // namespace esphome +} // namespace esphome::socket #endif // USE_SOCKET_IMPL_LWIP_TCP diff --git a/esphome/components/socket/lwip_sockets_impl.cpp b/esphome/components/socket/lwip_sockets_impl.cpp index d94c1fb2ff..23fb1a7f6f 100644 --- a/esphome/components/socket/lwip_sockets_impl.cpp +++ b/esphome/components/socket/lwip_sockets_impl.cpp @@ -7,8 +7,7 @@ #include #include "esphome/core/application.h" -namespace esphome { -namespace socket { +namespace esphome::socket { std::string format_sockaddr(const struct sockaddr_storage &storage) { if (storage.ss_family == AF_INET) { @@ -37,11 +36,11 @@ class LwIPSocketImpl : public Socket { LwIPSocketImpl(int fd, bool monitor_loop = false) : fd_(fd) { #ifdef USE_SOCKET_SELECT_SUPPORT // Register new socket with the application for select() if monitoring requested - if (monitor_loop && fd_ >= 0) { + if (monitor_loop && this->fd_ >= 0) { // Only set loop_monitored_ to true if registration succeeds - loop_monitored_ = App.register_socket_fd(fd_); + this->loop_monitored_ = App.register_socket_fd(this->fd_); } else { - loop_monitored_ = false; + this->loop_monitored_ = false; } #else // Without select support, ignore monitor_loop parameter @@ -49,96 +48,108 @@ class LwIPSocketImpl : public Socket { #endif } ~LwIPSocketImpl() override { - if (!closed_) { - close(); // NOLINT(clang-analyzer-optin.cplusplus.VirtualCall) + if (!this->closed_) { + this->close(); // NOLINT(clang-analyzer-optin.cplusplus.VirtualCall) } } - int connect(const struct sockaddr *addr, socklen_t addrlen) override { return lwip_connect(fd_, addr, addrlen); } + int connect(const struct sockaddr *addr, socklen_t addrlen) override { + return lwip_connect(this->fd_, addr, addrlen); + } std::unique_ptr accept(struct sockaddr *addr, socklen_t *addrlen) override { - return accept_impl_(addr, addrlen, false); - } - std::unique_ptr accept_loop_monitored(struct sockaddr *addr, socklen_t *addrlen) override { - return accept_impl_(addr, addrlen, true); - } - - private: - std::unique_ptr accept_impl_(struct sockaddr *addr, socklen_t *addrlen, bool loop_monitored) { - int fd = lwip_accept(fd_, addr, addrlen); + int fd = lwip_accept(this->fd_, addr, addrlen); if (fd == -1) return {}; - return make_unique(fd, loop_monitored); + return make_unique(fd, false); + } + std::unique_ptr accept_loop_monitored(struct sockaddr *addr, socklen_t *addrlen) override { + int fd = lwip_accept(this->fd_, addr, addrlen); + if (fd == -1) + return {}; + return make_unique(fd, true); } - public: - int bind(const struct sockaddr *addr, socklen_t addrlen) override { return lwip_bind(fd_, addr, addrlen); } + int bind(const struct sockaddr *addr, socklen_t addrlen) override { return lwip_bind(this->fd_, addr, addrlen); } int close() override { - if (!closed_) { + if (!this->closed_) { #ifdef USE_SOCKET_SELECT_SUPPORT // Unregister from select() before closing if monitored - if (loop_monitored_) { - App.unregister_socket_fd(fd_); + if (this->loop_monitored_) { + App.unregister_socket_fd(this->fd_); } #endif - int ret = lwip_close(fd_); - closed_ = true; + int ret = lwip_close(this->fd_); + this->closed_ = true; return ret; } return 0; } - int shutdown(int how) override { return lwip_shutdown(fd_, how); } + int shutdown(int how) override { return lwip_shutdown(this->fd_, how); } - int getpeername(struct sockaddr *addr, socklen_t *addrlen) override { return lwip_getpeername(fd_, addr, addrlen); } + int getpeername(struct sockaddr *addr, socklen_t *addrlen) override { + return lwip_getpeername(this->fd_, addr, addrlen); + } std::string getpeername() override { struct sockaddr_storage storage; socklen_t len = sizeof(storage); - int err = this->getpeername((struct sockaddr *) &storage, &len); - if (err != 0) + if (lwip_getpeername(this->fd_, (struct sockaddr *) &storage, &len) != 0) return {}; return format_sockaddr(storage); } - int getsockname(struct sockaddr *addr, socklen_t *addrlen) override { return lwip_getsockname(fd_, addr, addrlen); } + int getsockname(struct sockaddr *addr, socklen_t *addrlen) override { + return lwip_getsockname(this->fd_, addr, addrlen); + } std::string getsockname() override { struct sockaddr_storage storage; socklen_t len = sizeof(storage); - int err = this->getsockname((struct sockaddr *) &storage, &len); - if (err != 0) + if (lwip_getsockname(this->fd_, (struct sockaddr *) &storage, &len) != 0) return {}; return format_sockaddr(storage); } int getsockopt(int level, int optname, void *optval, socklen_t *optlen) override { - return lwip_getsockopt(fd_, level, optname, optval, optlen); + return lwip_getsockopt(this->fd_, level, optname, optval, optlen); } int setsockopt(int level, int optname, const void *optval, socklen_t optlen) override { - return lwip_setsockopt(fd_, level, optname, optval, optlen); + return lwip_setsockopt(this->fd_, level, optname, optval, optlen); } - int listen(int backlog) override { return lwip_listen(fd_, backlog); } - ssize_t read(void *buf, size_t len) override { return lwip_read(fd_, buf, len); } + int listen(int backlog) override { return lwip_listen(this->fd_, backlog); } + ssize_t read(void *buf, size_t len) override { return lwip_read(this->fd_, buf, len); } ssize_t recvfrom(void *buf, size_t len, sockaddr *addr, socklen_t *addr_len) override { - return lwip_recvfrom(fd_, buf, len, 0, addr, addr_len); + return lwip_recvfrom(this->fd_, buf, len, 0, addr, addr_len); } - ssize_t readv(const struct iovec *iov, int iovcnt) override { return lwip_readv(fd_, iov, iovcnt); } - ssize_t write(const void *buf, size_t len) override { return lwip_write(fd_, buf, len); } - ssize_t send(void *buf, size_t len, int flags) { return lwip_send(fd_, buf, len, flags); } - ssize_t writev(const struct iovec *iov, int iovcnt) override { return lwip_writev(fd_, iov, iovcnt); } + ssize_t readv(const struct iovec *iov, int iovcnt) override { return lwip_readv(this->fd_, iov, iovcnt); } + ssize_t write(const void *buf, size_t len) override { return lwip_write(this->fd_, buf, len); } + ssize_t send(void *buf, size_t len, int flags) { return lwip_send(this->fd_, buf, len, flags); } + ssize_t writev(const struct iovec *iov, int iovcnt) override { return lwip_writev(this->fd_, iov, iovcnt); } ssize_t sendto(const void *buf, size_t len, int flags, const struct sockaddr *to, socklen_t tolen) override { - return lwip_sendto(fd_, buf, len, flags, to, tolen); + return lwip_sendto(this->fd_, buf, len, flags, to, tolen); } int setblocking(bool blocking) override { - int fl = lwip_fcntl(fd_, F_GETFL, 0); + int fl = lwip_fcntl(this->fd_, F_GETFL, 0); if (blocking) { fl &= ~O_NONBLOCK; } else { fl |= O_NONBLOCK; } - lwip_fcntl(fd_, F_SETFL, fl); + lwip_fcntl(this->fd_, F_SETFL, fl); return 0; } - int get_fd() const override { return fd_; } + int get_fd() const override { return this->fd_; } + +#ifdef USE_SOCKET_SELECT_SUPPORT + bool ready() const override { + if (!this->loop_monitored_) + return true; + return App.is_socket_ready(this->fd_); + } +#endif protected: int fd_; - bool closed_ = false; + bool closed_{false}; +#ifdef USE_SOCKET_SELECT_SUPPORT + bool loop_monitored_{false}; +#endif }; // Helper to create a socket with optional monitoring @@ -157,7 +168,6 @@ std::unique_ptr socket_loop_monitored(int domain, int type, int protocol return create_socket(domain, type, protocol, true); } -} // namespace socket -} // namespace esphome +} // namespace esphome::socket #endif // USE_SOCKET_IMPL_LWIP_SOCKETS diff --git a/esphome/components/socket/socket.cpp b/esphome/components/socket/socket.cpp index cc9232d21a..ffe0233abc 100644 --- a/esphome/components/socket/socket.cpp +++ b/esphome/components/socket/socket.cpp @@ -6,33 +6,10 @@ #include "esphome/core/log.h" #include "esphome/core/application.h" -namespace esphome { -namespace socket { +namespace esphome::socket { Socket::~Socket() {} -bool Socket::ready() const { -#ifdef USE_SOCKET_SELECT_SUPPORT - if (!loop_monitored_) { - // Non-monitored sockets always return true (assume data may be available) - return true; - } - - // For loop-monitored sockets, check with the Application's select() results - int fd = this->get_fd(); - if (fd < 0) { - // No valid file descriptor, assume ready (fallback behavior) - return true; - } - - return App.is_socket_ready(fd); -#else - // Without select() support, we can't monitor sockets in the loop - // Always return true (assume data may be available) - return true; -#endif -} - std::unique_ptr socket_ip(int type, int protocol) { #if USE_NETWORK_IPV6 return socket(AF_INET6, type, protocol); @@ -113,6 +90,5 @@ socklen_t set_sockaddr_any(struct sockaddr *addr, socklen_t addrlen, uint16_t po return sizeof(sockaddr_in); #endif /* USE_NETWORK_IPV6 */ } -} // namespace socket -} // namespace esphome +} // namespace esphome::socket #endif diff --git a/esphome/components/socket/socket.h b/esphome/components/socket/socket.h index 8936b2cd10..75eb07de4a 100644 --- a/esphome/components/socket/socket.h +++ b/esphome/components/socket/socket.h @@ -6,8 +6,7 @@ #include "headers.h" #if defined(USE_SOCKET_IMPL_LWIP_TCP) || defined(USE_SOCKET_IMPL_LWIP_SOCKETS) || defined(USE_SOCKET_IMPL_BSD_SOCKETS) -namespace esphome { -namespace socket { +namespace esphome::socket { class Socket { public: @@ -54,12 +53,7 @@ class Socket { /// Check if socket has data ready to read /// For loop-monitored sockets, checks with the Application's select() results /// For non-monitored sockets, always returns true (assumes data may be available) - bool ready() const; - - protected: -#ifdef USE_SOCKET_SELECT_SUPPORT - bool loop_monitored_{false}; ///< Whether this socket is monitored by the event loop -#endif + virtual bool ready() const { return true; } }; /// Create a socket of the given domain, type and protocol. @@ -91,6 +85,5 @@ void socket_delay(uint32_t ms); void socket_wake(); #endif -} // namespace socket -} // namespace esphome +} // namespace esphome::socket #endif