stm32/usbd_cdc_interface: Remove full==size-1 limitation on tx ringbuf.

Before this commit the USB VCP TX ring-buffer used the basic implementation
where it can only be filled to a maximum of buffer size-1.  For a 1024 size
buffer this means the largest packet that can be sent is 1023.  Once a
packet of this size is sent the next byte copied in goes to the final byte
in the buffer, so must be sent as a 1 byte packet before the read pointer
can be wrapped around to the beginning.  So in large streaming transfers,
watching the USB sniffer you basically get alternating 1023 byte packets
then 1 byte packets.

This commit changes the ring-buffer implementation to a scheme that doesn't
have the full-size limitation, and the USB VCP driver can now achieve a
constant stream of full-sized packets.  This scheme introduces a
restriction on the size of the buffer: it must be a power of 2, and the
maximum size is half of the size of the index (in this case the index is
16-bit, so the maximum size would be 32767 bytes rounded to 16384 for a
power-of-2).  But this is not a big limitation because the size of the
ring-buffer prior to this commit was restricted to powers of 2 because it
was using a mask-based method to wrap the indices.

For an explanation of the new scheme see
https://www.snellman.net/blog/archive/2016-12-13-ring-buffers/

The RX buffer could likely do with a similar change, though as it's not
read from in chunks like the TX buffer it doesn't present the same issue,
all that's lost is one byte capacity of the buffer.

USB VCP TX throughput is improved by this change, potentially doubling the
speed in certain cases.

ports/stm32/usbd_cdc_interface.c

@@ -171,32 +171,56 @@ int8_t usbd_cdc_control(usbd_cdc_state_t *cdc_in, uint8_t cmd, uint8_t *pbuf, ui
     return USBD_OK;
 }
 
