UDP does not bind on send only port. Fixes: #3127 (#3143)

* UDP does not bind on send only port. Fixes: #3127

* Refining autoconnection behavior. Fixes: #3144

* Simplifying read loop

* I should probably at least build before pushing...

* sp

* Adding UTs to catch UDP send only

* Adding auto-connect disabled UTs

* Disabling UDP configure, reconnect -> private, and SDD fixes

* Renaming reconnect to reopen

* Fixing deployment examples

* Add links to SDD

* sp

.github/actions/spelling/expect.txt

@@ -259,6 +259,7 @@ dps
 DPWRITER
 DRAINBUFFERS
 drv
+drvtcpserversocket
 dsdl
 dspal
 Dstate

Drv/Ip/IpSocket.hpp

@@ -43,7 +43,9 @@ enum SocketIpStatus {
     SOCK_NOT_STARTED = -14,                  //!< Socket has not been started
     SOCK_FAILED_TO_READ_BACK_PORT = -15,     //!< Failed to read back port from connection
     SOCK_NO_DATA_AVAILABLE = -16,            //!< No data available or read operation would block
-    SOCK_ANOTHER_THREAD_OPENING = -17        //!< Another thread is opening
+    SOCK_ANOTHER_THREAD_OPENING = -17,       //!< Another thread is opening
+    SOCK_AUTO_CONNECT_DISABLED = -18,        //!< Automatic connections are disabled
+    SOCK_INVALID_CALL = -19                  //!< Operation is invalid
 };
 
 /**
@@ -74,8 +76,8 @@ class IpSocket {
      * \param send_timeout_microseconds: send timeout microseconds portion. Must be less than 1000000
      * \return status of configure
      */
