diff --git a/docs/reference/class/audio.rst b/docs/reference/class/audio.rst index f2ca5178b2..f551256a04 100644 --- a/docs/reference/class/audio.rst +++ b/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. diff --git a/docs/reference/class/cdc.rst b/docs/reference/class/cdc.rst index 138ebdf3f5..1f1224b449 100644 --- a/docs/reference/class/cdc.rst +++ b/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. diff --git a/src/common/tusb_fifo.c b/src/common/tusb_fifo.c index e5855fcbc0..7f100d97b3 100644 --- a/src/common/tusb_fifo.c +++ b/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; } diff --git a/src/common/tusb_fifo.h b/src/common/tusb_fifo.h index c5a0aaaef8..f56fb53f99 100644 --- a/src/common/tusb_fifo.h +++ b/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 diff --git a/test/unit-test/test/test_fifo.c b/test/unit-test/test/test_fifo.c index 30420a5279..c0f82bdf86 100644 --- a/test/unit-test/test/test_fifo.c +++ b/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) {