From 645dc8addf58bbfcda7a5a061a96f3eed87a7e67 Mon Sep 17 00:00:00 2001 From: Sudeep Mohanty Date: Mon, 16 Sep 2024 13:10:42 +0200 Subject: [PATCH 1/2] 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 8f0f7af619..54baf8126e 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 e048d3c9f2..2781a5bf19 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); +} From d07c700af88e1039f0d4b3cbfd7aebc84affb06c Mon Sep 17 00:00:00 2001 From: Sudeep Mohanty Date: Fri, 20 Sep 2024 10:14:12 +0200 Subject: [PATCH 2/2] test(freertos): Added delay to freertos test to avoid memory leaks This commit adds a small delay as tolerance to the "Test xTaskResumeAll resumes pended tasks" test to let the idle task clean up all suspended test tasks and avoid memory leaks at the end of the test. --- .../tasks/test_vTaskSuspendAll_xTaskResumeAll.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/components/freertos/test/integration/tasks/test_vTaskSuspendAll_xTaskResumeAll.c b/components/freertos/test/integration/tasks/test_vTaskSuspendAll_xTaskResumeAll.c index 4ef4f081e4..f5d62c1c79 100644 --- a/components/freertos/test/integration/tasks/test_vTaskSuspendAll_xTaskResumeAll.c +++ b/components/freertos/test/integration/tasks/test_vTaskSuspendAll_xTaskResumeAll.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -487,6 +487,9 @@ static void test_pended_running_task(void *arg) TEST_ASSERT_EQUAL(true, has_run[i]); } + // Short delay to let the tasks be suspended + vTaskDelay(10); + // Clean up the interrupt and tasks deregister_intr_cb(); for (int i = 0; i < TEST_PENDED_NUM_BLOCKED_TASKS; i++) { @@ -506,6 +509,8 @@ TEST_CASE("Test xTaskResumeAll resumes pended tasks", "[freertos]") TEST_ASSERT_EQUAL(pdTRUE, xTaskCreatePinnedToCore(test_pended_running_task, "susp", 2048, (void *)xTaskGetCurrentTaskHandle(), UNITY_FREERTOS_PRIORITY + 1, &susp_tsk_hdl, i)); // Wait for to be notified to test completion ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + // Short delay to let the task be suspended + vTaskDelay(10); vTaskDelete(susp_tsk_hdl); } // Add a short delay to allow the idle task to free any remaining task memory