+static inline uint16_t usbd_cdc_tx_buffer_mask(uint16_t val) {
+    return val & (USBD_CDC_TX_DATA_SIZE - 1);
+}
+
+static inline uint16_t usbd_cdc_tx_buffer_size(usbd_cdc_itf_t *cdc) {
+    return cdc->tx_buf_ptr_in - cdc->tx_buf_ptr_out;
+}
+
+static inline bool usbd_cdc_tx_buffer_empty(usbd_cdc_itf_t *cdc) {
+    return cdc->tx_buf_ptr_out == cdc->tx_buf_ptr_in;
+}
+
+static inline bool usbd_cdc_tx_buffer_will_be_empty(usbd_cdc_itf_t *cdc) {
+    return cdc->tx_buf_ptr_out_next == cdc->tx_buf_ptr_in;
+}
+
+static inline bool usbd_cdc_tx_buffer_full(usbd_cdc_itf_t *cdc) {
+    return usbd_cdc_tx_buffer_size(cdc) == USBD_CDC_TX_DATA_SIZE;
+}
+
+static uint16_t usbd_cdc_tx_send_length(usbd_cdc_itf_t *cdc) {
+    uint16_t to_end = USBD_CDC_TX_DATA_SIZE - usbd_cdc_tx_buffer_mask(cdc->tx_buf_ptr_out);
+    return MIN(usbd_cdc_tx_buffer_size(cdc), to_end);
+}
+
+static void usbd_cdc_tx_buffer_put(usbd_cdc_itf_t *cdc, uint8_t data) {
+    cdc->tx_buf[usbd_cdc_tx_buffer_mask(cdc->tx_buf_ptr_in)] = data;
+    cdc->tx_buf_ptr_in++;
+}
+
+static uint8_t *usbd_cdc_tx_buffer_getp(usbd_cdc_itf_t *cdc, uint16_t len) {
+    cdc->tx_buf_ptr_out_next += len;
+    return &cdc->tx_buf[usbd_cdc_tx_buffer_mask(cdc->tx_buf_ptr_out)];
+}
+
 // Called when the USB IN endpoint is ready to receive more data
 // (cdc.base.tx_in_progress must be 0)
 void usbd_cdc_tx_ready(usbd_cdc_state_t *cdc_in) {
 
     usbd_cdc_itf_t *cdc = (usbd_cdc_itf_t *)cdc_in;
-    cdc->tx_buf_ptr_out = cdc->tx_buf_ptr_out_shadow;
+    cdc->tx_buf_ptr_out = cdc->tx_buf_ptr_out_next;
 
-    if (cdc->tx_buf_ptr_out == cdc->tx_buf_ptr_in && !cdc->tx_need_empty_packet) {
+    if (usbd_cdc_tx_buffer_empty(cdc) && !cdc->tx_need_empty_packet) {
         // No outstanding data to send
         return;
     }
-
-    uint32_t len;
-    if (cdc->tx_buf_ptr_out > cdc->tx_buf_ptr_in) { // rollback
-        len = USBD_CDC_TX_DATA_SIZE - cdc->tx_buf_ptr_out;
-    } else {
-        len = cdc->tx_buf_ptr_in - cdc->tx_buf_ptr_out;
-    }
-
+    uint16_t len = usbd_cdc_tx_send_length(cdc);
     // Should always succeed because cdc.base.tx_in_progress==0
-    USBD_CDC_TransmitPacket(&cdc->base, len, &cdc->tx_buf[cdc->tx_buf_ptr_out]);
+    USBD_CDC_TransmitPacket(&cdc->base, len, usbd_cdc_tx_buffer_getp(cdc, len));
 
-    cdc->tx_buf_ptr_out_shadow += len;
-    if (cdc->tx_buf_ptr_out_shadow == USBD_CDC_TX_DATA_SIZE) {
-        cdc->tx_buf_ptr_out_shadow = 0;
-    }
 
     // According to the USB specification, a packet size of 64 bytes (CDC_DATA_FS_MAX_PACKET_SIZE)
     // gets held at the USB host until the next packet is sent.  This is because a
@@ -204,7 +228,7 @@ void usbd_cdc_tx_ready(usbd_cdc_state_t *cdc_in) {
     // the host waits for all data to arrive (ie, waits for a packet < max packet size).
     // To flush a packet of exactly max packet size, we need to send a zero-size packet.
     // See eg http://www.cypress.com/?id=4&rID=92719
-    cdc->tx_need_empty_packet = (len > 0 && len % usbd_cdc_max_packet(cdc->base.usbd->pdev) == 0 && cdc->tx_buf_ptr_out_shadow == cdc->tx_buf_ptr_in);
+    cdc->tx_need_empty_packet = (len > 0 && len % usbd_cdc_max_packet(cdc->base.usbd->pdev) == 0 && usbd_cdc_tx_buffer_will_be_empty(cdc));
 }
 
 // Attempt to queue data on the USB IN endpoint
@@ -291,10 +315,7 @@ int8_t usbd_cdc_receive(usbd_cdc_state_t *cdc_in, size_t len) {
 }
 
 int usbd_cdc_tx_half_empty(usbd_cdc_itf_t *cdc) {
-    int32_t tx_waiting = (int32_t)cdc->tx_buf_ptr_in - (int32_t)cdc->tx_buf_ptr_out;
-    if (tx_waiting < 0) {
-        tx_waiting += USBD_CDC_TX_DATA_SIZE;
-    }
+    int32_t tx_waiting = usbd_cdc_tx_buffer_size(cdc);
     return tx_waiting <= USBD_CDC_TX_DATA_SIZE / 2;
 }
 
@@ -317,8 +338,7 @@ int usbd_cdc_tx(usbd_cdc_itf_t *cdc, const uint8_t *buf, uint32_t len, uint32_t
     for (uint32_t i = 0; i < len; i++) {
         // Wait until the device is connected and the buffer has space, with a given timeout
         uint32_t start = HAL_GetTick();
-        while (cdc->connect_state == USBD_CDC_CONNECT_STATE_DISCONNECTED
-               || ((cdc->tx_buf_ptr_in + 1) & (USBD_CDC_TX_DATA_SIZE - 1)) == cdc->tx_buf_ptr_out) {
+        while (cdc->connect_state == USBD_CDC_CONNECT_STATE_DISCONNECTED || usbd_cdc_tx_buffer_full(cdc)) {
             usbd_cdc_try_tx(cdc);
             // Wraparound of tick is taken care of by 2's complement arithmetic.
             if (HAL_GetTick() - start >= timeout) {
@@ -333,8 +353,7 @@ int usbd_cdc_tx(usbd_cdc_itf_t *cdc, const uint8_t *buf, uint32_t len, uint32_t
         }
 
         // Write data to device buffer
-        cdc->tx_buf[cdc->tx_buf_ptr_in] = buf[i];
-        cdc->tx_buf_ptr_in = (cdc->tx_buf_ptr_in + 1) & (USBD_CDC_TX_DATA_SIZE - 1);
+        usbd_cdc_tx_buffer_put(cdc, buf[i]);
     }
 
     usbd_cdc_try_tx(cdc);
@@ -357,7 +376,7 @@ void usbd_cdc_tx_always(usbd_cdc_itf_t *cdc, const uint8_t *buf, uint32_t len) {
             // If the buffer is full, wait until it gets drained, with a timeout of 500ms
             // (wraparound of tick is taken care of by 2's complement arithmetic).
             uint32_t start = HAL_GetTick();
-            while (((cdc->tx_buf_ptr_in + 1) & (USBD_CDC_TX_DATA_SIZE - 1)) == cdc->tx_buf_ptr_out && HAL_GetTick() - start <= 500) {
+            while (usbd_cdc_tx_buffer_full(cdc) && HAL_GetTick() - start <= 500) {
                 usbd_cdc_try_tx(cdc);
                 if (query_irq() == IRQ_STATE_DISABLED) {
                     // IRQs disabled so buffer will never be drained; exit loop
@@ -367,8 +386,7 @@ void usbd_cdc_tx_always(usbd_cdc_itf_t *cdc, const uint8_t *buf, uint32_t len) {
             }
         }
 
-        cdc->tx_buf[cdc->tx_buf_ptr_in] = buf[i];
-        cdc->tx_buf_ptr_in = (cdc->tx_buf_ptr_in + 1) & (USBD_CDC_TX_DATA_SIZE - 1);
+        usbd_cdc_tx_buffer_put(cdc, buf[i]);
     }
     usbd_cdc_try_tx(cdc);
 }

ports/stm32/usbd_cdc_interface.h

@@ -35,7 +35,7 @@
 #define USBD_CDC_RX_DATA_SIZE (1024) // this must be 2 or greater, and a power of 2
 #endif
 #ifndef USBD_CDC_TX_DATA_SIZE
-#define USBD_CDC_TX_DATA_SIZE (1024) // I think this can be any value (was 2048)
+#define USBD_CDC_TX_DATA_SIZE (1024) // This must be a power of 2 and no greater than 16384
 #endif
 
 // Values for connect_state
@@ -60,7 +60,7 @@ typedef struct _usbd_cdc_itf_t {
     uint8_t tx_buf[USBD_CDC_TX_DATA_SIZE]; // data for USB IN endpoind is stored in this buffer
     uint16_t tx_buf_ptr_in; // increment this pointer modulo USBD_CDC_TX_DATA_SIZE when new data is available
     volatile uint16_t tx_buf_ptr_out; // increment this pointer modulo USBD_CDC_TX_DATA_SIZE when data is drained
-    uint16_t tx_buf_ptr_out_shadow; // shadow of above
+    uint16_t tx_buf_ptr_out_next; // next position of above once transmission finished
     uint8_t tx_need_empty_packet; // used to flush the USB IN endpoint if the last packet was exactly the endpoint packet size
 
     volatile uint8_t connect_state; // indicates if we are connected