Merge pull request #8268 from miri64/slipdev/enh/simplify-byte-unstuffing

slipdev: simplify and solidify byte-unstuffing

drivers/Makefile.dep

@@ -246,6 +246,7 @@ ifneq (,$(filter si70%,$(USEMODULE)))
 endif
 
 ifneq (,$(filter slipdev,$(USEMODULE)))
+  USEMODULE += tsrb
   FEATURES_REQUIRED += periph_uart
 endif
 

drivers/include/slipdev.h

@@ -26,7 +26,7 @@
 #include "cib.h"
 #include "net/netdev.h"
 #include "periph/uart.h"
-#include "ringbuffer.h"
+#include "tsrb.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -36,20 +36,12 @@ extern "C" {
  * @brief   UART buffer size used for TX and RX buffers
  *
  * Reduce this value if your expected traffic does not include full IPv6 MTU
- * sized packets
- */
-#ifndef SLIPDEV_BUFSIZE
-#define SLIPDEV_BUFSIZE (1500U)
-#endif
-
-/**
- * @brief   Packet FIFO size
+ * sized packets.
  *
- * @note    For GNRC it is recommended to have it the same size as the link-layer
- *          thread's message queue, but it MUST be of power of 2
+ * @pre Needs to be power of two and `<= INT_MAX`
  */
-#ifndef SLIPDEV_PKTFIFO_SIZE
-#define SLIPDEV_PKTFIFO_SIZE    (8U)
+#ifndef SLIPDEV_BUFSIZE
+#define SLIPDEV_BUFSIZE (2048U)
 #endif
 
 /**
@@ -68,13 +60,8 @@ typedef struct {
 typedef struct {
     netdev_t netdev;                        /**< parent class */
     slipdev_params_t config;                /**< configuration parameters */
-    ringbuffer_t inbuf;                     /**< RX buffer */
+    tsrb_t inbuf;                           /**< RX buffer */
     char rxmem[SLIPDEV_BUFSIZE];            /**< memory used by RX buffer */
-    uint16_t pktfifo[SLIPDEV_PKTFIFO_SIZE]; /**< FIFO of sizes of fully received
-                                             *   packets */
-    cib_t pktfifo_idx;                      /**< CIB for slipdev_t::pktfifo */
-    uint16_t inbytes;                       /**< the number of bytes received of
-                                             *   a currently incoming packet */
     uint16_t inesc;                         /**< device previously received an escape
                                              *   byte */
 } slipdev_t;

drivers/slipdev/slipdev.c

@@ -48,53 +48,17 @@ void slipdev_setup(slipdev_t *dev, const slipdev_params_t *params)
 {
     /* set device descriptor fields */
     memcpy(&dev->config, params, sizeof(dev->config));
-    dev->inbytes = 0U;
     dev->inesc = 0U;
     dev->netdev.driver = &slip_driver;
 }
 
