Merge branch 'bugfix/ringbuf_buflen_bugfix' into 'master'

Bugfix/ringbuf buflen bugfix

See merge request !1562

Author: igrr

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 <freertos/queue.h>
+
 //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.
  *

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.

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);
 }