fifo: keep overwritable fifo full and ordered on double overflow

docs/reference/class/audio.rst

@@ -93,7 +93,8 @@ Data path
    * - ``tud_audio_write()``
      - Copies microphone bytes into the IN software FIFO and returns the number
        written.  When the FIFO is full the oldest unsent bytes are overwritten,
-       so pace writes to the stream.
+       so pace writes to the stream.  An overwrite can tear bytes the endpoint
+       is copying out at that moment (partly old, partly new data).
    * - ``tud_audio_clear_ep_*_ff()``
      - Discards queued samples in the selected endpoint FIFO, useful when a
        streaming alternate setting closes.

docs/reference/class/cdc.rst

@@ -45,7 +45,10 @@ Enable ``CFG_TUD_CDC`` with the required port count and add one
    * - ``CFG_TUD_CDC_TX_OVERWRITABLE_IF_NOT_CONNECTED``
      - ``1``
      - Allows writes made before DTR connection to replace old queued data
-       rather than permanently filling the FIFO.
+       rather than permanently filling the FIFO.  When the FIFO overflows the
+       oldest bytes are dropped, and bytes the endpoint is copying out at that
+       moment can be torn (partly old, partly new data); this affects only
+       data written while DTR is clear.
 
 Use speed-dependent values from an example when the device can enumerate at
 high speed.

src/common/tusb_fifo.c

@@ -411,6 +411,17 @@ static uint16_t advance_index(uint16_t depth, uint16_t idx, uint16_t offset) {
   return new_idx;
 }
 