-static inline void _add_byte_to_inbuf(slipdev_t *dev, uint8_t byte)
-{
-    if (ringbuffer_add_one(&dev->inbuf, byte) < 0) {
-        dev->inbytes++;
-    }
-}
-
-static void _slip_rx_cb(void *arg, uint8_t data)
+static void _slip_rx_cb(void *arg, uint8_t byte)
 {
     slipdev_t *dev = arg;
 
-    if ((data == SLIP_END) && (dev->netdev.event_callback != NULL)) {
-        int idx = cib_put(&dev->pktfifo_idx);
-        if (idx >= 0) {
-            dev->netdev.event_callback((netdev_t *)dev, NETDEV_EVENT_ISR);
-            dev->pktfifo[idx] = dev->inbytes;
-        }
-        else {
-            /* can't handover packet => dropping it */
-            ringbuffer_remove(&dev->inbuf, dev->inbytes);
-        }
-        dev->inbytes = 0;
-    }
-    else if (dev->inesc) {
-        dev->inesc = 0U;
-        uint8_t actual = (data == SLIP_END_ESC) ? SLIP_END :
-                         ((data == SLIP_ESC_ESC) ? SLIP_ESC : 0);
-
-        switch (data) {
-            case SLIP_END_ESC:
-            case SLIP_ESC_ESC:
-                _add_byte_to_inbuf(dev, actual);
-                break;
-            default:
-                break;
-        }
-    }
-    else if (data == SLIP_ESC) {
-        dev->inesc = 1U;
-    }
-    else {
-        _add_byte_to_inbuf(dev, data);
+    tsrb_add_one(&dev->inbuf, byte);
+    if ((byte == SLIP_END) && (dev->netdev.event_callback != NULL)) {
+        dev->netdev.event_callback((netdev_t *)dev, NETDEV_EVENT_ISR);
     }
 }
 
@@ -105,8 +69,7 @@ static int _init(netdev_t *netdev)
     DEBUG("slipdev: initializing device %p on UART %i with baudrate %" PRIu32 "\n",
           (void *)dev, dev->config.uart, dev->config.baudrate);
     /* initialize buffers */
-    ringbuffer_init(&dev->inbuf, dev->rxmem, sizeof(dev->rxmem));
-    cib_init(&dev->pktfifo_idx, SLIPDEV_PKTFIFO_SIZE);
+    tsrb_init(&dev->inbuf, dev->rxmem, sizeof(dev->rxmem));
     if (uart_init(dev->config.uart, dev->config.baudrate, _slip_rx_cb,
                   dev) != UART_OK) {
         LOG_ERROR("slipdev: error initializing UART %i with baudrate %" PRIu32 "\n",
@@ -155,30 +118,69 @@ static int _send(netdev_t *netdev, const struct iovec *vector, unsigned count)
 static int _recv(netdev_t *netdev, void *buf, size_t len, void *info)
 {
     slipdev_t *dev = (slipdev_t *)netdev;
-    int res, idx = cib_peek(&dev->pktfifo_idx);
+    int res = 0;
 
     (void)info;
-    if (idx < 0) {
-        return -EFAULT;
-    }
     if (buf == NULL) {
         if (len > 0) {
-            /* drop packet */
-            cib_get(&dev->pktfifo_idx);
-            /* and remove data */
-            res = ringbuffer_remove(&dev->inbuf, len);
-        }
-        else {
-            res = dev->pktfifo[idx];
+            /* remove data */
+            for (; len > 0; len--) {
+                int byte = tsrb_get_one(&dev->inbuf);
+                if ((byte == (int)SLIP_END) || (byte < 0)) {
+                    /* end early if end of packet or ringbuffer is reached;
+                     * len might be larger than the actual packet */
+                    break;
+                }
+            }
+        } else {
+            /* the user was warned not to use a buffer size > `INT_MAX` ;-) */
+            res = (int)tsrb_avail(&dev->inbuf);
         }
     }
-    else if (len < dev->pktfifo[idx]) {
-        res = -ENOBUFS;
-    }
     else {
-        size_t bytes = dev->pktfifo[cib_get(&dev->pktfifo_idx)];
-        bytes = ringbuffer_get(&dev->inbuf, buf, bytes);
-        res = bytes;
+        int byte;
+        uint8_t *ptr = buf;
+
+        do {
+            if ((byte = tsrb_get_one(&dev->inbuf)) < 0) {
+                /* something went wrong, return error */
+                return -EIO;
+            }
+            switch (byte) {
+                case SLIP_END:
+                    break;
+                case SLIP_ESC:
+                    dev->inesc = 1;
+                    break;
+                case SLIP_END_ESC:
+                    if (dev->inesc) {
+                        *(ptr++) = SLIP_END;
+                        res++;
+                        dev->inesc = 0;
+                        break;
+                    }
+                    /* falls through intentionally to default when !dev->inesc */
+                case SLIP_ESC_ESC:
+                    if (dev->inesc) {
+                        *(ptr++) = SLIP_ESC;
+                        res++;
+                        dev->inesc = 0;
+                        break;
+                    }
+                    /* falls through intentionally to default when !dev->inesc */
+                default:
+                    *(ptr++) = (uint8_t)byte;
+                    res++;
+                    break;
+            }
+            if ((unsigned)res > len) {
+                while (byte != SLIP_END) {
+                    /* clear out unreceived packet */
+                    byte = tsrb_get_one(&dev->inbuf);
+                }
+                return -ENOBUFS;
+            }
+        } while (byte != SLIP_END);
     }
     return res;
 }