0
\$\begingroup\$

I have an ISR putting in bytes from the UART on an AVR XMEGA into a circular buffer. Every byte received, also increments RX_COUNT. I copy the circular buffer into a local buffer if it is at least two bytes for a potential "#A" command.

The protocol I am using is #A, #B, #C, etc. The problem is I am searching for the sync char, '#' and they seeing what the next byte is, and checking if it is a valid command. The issue is, sometimes if I send a string like aaaaaaa#A it doesn't execute the command. I'm simulating garbage data to see if it will ever sync to the byte. How can I make sure that I will eventually sync to the start of the message?

Periodically, I have from my main function, check how much data is available in the circular buffer. If there is at least 2 bytes available, I copy the circular buffer into a local buffer:

USART_RXBufferData_Available() returns true or false is there is any data in the circular buffer.

Another question is, do I even have to copy the circular buffer into a local buffer? What is generally the best practice to decode a serial packet similar to this?

    #define MIN_MSG_SIZE 2 // Command is #A, #B, #C, etc...
    bool dataAvailable = false; 

    ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
    {

        // Copy Circular buffer into local buffer

        if (USART_RXBufferData_Available(&UARTCom1))
        {
            if (UARTCom1.buffer.RX_Count >= MIN_MSG_SIZE)
            {
                for (int i = 0; i < MIN_MSG_SIZE; i++)
                {
                    localBuff[i] = USART_RXBuffer_GetByte(&UARTCom1);

                    UARTCom1.buffer.RX_Count--; // decrement RX count
                }

                dataAvailable = true;
            }
        }   

        else

        {

              dataAvailable = false;

        }
   }

if ((dataAvailable) && localBuff[0] == '#') // Check if there is a new Message AND Check Sync Byte
{   
        uint8_t data = localBuff[1];
        PORTR_DIR = 0b00000011;

        char led0Msg[] = "Turning ON LED0\r\n";
        char led1Msg[] = "Turning ON LED1\r\n";
        char ledoffMsg[] = "Turning OFF LEDS\r\n";

        switch (data)
        {
            case 'A':
            UART->transmit(led0Msg, strlen(led0Msg));
            PORTR_OUT = ~(PIN0_bm); // Turn on LED0
            break;

            case 'B':
            UART->transmit(led1Msg, strlen(led1Msg));
            PORTR_OUT = ~(PIN1_bm); // Turn on LED1
            break;

            default:
            UART->transmit(ledoffMsg, strlen(ledoffMsg));
            PORTR_OUT = (PIN1_bm | PIN0_bm); // Turn off BOTH LEDS

        }

        memset(localBuff, 0, sizeof(localBuff)); // Clear the buffer
        return true;

    }
\$\endgroup\$
  • \$\begingroup\$ You say that you're "searching for the sync char, '#'", but I don't see where you're doing that? Also, where is localLan1Buff and how do you know it has '#' at index 0? You're also assuming that your command character will be in localBuff[1], but will it be every time? Also, don't see where you are setting dataAvailable back to false. \$\endgroup\$ – DigitalNinja Oct 4 '16 at 18:44
  • \$\begingroup\$ I searched for it in if ((dataAvailable) && localBuff[0] == '#'). I figured it should always be the first char right? or do I have to actually parse through the buffer for '#'? . I know I assume, that is the basis for my question, how can I make it so it can recover and sync to data if it isn't at localBuff[1] \$\endgroup\$ – zacharoni16 Oct 4 '16 at 19:08
  • \$\begingroup\$ If you're sending something like "aaaaaaa#A" or for some reason you get garbage characters, then with your current code you will have 'a' in localBuff[0] and potentially also in localBuff[1]. You could also end up with 'a' in localBuff[0] and '#' in localBuff[1]. \$\endgroup\$ – DigitalNinja Oct 4 '16 at 19:35
  • \$\begingroup\$ So I should clear localBuff if localBuff[0] != '#' ? \$\endgroup\$ – zacharoni16 Oct 4 '16 at 19:41
  • 1
    \$\begingroup\$ "If there is at least 2 bytes available, I copy the circular buffer into a local buffer" - Not a good idea. What if I send a#AB? Possibly, what you'll see in your local buffer is a# and AB, while you'd want #A. So don't copy from the circular buffer this way. \$\endgroup\$ – JimmyB Oct 5 '16 at 14:31