+// Advance write index by n after data is written. In overwritable mode, if the write would make the overflow count
+// reach 2*depth (double overflow), the index would alias to an empty fifo. Drop one depth from it instead: the
+// write pointer is unchanged and the fifo is still full, holding the last depth items in order.
+static uint16_t advance_write_index(uint16_t depth, uint16_t wr_idx, uint16_t rd_idx, uint16_t n) {
+  const uint16_t new_idx = advance_index(depth, wr_idx, n);
+  if (tu_ff_overflow_count(depth, wr_idx, rd_idx) + n >= 2 * depth) {
+    return advance_index(depth, new_idx, depth);
+  }
+  return new_idx;
+}
+
 // index to pointer (0..depth-1), simply a modulo with minus.
 TU_ATTR_ALWAYS_INLINE static inline uint16_t idx2ptr(uint16_t depth, uint16_t idx) {
   // Only run at most 3 times since index is limit in the range of [0..2*depth)
@@ -534,25 +545,9 @@ uint16_t tu_fifo_write_n_access_mode(tu_fifo_t *f, const void *data, uint16_t n,
 
       // We start writing at the read pointer's position since we fill the whole buffer
       wr_idx = rd_idx;
-    } else {
-      const uint16_t overflowable_count = tu_ff_overflow_count(f->depth, wr_idx, rd_idx);
-      if (overflowable_count + n >= 2 * f->depth) {
-        // Double overflowed
-        // Index is bigger than the allowed range [0,2*depth)
-        // re-position write index to have a full fifo after pushed
-        wr_idx = advance_index(f->depth, rd_idx, f->depth - n);
-
-        // TODO we should also shift out n bytes from read index since we avoid changing rd index !!
-        // However memmove() is expensive due to actual copying + wrapping consideration.
-        // Also race condition could happen anyway if read() is invoke while moving result in corrupted memory
-        // currently deliberately not implemented --> result in incorrect data read back
-      } else {
-        // normal + single overflowed:
-        // Index is in the range of [0,2*depth) and thus detect and recoverable. Recovering is handled in read()
-        // Therefore we just increase write index
-        // we will correct (re-position) read index later on in fifo_read() function
-      }
     }
+    // Otherwise (n < depth) the data is written at the write index. Single overflow is recoverable (handled in
+    // read(), which re-positions the read index), double overflow is handled by advance_write_index()
   }
 
   if (n) {
@@ -567,7 +562,7 @@ uint16_t tu_fifo_write_n_access_mode(tu_fifo_t *f, const void *data, uint16_t n,
     {
       ff_push_n(f, buf8, n, wr_ptr);
     }
-    f->wr_idx = advance_index(f->depth, wr_idx, n);
+    f->wr_idx = advance_write_index(f->depth, wr_idx, rd_idx, n);
 
     TU_LOG(TU_FIFO_DBG, "\tnew_wr = %u\r\n", f->wr_idx);
   }
@@ -645,7 +640,7 @@ bool tu_fifo_write(tu_fifo_t *f, const void *data) {
   } else {
     const uint16_t wr_ptr = idx2ptr(f->depth, wr_idx);
     memcpy(f->buffer + wr_ptr, data, 1);
-    f->wr_idx = advance_index(f->depth, wr_idx, 1);
+    f->wr_idx = advance_write_index(f->depth, wr_idx, f->rd_idx, 1);
     ret       = true;
   }
 

src/common/tusb_fifo.h

@@ -80,22 +80,17 @@ extern "C" {
  *    This occurs when we continue to write after 1st overflowed to 2nd overflowed. e.g:
  *      write(3), write(1), write(2)
  *    This must be prevented since it will cause unrecoverable state, in above example
- *    if not handled the fifo will be empty instead of continue-to-be full. Since we must not modify
- *    read index in write() function, which cause race condition. We will re-position write index so that
- *    after data is written it is a full fifo i.e W = depth - R
+ *    if not handled the write index catches up with the read index and the fifo reads as empty instead of
+ *    continue-to-be full. Since we must not modify read index in write() function, which cause race condition,
+ *    the data is written at the write index as usual, then the write index is moved back by one depth i.e it
+ *    still points to the same location. The fifo is full and holds the last depth items in order.
  *
- *      re-position W = 1 before write(2)
- *      Note: we should also move data from mem[3] to read index as well, but deliberately skipped here
- *      since it is an expensive operation !!!
- *                  |
- *      -------------------------
- *      | R | W | 2 | 3 | 4 | 5 |
- *
- *      perform write(2), result is still a full fifo.
- *
- *                  |
- *      -------------------------
- *      | R | 1 | 2 | W | 4 | 5 |
+ * Overwritable mode and concurrency: an overwriting write() only holds the write mutex, and it may overwrite items
+ * that a concurrent read()/peek() (holding the read mutex or running in another context e.g. ISR) is copying out.
+ * The data read in that case can be torn i.e a mix of old and new items. The write index is also derived from a
+ * snapshot of the read index, so if read()/peek() re-positions the read index meanwhile, the item count can end up
+ * wrong and some items may be skipped or replayed. A write that does not overflow is not affected.
+ * Use overwritable mode only when losing/tearing the oldest data on overflow is acceptable.
  */
 typedef struct {
   uint8_t *buffer;              // buffer pointer

test/unit-test/test/test_fifo.c

@@ -150,8 +150,61 @@ void test_write_double_overflowed(void) {
 
   TEST_ASSERT_EQUAL_MEMORY(buf - 16, rd_buf + FIFO_SIZE - 16, 16);
 
-  // TODO whole buffer should match, but we deliberately not implement it
-  // TEST_ASSERT_EQUAL_MEMORY(buf-FIFO_SIZE, rd_buf, FIFO_SIZE);
+  // whole buffer holds the last FIFO_SIZE bytes written, in order
+  TEST_ASSERT_EQUAL_MEMORY(buf - FIFO_SIZE, rd_buf, FIFO_SIZE);
+}
+
+void test_write_overwritable_single_byte(void) {
+  tu_fifo_set_overwritable(ff, true);
+
+  // keep writing one byte at a time, never reading: the fifo must stay full across several index wraps
+  for (uint16_t i = 0; i < 3 * FIFO_SIZE + 5; i++) {
+    TEST_ASSERT_TRUE(tu_fifo_write(ff, &test_data[i]));
+    TEST_ASSERT_EQUAL(tu_min16(i + 1, FIFO_SIZE), tu_fifo_count(ff));
+  }
+
+  // oldest bytes are gone, the last FIFO_SIZE bytes are read back in order
+  uint8_t rd[FIFO_SIZE] = {0};
+  TEST_ASSERT_EQUAL(FIFO_SIZE, tu_fifo_read_n(ff, rd, FIFO_SIZE));
+  TEST_ASSERT_EQUAL_MEMORY(test_data + 2 * FIFO_SIZE + 5, rd, FIFO_SIZE);
+  TEST_ASSERT_TRUE(tu_fifo_empty(ff));
+}
+
+void test_write_overwritable_mixed(void) {
+  tu_fifo_set_overwritable(ff, true);
+
+  // deterministic mix of single and bulk writes, checked against the last FIFO_SIZE bytes.
+  // Each write is below depth so double overflows are not masked by whole-buffer rewrites.
+  uint8_t  stream[4 * FIFO_SIZE * 64];
+  uint32_t total = 0;
+  uint32_t seed  = 12345;
+
+  for (int iter = 0; iter < 2000; iter++) {
+    seed       = seed * 1103515245u + 12345u;
+    uint16_t n = (seed >> 16) % FIFO_SIZE;
+    for (uint16_t i = 0; i < n; i++) {
+      stream[total + i] = (uint8_t)(seed >> (i % 16)) ^ (uint8_t)i;
+    }
+    if (n == 1) {
+      TEST_ASSERT_TRUE(tu_fifo_write(ff, &stream[total]));
+    } else {
+      TEST_ASSERT_EQUAL(n, tu_fifo_write_n(ff, &stream[total], n));
+    }
+    total += n;
+
+    const uint16_t expected = tu_min16(total, FIFO_SIZE);
+    TEST_ASSERT_EQUAL(expected, tu_fifo_count(ff));
+
+    if (iter % 5 == 0) {
+      uint8_t rd[FIFO_SIZE] = {0};
+      TEST_ASSERT_EQUAL(expected, tu_fifo_peek_n(ff, rd, FIFO_SIZE));
+      TEST_ASSERT_EQUAL_MEMORY(stream + total - expected, rd, expected);
+    }
+
+    if (total + FIFO_SIZE > sizeof(stream)) {
+      break;
+    }
+  }
 }
 
 static uint16_t help_write(uint16_t total, uint16_t n) {