-    SocketIpStatus configure(const char* hostname, const U16 port, const U32 send_timeout_seconds,
-                             const U32 send_timeout_microseconds);
+    virtual SocketIpStatus configure(const char* hostname, const U16 port, const U32 send_timeout_seconds,
+                                     const U32 send_timeout_microseconds);
 
     /**
      * \brief open the IP socket for communications

Drv/Ip/SocketComponentHelper.cpp

@@ -22,13 +22,11 @@ SocketComponentHelper::SocketComponentHelper() {}
 SocketComponentHelper::~SocketComponentHelper() {}
 
 void SocketComponentHelper::start(const Fw::StringBase &name,
-                                     const bool reconnect,
-                                     const Os::Task::ParamType priority,
-                                     const Os::Task::ParamType stack,
-                                     const Os::Task::ParamType cpuAffinity) {
+                                  const Os::Task::ParamType priority,
+                                  const Os::Task::ParamType stack,
+                                  const Os::Task::ParamType cpuAffinity) {
     FW_ASSERT(m_task.getState() == Os::Task::State::NOT_STARTED);  // It is a coding error to start this task multiple times
     this->m_stop = false;
-    m_reconnect = reconnect;
     // Note: the first step is for the IP socket to open the port
     Os::Task::Arguments arguments(name, SocketComponentHelper::readTask, this, priority, stack, cpuAffinity);
     Os::Task::Status stat = m_task.start(arguments);
@@ -77,13 +75,28 @@ bool SocketComponentHelper::isOpened() {
     return is_open;
 }
 
-SocketIpStatus SocketComponentHelper::reconnect() {
+void SocketComponentHelper::setAutomaticOpen(bool auto_open) {
+    Os::ScopeLock scopedLock(this->m_lock);
+    this->m_reopen = auto_open;
+}
+
+SocketIpStatus SocketComponentHelper::reopen() {
     SocketIpStatus status = SOCK_SUCCESS;
-    // Open a network connection if it has not already been open
     if (not this->isOpened()) {
-        status = this->open();
-        if (status == SocketIpStatus::SOCK_ANOTHER_THREAD_OPENING) {
-            status = SocketIpStatus::SOCK_SUCCESS;
+        // Check for auto-open before attempting to reopen
+        bool reopen = false;
+        {
+            Os::ScopeLock scopedLock(this->m_lock);
+            reopen = this->m_reopen;
+        }
+        // Open a network connection if it has not already been open
+        if (not reopen) {
+            status = SOCK_AUTO_CONNECT_DISABLED;
+        } else {
+            status = this->open();
+            if (status == SocketIpStatus::SOCK_ANOTHER_THREAD_OPENING) {
+                status = SocketIpStatus::SOCK_SUCCESS;
+            }
         }
     }
     return status;
@@ -96,12 +109,12 @@ SocketIpStatus SocketComponentHelper::send(const U8* const data, const U32 size)
     this->m_lock.unlock();
     // Prevent transmission before connection, or after a disconnect
     if (descriptor.fd == -1) {
-        status = this->reconnect();
-        // if reconnect wasn't successful, pass the that up to the caller
+        status = this->reopen();
+        // if reopen wasn't successful, pass the that up to the caller
         if(status != SOCK_SUCCESS) {
             return status;
         }
-        // Refresh local copy after reconnect
+        // Refresh local copy after reopen
         this->m_lock.lock();
         descriptor = this->m_descriptor;
         this->m_lock.unlock();
@@ -165,8 +178,13 @@ void SocketComponentHelper::readLoop() {
     do {
         // Prevent transmission before connection, or after a disconnect
         if ((not this->isOpened()) and this->running()) {
-            status = this->reconnect();
-            if (status != SOCK_SUCCESS) {
+            status = this->reopen();
+            // When reopen is disabled, just break as this is a exit condition for the loop
+            if (status == SOCK_AUTO_CONNECT_DISABLED) {
+                break;
+            }
+            // If the reconnection failed in any other way, warn, wait, and retry
+            else if (status != SOCK_SUCCESS) {
                 Fw::Logger::log("[WARNING] Failed to open port with status %d and errno %d\n", status, errno);
                 (void)Os::Task::delay(SOCKET_RETRY_INTERVAL);
                 continue;
@@ -193,9 +211,8 @@ void SocketComponentHelper::readLoop() {
             this->sendBuffer(buffer, status);
         }
     }
-    // As long as not told to stop, and we are successful interrupted or ordered to retry, keep receiving
-    while (this->running() &&
-           (status == SOCK_SUCCESS || (status == SOCK_NO_DATA_AVAILABLE) || status == SOCK_INTERRUPTED_TRY_AGAIN || this->m_reconnect));
+    // This will loop until stopped. If auto-open is disabled, this will break when reopen returns disabled status
+    while (this->running());
     // Close the socket
     this->close(); // Close the port entirely
 }

Drv/Ip/SocketComponentHelper.hpp

@@ -46,27 +46,26 @@ class SocketComponentHelper {
     /**
      * \brief start the socket read task to start producing data
      *
-     * Starts up the socket reading task and opens socket. This should be called before send calls are expected to
-     * work. Will connect to the previously configured port and host. priority, stack, and cpuAffinity are provided
-     * to the Os::Task::start call.
+     * Starts up the socket reading task and when reopen was configured, will open up the socket. 
+     * 
+     * \note: users must now use `setAutomaticOpen` to configure the socket to automatically open connections. The
+     *        default behavior is to automatically open connections.
      *
      * \param name: name of the task
-     * \param reconnect: automatically reconnect socket when closed. Default: true.
      * \param priority: priority of the started task. See: Os::Task::start. Default: TASK_DEFAULT, not prioritized
      * \param stack: stack size provided to the task. See: Os::Task::start. Default: TASK_DEFAULT, posix threads default
      * \param cpuAffinity: cpu affinity provided to task. See: Os::Task::start. Default: TASK_DEFAULT, don't care
      */
     void start(const Fw::StringBase &name,
-                         const bool reconnect = true,
-                         const Os::Task::ParamType priority = Os::Task::TASK_DEFAULT,
-                         const Os::Task::ParamType stack = Os::Task::TASK_DEFAULT,
-                         const Os::Task::ParamType cpuAffinity = Os::Task::TASK_DEFAULT);
+               const Os::Task::ParamType priority = Os::Task::TASK_DEFAULT,
+               const Os::Task::ParamType stack = Os::Task::TASK_DEFAULT,
+               const Os::Task::ParamType cpuAffinity = Os::Task::TASK_DEFAULT);
 
     /**
      * \brief open the socket for communications
      *
-     * Typically the socket read task will open the connection and keep it open. However, in cases where the read task
-     * will not be started, this function may be used to open the socket.
+     * Typically the socket read task will open the connection and keep it open. However, in cases where the socket is
+     * not automatically opening, this call will open the socket. This will block until the socket is opened.
      *
      * Note: this just delegates to the handler
      *
@@ -85,13 +84,15 @@ class SocketComponentHelper {
      */
     bool isOpened();
 
-     /**
-     * \brief Re-open port if it has been disconnected
-     *
+    /**
+     * \brief set socket to automatically open connections when true, or not when false
      *
-     * \return status of reconnect, SOCK_SUCCESS for success, something else on error
+     * When passed `true`, this instructs the socket to automatically open a socket and reopen socket failed connections. When passed `false`
+     * the user must explicitly call the `open` method to open the socket initially and when a socket fails.  
+     * 
+     * \param auto_open: true to automatically open and reopen sockets, false otherwise
      */
-    SocketIpStatus reconnect();
+    void setAutomaticOpen(bool auto_open);
 
     /**
      * \brief send data to the IP socket from the given buffer
@@ -209,10 +210,23 @@ class SocketComponentHelper {
      */
     static void readTask(void* pointer);
 
