Is it bad coding practice to design a buffer using pointers?

Question

I coded a ring buffer for my Arduino to buffer data from either the SPI or TWI ports. In the .h file I used a pointer for the buffer:

typedef uint8_t *ring_buffer_t;
typedef uint8_t ring_count_t;
typedef uint8_t ring_index_t;
typedef uint8_t ring_size_t;

typedef struct ring_t 
{
    ring_buffer_t buffer;
    ring_count_t  count;
    ring_index_t  head;
    ring_index_t  tail;
    ring_size_t   size;
}rint_t;

int ring_init(ring_t *r, ring_buffer_t buffer, ring_size_t size);

// other function prototypes to add and remove from the ring buffer

There is more code to handle adding and removing from the buffer, but I am simplifying here. Then in my .c file I have:

#include<string.h>

void ring_init(ring_t *r, ring_buffer_t buffer, ring_size_t size)
{
    r->buffer = buffer;
    r->size   = size;
    r->head   = 0;
    r->tail   = 0;
    r->count  = 0;

    // clear out the buffer memset cheaper by 12 bytes rather than for loop
    memset((void *)r->buffer, 0, r->size);
}

Then somewhere in code where the ring buffer is going to be used:

In SPI.h ...
#define MAX_BUFFER_SIZE 32
ring_t  rx_ring;
uint8_t spi_rx_buffer[MAX_BUFFER_SIZE];

#define MAX_BUFFER_SIZE 32
ring_t  tx_ring;
uint8_t spi_tx_buffer[MAX_BUFFER_SIZE];


In SPI.c ...
void spi_init( ... )
{
    ring_init(&rx_ring, spi_rx_buffer, MAX_BUFFER_SIZE);
    ring_init(&tx_ring, spi_tx_buffer, MAX_BUFFER_SIZE);

    // initialize other spi parameters
}

Are there any issues with my code that are obvious that I am not seeing? I ask because I ran the code and it works, but I feel I am doing something wrong. I can code in C and remember something about malloc() for dynamic memory allocation, just not sure if what I am doing qualifies. Or should this work creating a buffer of some defined size?

Thanks for any help or ideas.

EDIT: 07/27/2014 Added code to try and make what I am trying to do clear. Let me know, thanks.

Answer 1

Don't bother with malloc unless you need to. If you explicitly define your buffer before hand then you can use avr-size to check your SRAM usage.

Also the C compiler automatically adds code to initialise global variables to 0, so you don't need ring_init if your define the array explicitly. See https://stackoverflow.com/questions/2218254/variable-initialization-in-c for initialisation comments. Even if buffer was filled with non-zeros it wouldn't matter because it should be written to before being read from.

If you change the ring buffer from chars to ints, etc. you will have to change memset, as it assumes chars.

Will MAX_BUFFER_SIZE change? If not, you could put the buffer in ring_t.

Answer 2

Not enough info. You have a struct ring_t that has a pointer to an array of uint8_t's. In your ring_init method, your code assumes that the pointer points to valid memory. If you have not explicitly pointed the pointer to a block of memory, either stack memory or heap memory, then yes, there is a problem.

You COULD malloc the ring buffer, or you could create it as a global variable. If you've only got one ring buffer it's probably just as well to make the buffer a global and set the pointer in your setup method.

Answer 3

Simple answer: No, it isn't bad practice at all. I write with pointers all the time. Pointer referencing usually compiles to more efficient code than does array referencing.

But: It looks like you've confused a pointer with the thing it is supposed to point to. The member you call 'buffer' is declared as pointer to unsigned byte. It doesn't appear to get initialized anywhere; it should be initialized with the address of an array of unsigned bytes (at least) as large as your ring buffer needs to be. I don't see this in anywhere in your code.

As others have described, the buffer could be allocated at run time with malloc() (but it's best not to save malloc() & free() for things you have to create and destroy on the fly; or direct the compiler to allocate it before your program gets control, with something like:

#define RINGBUFSIZE 32

uint8_t ring_buffer_memory[RINGBUFSIZE];

and initialize your pointer to it with:

r->buffer = ring_buffer_memory;

This will initialize your pointer to the first of (in this case) 32 bytes of zeroed memory reserved for the ring buffer. In your code, it's quite likely the pointer contains 0, thus pointing to memory location zero and is definitely not what you mean to do.

A word on variable names - adopt a convention for naming your variables that help a future reader (probably you!) understand/remember what that variable is for. In this case I'd probably use ringBuf for the array, and pRingBuf for its pointer. As a new reader of this code, I'd find it pretty inconvenient to have to have to remember that buffer is really a pointer (and what the heck was the buffer called!?).