Bug in RingBuf library - nrf_ringbuf_free does not free up bytes in buffer

[veletron]

Hi

I am having an issue with the RinBuf library - I am accepting packets from SPI, buffering them in a ringbuf until I can send onwards via BLE. The specific issue I am having is that calling nrf_ringbuf_free does not actually free bytes in the ringbuf!

NRF_RINGBUF_DEF(m_RingBufBleNormalPriority, 2048);
#define MESSAGE_PLUS_PAYLOAD_BYTES 244

void foo(void)
{
    nrf_ringbuf_init(&m_RingBufBleNormalPriority);
    ret_code_t err_code = NRF_SUCCESS;
    uint8_t * p_buffer;
    uint32_t BufferLength = 0;
    uint32_t a;
    
    //do this 8 times = 8*244 bytes = 1952 bytes used
    for (a=0;  a<8; a++)
    {
        BufferLength = MESSAGE_PLUS_PAYLOAD_BYTES;
    
        err_code = nrf_ringbuf_alloc(&m_RingBufBleNormalPriority, &p_buffer, &BufferLength, true);
    
        if (err_code != NRF_SUCCESS)
        {
            NRF_LOG_INFO("Error - nrf_ringbuf_alloc err: %d",err_code);
            return;
        }
    
        if (BufferLength != MESSAGE_PLUS_PAYLOAD_BYTES)
        {
            NRF_LOG_INFO("Error - Wrong length allocated");
            nrf_ringbuf_put(&m_RingBufBleNormalPriority, 0);
            return;
        }
    
        //would ordinarily fill the buffer with data from SPI here
    
        nrf_ringbuf_put(&m_RingBufBleNormalPriority, MESSAGE_PLUS_PAYLOAD_BYTES);
    }
    
    //At this point, 1952 bytes out of the total 2028 bytes utilised in the ring buffer
    
    
    
    //Get one record == 244 bytes from Ring Buffer
    BufferLength = MESSAGE_PLUS_PAYLOAD_BYTES;
    err_code = nrf_ringbuf_get(&m_RingBufBleNormalPriority, &p_buffer, &BufferLength, true);
    
    if (err_code != NRF_SUCCESS)
    {
        NRF_LOG_INFO("Error - nrf_ringbuf_get err: %d",err_code);
        return;
    }
    
    if (BufferLength != MESSAGE_PLUS_PAYLOAD_BYTES)
    {
        NRF_LOG_INFO("Error - Wrong length read from buffer");
        nrf_ringbuf_free(&m_RingBufBleNormalPriority, 0);
        return;
    }
    
    nrf_ringbuf_free(&m_RingBufBleNormalPriority, MESSAGE_PLUS_PAYLOAD_BYTES);
    
    //At this point, buffer utilisation is 1952-244 bytes = 1708 with 340 bytes remaining
    
    //Add another 244 bytes to the Ring Buffer (should be space)
    
    BufferLength = MESSAGE_PLUS_PAYLOAD_BYTES;
    
    err_code = nrf_ringbuf_alloc(&m_RingBufBleNormalPriority, &p_buffer, &BufferLength, true);
    
    if (err_code != NRF_SUCCESS || BufferLength != MESSAGE_PLUS_PAYLOAD_BYTES)
    {
        NRF_LOG_INFO("Error - nrf_ringbuf_alloc err: %d",err_code);
        NRF_LOG_INFO("Requested: %d | Allocated: %d",MESSAGE_PLUS_PAYLOAD_BYTES,BufferLength);
        nrf_ringbuf_put(&m_RingBufBleNormalPriority, 0);
        return;
        
        
        
    /*************** THIS FAILS HERE ***************/
    //BufferLength != MESSAGE_PLUS_PAYLOAD_BYTES only 60 bytes have been allocated
    //there are 340 available
    //If I do another GET operation then I get the same result, 60 bytes available!
    //The function nrf_ringbuf_free DOES NOT free up the 244 bytes I have asked it to!!
    }
    
    
    nrf_ringbuf_put(&m_RingBufBleNormalPriority, MESSAGE_PLUS_PAYLOAD_BYTES);
}   

    
    
    
    
    
    nrf_ringbuf_put(&m_RingBufBleNormalPriority, 0);

See the comments in the example above. I put and commit 244 bytes into the run buffer 8 times over, I then get and free 244 bytes, leaving 340 available

I then attempt to put another 244 bytes into the ring buffer, but I am only given an allocation of 60 bytes despite there being 340 bytes available. The example is monolithic so I am not checking for NRF_ERROR_BUSY (although I do in my real app), neither am I doing anything with the allocated buffers in the above. Incidentally, the initial 8x244 bytes is 1952, 2048-1952  is 60 . Sixty is what I am being offered in my final request DESPITE having Gotten and Free'd 244 bytes beforehand. The nrf_ringbuf_free returns NRF_SUCCESS. If I entend the size of the ringbuff then it just delays the issue, eventually it fills despite having been freed.

