From 645dc8addf58bbfcda7a5a061a96f3eed87a7e67 Mon Sep 17 00:00:00 2001 From: Sudeep Mohanty Date: Mon, 16 Sep 2024 13:10:42 +0200 Subject: [PATCH] fix(esp_ringbuf): Fixed a bug where in a no-split buffer received items prematurely This commit fixes a bug in the no-split buffer which could receive an item prematurely if the space on the buffer is acquired until the buffer is full. The commit also adds a unit test for this scenario. Closes https://github.com/espressif/esp-idf/issues/14568 --- components/esp_ringbuf/ringbuf.c | 10 ++- components/esp_ringbuf/test/test_ringbuf.c | 84 +++++++++++++++++++++- 2 files changed, 89 insertions(+), 5 deletions(-) diff --git a/components/esp_ringbuf/ringbuf.c b/components/esp_ringbuf/ringbuf.c index 8f0f7af6192..54baf8126e5 100644 --- a/components/esp_ringbuf/ringbuf.c +++ b/components/esp_ringbuf/ringbuf.c @@ -510,6 +510,13 @@ static BaseType_t prvCheckItemAvail(Ringbuffer_t *pxRingbuffer) return pdFALSE; //Byte buffers do not allow multiple retrievals before return } if ((pxRingbuffer->xItemsWaiting > 0) && ((pxRingbuffer->pucRead != pxRingbuffer->pucWrite) || (pxRingbuffer->uxRingbufferFlags & rbBUFFER_FULL_FLAG))) { + // If the ring buffer is a no-split buffer, the read pointer must point to an item that has been written to. + if ((pxRingbuffer->uxRingbufferFlags & (rbBYTE_BUFFER_FLAG | rbALLOW_SPLIT_FLAG)) == 0) { + ItemHeader_t *pxHeader = (ItemHeader_t *)pxRingbuffer->pucRead; + if ((pxHeader->uxItemFlags & rbITEM_WRITTEN_FLAG) == 0) { + return pdFALSE; + } + } return pdTRUE; //Items/data available for retrieval } else { return pdFALSE; //No items/data available for retrieval @@ -926,9 +933,6 @@ BaseType_t xRingbufferSendAcquire(RingbufHandle_t xRingbuffer, void **ppvItem, s if (xItemSize > pxRingbuffer->xMaxItemSize) { return pdFALSE; //Data will never ever fit in the queue. } - if ((pxRingbuffer->uxRingbufferFlags & rbBYTE_BUFFER_FLAG) && xItemSize == 0) { - return pdTRUE; //Sending 0 bytes to byte buffer has no effect - } //Attempt to send an item BaseType_t xReturn = pdFALSE; diff --git a/components/esp_ringbuf/test/test_ringbuf.c b/components/esp_ringbuf/test/test_ringbuf.c index e048d3c9f24..2781a5bf190 100644 --- a/components/esp_ringbuf/test/test_ringbuf.c +++ b/components/esp_ringbuf/test/test_ringbuf.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -804,6 +804,8 @@ TEST_CASE("Test ring buffer ISR", "[esp_ringbuf]") * tested. */ +#if !CONFIG_FREERTOS_UNICORE + #define SRAND_SEED 3 //Arbitrarily chosen srand() seed #define SMP_TEST_ITERATIONS 4 @@ -1018,6 +1020,7 @@ TEST_CASE("Test static ring buffer SMP", "[esp_ringbuf]") cleanup(); } #endif +#endif //!CONFIG_FREERTOS_UNICORE #if !CONFIG_RINGBUF_PLACE_FUNCTIONS_INTO_FLASH && !CONFIG_RINGBUF_PLACE_ISR_FUNCTIONS_INTO_FLASH /* -------------------------- Test ring buffer IRAM ------------------------- */ @@ -1027,7 +1030,7 @@ static IRAM_ATTR __attribute__((noinline)) bool iram_ringbuf_test(void) bool result = true; uint8_t item[4]; size_t item_size; - RingbufHandle_t handle = xRingbufferCreate(CONT_DATA_TEST_BUFF_LEN, RINGBUF_TYPE_NOSPLIT); + RingbufHandle_t handle = xRingbufferCreate(BUFFER_SIZE, RINGBUF_TYPE_NOSPLIT); result = result && (handle != NULL); spi_flash_guard_get()->start(); // Disables flash cache @@ -1046,3 +1049,80 @@ TEST_CASE("Test ringbuffer functions work with flash cache disabled", "[esp_ring TEST_ASSERT( iram_ringbuf_test() ); } #endif /* !CONFIG_RINGBUF_PLACE_FUNCTIONS_INTO_FLASH && !CONFIG_RINGBUF_PLACE_ISR_FUNCTIONS_INTO_FLASH */ + +/* ---------------------------- Test no-split ring buffer SendAquire and SendComplete --------------------------- + * The following test case tests the SendAquire and SendComplete functions of the no-split ring buffer. + * + * The test case will do the following... + * 1) Create a no-split ring buffer. + * 2) Acquire space on the buffer to send an item. + * 3) Send the item to the buffer. + * 4) Verify that the item is received correctly. + * 5) Acquire space on the buffer until the buffer is full. + * 6) Send the items out-of-order to the buffer. + * 7) Verify that the items are not received until the first item is sent. + * 8) Send the first item. + * 9) Verify that the items are received in the correct order. + */ +TEST_CASE("Test no-split buffers always receive items in order", "[esp_ringbuf]") +{ + // Create buffer + RingbufHandle_t buffer_handle = xRingbufferCreate(BUFFER_SIZE, RINGBUF_TYPE_NOSPLIT); + TEST_ASSERT_MESSAGE(buffer_handle != NULL, "Failed to create ring buffer"); + + // Acquire space on the buffer to send an item and write to the item + void *item1; + TEST_ASSERT_EQUAL(pdTRUE, xRingbufferSendAcquire(buffer_handle, &item1, MEDIUM_ITEM_SIZE, TIMEOUT_TICKS)); + *(uint32_t *)item1 = 0x123; + + // Send the item to the buffer + TEST_ASSERT_EQUAL(pdTRUE, xRingbufferSendComplete(buffer_handle, item1)); + + // Verify that the item is received correctly + size_t item_size; + uint32_t *received_item = xRingbufferReceive(buffer_handle, &item_size, TIMEOUT_TICKS); + TEST_ASSERT_NOT_NULL(received_item); + TEST_ASSERT_EQUAL(item_size, MEDIUM_ITEM_SIZE); + TEST_ASSERT_EQUAL(*(uint32_t *)received_item, 0x123); + + // Return the space to the buffer after receiving the item + vRingbufferReturnItem(buffer_handle, received_item); + + // At this point, the buffer should be empty + UBaseType_t items_waiting; + vRingbufferGetInfo(buffer_handle, NULL, NULL, NULL, NULL, &items_waiting); + TEST_ASSERT_MESSAGE(items_waiting == 0, "Incorrect items waiting"); + + // Acquire space on the buffer until the buffer is full +#define MAX_NUM_ITEMS ( BUFFER_SIZE / ( MEDIUM_ITEM_SIZE + ITEM_HDR_SIZE ) ) + void *items[MAX_NUM_ITEMS]; + for (int i = 0; i < MAX_NUM_ITEMS; i++) { + TEST_ASSERT_EQUAL(pdTRUE, xRingbufferSendAcquire(buffer_handle, &items[i], MEDIUM_ITEM_SIZE, TIMEOUT_TICKS)); + TEST_ASSERT_NOT_NULL(items[i]); + *(uint32_t *)items[i] = (0x100 + i); + } + + // Verify that the buffer is full by attempting to acquire space for another item + void *another_item; + TEST_ASSERT_EQUAL(pdFALSE, xRingbufferSendAcquire(buffer_handle, &another_item, MEDIUM_ITEM_SIZE, TIMEOUT_TICKS)); + + // Send the items out-of-order to the buffer. Verify that the items are not received until the first item is sent. + // In this case, we send the items in the reverse order until the first item is sent. + for (int i = MAX_NUM_ITEMS - 1; i > 0; i--) { + TEST_ASSERT_EQUAL(pdTRUE, xRingbufferSendComplete(buffer_handle, items[i])); + TEST_ASSERT_NULL(xRingbufferReceive(buffer_handle, &item_size, 0)); + } + + // Send the first item + TEST_ASSERT_EQUAL(pdTRUE, xRingbufferSendComplete(buffer_handle, items[0])); + + // Verify that the items are received in the correct order + for (int i = 0; i < MAX_NUM_ITEMS; i++) { + received_item = xRingbufferReceive(buffer_handle, &item_size, TIMEOUT_TICKS); + TEST_ASSERT_EQUAL(*(uint32_t *)received_item, (0x100 + i)); + vRingbufferReturnItem(buffer_handle, received_item); + } + + // Cleanup + vRingbufferDelete(buffer_handle); +}