mirror of
https://github.com/espressif/esp-idf.git
synced 2025-10-02 18:10:57 +02:00
Merge branch 'fix/esp_ringbuf_nosplit_full_return_one_v5.4' into 'release/v5.4'
feat(esp_ringbuf): Fixes full no-split buffer frees item incorrectly (v5.4) See merge request espressif/esp-idf!41931
This commit is contained in:
Marius Vikhammer
@@ -1,5 +1,5 @@
|
||||
/*
|
||||
* SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD
|
||||
* SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD
|
||||
*
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
@@ -629,8 +629,21 @@ static void prvReturnItemDefault(Ringbuffer_t *pxRingbuffer, uint8_t *pucItem)
|
||||
* freed or items with dummy data should be skipped over
|
||||
*/
|
||||
pxCurHeader = (ItemHeader_t *)pxRingbuffer->pucFree;
|
||||
|
||||
// allow a single advancement either when buffer is FULL (to release the oldest block)
|
||||
// or when buffer is EMPTY (to skip a FREE block sitting at the head)
|
||||
BaseType_t allow_advance_free_pointer =
|
||||
((pxRingbuffer->uxRingbufferFlags & rbBUFFER_FULL_FLAG) ? pdTRUE : pdFALSE) ||
|
||||
((pxRingbuffer->xItemsWaiting == 0) ? pdTRUE : pdFALSE);
|
||||
BaseType_t freed_any = pdFALSE;
|
||||
|
||||
//Skip over Items that have already been freed or are dummy items
|
||||
while (((pxCurHeader->uxItemFlags & rbITEM_FREE_FLAG) || (pxCurHeader->uxItemFlags & rbITEM_DUMMY_DATA_FLAG)) && pxRingbuffer->pucFree != pxRingbuffer->pucRead) {
|
||||
while (((pxCurHeader->uxItemFlags & (rbITEM_FREE_FLAG | rbITEM_DUMMY_DATA_FLAG)) != 0) &&
|
||||
((pxRingbuffer->pucFree != pxRingbuffer->pucRead) || allow_advance_free_pointer)) {
|
||||
|
||||
allow_advance_free_pointer = pdFALSE;
|
||||
freed_any = pdTRUE;
|
||||
|
||||
if (pxCurHeader->uxItemFlags & rbITEM_DUMMY_DATA_FLAG) {
|
||||
pxCurHeader->uxItemFlags |= rbITEM_FREE_FLAG; //Mark as freed (not strictly necessary but adds redundancy)
|
||||
pxRingbuffer->pucFree = pxRingbuffer->pucHead; //Wrap around due to dummy data
|
||||
@@ -648,14 +661,9 @@ static void prvReturnItemDefault(Ringbuffer_t *pxRingbuffer, uint8_t *pucItem)
|
||||
pxCurHeader = (ItemHeader_t *)pxRingbuffer->pucFree; //Update header to point to item
|
||||
}
|
||||
|
||||
//Check if the buffer full flag should be reset
|
||||
if (pxRingbuffer->uxRingbufferFlags & rbBUFFER_FULL_FLAG) {
|
||||
if (pxRingbuffer->pucFree != pxRingbuffer->pucAcquire) {
|
||||
pxRingbuffer->uxRingbufferFlags &= ~rbBUFFER_FULL_FLAG;
|
||||
} else if (pxRingbuffer->pucFree == pxRingbuffer->pucAcquire && pxRingbuffer->pucFree == pxRingbuffer->pucRead) {
|
||||
//Special case where a full buffer is completely freed in one go
|
||||
pxRingbuffer->uxRingbufferFlags &= ~rbBUFFER_FULL_FLAG;
|
||||
}
|
||||
// clear FULL only if we actually freed something
|
||||
if ((pxRingbuffer->uxRingbufferFlags & rbBUFFER_FULL_FLAG) && freed_any) {
|
||||
pxRingbuffer->uxRingbufferFlags &= ~rbBUFFER_FULL_FLAG;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1356,12 +1364,80 @@ void xRingbufferPrintInfo(RingbufHandle_t xRingbuffer)
|
||||
{
|
||||
Ringbuffer_t *pxRingbuffer = (Ringbuffer_t *)xRingbuffer;
|
||||
configASSERT(pxRingbuffer);
|
||||
printf("Rb size:%" PRId32 "\tfree: %" PRId32 "\trptr: %" PRId32 "\tfreeptr: %" PRId32 "\twptr: %" PRId32 ", aptr: %" PRId32 "\n",
|
||||
(int32_t)pxRingbuffer->xSize, (int32_t)prvGetFreeSize(pxRingbuffer),
|
||||
|
||||
uint32_t RingbufferFlags = pxRingbuffer->uxRingbufferFlags;
|
||||
printf("RingBuffer Size: %" PRId32 ", FreeSize: %" PRId32 "\n"
|
||||
" Read: %" PRId32 ", Free: %" PRId32 ", Write: %" PRId32 ", Acquire: %" PRId32 ", Waiting: %" PRId32 ", Flags: 0x%" PRIx32 " [",
|
||||
(int32_t)pxRingbuffer->xSize,
|
||||
(int32_t)prvGetFreeSize(pxRingbuffer),
|
||||
(int32_t)(pxRingbuffer->pucRead - pxRingbuffer->pucHead),
|
||||
(int32_t)(pxRingbuffer->pucFree - pxRingbuffer->pucHead),
|
||||
(int32_t)(pxRingbuffer->pucWrite - pxRingbuffer->pucHead),
|
||||
(int32_t)(pxRingbuffer->pucAcquire - pxRingbuffer->pucHead));
|
||||
(int32_t)(pxRingbuffer->pucAcquire - pxRingbuffer->pucHead),
|
||||
(int32_t)pxRingbuffer->xItemsWaiting,
|
||||
RingbufferFlags);
|
||||
|
||||
if (RingbufferFlags) {
|
||||
if (RingbufferFlags & rbALLOW_SPLIT_FLAG) {
|
||||
printf(" [ALLOW_SPLIT]");
|
||||
}
|
||||
if (RingbufferFlags & rbBYTE_BUFFER_FLAG) {
|
||||
printf(" [BYTE_BUFFER]");
|
||||
}
|
||||
if (RingbufferFlags & rbBUFFER_FULL_FLAG) {
|
||||
printf(" [FULL]");
|
||||
}
|
||||
if (RingbufferFlags & rbBUFFER_STATIC_FLAG) {
|
||||
printf(" [STATIC]");
|
||||
}
|
||||
if (RingbufferFlags & rbUSING_QUEUE_SET) {
|
||||
printf(" [USING_QUEUE_SET]");
|
||||
}
|
||||
}
|
||||
printf(" ]\n Items:\n");
|
||||
|
||||
uint8_t *Head = pxRingbuffer->pucHead;
|
||||
uint8_t *ptr = Head;
|
||||
size_t max_steps = (pxRingbuffer->xSize / rbHEADER_SIZE) + 4;
|
||||
for (size_t steps = 0; ptr < pxRingbuffer->pucTail && steps < max_steps; ++steps) {
|
||||
unsigned offset = ptr - Head;
|
||||
ItemHeader_t *hdr = (ItemHeader_t *)ptr;
|
||||
unsigned ItemFlags = hdr->uxItemFlags;
|
||||
unsigned ItemLen = hdr->xItemLen;
|
||||
printf(" [%4u] Size: %u Flags: 0x%x [", offset, ItemLen, ItemFlags);
|
||||
if (ItemLen > pxRingbuffer->xMaxItemSize && !(ItemFlags & rbITEM_DUMMY_DATA_FLAG)) {
|
||||
printf(" [INVALID HEADER or UNUSED SPACE]\n");
|
||||
break;
|
||||
}
|
||||
if (ItemFlags) {
|
||||
if (ItemFlags & rbITEM_FREE_FLAG) {
|
||||
printf(" [FREE]");
|
||||
}
|
||||
if (ItemFlags & rbITEM_DUMMY_DATA_FLAG) {
|
||||
printf(" [DUMMY]");
|
||||
}
|
||||
if (ItemFlags & rbITEM_SPLIT_FLAG) {
|
||||
printf(" [SPLIT]");
|
||||
}
|
||||
if (ItemFlags & rbITEM_WRITTEN_FLAG) {
|
||||
printf(" [WRITTEN]");
|
||||
}
|
||||
}
|
||||
printf(" ]\n");
|
||||
|
||||
if (ItemFlags & rbITEM_DUMMY_DATA_FLAG) {
|
||||
ptr = Head;
|
||||
} else {
|
||||
size_t step = rbHEADER_SIZE + rbALIGN_SIZE(ItemLen);
|
||||
if (step == 0) {
|
||||
break;
|
||||
}
|
||||
ptr += step;
|
||||
}
|
||||
if (ptr == Head) {
|
||||
break; // full circle
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
BaseType_t xRingbufferGetStaticBuffer(RingbufHandle_t xRingbuffer, uint8_t **ppucRingbufferStorage, StaticRingbuffer_t **ppxStaticRingbuffer)
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
/*
|
||||
* SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD
|
||||
* SPDX-FileCopyrightText: 2024-2025 Espressif Systems (Shanghai) CO LTD
|
||||
*
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
@@ -12,6 +12,7 @@
|
||||
#include "sdkconfig.h"
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
#include "freertos/FreeRTOS.h"
|
||||
#include "freertos/task.h"
|
||||
#include "freertos/queue.h"
|
||||
@@ -1096,12 +1097,12 @@ TEST_CASE("Test no-split buffers can receive items if the acquire pointer wraps
|
||||
}
|
||||
|
||||
// At this point, the buffer should be full and the acquire pointer must have wrapped around
|
||||
UBaseType_t *uxFree = NULL;
|
||||
UBaseType_t *uxRead = NULL;
|
||||
UBaseType_t *uxWrite = NULL;
|
||||
UBaseType_t *uxAcquire = NULL;
|
||||
vRingbufferGetInfo(buffer_handle, uxFree, uxRead, uxWrite, uxAcquire, &items_waiting);
|
||||
TEST_ASSERT_EQUAL(uxAcquire, uxWrite);
|
||||
xRingbufferPrintInfo(buffer_handle);
|
||||
UBaseType_t uFree, uRead, uWrite, uAcquire;
|
||||
vRingbufferGetInfo(buffer_handle, &uFree, &uRead, &uWrite, &uAcquire, &items_waiting);
|
||||
TEST_ASSERT_EQUAL_UINT32(uAcquire, uWrite);
|
||||
TEST_ASSERT_EQUAL_UINT32(uAcquire, uRead);
|
||||
TEST_ASSERT_EQUAL_UINT32(uAcquire, uFree);
|
||||
TEST_ASSERT_EQUAL(0U, xRingbufferGetCurFreeSize(buffer_handle));
|
||||
|
||||
// Send the items out-of-order to the buffer. Verify that the items are not received until the first item is sent.
|
||||
@@ -1124,3 +1125,196 @@ TEST_CASE("Test no-split buffers can receive items if the acquire pointer wraps
|
||||
// Cleanup
|
||||
vRingbufferDelete(buffer_handle);
|
||||
}
|
||||
|
||||
static void acquire_all_items(RingbufHandle_t buffer_handle, void **items, int capacity)
|
||||
{
|
||||
// Acquire all items with SendAcquire
|
||||
for (int i = 0; i < capacity; i++) {
|
||||
printf("Acquire %d item\n", i);
|
||||
TEST_ASSERT_EQUAL(pdTRUE, xRingbufferSendAcquire(buffer_handle, &items[i], MEDIUM_ITEM_SIZE, 0));
|
||||
TEST_ASSERT_NOT_NULL(items[i]);
|
||||
}
|
||||
}
|
||||
|
||||
static void write_and_send_all_items(RingbufHandle_t buffer_handle, void **items, int capacity, uint8_t data[])
|
||||
{
|
||||
// Write to all acquired items and send them with SendComplete
|
||||
for (int i = 0; i < capacity; i++) {
|
||||
printf("Write and Send %d item\n", i);
|
||||
memset(items[i], data[i], MEDIUM_ITEM_SIZE);
|
||||
TEST_ASSERT_EQUAL(pdTRUE, xRingbufferSendComplete(buffer_handle, items[i]));
|
||||
}
|
||||
}
|
||||
|
||||
static void receive_all_items(RingbufHandle_t buffer_handle, void **rx_items, int capacity)
|
||||
{
|
||||
// Receive all items
|
||||
for (int i = 0; i < capacity; i++) {
|
||||
printf("Receive %d item\n", i);
|
||||
size_t item_size;
|
||||
rx_items[i] = xRingbufferReceive(buffer_handle, &item_size, 0);
|
||||
TEST_ASSERT_NOT_NULL(rx_items[i]);
|
||||
TEST_ASSERT_EQUAL(MEDIUM_ITEM_SIZE, item_size);
|
||||
}
|
||||
}
|
||||
|
||||
static void read_and_return_item(RingbufHandle_t buffer_handle, void **rx_items, int index, uint8_t data)
|
||||
{
|
||||
TEST_ASSERT_NOT_NULL(rx_items[index]);
|
||||
TEST_ASSERT_EQUAL(((uint8_t *)rx_items[index])[0], data);
|
||||
vRingbufferReturnItem(buffer_handle, rx_items[index]);
|
||||
}
|
||||
|
||||
static void read_received_all_items(RingbufHandle_t buffer_handle, void **rx_items, int capacity, uint8_t data[])
|
||||
{
|
||||
for (int i = 0; i < capacity; i++) {
|
||||
printf("Read and return %d item\n", i);
|
||||
read_and_return_item(buffer_handle, rx_items, i, data[i]);
|
||||
}
|
||||
}
|
||||
|
||||
TEST_CASE("Test no-split buffers returning oldest item when full frees one slot", "[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");
|
||||
const int capacity = BUFFER_SIZE / (ITEM_HDR_SIZE + MEDIUM_ITEM_SIZE);
|
||||
TEST_ASSERT_EQUAL(4, capacity);
|
||||
void *rx_items[capacity];
|
||||
uint8_t data[] = {10, 11, 12, 13};
|
||||
|
||||
acquire_all_items(buffer_handle, rx_items, capacity);
|
||||
write_and_send_all_items(buffer_handle, rx_items, capacity, data);
|
||||
receive_all_items(buffer_handle, rx_items, capacity);
|
||||
|
||||
const int free_idx = 0;
|
||||
printf("-- Read and returning only %d item\n", free_idx);
|
||||
TEST_ASSERT_EQUAL(((uint8_t *)rx_items[free_idx])[0], data[free_idx]);
|
||||
vRingbufferReturnItem(buffer_handle, rx_items[free_idx]);
|
||||
xRingbufferPrintInfo(buffer_handle);
|
||||
|
||||
printf("-- Verify that only the first item is returned.\n");
|
||||
TEST_ASSERT_EQUAL(pdTRUE, xRingbufferSendAcquire(buffer_handle, &rx_items[free_idx], MEDIUM_ITEM_SIZE, 0));
|
||||
xRingbufferPrintInfo(buffer_handle);
|
||||
data[free_idx] = 20;
|
||||
memset(rx_items[free_idx], data[free_idx], MEDIUM_ITEM_SIZE);
|
||||
TEST_ASSERT_EQUAL(pdTRUE, xRingbufferSendComplete(buffer_handle, rx_items[free_idx]));
|
||||
|
||||
printf("-- Attempting to acquire a second item should fail, as expected.\n");
|
||||
TEST_ASSERT_EQUAL(pdFALSE, xRingbufferSendAcquire(buffer_handle, &rx_items[free_idx], MEDIUM_ITEM_SIZE, 0));
|
||||
|
||||
printf("-- Receive the first item second time, making it available for reading\n");
|
||||
size_t item_size;
|
||||
rx_items[free_idx] = xRingbufferReceive(buffer_handle, &item_size, 0);
|
||||
TEST_ASSERT_NOT_NULL(rx_items[free_idx]);
|
||||
TEST_ASSERT_EQUAL(((uint8_t *)rx_items[free_idx])[0], data[free_idx]);
|
||||
|
||||
printf("-- Read and return all items\n");
|
||||
read_received_all_items(buffer_handle, rx_items, capacity, data);
|
||||
xRingbufferPrintInfo(buffer_handle);
|
||||
|
||||
printf("-- Repeat the process: Acquire -> Write -> Send -> Receive -> Read -> Return\n");
|
||||
for (int i = 0; i < capacity; i++) {
|
||||
data[i] = 100 + i;
|
||||
}
|
||||
acquire_all_items(buffer_handle, rx_items, capacity);
|
||||
write_and_send_all_items(buffer_handle, rx_items, capacity, data);
|
||||
receive_all_items(buffer_handle, rx_items, capacity);
|
||||
read_received_all_items(buffer_handle, rx_items, capacity, data);
|
||||
|
||||
// Cleanup
|
||||
vRingbufferDelete(buffer_handle);
|
||||
}
|
||||
|
||||
TEST_CASE("Test no-split buffers returning non-oldest item when full does not free slot", "[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");
|
||||
const int capacity = BUFFER_SIZE / (ITEM_HDR_SIZE + MEDIUM_ITEM_SIZE);
|
||||
TEST_ASSERT_EQUAL(4, capacity);
|
||||
void *rx_items[capacity];
|
||||
uint8_t data[] = {10, 11, 12, 13};
|
||||
|
||||
acquire_all_items(buffer_handle, rx_items, capacity);
|
||||
write_and_send_all_items(buffer_handle, rx_items, capacity, data);
|
||||
receive_all_items(buffer_handle, rx_items, capacity);
|
||||
|
||||
const int free_idx = 1;
|
||||
printf("-- Read and returning only %d item\n", free_idx);
|
||||
TEST_ASSERT_EQUAL(((uint8_t *)rx_items[free_idx])[0], data[free_idx]);
|
||||
vRingbufferReturnItem(buffer_handle, rx_items[free_idx]);
|
||||
|
||||
printf("-- Verify that only the second item is returned. Can not acquire the second item\n");
|
||||
TEST_ASSERT_EQUAL(pdFALSE, xRingbufferSendAcquire(buffer_handle, &rx_items[free_idx], MEDIUM_ITEM_SIZE, 0));
|
||||
|
||||
xRingbufferPrintInfo(buffer_handle);
|
||||
printf("-- Read and return the rest of the items\n");
|
||||
for (int i = 0; i < capacity; i++) {
|
||||
if (i != free_idx) {
|
||||
read_and_return_item(buffer_handle, rx_items, i, data[i]);
|
||||
}
|
||||
}
|
||||
xRingbufferPrintInfo(buffer_handle);
|
||||
|
||||
printf("-- Repeat the process: Acquire -> Write -> Send -> Receive -> Read -> Return\n");
|
||||
for (int i = 0; i < capacity; i++) {
|
||||
data[i] = 100 + i;
|
||||
}
|
||||
acquire_all_items(buffer_handle, rx_items, capacity);
|
||||
write_and_send_all_items(buffer_handle, rx_items, capacity, data);
|
||||
receive_all_items(buffer_handle, rx_items, capacity);
|
||||
read_received_all_items(buffer_handle, rx_items, capacity, data);
|
||||
|
||||
// Cleanup
|
||||
vRingbufferDelete(buffer_handle);
|
||||
}
|
||||
|
||||
TEST_CASE("Test no-split full buffer becomes empty when oldest is returned last", "[esp_ringbuf][linux]")
|
||||
{
|
||||
RingbufHandle_t buffer_handle = xRingbufferCreate(BUFFER_SIZE, RINGBUF_TYPE_NOSPLIT);
|
||||
TEST_ASSERT_MESSAGE(buffer_handle != NULL, "Failed to create ring buffer");
|
||||
const int capacity = BUFFER_SIZE / (ITEM_HDR_SIZE + MEDIUM_ITEM_SIZE);
|
||||
TEST_ASSERT_EQUAL(4, capacity);
|
||||
uint8_t data[] = {10, 11, 12, 13};
|
||||
|
||||
// 1) Acquire, write, complete: buffer becomes FULL
|
||||
void *rx_items[capacity];
|
||||
acquire_all_items(buffer_handle, rx_items, capacity);
|
||||
write_and_send_all_items(buffer_handle, rx_items, capacity, data);
|
||||
TEST_ASSERT_EQUAL(0U, xRingbufferGetCurFreeSize(buffer_handle)); // FULL
|
||||
|
||||
// 2) Receive all items but DON'T return yet (hold them)
|
||||
receive_all_items(buffer_handle, rx_items, capacity);
|
||||
UBaseType_t items_waiting_after_recv;
|
||||
vRingbufferGetInfo(buffer_handle, NULL, NULL, NULL, NULL, &items_waiting_after_recv);
|
||||
TEST_ASSERT_EQUAL(0, items_waiting_after_recv);
|
||||
TEST_ASSERT_EQUAL(0U, xRingbufferGetCurFreeSize(buffer_handle)); // still FULL
|
||||
|
||||
// 3) Return non-oldest first — still no space (FULL must remain)
|
||||
for (int i = 1; i < capacity; ++i) {
|
||||
read_and_return_item(buffer_handle, rx_items, i, data[i]);
|
||||
}
|
||||
void *tmp;
|
||||
TEST_ASSERT_EQUAL(pdFALSE, xRingbufferSendAcquire(buffer_handle, &tmp, MEDIUM_ITEM_SIZE, 0));
|
||||
TEST_ASSERT_NULL(tmp);
|
||||
|
||||
// 4) Return the oldest last — now FULL must clear, buffer becomes truly empty
|
||||
xRingbufferPrintInfo(buffer_handle);
|
||||
read_and_return_item(buffer_handle, rx_items, 0, data[0]);
|
||||
xRingbufferPrintInfo(buffer_handle);
|
||||
|
||||
// Verify: pointers collapse, not FULL anymore, new acquires succeed
|
||||
UBaseType_t uFree, uRead, uWrite, uAcquire, waiting;
|
||||
vRingbufferGetInfo(buffer_handle, &uFree, &uRead, &uWrite, &uAcquire, &waiting);
|
||||
TEST_ASSERT_EQUAL_UINT32(waiting, 0);
|
||||
TEST_ASSERT_EQUAL_UINT32(uFree, uRead);
|
||||
TEST_ASSERT_EQUAL_UINT32(uFree, uAcquire);
|
||||
|
||||
TEST_ASSERT_GREATER_THAN_UINT32(0, xRingbufferGetCurFreeSize(buffer_handle)); // FULL cleared
|
||||
|
||||
void *slot;
|
||||
TEST_ASSERT_EQUAL(pdTRUE, xRingbufferSendAcquire(buffer_handle, &slot, MEDIUM_ITEM_SIZE, 0));
|
||||
TEST_ASSERT_NOT_NULL(slot);
|
||||
vRingbufferDelete(buffer_handle);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user