From 62f44e45df111fcfcc68e9a56bbfb562d2246154 Mon Sep 17 00:00:00 2001 From: Piyush Shah Date: Fri, 17 Nov 2017 15:24:17 +0530 Subject: [PATCH 1/3] ringbuf: Bugfixes for managing arbitrary sizes of ring buffer It was observed that if the ring buffer size provided by application is not a multiple of 4, some checks were failing (as read_ptr and write_ptr could shoot beyond the ring buffer boundary), thereby causing asserts. Simply wrapping around the pointers for such cases fixes the issue. Moreover, because of the logic for maintaining 4-byte boundary, it was also possible that a wrap-around occurred for some data, even when the actual size remaining was sufficient for it. Eg. Buffer available: 34, data size: 34, 4-byte aligned size: 36 Since the logic compares against 36, it writes 34 bytes and does a wraparound. But since all 34 bytes are already written, there is nothing to write after wrapping. It is better to just re-set the write pointer to the dtart of ring buffer in such cases. Tested send/receive for various arbitrary sizes of data and for arbitrary sizes of ring buffer. Alternative Solutions: 1) Allow only sizes which are multiples of 4, and return error otherwise. Appropriate code and documentation change would be required 2) Allow arbitrary sizes, but internally add upto 3 bytes to make the total size a multiple of 4 Signed-off-by: Piyush Shah --- components/freertos/ringbuf.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/components/freertos/ringbuf.c b/components/freertos/ringbuf.c index ec4322df8..d7ee790a5 100644 --- a/components/freertos/ringbuf.c +++ b/components/freertos/ringbuf.c @@ -190,8 +190,20 @@ static BaseType_t copyItemToRingbufAllowSplit(ringbuf_t *rb, uint8_t *buffer, si memcpy(rb->write_ptr, buffer, rem_len); //Update vars so the code later on will write the rest of the data. buffer+=rem_len; - rbuffer_size-=rem_len; buffer_size-=rem_len; + //Re-adjust the rbuffer value to be 4 byte aligned + rbuffer_size=(buffer_size+3)&~3; + //It is possible that we are here because we checked for 4byte aligned + //size, but actual data was smaller. + //Eg. For buffer_size = 34, rbuffer_size will be 36. Suppose we had only + //42 bytes of memory available, the top level check will fail, as it will + //check for availability of 36 + 8 = 44 bytes. + //However, the 42 bytes available memory is sufficient for 34 + 8 bytes data + //and so, we can return after writing the data. Hence, this check + if (buffer_size == 0) { + rb->write_ptr=rb->data; + return pdTRUE; + } } else { //Huh, only the header fit. Mark as dummy so the receive function doesn't receive //an useless zero-byte packet. @@ -286,7 +298,10 @@ static uint8_t *getItemFromRingbufDefault(ringbuf_t *rb, size_t *length, int wan //...and move the read pointer past the data. rb->read_ptr+=sizeof(buf_entry_hdr_t)+((hdr->len+3)&~3); //The buffer will wrap around if we don't have room for a header anymore. - if ((rb->data + rb->size) - rb->read_ptr < sizeof(buf_entry_hdr_t)) { + //Integer typecasting is used because the first operand can result into a -ve + //value for cases wherein the ringbuffer size is not a multiple of 4, but the + //implementation logic aligns read_ptr to 4-byte boundary + if ((int)((rb->data + rb->size) - rb->read_ptr) < (int)sizeof(buf_entry_hdr_t)) { rb->read_ptr=rb->data; } return ret; @@ -355,12 +370,20 @@ static void returnItemToRingbufDefault(ringbuf_t *rb, void *item) { rb->free_ptr=rb->data; } else { //Skip past item + rb->free_ptr+=sizeof(buf_entry_hdr_t); + //Check if the free_ptr overshoots the buffer. + //Checking this before aligning free_ptr since it is possible that alignment + //will cause pointer to overshoot, if the ringbuf size is not a multiple of 4 + configASSERT(rb->free_ptr+hdr->len<=rb->data+rb->size); + //Align free_ptr to 4 byte boundary. Overshoot condition will result in wrap around below size_t len=(hdr->len+3)&~3; - rb->free_ptr+=len+sizeof(buf_entry_hdr_t); - configASSERT(rb->free_ptr<=rb->data+rb->size); + rb->free_ptr+=len; } //The buffer will wrap around if we don't have room for a header anymore. - if ((rb->data+rb->size)-rb->free_ptr < sizeof(buf_entry_hdr_t)) { + //Integer typecasting is used because the first operand can result into a -ve + //value for cases wherein the ringbuffer size is not a multiple of 4, but the + //implementation logic aligns free_ptr to 4-byte boundary + if ((int)((rb->data+rb->size)-rb->free_ptr) < (int)sizeof(buf_entry_hdr_t)) { rb->free_ptr=rb->data; } //The free_ptr can not exceed read_ptr, otherwise write_ptr might overwrite read_ptr. From 7dd9c4f57fb89e2abeebf7fdbf938f7532e955b5 Mon Sep 17 00:00:00 2001 From: Piyush Shah Date: Fri, 17 Nov 2017 15:50:49 +0530 Subject: [PATCH 2/3] ringbuf: Fixes to header files for better understanding 1. Usage of this module required applications to include additional files. What files to include is not very intuitive. Instead, it is better for the header file itself to include other files as required. 2. Even though it may seem logical, it is better to explicitly mention that an item needs to be "Returned" after a Receive Signed-off-by: Piyush Shah --- components/freertos/include/freertos/ringbuf.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/components/freertos/include/freertos/ringbuf.h b/components/freertos/include/freertos/ringbuf.h index 93ba30758..2449cc193 100644 --- a/components/freertos/include/freertos/ringbuf.h +++ b/components/freertos/include/freertos/ringbuf.h @@ -36,6 +36,8 @@ maximum size is (buffer_size/2)-8 bytes. The bytebuf can fill the entire buffer no overhead. */ +#include + //An opaque handle for a ringbuff object. typedef void * RingbufHandle_t; @@ -110,6 +112,8 @@ BaseType_t xRingbufferSendFromISR(RingbufHandle_t ringbuf, void *data, size_t da /** * @brief Retrieve an item from the ring buffer * + * @note A call to vRingbufferReturnItem() is required after this to free up the data received. + * * @param ringbuf - Ring buffer to retrieve the item from * @param item_size - Pointer to a variable to which the size of the retrieved item will be written. * @param xTicksToWait - Ticks to wait for items in the ringbuffer. @@ -123,6 +127,8 @@ void *xRingbufferReceive(RingbufHandle_t ringbuf, size_t *item_size, TickType_t /** * @brief Retrieve an item from the ring buffer from an ISR * + * @note A call to vRingbufferReturnItemFromISR() is required after this to free up the data received + * * @param ringbuf - Ring buffer to retrieve the item from * @param item_size - Pointer to a variable to which the size of the retrieved item will be written. * @@ -135,6 +141,8 @@ void *xRingbufferReceiveFromISR(RingbufHandle_t ringbuf, size_t *item_size); /** * @brief Retrieve bytes from a ByteBuf type of ring buffer, specifying the maximum amount of bytes * to return + + * @note A call to vRingbufferReturnItem() is required after this to free up the data received. * * @param ringbuf - Ring buffer to retrieve the item from * @param item_size - Pointer to a variable to which the size of the retrieved item will be written. @@ -150,6 +158,8 @@ void *xRingbufferReceiveUpTo(RingbufHandle_t ringbuf, size_t *item_size, TickTyp * @brief Retrieve bytes from a ByteBuf type of ring buffer, specifying the maximum amount of bytes * to return. Call this from an ISR. * + * @note A call to vRingbufferReturnItemFromISR() is required after this to free up the data received + * * @param ringbuf - Ring buffer to retrieve the item from * @param item_size - Pointer to a variable to which the size of the retrieved item will be written. * From 4f33339c1d7ef86921246ca257d21e929c0170ec Mon Sep 17 00:00:00 2001 From: Piyush Shah Date: Fri, 17 Nov 2017 17:23:43 +0530 Subject: [PATCH 3/3] test_ringbuf: Add tests for arbitrary length ring buffer This will test the ring buffer for buffer length that is not a multiple of 4 Signed-off-by: Piyush Shah --- components/freertos/test/test_ringbuf.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/components/freertos/test/test_ringbuf.c b/components/freertos/test/test_ringbuf.c index 84e115999..6a721aae3 100644 --- a/components/freertos/test/test_ringbuf.c +++ b/components/freertos/test/test_ringbuf.c @@ -149,11 +149,16 @@ static void uartRxDeinit() esp_intr_free(s_intr_handle); } -static void testRingbuffer(int type) +static void testRingbuffer(int type, bool arbitrary) { TaskHandle_t th[2]; int i; - rb = xRingbufferCreate(32 * 3, type); + /* Arbitrary Length means buffer length which is not a multiple of 4 */ + if (arbitrary) { + rb = xRingbufferCreate(31 * 3, type); + } else { + rb = xRingbufferCreate(32 * 3, type); + } testtype = TST_MOSTLYFILLED; @@ -179,12 +184,22 @@ static void testRingbuffer(int type) // TODO: split this thing into separate orthogonal tests TEST_CASE("FreeRTOS ringbuffer test, no splitting items", "[freertos]") { - testRingbuffer(0); + testRingbuffer(0, false); } TEST_CASE("FreeRTOS ringbuffer test, w/ splitting items", "[freertos]") { - testRingbuffer(1); + testRingbuffer(1, false); +} + +TEST_CASE("FreeRTOS ringbuffer test, no splitting items, arbitrary length buffer", "[freertos]") +{ + testRingbuffer(0, true); +} + +TEST_CASE("FreeRTOS ringbuffer test, w/ splitting items, arbitrary length buffer", "[freertos]") +{ + testRingbuffer(1, true); }