Skip to content

Commit

Permalink
fix(esp_ringbuf): Fixed a bug where in a no-split buffer received ite…
Browse files Browse the repository at this point in the history
…ms 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 #14568
  • Loading branch information
sudeep-mohanty committed Sep 18, 2024
1 parent 5c43055 commit 6a82497
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 3 deletions.
10 changes: 7 additions & 3 deletions components/esp_ringbuf/ringbuf.c
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,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
Expand Down Expand Up @@ -979,9 +986,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
}

return prvSendAcquireGeneric(pxRingbuffer, NULL, ppvItem, xItemSize, xTicksToWait);
}
Expand Down
81 changes: 81 additions & 0 deletions components/esp_ringbuf/test_apps/main/test_ringbuf_common.c
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,8 @@ size_t continuous_test_string_len(void)
return sizeof(continuous_data);
}

#if !CONFIG_FREERTOS_UNICORE

void send_to_buffer(RingbufHandle_t buffer, size_t max_item_size)
{
for (int iter = 0; iter < SMP_TEST_ITERATIONS; iter++) {
Expand Down Expand Up @@ -916,6 +918,8 @@ TEST_CASE("Test static ring buffer SMP", "[esp_ringbuf][linux]")
}
#endif

#endif //!CONFIG_FREERTOS_UNICORE

/* ------------------------ Test ring buffer 0 Item Size -----------------------
* The following test case tests that sending/acquiring an item/bytes of 0 size
* is permissible.
Expand Down Expand Up @@ -961,3 +965,80 @@ TEST_CASE("Test ringbuffer 0 item size", "[esp_ringbuf][linux]")
vRingbufferDelete(allow_split_rb);
vRingbufferDelete(byte_rb);
}

/* ---------------------------- 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][linux]")
{
// 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);
}

0 comments on commit 6a82497

Please sign in to comment.