Any idea's - maybe I just cannot use a ringbuff in this manner?

Nigel

[veletron]

Hi

So I have realised whats going on and I am being a bit daft. Its a circular buffer. When I ask for 244 bytes in the final request, I can only be granted 0x60 (96) *contiguous* bytes as after that the buffer rolls over. If I wanted 244 bytes, I would need to make another request after obtaining the 96 bytes of 148 bytes and then deal with a non-contiguous buffer.

Of course there is an easier way that guarantees getting contiguous buffers which is to request buffer sizes that fit exactly into the size of the circular buffer (eg 256). This is what I am now doing and it works fine.

It has to be said that the documentation here is pretty poor. I arrive at this after having spent too much time doing C#, a simple statement 'requesting add-hoc buffer allocations from the circular buffer may result in allocations of less than the amount requested where the buffer rolls over, and is unable to provide a contiguous allocation - to avoid this, the buffer capacity should be an exact multiple of the buffer requests and all buffer requests should be identical'.

Obviously, one could also detect the roll-over (less bytes allocated than requested), and commit those bytes before requesting another buffer, ditto at the getter end.

I suspect that this only effects the use of the ringbuf where direct memory access is being used. eg the _cpy_ functions will handle the roll-over auto-magically.

Nigel

[gordie]

I have this issue with the ringbuffer module as well, using SDK 15.3 with an nRF52840. When you say "request buffer sizes that fit exactly into the size of the circular buffer (eg 256)", does this mean you can have a request that is less than 256, but still a power of two say, 32 byte requests with circular buffer size of 256?

[veletron]

If you can divide the size of your overall buffer by the size of each allocation from the buffer without getting any remainder then you are good to go!! 256/32 = 8 so good to go! I was allocating random sizes from the ringbuff due to my payloads varying in size - its this that will not work (unless you manually handle the wrap-around)

The power-2 requirement is for the size of the ringbuff, not for the allocations from it. That said, to get an allocation size that exactly fits the overall buffer size, you have to respect Power-2 anyway - or be prepared to deal with the wrap-around manually.

My code below. Note that I have wrapped the put and get operations with a timer such as there is a get-out-of-jail if the ringbuff were to keep returning 'busy'. My underlying protocol deals with this state.

ringbuff.c

#include <stdbool.h>
#include <stdint.h>
#include <string.h>
#include <stdlib.h>
#include "RingBuffer.h"

#include "nrf_log.h"
#include "nrf_log_ctrl.h"
#include "nrf_log_default_backends.h"
#include "nrf_ringbuf.h"
#include "app_timer.h"

NRF_RINGBUF_DEF(m_RingBufBleNormalPriority, 16384);
#define RINGBUFF_WAIT_TIMEOUT 100    // (mS) timeout for transfer of SPI data Master <--> Slave
__attribute__((aligned(4))) APP_TIMER_DEF(m_RingBuffGetMutexTimer);

static void RingBuffGetWaitMutexTimerExpired(void * p_context);
static volatile bool fRingBuffGetWaitMutexTimerExpired = false;

__attribute__((aligned(4))) APP_TIMER_DEF(m_RingBuffPutMutexTimer);

static void RingBuffPutWaitMutexTimerExpired(void * p_context);
static volatile bool fRingBuffPutWaitMutexTimerExpired = false;

/**@brief Initialises buffers - MUST be called ONCE before the other functions in this file. May also be called to force a reset of allocated buffers
 *
 * @returns   Nothing
 */
void Init_RingBuffer(void)
{
    nrf_ringbuf_init(&m_RingBufBleNormalPriority);
    APP_ERROR_CHECK(app_timer_create(&m_RingBuffGetMutexTimer, APP_TIMER_MODE_SINGLE_SHOT, RingBuffGetWaitMutexTimerExpired));
    APP_ERROR_CHECK(app_timer_create(&m_RingBuffPutMutexTimer, APP_TIMER_MODE_SINGLE_SHOT, RingBuffPutWaitMutexTimerExpired));
}


#define ACTUAL_BYTES_REQUESTED 256


void RingBuffClear(void)
{
    uint8_t * p_buffer;
    uint32_t BufferLengthReqGranted = ACTUAL_BYTES_REQUESTED;  //Default allocation request
    
    ret_code_t err_code = NRF_SUCCESS;
    do
    {
        while ((err_code = nrf_ringbuf_get(&m_RingBufBleNormalPriority, &p_buffer, &BufferLengthReqGranted, true)) == NRF_ERROR_BUSY)
        {
            //do nothing
        }
        
        if (err_code == NRF_SUCCESS && BufferLengthReqGranted > 0)
            nrf_ringbuf_free(&m_RingBufBleNormalPriority,BufferLengthReqGranted);
        
    } while (BufferLengthReqGranted > 0);
}


