Circular buffer implementation and Serial.print()

Question

I am trying to implement the simplest circular buffer in Arduino. I think I am able to implement it because my push and pop functions do not return any errors. However, when I want to print the data in the buffer using Serial.print(), the code goes into an infinite loop. Here is my code:

#include <stdio.h>

typedef struct circular_buffer
{
    int buffer_arr[5];
    int head;
    int tail;
} circular_buffer;

void setup()
{  
  Serial.begin(9600); 
  Serial.println("Circular buffer started");
}

void loop()
{  
  circular_buffer *cb;
  cb_init(cb);
  if(Serial.available()){
    char instruction = Serial.read();
    Serial.println(instruction);
    if(instruction == 'a'){
      if(Serial.available()){
        char instruction = Serial.read();
        Serial.println("here");
      }
      char key = Serial.read();
      cb_push_front(cb, key);
      Serial.print(String(key));
      Serial.println("    is entered");
    }
    else if(instruction == 'd'){
      cb_pop_back(cb);
      Serial.println("last item removed");
    }
//    for(int i=0; i<5; i++){
//      Serial.print(cb->buffer_arr[i]);
//      
//    }
    Serial.println("-");
  }
}



void cb_init(circular_buffer *cb)
{
    // cb->buffer_arr = {0,0,0,0,0};
    for(int i=0; i<5; i++){
      cb->buffer_arr[i] = 'c';
//      Serial.print(sizeof(*cb));
    }
//      Serial.print(cb->buffer_arr[0]);
    cb->head = 0;
    cb->tail = 0;
}


void cb_push_front(circular_buffer *cb, char item)
{
    cb->head++;
    cb->buffer_arr[cb->head] = item;
}

void cb_pop_back(circular_buffer *cb)
{
    cb->tail++;
    cb->buffer_arr[cb->tail] = 0;
}

The for loop which I commented out is the problematic one - or at least what I believe. My problem can be very much related to my poor C-language skills. I thought that cb->buffer_arr = {0,0,0,0,0}; code piece in the initialization function should have worked (thinking very much Python-ic way) but it didn't.

So, if anyone can see the issue and help me fixing it, I'd be very much appreciated. Thanks in advance.

EDIT

After Majenko's answer I have updated the code like that. I still have issues how to print though. I am not sure whether I am printing the address of the circular buffer as ASCII in this way. I tried to pass the cb to a print function in two ways. None seems to work, though...

#include <stdio.h>

typedef struct circular_buffer
{
    int buffer_arr[5];
    int head;
    int tail;
} circular_buffer;

circular_buffer cb;

void setup()
{  
  Serial.begin(9600); 
  cb_init(cb);
  Serial.println("Circular buffer started");
}

void loop()
{  
  if(Serial.available()){
    char instruction = Serial.read();
    Serial.println(instruction);
    if(instruction == 'a'){
      Serial.print("a pressed");  
      cb_push_front(cb, instruction);
    }
    else if(instruction == 'd'){
      Serial.print("d pressed");
    }
    else{
      Serial.print("UNKNOWN");
    }
    Serial.println("-");
  }
}



void cb_init(circular_buffer &cb)
{
    for(int i=0; i<5; i++){
      cb.buffer_arr[i] = 'c';
    }
    Serial.println("here init");
    cb_print1(&cb);
    cb_print2(cb);
    cb.head = 0;
    cb.tail = 0;
}


void cb_push_front(circular_buffer &cb, char item)
{
    cb.head++;
    cb.buffer_arr[cb.head] = item;

    cb_print1(&cb);
    cb_print2(cb);
}

void cb_pop_back(circular_buffer &cb)
{
    cb.tail++;
    cb.buffer_arr[cb.tail] = 0;
}

void cb_print1(circular_buffer *cb)
{
  Serial.println("here print1");
  for(int i=0; i<5; i++){
    Serial.print(cb->buffer_arr[i]);
    
  }
}

