Implementing an I2C buffer in C

Question

I'm implementing a read-only I²C slave on a PIC18F4620. I have made a -working- ISR handler for the MSSP module:

unsigned char dataFromMaster;

unsigned char SSPISR(void) {
    unsigned char temp = SSPSTAT & 0x2d;
    if ((temp ^ 0x09) == 0x00) {
        // State 1: write operation, last byte was address
        ReadI2C();
        return 1;
    } else if ((temp ^ 0x29) == 0x00) { 
        // State 2: write operation, last byte was data
        dataFromMaster = ReadI2C();
        return 2;
    } else if (((temp & 0x2c) ^ 0x0c) == 0x00) {
        // State 3: read operation, last byte was address
        WriteI2C(0x00);
        return 3;
    } else if (!SSPCON1bits.CKP) {
        // State 4: read operation, last byte was data
        WriteI2C(0x00);
        return 4;
    } else {                                        
        // State 5: slave logic reset by NACK from master
        return 5;
    }
}

This is just a port to C from a part of the ASM code in appendix B of AN734.

In my main loop, I'm checking if there is new data, like this:

void main(void) {
    if (dataFromMaster != 0x00) {
        doSomething(dataFromMaster);
        dataFromMaster = 0x00;
    }
}

This gives a problem when the master sends bytes very fast, and new data comes in before the main loop gets to doSomething. I therefore want to implement a buffer where data from the master is stored. I need a 16-char null-terminated array (the null won't be used as a command for the slave). The ISR has to write new data to that array, and the main loop should read it from the array in the order it was received, and clear the array.

I have no idea how to implement this. Do you?

Answer 1

I have no experience with PIC, but the problem seems generic enough. I would create a simple array with two independent pointers into the array: one read pointer and one write pointer. Whenever you receive a byte, you increment the write pointer and write at the new position; in your main loop you could then check if the read pointer and the write pointer are the same. If not, you simply read and process from the buffer and increase the read pointer for every byte until they are.

You could then either reset the pointers to the beginning of the array, or let them "flow over" to the beginning making essentially a circular buffer. This is easiest if the size of the array is a factor of 2 as you can then simply bitmask both pointers after their increments.

Some example (pseudo)code:

volatile unsigned int readPointer= 0;
volatile unsigned int writePointer=0;
volatile char theBuffer[32];
...
//in your ISR
writePointer = (writePointer+1) & 0x1F;
theBuffer[writePointer] = ReadI2C(); // assuming this is the easiest way to do it
                                     // I would probably just read the register directly
...
//in main
while (readPointer != writePointer) {
  readPointer = (readPointer+1) & 0x1F;
  nextByte = theBuffer[readPointer];
  // do whatever necessary with nextByte
}

Answer 2

If you want to do this right, the best solution is to implement some kind of ring buffer.
But realize, that the implementation has to be "interrupt safe". This is required, because while the buffer contents are processed in the main-loop, there may arrive additional data at any time in your SPI ISR!
So you might need to take look at using volatile and "ATOMIC" operations if you are not familiar with those.

Answer 3

From the pseudocode of fm_andreas' answer, I made a working C18 code:

#define bufferSize 0x20
static volatile unsigned char buffer[bufferSize] = {0}; // This is the actual buffer
static volatile unsigned char readPointer = 0;          // The pointer to read data
static volatile unsigned char writePointer = 0;         // The pointer to write data
static volatile unsigned bufferOverflow = 0;            // Indicates a buffer overflow

// In the ISR...
if (buffer[writePointer] == 0x00) {                     // If there is no data at the pointer
    buffer[writePointer] = SSPBUF;                      // Put the data in the buffer
    writePointer = (writePointer+1)%bufferSize;         // Increase the pointer, reset if >32
} else {                                                // If there is data...
    bufferOverflow = 1;                                 // Set the overflow flag
}

// In the main loop...
while (1) {
    // Do some other stuff
    if (readPointer != writePointer) {                  // If there is a new byte
        putc(buffer[readPointer], stdout);              // Do something with the data
        buffer[readPointer] = 0x00;                     // Reset the data
        readPointer = (readPointer+1)%bufferSize;       // Increase the pointer, reset if >32
    }
}

The beauty of this code is that it's a ring buffer:

It is therefore less likely to overflow when large data bulks are sent at once. This is discussed in the comments on Nick Alexeev's answer.

Answer 4

@fm-andreas has beat me to this. I was going to propose the same thing: burst buffer with read and write positions. (This is not a ring buffer. It's simpler. It doesn't wrap around.) A burst buffer can store a burst of data. The system should be designed such that there is sufficient time between the bursts to process the data.

Here's my version (pseudo-code):

const unsigned char g_BUFF_LEN = 16;
unsigned char   g_dataBuff[BUFF_LEN];       // buffer
unsigned char   g_g_iWriteOffset, g_g_iReadOffset;      // write and read offsets
unsigned char   g_iFlags;


void main()
{
    resetBuffer();      // initialize the burst buffer

    // process the contents buffer
    while (1)
    {
        // other code that lives in the main loop

        while (g_iWriteOffset > g_iReadOffset)      // inner loop for processing the received data
        {
            doSomething(g_dataBuff[g_iReadOffset]);     

            // disable the receiveISR.  If the ISR occurs in this block, it can corrupr the buffer.
            ++g_iReadOffset;        // advance the read offset
            if (g_iReadOffset == g_iWriteOffset)    // is there remaining unprocessed data in the buffer?
            {
                resetBuffer();
            }
            // re-enable the receive ISR
        }
    }
}


void resetBuffer()
{
    g_iWriteOffset = 0;
    g_iReadOffset = 0;
}


void receiveISR()
{
    /*  Receive the byte.
        Specific code for keeping the hardware happy goes here.
        Keelan, you've already posted it in the O.P.  I'll save some time and not repeat it.  */

    g_dataBuff[g_iWriteOffset] = newByte;

    ++g_iWriteOffset;       // advance the write offset
    if (g_iWriteOffset >= g_BUFF_LEN)   // have we got a buffer overflow?
    {
        g_iFlags |= COMM_BUFF_OVERFLOW;
        /*  Handling of errors (such as this overflow) is an interesting topic.
            However, is depends on the nature of the instrument.
            It's somewhat outside the sope of the question. */
    }
}

P.S.
On a related note, look into ping-pong buffers. This is handy when you have multi-byte packets (or commands), and you need to receive the whole packet before you can start processing it.

2x identical buffers (or more than 2x). ISR is filling one buffer until it detects the end of the command. Meanwhile, the main() loop processes the other buffer. If the ISR has received the next complete command and the main() is done with processing the previous command, then the buffer pointers are swapped.