4
\$\begingroup\$

I'm implementing a read-only I2C 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?

\$\endgroup\$
  • \$\begingroup\$ Yes I do, and have done so routinely. \$\endgroup\$ – Olin Lathrop Apr 15 '13 at 17:13
  • \$\begingroup\$ @OlinLathrop if you have something to add to the existing answer, please do so! \$\endgroup\$ – user17592 Apr 15 '13 at 19:37
2
\$\begingroup\$

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
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I've thought about pointers too, but have no idea how the code would look like. Could you give me a basic example? C is indeed quite generic. \$\endgroup\$ – user17592 Apr 14 '13 at 16:06
  • \$\begingroup\$ Added some code. I guess it's technically not a real "pointer", more a counter. \$\endgroup\$ – fm_andreas Apr 14 '13 at 16:16
  • \$\begingroup\$ Thanks! This was really helpful. For reference, ReadI2C() in slave mode doesn't do more than waiting for the Buffer Full flag to be set and then return the buffer. (I only used this function instead of just reading the register for readability.) \$\endgroup\$ – user17592 Apr 14 '13 at 17:26
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I don't see how atomic operations might be relevant here. AFAIK they are primarily important for read-modify-write operations, if one is trying to modify individual bits of a byte, and an ISR could potentially alter some of the other bits. None of this should happen here. \$\endgroup\$ – fm_andreas Apr 15 '13 at 8:00
  • 1
    \$\begingroup\$ @fm_andreas: Using the ring-buffer in the main-loop may require to disable and enable interrupts to make that code atomic against interrupt handler code. I took a look at the other answers and Nick is more or less referring to what i mean in his code comments "disable the receiveISR. If the ISR occurs in this block, it can corrupr the buffer". \$\endgroup\$ – Rev1.0 Apr 15 '13 at 10:55
  • \$\begingroup\$ Yes, in his burst buffer implementation this may very well be true, but the beauty of a ring buffer is that new data can be written into the buffer (with an ISR) even while the main loop is processing the data (as long as the buffer size is big enough). Again, this is the beauty of a ring buffer: you can start processing incoming data with the first byte, and still receive new data. \$\endgroup\$ – fm_andreas Apr 15 '13 at 11:50
  • \$\begingroup\$ @fm_andreas: The ring-buffer still has to manage at least two multi-byte pointers (probably 16 bit). The mentioned PIC is a 8-bit controller, thus there are multiple writes required to update any pointer. From what i understand, this is not "thread safe". \$\endgroup\$ – Rev1.0 Apr 15 '13 at 13:00
0
\$\begingroup\$

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:

enter image description here

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.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ You can cut out a decision by using a modulo instead of the comparison, and if your buffer is a power of two in size its very efficient. \$\endgroup\$ – Scott Seidman Apr 15 '13 at 1:11
  • \$\begingroup\$ E.g., readpointer = (readpointer + 1) % buffersize \$\endgroup\$ – Scott Seidman Apr 15 '13 at 1:14
  • 3
    \$\begingroup\$ makes the code kind of sweet looking, huh? In general, if a non-branch statement can be used, it usually makes the code more readable and can often cut clock ticks. If it makes the code less readable, then you have to decide if its worth it. When I program, I spend time trying to make it understandable by two people: Me, and Me 6 weeks after I'm done! \$\endgroup\$ – Scott Seidman Apr 15 '13 at 15:33
  • \$\begingroup\$ if (buffer[writePointer] == 0x00) I don't think that's a good condition to check if there is no data. 0x00 could be a valid data.. \$\endgroup\$ – m.Alin Jun 11 '13 at 12:15
0
\$\begingroup\$

@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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Wouldn't implementing a ring buffer be better? For example, for when the buffer is 32 chars and there have been inserted 31 and they've been read out as well. When there would come in two chars very fast, that would create an overflow with this code, but not with a ring buffer. Am I right, or am I missing something? \$\endgroup\$ – user17592 Apr 14 '13 at 19:32
  • \$\begingroup\$ @CamilStaps You're correct from the computer science perspective. But let's look at the problem from the systems perspective. If the slave processes the data more slowly than the master sends it, the buffer will eventually overflow. A 32-byte ring buffer may take a 100ms longer to overflow than a 32-byte plain buffer. \$\endgroup\$ – Nick Alexeev Apr 14 '13 at 19:41
  • \$\begingroup\$ I understand. I'm saying this because the end application will probably send the data in large bulks, and then wait a long time. When the bulks are of a different size (but smaller) than the buffer, that would cause problems with the plain buffer, whilst it wouldn't with the ring buffer - if I understand correctly. Still, +1 for a clear code & explanation! \$\endgroup\$ – user17592 Apr 14 '13 at 19:44
  • \$\begingroup\$ @CamilStaps Added on ping-pong buffers to my post. You may find it interesting. \$\endgroup\$ – Nick Alexeev Apr 14 '13 at 20:05
  • \$\begingroup\$ Very interesting indeed. Not likely to get implemented in this project, but I'll put that in the little box in my brain labelled 'fun and useful stuff'. Thanks! \$\endgroup\$ – user17592 Apr 14 '13 at 20:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy