diff --git a/components/freertos/include/freertos/ringbuf.h b/components/freertos/include/freertos/ringbuf.h index 362381ab6..23ea86868 100644 --- a/components/freertos/include/freertos/ringbuf.h +++ b/components/freertos/include/freertos/ringbuf.h @@ -44,6 +44,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; @@ -118,6 +120,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. @@ -131,6 +135,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. * @@ -143,6 +149,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. @@ -158,6 +166,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. * 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. 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); }