Merge pull request #14222 from brummer-simon/gnrc_tcp-rewrite_internal_messaging

gnrc_tcp: rewrite API internal messaging

sys/include/net/gnrc/tcp/config.h

@@ -167,6 +167,24 @@ extern "C" {
 #ifndef CONFIG_GNRC_TCP_PROBE_UPPER_BOUND
 #define CONFIG_GNRC_TCP_PROBE_UPPER_BOUND (60U * US_PER_SEC)
 #endif
+
+/**
+ * @brief Message queue size for TCP API internal messaging
+ * @note The number of elements in a message queue must be a power of two.
+ *       This value defines the exponent of 2^n.
+ */
+#ifndef CONFIG_GNRC_TCP_MSG_QUEUE_SIZE_EXP
+#define CONFIG_GNRC_TCP_MSG_QUEUE_SIZE_EXP (2U)
+#endif
+
+/**
+ * @brief Message queue size for the TCP eventloop
+ * @note The number of elements in a message queue must be a power of two.
+ *       This value defines the exponent of 2^n.
+ */
+#ifndef CONFIG_GNRC_TCP_EVENTLOOP_MSG_QUEUE_SIZE_EXP
+#define CONFIG_GNRC_TCP_EVENTLOOP_MSG_QUEUE_SIZE_EXP (3U)
+#endif
 /** @} */
 
 #ifdef __cplusplus

sys/include/net/gnrc/tcp/tcb.h

@@ -38,27 +38,6 @@
 extern "C" {
 #endif
 
-/**
- * @{
- * @ingroup net_gnrc_tcp_conf
- * @brief Size of the TCB mbox (as exponent of 2^n).
- *
- *        As the mbox buffer size ALWAYS needs to be power of two, this option
- *        represents the exponent of 2^n, which will be used as the size of the
- *        mbox.
- */
-#ifndef CONFIG_GNRC_TCP_TCB_MBOX_SIZE_EXP
-#define CONFIG_GNRC_TCP_TCB_MBOX_SIZE_EXP (3U)
-#endif
-/** @} */
-
-/**
- * @brief Size of the TCB mbox
- */
-#ifndef GNRC_TCP_TCB_MBOX_SIZE
-#define GNRC_TCP_TCB_MBOX_SIZE (1 << CONFIG_GNRC_TCP_TCB_MBOX_SIZE_EXP)
-#endif
-
 /**
  * @brief Transmission control block of GNRC TCP.
  */
@@ -91,8 +70,7 @@ typedef struct _transmission_control_block {
     xtimer_t tim_tout;     /**< Timer struct for timeouts */
     msg_t msg_tout;        /**< Message, sent on timeouts */
     gnrc_pktsnip_t *pkt_retransmit;   /**< Pointer to packet in "retransmit queue" */
-    msg_t mbox_raw[GNRC_TCP_TCB_MBOX_SIZE];   /**< Msg queue for mbox */
+    mbox_t *mbox;            /**< TCB mbox for synchronization */
-    mbox_t mbox;             /**< TCB mbox for synchronization */
     uint8_t *rcv_buf_raw;    /**< Pointer to the receive buffer */
     ringbuffer_t rcv_buf;    /**< Receive buffer data structure */
     mutex_t fsm_lock;        /**< Mutex for FSM access synchronization */

sys/net/gnrc/transport_layer/tcp/Kconfig

@@ -112,12 +112,18 @@ config GNRC_TCP_PROBE_UPPER_BOUND
     int "Lower bound for the duration between probes in microseconds"
     default 60000000
 
-config GNRC_TCP_TCB_MBOX_SIZE_EXP
+config GNRC_TCP_MSG_QUEUE_SIZE_SIZE_EXP
-    int "Size of the TCB mbox (as exponent of 2^n)"
+    int "Message queue size for TCP API internal messaging (as exponent of 2^n)"
+    default 2
+    help
+        The number of elements in a message queue must be always a power of two.
+        This value defines the exponent of 2^n.
+
+config GNRC_TCP_EVENTLOOP_MSG_QUEUE_SIZE_SIZE_EXP
+    int "Message queue size for the TCP eventloop (as exponent of 2^n)"
     default 3
     help
-        As the mbox buffer size ALWAYS needs to be power of two, this option
+        The number of elements in a message queue must be always a power of two.
-        represents the exponent of 2^n, which will be used as the size of the
+        This value defines the exponent of 2^n.
-        mbox.
 
 endif # KCONFIG_MODULE_GNRC_TCP

sys/net/gnrc/transport_layer/tcp/gnrc_tcp.c

@@ -21,6 +21,7 @@
 #include <string.h>
 #include <utlist.h>
 
+#include "mbox.h"
 #include "net/af.h"
 #include "net/gnrc.h"
 #include "net/gnrc/tcp.h"
@@ -38,6 +39,8 @@
 #define ENABLE_DEBUG (0)
 #include "debug.h"
 
+#define TCP_MSG_QUEUE_SIZE (1 << CONFIG_GNRC_TCP_MSG_QUEUE_SIZE_EXP)
+
 /**
  * @brief Allocate memory for GNRC TCP thread stack.
  */
@@ -119,8 +122,10 @@ static int _gnrc_tcp_open(gnrc_tcp_tcb_t *tcb, const gnrc_tcp_ep_t *remote,
                           const uint8_t *local_addr, uint16_t local_port, int passive)
 {
     msg_t msg;
+    msg_t msg_queue[TCP_MSG_QUEUE_SIZE];
+    mbox_t mbox = MBOX_INIT(msg_queue, TCP_MSG_QUEUE_SIZE);
     xtimer_t connection_timeout;
-    cb_arg_t connection_timeout_arg = {MSG_TYPE_CONNECTION_TIMEOUT, &(tcb->mbox)};
+    cb_arg_t connection_timeout_arg = {MSG_TYPE_CONNECTION_TIMEOUT, &mbox};
     int ret = 0;
 
     /* Lock the TCB for this function call */
@@ -132,12 +137,8 @@ static int _gnrc_tcp_open(gnrc_tcp_tcb_t *tcb, const gnrc_tcp_ep_t *remote,
         return -EISCONN;
     }
 
-    /* Mark TCB as waiting for incoming messages */
+    /* Setup messaging */
-    tcb->status |= STATUS_WAIT_FOR_MSG;
+    _fsm_set_mbox(tcb, &mbox);
-
-    /* 'Flush' mbox */
-    while (mbox_try_get(&(tcb->mbox), &msg) != 0) {
-    }
 
     /* Setup passive connection */
     if (passive) {
@@ -202,7 +203,7 @@ static int _gnrc_tcp_open(gnrc_tcp_tcb_t *tcb, const gnrc_tcp_ep_t *remote,
     /* Wait until a connection was established or closed */
     while (ret >= 0 && tcb->state != FSM_STATE_CLOSED && tcb->state != FSM_STATE_ESTABLISHED &&
            tcb->state != FSM_STATE_CLOSE_WAIT) {
-        mbox_get(&(tcb->mbox), &msg);
+        mbox_get(&mbox, &msg);
         switch (msg.type) {
             case MSG_TYPE_NOTIFY_USER:
                 DEBUG("gnrc_tcp.c : _gnrc_tcp_open() : MSG_TYPE_NOTIFY_USER\n");
@@ -239,11 +240,11 @@ static int _gnrc_tcp_open(gnrc_tcp_tcb_t *tcb, const gnrc_tcp_ep_t *remote,
     }
 
     /* Cleanup */
+    _fsm_set_mbox(tcb, NULL);
     xtimer_remove(&connection_timeout);
     if (tcb->state == FSM_STATE_CLOSED && ret == 0) {
         ret = -ECONNREFUSED;
     }
-    tcb->status &= ~STATUS_WAIT_FOR_MSG;
     mutex_unlock(&(tcb->function_lock));
     return ret;
 }
@@ -406,7 +407,6 @@ void gnrc_tcp_tcb_init(gnrc_tcp_tcb_t *tcb)
     tcb->rtt_var = RTO_UNINITIALIZED;
     tcb->srtt = RTO_UNINITIALIZED;
     tcb->rto = RTO_UNINITIALIZED;
-    mbox_init(&(tcb->mbox), tcb->mbox_raw, GNRC_TCP_TCB_MBOX_SIZE);
     mutex_init(&(tcb->fsm_lock));
     mutex_init(&(tcb->function_lock));
 }
@@ -466,12 +466,14 @@ ssize_t gnrc_tcp_send(gnrc_tcp_tcb_t *tcb, const void *data, const size_t len,
     assert(data != NULL);
 
     msg_t msg;
+    msg_t msg_queue[TCP_MSG_QUEUE_SIZE];
+    mbox_t mbox = MBOX_INIT(msg_queue, TCP_MSG_QUEUE_SIZE);
     xtimer_t connection_timeout;
-    cb_arg_t connection_timeout_arg = {MSG_TYPE_CONNECTION_TIMEOUT, &(tcb->mbox)};
+    cb_arg_t connection_timeout_arg = {MSG_TYPE_CONNECTION_TIMEOUT, &mbox};
     xtimer_t user_timeout;
-    cb_arg_t user_timeout_arg = {MSG_TYPE_USER_SPEC_TIMEOUT, &(tcb->mbox)};
+    cb_arg_t user_timeout_arg = {MSG_TYPE_USER_SPEC_TIMEOUT, &mbox};
     xtimer_t probe_timeout;
-    cb_arg_t probe_timeout_arg = {MSG_TYPE_PROBE_TIMEOUT, &(tcb->mbox)};
+    cb_arg_t probe_timeout_arg = {MSG_TYPE_PROBE_TIMEOUT, &mbox};
     uint32_t probe_timeout_duration_us = 0;
     ssize_t ret = 0;
     bool probing_mode = false;
@@ -485,18 +487,11 @@ ssize_t gnrc_tcp_send(gnrc_tcp_tcb_t *tcb, const void *data, const size_t len,
         return -ENOTCONN;
     }
 
-    /* Mark TCB as waiting for incoming messages */
+    /* Setup messaging */
-    tcb->status |= STATUS_WAIT_FOR_MSG;
+    _fsm_set_mbox(tcb, &mbox);
-
-    /* 'Flush' mbox */
-    while (mbox_try_get(&(tcb->mbox), &msg) != 0) {
-    }
-
-    /* Setup connection timeout: Put timeout message in tcb's mbox on expiration */
     _setup_timeout(&connection_timeout, CONFIG_GNRC_TCP_CONNECTION_TIMEOUT_DURATION,
                    _cb_mbox_put_msg, &connection_timeout_arg);
 
-    /* Setup user specified timeout if timeout_us is greater than zero */
     if (timeout_duration_us > 0) {
         _setup_timeout(&user_timeout, timeout_duration_us, _cb_mbox_put_msg, &user_timeout_arg);
     }
@@ -527,7 +522,7 @@ ssize_t gnrc_tcp_send(gnrc_tcp_tcb_t *tcb, const void *data, const size_t len,
         }
 
         /* Wait for responses */
-        mbox_get(&(tcb->mbox), &msg);
+        mbox_get(&mbox, &msg);
         switch (msg.type) {
             case MSG_TYPE_CONNECTION_TIMEOUT:
                 DEBUG("gnrc_tcp.c : gnrc_tcp_send() : CONNECTION_TIMEOUT\n");
@@ -576,10 +571,10 @@ ssize_t gnrc_tcp_send(gnrc_tcp_tcb_t *tcb, const void *data, const size_t len,
     }
 
     /* Cleanup */
+    _fsm_set_mbox(tcb, NULL);
     xtimer_remove(&probe_timeout);
     xtimer_remove(&connection_timeout);
     xtimer_remove(&user_timeout);
-    tcb->status &= ~STATUS_WAIT_FOR_MSG;
     mutex_unlock(&(tcb->function_lock));
     return ret;
 }
@@ -591,10 +586,12 @@ ssize_t gnrc_tcp_recv(gnrc_tcp_tcb_t *tcb, void *data, const size_t max_len,
     assert(data != NULL);
 
     msg_t msg;
+    msg_t msg_queue[TCP_MSG_QUEUE_SIZE];
+    mbox_t mbox = MBOX_INIT(msg_queue, TCP_MSG_QUEUE_SIZE);
     xtimer_t connection_timeout;
-    cb_arg_t connection_timeout_arg = {MSG_TYPE_CONNECTION_TIMEOUT, &(tcb->mbox)};
+    cb_arg_t connection_timeout_arg = {MSG_TYPE_CONNECTION_TIMEOUT, &mbox};
     xtimer_t user_timeout;
-    cb_arg_t user_timeout_arg = {MSG_TYPE_USER_SPEC_TIMEOUT, &(tcb->mbox)};
+    cb_arg_t user_timeout_arg = {MSG_TYPE_USER_SPEC_TIMEOUT, &mbox};
     ssize_t ret = 0;
 
     /* Lock the TCB for this function call */
@@ -625,18 +622,10 @@ ssize_t gnrc_tcp_recv(gnrc_tcp_tcb_t *tcb, void *data, const size_t max_len,
         return ret;
     }
 
-    /* Mark TCB as waiting for incoming messages */
+    /* Setup messaging */
-    tcb->status |= STATUS_WAIT_FOR_MSG;
+    _fsm_set_mbox(tcb, &mbox);
-
-    /* 'Flush' mbox */
-    while (mbox_try_get(&(tcb->mbox), &msg) != 0) {
-    }
-
-    /* Setup connection timeout: Put timeout message in tcb's mbox on expiration */
     _setup_timeout(&connection_timeout, CONFIG_GNRC_TCP_CONNECTION_TIMEOUT_DURATION,
                    _cb_mbox_put_msg, &connection_timeout_arg);
-
-    /* Setup user specified timeout */
     _setup_timeout(&user_timeout, timeout_duration_us, _cb_mbox_put_msg, &user_timeout_arg);
 
     /* Processing loop */
@@ -657,7 +646,7 @@ ssize_t gnrc_tcp_recv(gnrc_tcp_tcb_t *tcb, void *data, const size_t max_len,
 
         /* If there was no data: Wait for next packet or until the timeout fires */
         if (ret <= 0) {
-            mbox_get(&(tcb->mbox), &msg);
+            mbox_get(&mbox, &msg);
             switch (msg.type) {
                 case MSG_TYPE_CONNECTION_TIMEOUT:
                     DEBUG("gnrc_tcp.c : gnrc_tcp_recv() : CONNECTION_TIMEOUT\n");
@@ -682,9 +671,9 @@ ssize_t gnrc_tcp_recv(gnrc_tcp_tcb_t *tcb, void *data, const size_t max_len,
     }
 
     /* Cleanup */
+    _fsm_set_mbox(tcb, NULL);
     xtimer_remove(&connection_timeout);
     xtimer_remove(&user_timeout);
-    tcb->status &= ~STATUS_WAIT_FOR_MSG;
     mutex_unlock(&(tcb->function_lock));
     return ret;
 }
@@ -694,8 +683,10 @@ void gnrc_tcp_close(gnrc_tcp_tcb_t *tcb)
     assert(tcb != NULL);
 
     msg_t msg;
+    msg_t msg_queue[TCP_MSG_QUEUE_SIZE];
+    mbox_t mbox = MBOX_INIT(msg_queue, TCP_MSG_QUEUE_SIZE);
     xtimer_t connection_timeout;
-    cb_arg_t connection_timeout_arg = {MSG_TYPE_CONNECTION_TIMEOUT, &(tcb->mbox)};
+    cb_arg_t connection_timeout_arg = {MSG_TYPE_CONNECTION_TIMEOUT, &mbox};
 
     /* Lock the TCB for this function call */
     mutex_lock(&(tcb->function_lock));
@@ -706,14 +697,8 @@ void gnrc_tcp_close(gnrc_tcp_tcb_t *tcb)
         return;
     }
 
-    /* Mark TCB as waiting for incoming messages */
+    /* Setup messaging */
-    tcb->status |= STATUS_WAIT_FOR_MSG;
+    _fsm_set_mbox(tcb, &mbox);
-
-    /* 'Flush' mbox */
-    while (mbox_try_get(&(tcb->mbox), &msg) != 0) {
-    }
-
-    /* Setup connection timeout: Put timeout message in tcb's mbox on expiration */
     _setup_timeout(&connection_timeout, CONFIG_GNRC_TCP_CONNECTION_TIMEOUT_DURATION,
                    _cb_mbox_put_msg, &connection_timeout_arg);
 
@@ -722,7 +707,7 @@ void gnrc_tcp_close(gnrc_tcp_tcb_t *tcb)
 
     /* Loop until the connection has been closed */
     while (tcb->state != FSM_STATE_CLOSED) {
-        mbox_get(&(tcb->mbox), &msg);
+        mbox_get(&mbox, &msg);
         switch (msg.type) {
             case MSG_TYPE_CONNECTION_TIMEOUT:
                 DEBUG("gnrc_tcp.c : gnrc_tcp_close() : CONNECTION_TIMEOUT\n");
@@ -739,8 +724,8 @@ void gnrc_tcp_close(gnrc_tcp_tcb_t *tcb)
     }
 
     /* Cleanup */
+    _fsm_set_mbox(tcb, NULL);
     xtimer_remove(&connection_timeout);
-    tcb->status &= ~STATUS_WAIT_FOR_MSG;
     mutex_unlock(&(tcb->function_lock));
 }
 

sys/net/gnrc/transport_layer/tcp/gnrc_tcp_eventloop.c

@@ -34,6 +34,8 @@
 #define ENABLE_DEBUG (0)
 #include "debug.h"
 
+#define TCP_EVENTLOOP_MSG_QUEUE_SIZE (1 << CONFIG_GNRC_TCP_EVENTLOOP_MSG_QUEUE_SIZE_EXP)
+
 static msg_t _eventloop_msg_queue[TCP_EVENTLOOP_MSG_QUEUE_SIZE];
 
 /**

sys/net/gnrc/transport_layer/tcp/gnrc_tcp_fsm.c

@@ -396,8 +396,6 @@ static int _fsm_rcvd_pkt(gnrc_tcp_tcb_t *tcb, gnrc_pktsnip_t *in_pkt)
     uint32_t seg_seq = 0;            /* Sequence number of the incoming packet*/
     uint32_t seg_ack = 0;            /* Acknowledgment number of the incoming packet */
     uint32_t seg_wnd = 0;            /* Receive window of the incoming packet */
-    uint32_t seg_len = 0;            /* Segment length of the incoming packet */
-    uint32_t pay_len = 0;            /* Payload length of the incoming packet */
 
     DEBUG("gnrc_tcp_fsm.c : _fsm_rcvd_pkt()\n");
     /* Search for TCP header. */
@@ -462,7 +460,10 @@ static int _fsm_rcvd_pkt(gnrc_tcp_tcb_t *tcb, gnrc_pktsnip_t *in_pkt)
                 lst = lst->next;
             }
             /* Return if connection is already handled (port and addresses match) */
-            if (lst != NULL) {
+            /* cppcheck-suppress knownConditionTrueFalse
+             * (reason: tmp *lst* can be true at runtime
+             */
+            if (lst) {
                 DEBUG("gnrc_tcp_fsm.c : _fsm_rcvd_pkt() : Connection already handled\n");
                 return 0;
             }
@@ -570,8 +571,8 @@ static int _fsm_rcvd_pkt(gnrc_tcp_tcb_t *tcb, gnrc_pktsnip_t *in_pkt)
     }
     /* Handle other states */
     else {
-        seg_len = _pkt_get_seg_len(in_pkt);
+        uint32_t seg_len = _pkt_get_seg_len(in_pkt);
-        pay_len = _pkt_get_pay_len(in_pkt);
+        uint32_t pay_len = _pkt_get_pay_len(in_pkt);
         /* 1) Verify sequence number ... */
         if (_pkt_chk_seq_num(tcb, seg_seq, pay_len)) {
             /* ... if invalid, and RST not set, reply with pure ACK, return */
@@ -888,12 +889,20 @@ int _fsm(gnrc_tcp_tcb_t *tcb, fsm_event_t event, gnrc_pktsnip_t *in_pkt, void *b
     int32_t result = _fsm_unprotected(tcb, event, in_pkt, buf, len);
 
     /* Notify blocked thread if something interesting happened */
-    if ((tcb->status & STATUS_NOTIFY_USER) && (tcb->status & STATUS_WAIT_FOR_MSG)) {
+    if ((tcb->status & STATUS_NOTIFY_USER) && tcb->mbox) {
         msg_t msg;
         msg.type = MSG_TYPE_NOTIFY_USER;
-        mbox_try_put(&(tcb->mbox), &msg);
+        msg.content.ptr = tcb;
+        mbox_try_put(tcb->mbox, &msg);
     }
     /* Unlock FSM */
     mutex_unlock(&(tcb->fsm_lock));
     return result;
 }
+
+void _fsm_set_mbox(gnrc_tcp_tcb_t *tcb, mbox_t *mbox)
+{
+    mutex_lock(&(tcb->fsm_lock));
+    tcb->mbox = mbox;
+    mutex_unlock(&(tcb->fsm_lock));
+}

sys/net/gnrc/transport_layer/tcp/internal/common.h

@@ -46,14 +46,12 @@ extern "C" {
 #define STATUS_PASSIVE        (1 << 0)
 #define STATUS_ALLOW_ANY_ADDR (1 << 1)
 #define STATUS_NOTIFY_USER    (1 << 2)
-#define STATUS_WAIT_FOR_MSG   (1 << 3)
 /** @} */
 
 /**
  * @brief Defines for "eventloop" thread settings.
  * @{
  */
-#define TCP_EVENTLOOP_MSG_QUEUE_SIZE (8U)
 #define TCP_EVENTLOOP_PRIO       (THREAD_PRIORITY_MAIN - 2U)
 #define TCP_EVENTLOOP_STACK_SIZE (THREAD_STACKSIZE_DEFAULT)
 /** @} */

sys/net/gnrc/transport_layer/tcp/internal/fsm.h

@@ -21,6 +21,7 @@
 #define FSM_H
 
 #include <stdint.h>
+#include "mbox.h"
 #include "net/gnrc.h"
 #include "net/gnrc/tcp/tcb.h"
 
@@ -77,6 +78,15 @@ typedef enum {
  */
 int _fsm(gnrc_tcp_tcb_t *tcb, fsm_event_t event, gnrc_pktsnip_t *in_pkt, void *buf, size_t len);
 
+/**
+ * @brief Associate mbox with tcb. Messages sent from the FSM will be stored in the mbox.
+ *
+ * @param[in, out] tcb   TCB to set message box on.
+ * @param[in]      mbox  Message box used to store messages from the FSM.
+ *                       If @p mbox is NULL, no messages will be stored.
+ */
+void _fsm_set_mbox(gnrc_tcp_tcb_t *tcb, mbox_t *mbox);
+
 #ifdef __cplusplus
 }
 #endif