1
\$\begingroup\$

Creating your own protocol and processing messages can sometimes be tricky (unexpected results), and you won't notice the details until you start testing and debugging. The best thing you can do is define what your message should look like, and try to make the processing as robust as possible. You can also use things like a CRC or checksum, and/or acknowledgements. This is a quick/short example and does not use any of that.

Normally, it's good to have a start byte and an end byte, so it's easy to detect the beginning of a message and then know when the message is complete/was sent successfully. In your case since you only have a single byte for the command/message the ending byte isn't really necessary.

However, you say minimum message size, so if it's going to end up being longer I'd definitely add an ending byte. I'm going to show it with an ending byte. You can remove it if you want, and adjust the code accordingly.

So let's define your message: #cmd!, where '#' is your start byte, '!' is the end byte, cmd is what you're interested in, and the length is 3.

#define MIN_MSG_SIZE 3 
#define START_BYTE 0x23 //'#' 
#define END_BYTE 0x21   //'!'

Also, to make it easy, use a few global variables to control you processing. Like this:

bool bMsgStart = false;
bool bExpectingEnd = false; // helpful when you know the message should be ending
uint8_t cmd = '\0';         // This could also be a buffer of known message length if your are interested in more than one byte for the command (e.g. command and parameter)

Then for processing the received bytes, there are many ways you can do this depending on your needs, but try something like this:

while (USART_RXBufferData_Available(&UARTCom1))             /* Loop until empty */
{
    uint8_t byte = '\0';                                    /* temp storage for the received byte */

    if (UARTCom1.buffer.RX_Count > 0)
    {
        byte = USART_RXBuffer_GetByte(&UARTCom1);           /* Get the byte */

        if(bMsgStart)
        {
            if(bExpectingEnd && (byte == END_BYTE))         /* End of message */  
                processMessage(cmd);                        /* Process the message now */
            else if(!bExpectingEnd)                         /* Message buffer isn't full but the message could be complete */
            {
                cmd = byte;                                 /* Since you know the message has started and hasn't ended, this is the byte you are interested in */
                /* If you were interested in more than one byte for the command/message this is where you'd add it to the buffer and increment the index until you've received everything in between the start byte and end byte */
                bExpectingEnd = true;                       /* The next byte should be the end byte -> Set the message end flag */  
            }
            else if((bExpectingEnd && (byte != END_BYTE))   /* Something went wrong or you received a bad message -> start over */
                 MsgBytesInit();                            /* Reset */ 
            else
                MsgBytesInit();                             /* Shouldn't get here but just in case -> Start over */
        }
        else if(byte == START_BYTE)
            bMsgStart = true;                               /* Set the start of message flag */    
        else
            MsgBytesInit();                                 /* Not what we are looking for or isn't the start of a message -> Reset */
    }
}


void MsgBytesInit()
{
    bMsgStart = false;          /* Reset the message start flag */
    bExpectingEnd = false;      /* Reset the message end flag */
    cmd = '\0';                 /* Reset the command (or message buffer)*/                  
}


void processMessage(uint8_t cmd)
{
     PORTR_DIR = 0b00000011;

    char led0Msg[] = "Turning ON LED0\r\n";
    char led1Msg[] = "Turning ON LED1\r\n";
    char ledoffMsg[] = "Turning OFF LEDS\r\n";

    switch(cmd)
    {
         case 'A':
         {
            UART->transmit(led0Msg, strlen(led0Msg));
            PORTR_OUT = ~(PIN0_bm); // Turn on LED0
         }
         break;

        case 'B':
        {
            UART->transmit(led1Msg, strlen(led1Msg));
            PORTR_OUT = ~(PIN1_bm); // Turn on LED1
        }
        break;

        default:
        {
            UART->transmit(ledoffMsg, strlen(ledoffMsg));
            PORTR_OUT = (PIN1_bm | PIN0_bm); // Turn off BOTH LEDS
        }
        break;
    }

    /* Done with the message */
    MsgBytesInit(); /* Reset */             
}
\$\endgroup\$
  • \$\begingroup\$ Thanks was able to get my code working based on your suggestions ! \$\endgroup\$ – zacharoni16 Oct 4 '16 at 22:59

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.