void cb_print2(circular_buffer cb)
{
  Serial.println("here print2");
  for(int i=0; i<5; i++){
    Serial.print(cb.buffer_arr[i]);
    
  }
}

And the returned output is like that:

here init
here print1
9999999999here print2
9999999999Circular buffer started
a
a pressedhere print1
9997999999here print2
9997999999-
a
a pressedhere print1
9997979999here print2
9997979999-

Answer 1

You create a variable that can point to a circular buffer (*cb) but at no point do you actually point it at a circular buffer.

Either you need to create a new circular buffer or don't create a pointer, but create a statically allocated circular buffer. The latter is the preferred method.

Simply remove the * from your cb declaration, which makes a standard struct variable, then take the address of that (with &) to pass as a pointer to your other functions. Or change the other functions to pass by reference.

For example:

circular_buffer *cb;

becomes

circular_buffer cb;

And then

cb_init(cb);

becomes:

cb_init(&cb);

As do all the other functions that you pass the cb variable to.

Also every time you pass through loop() you will be wiping and making a new circular_buffer variable. Instead cb should really be global and the cb_init() call should be in setup().

Answer 2

There are multiple issues with this code. Majenko already showed the main one, I will add a few more here.

First, about the circular buffer implementation. As has been stated in a comment, for the buffer to be considered circular, the indices should wrap around. I would add a small nitpick: it is customary, in C++, to start filling an array from index 0. Thus you should post-increment (rather than pre-increment) the indices:

void cb_push_front(circular_buffer *cb, char item)
{
    cb->buffer_arr[cb->head] = item;
    cb->head++;                            // post-increment
    if(cb->head == circular_buffer_size)   // with wrap-around
      cb->head = 0;
}

There is still an issue though: the buffer can overflow. You should only add an item if there is room to do so. And you should only remove an item if the buffer is not empty. You know that the buffer is empty when tail=head, but how do you know when it is full? If you completely fill the buffer and the indices wrap around as they should, you end up with head=tail, and you cannot tell the difference between a full buffer and an empty buffer. There are two ways around this:

• You can keep an extra variable in the struct that helps you tell the difference. This could be a count of the elements in the buffer, or a boolean that tells you when the buffer is empty, or a boolean that tells you when it is full.

• You can refrain from completely filling the array and consider that the buffer is full when it has been filled to its full capacity, where the “capacity” is defined as the size of the array minus one. Then the buffer is full when
  (head + 1) = tail [modulo array length].

Here is an implementation of the second strategy:

void cb_push_front(circular_buffer *cb, char item)
{
    int next_head = cb->head + 1;          // increment
    if(next_head >= circular_buffer_size)  // with wrap around
      next_head = 0;
    if(next_head == cb->tail){
      Serial.println("buffer overflow");
      return;
    }
    cb->buffer_arr[cb->head] = item;
    cb->head = next_head;
}

int cb_pop_back(circular_buffer *cb)
{
    if(cb->tail == cb->head) {
      Serial.println("buffer underflow");
      return -1;
    }
    char item = cb->buffer_arr[cb->tail];
    cb->tail++;
    if(cb->tail >= circular_buffer_size)
      cb->tail = 0;
    return item;
}

Note that there is no point in cb_pop_back() overwriting the buffer. On the other hand, it is often very useful for this function to return the item that has just been popped.

Now, a few random comments:

• Rather than hard-coding the array size (5) in multiple places, define a constant at the beginning of the program (circular_buffer_size in my examples) and use it everywhere. This makes it clearer what the number means, and makes it easier to change the size if you ever need to.

• Since you are storing characters, use an array of char.

• Your code for handling Serial does not cope well with the situation where there is a small delay between the 'a' instruction and the character to be added. If you can afford blocking code, the simplest solution is to block until the user provides the character:

if(instruction == 'a'){
  while(!Serial.available())
    ;  // wait for the key to be entered
  char key = Serial.read();
  cb_push_front(&cb, key);
  Serial.print(key);
  Serial.println(" is entered");
}