+  PRIVATE:
+    /**
+     * \brief Re-open port if it has been disconnected
+     *
+     * This function is a helper to handle the situations where this code needs to safely reopen a socket. User code should
+     * connect using the `open` call. This is for opening/reopening in situations where automatic open is performed
+     * within this socket helper.
+     *
+     * \return status of reconnect, SOCK_SUCCESS for success, something else on error
+     */
+    SocketIpStatus reopen();
+
+  PROTECTED:
     Os::Task m_task;
     Os::Mutex m_lock;
     SocketDescriptor m_descriptor;
-    bool m_reconnect = false; //!< Force reconnection
+    bool m_reopen = true; //!< Force reopen on disconnect
     bool m_stop = true; //!< Stops the task when set to true
     OpenState m_open = OpenState::NOT_OPEN; //!< Have we successfully opened
 };

Drv/Ip/UdpSocket.cpp

@@ -59,6 +59,12 @@ UdpSocket::~UdpSocket() {
     delete m_state;
 }
 
+SocketIpStatus UdpSocket::configure(const char* const hostname, const U16 port, const U32 timeout_seconds, const U32 timeout_microseconds) {
+    FW_ASSERT(0); // Must use configureSend and/or configureRecv
+    return SocketIpStatus::SOCK_INVALID_CALL;
+}
+
+
 SocketIpStatus UdpSocket::configureSend(const char* const hostname, const U16 port, const U32 timeout_seconds, const U32 timeout_microseconds) {
     //Timeout is for the send, so configure send will work with the base class
     FW_ASSERT(port != 0, port); // Send cannot be on port 0
@@ -151,23 +157,35 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) {
         memcpy(&this->m_state->m_addr_send, &address, sizeof(this->m_state->m_addr_send));
     }
 
-    // When we are setting up for receiving as well, then we must bind to a port
-    if ((status = this->bind(socketFd)) != SOCK_SUCCESS) {
-        ::close(socketFd);
-        return status; // Not closing FD as it is still a valid send FD
-    }
+    // Receive port set up only done when configure receive was called
     U16 recv_port = this->m_recv_port;
+    if (recv_port != 0) {
+        status = this->bind(socketFd);
+        // When we are setting up for receiving as well, then we must bind to a port
+        if (status != SOCK_SUCCESS) {
+            (void) ::close(socketFd); // Closing FD as a retry will reopen send side
+            return status;
+        }
+    }
+
     // Log message for UDP