uint8_t* RingBuffAlloc(void)
{
    ret_code_t err_code = NRF_SUCCESS;
    uint8_t * p_buffer;
    uint32_t BufferLengthReqAlloc = ACTUAL_BYTES_REQUESTED;  //Default allocation request
    
    fRingBuffPutWaitMutexTimerExpired = false;
    APP_ERROR_CHECK(app_timer_start(m_RingBuffPutMutexTimer, APP_TIMER_TICKS(RINGBUFF_WAIT_TIMEOUT), NULL));
    
    while ((err_code = nrf_ringbuf_alloc(&m_RingBufBleNormalPriority, &p_buffer, &BufferLengthReqAlloc, true)) == NRF_ERROR_BUSY && !fRingBuffPutWaitMutexTimerExpired)
    {
        //do nothing
    }
    
    app_timer_stop(m_RingBuffPutMutexTimer);
    
    if (err_code != NRF_SUCCESS)  //this also traps the case where the request returned NRF_ERROR_BUSY and we timed out
        return NULL;
    
    if (ACTUAL_BYTES_REQUESTED == BufferLengthReqAlloc)
        return p_buffer;
    
    //We were not allocated the number of bytes requested - we should never get here
    nrf_ringbuf_put(&m_RingBufBleNormalPriority, 0);
    return NULL;
}


bool RingBuffCommit(bool fCommitAll)
{
    return (nrf_ringbuf_put(&m_RingBufBleNormalPriority, ((fCommitAll == true)?ACTUAL_BYTES_REQUESTED:0)) == NRF_SUCCESS);
    
}


uint8_t* RingBuffGet(void)
{
    uint8_t * p_buffer;
    uint32_t BufferLengthReqGranted = ACTUAL_BYTES_REQUESTED;  //Default allocation request
    
    ret_code_t err_code = NRF_SUCCESS;
    
    fRingBuffGetWaitMutexTimerExpired = false;
    APP_ERROR_CHECK(app_timer_start(m_RingBuffGetMutexTimer, APP_TIMER_TICKS(RINGBUFF_WAIT_TIMEOUT), NULL));
    
    while ((err_code = nrf_ringbuf_get(&m_RingBufBleNormalPriority, &p_buffer, &BufferLengthReqGranted, true)) == NRF_ERROR_BUSY && !fRingBuffGetWaitMutexTimerExpired)
    {
        //do nothing
    }
    
    app_timer_stop(m_RingBuffGetMutexTimer);
    
    if (err_code != NRF_SUCCESS)  //this also traps the case where the request returned NRF_ERROR_BUSY and we timed out
        return NULL;
    
    if (BufferLengthReqGranted == ACTUAL_BYTES_REQUESTED)
        return p_buffer;
    
    //We were not allocated the number of bytes requested - we should never get here
    nrf_ringbuf_free(&m_RingBufBleNormalPriority, 0);
    return NULL;
}

bool RingBuffFree(bool fFreeAll)
{
    return (nrf_ringbuf_free(&m_RingBufBleNormalPriority,((fFreeAll == true)?ACTUAL_BYTES_REQUESTED:0)) == NRF_SUCCESS);
}

/*
 * Callback used to indicate that the wait for the RingBuffer mutex has expired
*/
static void RingBuffGetWaitMutexTimerExpired(void * p_context)
{
    fRingBuffGetWaitMutexTimerExpired = true;
}


/*
 * Callback used to indicate that the wait for the RingBuffer mutex has expired
*/
static void RingBuffPutWaitMutexTimerExpired(void * p_context)
{
    fRingBuffPutWaitMutexTimerExpired = true;
}

ringbuff.h

#ifndef RING_BUFFER_H_
#define RING_BUFFER_H_

#include <stdint.h>
#include <stdbool.h>

void Init_RingBuffer(void);
void RingBuffClear(void);
uint8_t* RingBuffAlloc(void);
bool RingBuffCommit(bool fCommitAll);
uint8_t* RingBuffGet(void);
bool RingBuffFree(bool fFreeAll);

#endif

Usage

//Add Data To Ringbuff

if ((pBuffer = RingBuffAlloc()) != NULL)
{
    //Fill the pBuffer here

    if (SuccessfullyFilledTheBuffer(pBuffer))
        RingBuffCommit(true);   //commit it
    else
        RingBuffCommit(false);  //free it
}

//Get data from the Ringbuff

uint8_t *pBuffer =  RingBuffGet();
if (pBuffer != NULL)
{
    if (SuccessfullyDealtWithData(pBuffer))
        RingBuffFree(true);   //Free the data in the ringbuff
    else
        RingBuffFree(false);  //keep the data in the ringbuff (will retry later)
}

Nigel

[gordie]

Hi,

Interesting -  thanks for the reply.