Dynamic length binary data parsing

Question

I am currently writing code for a UART communication in my firmware. I am getting an interrupt every byte received (meaning I don't get a chunk of data, 1 byte at a time but, in the sample below I store them into a chunk of data for simulating my data parsing only).

I have a dynamic message length format, as shown in the code below. May I know what is the most efficient and elegant way to parse this message? I believe my current code could be further improved.

#include <stdint.h>

typedef struct {
    uint8_t header_DLE;
    uint16_t id;
    uint8_t msg_command;
    uint16_t msg_length;
    uint8_t data[400];
    uint8_t footer_DLE;
    uint8_t footer_BCC; 
} Serial_Rx_Data;

Serial_Rx_Data _rx_data;
static int _rx_index;
static uint8_t _rawData[512];

int main()
{
    uint8_t data[18];

    //Message format
    data[0] = 0x10;     //DLE   
    data[1] = 0x03;     //ID        
    data[2] = 0;        //ID        
    data[3] = 0x03;     //Command   
    data[4] = 0x03;     //Length - determines the data length
    data[5] = 0;        //Length    
    data[6] = 0x41;     //Data      
    data[7] = 0x42;     //Data      
    data[8] = 0x43;     //Data      
    data[9] = 0x03;     //DLE       
    data[10] = 0xfa;    //BCC

    //Simulate getting data every byte
    for (int i = 0; i <= 10; i++) {
        _rawData[_rx_index] = data[i];

        if (_rx_index == 5) {
            _rx_data.header_DLE = _rawData[0];
            _rx_data.id = _rawData[2] << 8 | _rawData[1];
            _rx_data.msg_command = _rawData[3];
            _rx_data.msg_length = _rawData[5] << 8 | _rawData[4];           
        }

        //Get the length of data
        if ((_rx_index == 6) && (_rx_data.msg_length > 0)) {
            for (int j = 0; j < _rx_data.msg_length; j++) {
                _rx_data.data[j] = _rawData[6 + j];
            }
        }

        //Get the last 2 byte
        if (_rx_index == (_rx_data.msg_length + 6 + 1)) {
            _rx_data.footer_DLE = _rawData[6 + _rx_data.msg_length];
            _rx_data.footer_BCC = _rawData[6 + _rx_data.msg_length + 1];

            //Complete parsing
            //Callback to other function
        }

        _rx_index++;
    }

    return 0;
}

Answer 1

Here are some things that may help you improve the quality of your code.

Don't handle message parsing in an interrupt

You don't explicitly say it, but from the context, it appears that you're attempting to parse a serially received message in an interrupt handler. This is usually not a good idea. An interrupt handler should be fast and do the minimum amount of work possible. For serial message protocols, this typically means reading each character, putting it into a memory buffer (most often implemented as a circular buffer) and updating pointers. Then the main routine that is NOT in an interrupt reads the contents of the buffer and does any message parsing/handling.

Don't parse until you have a complete message

In this case the message length appears to be 8 bytes (6 header + 2 trailer) plus the size of the enclosed data. It also appears that there is some kind of special marker for the beginning of the message. So a quick check for the message parser would be something like this:

1. are there at least 8 bytes?
2. is the first character the message marker byte?

If the answer to either question is "no," it can't be a real message and so the data may be left in the buffer until more is received. If the answer to both questions is "yes," then it might be a message. The next steps might be:

3. does the message have at least 8+msg_length bytes?
4. is the checksum correct?

If the answer to both questions is "yes" then you can actually act on the message (typically by passing it to some higher layer protocol handler).

Use a state machine

The typical way to parse messages like this is to use a state machine. In this case the states might be:

1. waiting for message marker byte
2. receiving header
3. receiving data
4. receiving trailer

There are also often states for error and for timeout.

Avoid the use of "magic numbers"

This code is littered with "magic numbers," that is, unnamed constants such as 400, 512, 10, 5, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "5" and then trying to determine if this particular 5 means an offset into the message structure or some other constant that happens to have the same value.

Beware of buffer overflow

The allocation of a fixed 400 bytes for data might be OK, but if so, then you absolutely need to make sure that the message will fit in that space before copying it. Failure to do so leads to the possibility of buffer overflow.