-    if (port == 0) {
-        Fw::Logger::log("Setup to receive udp at %s:%hu\n", m_recv_hostname,
+    if ((port == 0) && (recv_port > 0)) {
+        Fw::Logger::log("Setup to only receive udp at %s:%hu\n", m_recv_hostname,
                            recv_port);
-    } else {
+    } else if ((port > 0) && (recv_port == 0))  {
+        Fw::Logger::log("Setup to only send udp at %s:%hu\n", m_hostname,
+                           port);
+    } else if ((port > 0) && (recv_port > 0))  {
         Fw::Logger::log("Setup to receive udp at %s:%hu and send to %s:%hu\n",
                            m_recv_hostname,
                            recv_port,
                            m_hostname,
                            port);
     }
+    // Neither configuration method was called
+    else {
+        FW_ASSERT(port > 0 || recv_port > 0, port, recv_port);
+    }
     FW_ASSERT(status == SOCK_SUCCESS, status);
     socketDescriptor.fd = socketFd;
     return status;

Drv/Ip/UdpSocket.hpp

@@ -37,6 +37,14 @@ class UdpSocket : public IpSocket {
      */
     virtual ~UdpSocket();
 
+    /**
+     * \brief configure is disabled
+     * 
+     * \warning configure is disabled for UdpSocket. Use configureSend and configureRecv instead.
+     */
+    SocketIpStatus configure(const char* hostname, const U16 port, const U32 send_timeout_seconds,
+                             const U32 send_timeout_microseconds) override;
+
     /**
      * \brief configure the udp socket for outgoing transmissions
      *

Drv/Ip/docs/sdd.md

@@ -10,10 +10,10 @@ interacting with sockets generically. Drv::IpSocket delegates protocol specific
 implemented by the children concrete socket classes. i.e. Drv::IpSocket::open delegates to the functions
 Drv::TcpClientSocket::openProtocol to open up specifically a tcp client socket.
 
-Drv::SocketComponentHelper is a virtual base class that comes with the functionality for setting up a generic reading thread
-complete with the ability to reconnect to a closed/broken connection. It exists at the component level and serves as a passthrough for requests from the F` component to the IPv4 sockets. This virtual base class is intended to be
-inherited by an F´ component wrapper that need to support a receive thread such that this functionality need not be
-redundantly implemented.
+Drv::SocketComponentHelper is a virtual base class that comes with the functionality for setting up a generic interaction
+between this library and a wrapping component. It exists at the component level and serves as a passthrough for requests
+from the F´ component to the IPv4 sockets. This virtual base class is intended to be inherited by an F´ component wrapper
+that need to support a receive thread such that this functionality need not be redundantly implemented.
 
 Each of these classes is explained in more detail below.
 
@@ -23,6 +23,9 @@ Each of these classes is explained in more detail below.
 - [Drv::UdpSocket](#drvudpsocket-class)
 - [Drv::SocketComponentHelper](#drvsocketreadtask-virtual-baseclass)
 
+> ![WARNING]
+> The core library is not thread-safe. Users of this library (i.e. F´ components) must provide synchronization calls.
+
 ## Drv::IpSocket Baseclass
 
 The Drv::IpSocket class represents the external interface to IPv4 socket components. This class provides a top-level
@@ -41,13 +44,12 @@ however; those calls may detect an error and close the socket in response.
 
 `Drv::IpSocket::send` will attempt to send data across the socket. It will retry to transmit data a configured
 number of times on correctable errors before finally succeeding once all data has been transmitted or failing should the
-socket send fail. Interrupts and timeouts are the only recoverable errors. Other problems result in an error status and
-when a remote disconnect is detected `Drv::IpSocket::close` is closed to ensure the socket is ready for a subsequent
-call to `Drv::IpSocket::open`.
+socket send fail. Interrupts and timeouts are the only recoverable errors. Users must call `close` and `open` to recover
+from other errors.
 
 `Drv::IpSocket::recv` will attempt to read data from across the socket. It will block until data is received and
-in the case that the socket is interrupted without data, it will retry a configurable number of times. Other errors will
-result in an error status with a specific `Drv::IpSocket::close` call issued in the case of detected disconnects.
+in the case that the socket is interrupted without data, it will retry a configurable number of times. Users must call
+`close` and `open` to recover from other errors.
 
 A call to `Drv::IpSocket::close` will specifically shutdown and close the client connection. This has the effect of
 stopping any blocking reads on the socket, issuing a formal disconnect, and cleaning up the allocated resources. Once
@@ -92,7 +94,8 @@ Since this class is intended to communicate with exactly one client, no listen q
 from clients will be ignored until the primary client has been closed. Like the TCP client packet drops will result in an
 error.
 
-**Note:** the `Drv::TcpServerSocket::open` call will block until a client connects to the server.
+> ![NOTE]
+> The `Drv::TcpServerSocket::open` call will block until a client connects to the server.
 
 In order to startup the server to listen, the `Drv::TcpServerSocket::startup` method should be called. It will create a
 socket that will listen for incoming connections.  `Drv::TcpServerSocket::startup` should be called before any
@@ -132,9 +135,8 @@ is no guarantee that a sent packet is received, or even that the remote side is
 A UDP socket must be configured for each direction that it will communicate in. This can be done using calls to
 `Drv::UdpSocket::configureSend` and `Drv::UdpSocket::configureRecv`.  If either call is omitted only a single direction
 of communication will function. It is erroneous to omit both configuration calls.  Calling `Drv::UdpSocket::configure`
-is equivalent to  calling `Drv::UdpSocket::configureSend` for compatibility with `Drv::IpSocket`.  Other interaction
-with the UDP socket is as stipulated with `Drv::IpSocket`.  Examples of instantiation and configuration are provided
-below.
+will result in an assertion failure. Other interaction with the UDP socket is as stipulated with `Drv::IpSocket`.  Examples
+of instantiation and configuration are provided below.
 
 ```c++
 Drv::UdpSocket& socketSend = Drv::UdpSocket;
@@ -154,7 +156,8 @@ socketBoth.configureRecv(127.0.0.1, 60212);
 ## Drv::SocketComponentHelper Virtual Baseclass
 
 The Drv::SocketComponentHelper is intended as a base class used to add in the functionality of an automatically reconnecting
-receive thread to another class (typically an F´ component) as well as an interface between the component using an IP socket and the IP socket library functions implemented in this folder. In order for this thread to function, the inheritor must
+receive thread to another class (typically an F´ component) as well as an interface between the component using an IP socket
+and the IP socket library functions implemented in this folder. In order for this thread to function, the inheritor must
 implement several methods to provide the necessary interaction of this thread. These functions are described in the next
 section.
 
@@ -173,7 +176,7 @@ uplinkComm.start(name); // Default reconnect=true
 ...
 
 uplinkComm.stop();
-(void) uplinkComm.join(nullptr);
+(void) uplinkComm.join();
 ```
 
 `Drv::SocketComponentHelper::open` and `Drv::SocketComponentHelper::close` convenience methods are also provided to open and close the