Serial Communication Packet Organization for an Embedded System

Question

I have an embedded project and I am implementing a library to handle serial communication between 2 microprocessors. I have a structure in mind to have a scalable approach for packets that might get added as time passes.

There needs to be distinct packets that carry variable length information. Each type of packet is interpreted differently by the processor receiving it.

The reads are to be performed whenever a byte comes in. When a full packet comes consumer function is called to handle it asap.

I wanted to explain my approach and get some feedback on its' weaknesses and how It might be improved.

Current Approach:

• Create a structure that shows the data members and lengths.

#define PASSWORD_PACKET_BYTES   5
typedef union
{
    uint8_t PasswordBuffer[PASSWORD_PACKET_BYTES];
    struct
    {
        uint32_t Password;
        uint8_t  CRC;
    };
}password_packet_t;

• Create a union that holds all the types of packets and a buffer of uint_8.
• Create a structure that holds bytes read, current packet type being read(tag for the union),the escape state.

#define DISPLAY_PACKET_BYTES    16
typedef struct
{
    union
    {
        uint8_t SerialBuffer[DISPLAY_PACKET_BYTES];
        password_packet_t PasswordPacket;
    };
    struct
    {
        uint8_t CurrentOperation;
        uint8_t BytesRead;
        bool EscapeNext;
    };
}disp_receive_packet_t;

• Create an enum that shows the values for different types of packets(Also contains a value for the escape character).

enum
{
    RECEIVE_NO_OPERATION = 0xA0,
    ACK, 
    PASSWORD,
    VERSION_REQUEST,
    RECEIVE_ESCAPE,
}DISP_RECEIVE_OPERATIONS;

Implementation of the functions: -Whenever a byte is read, check if it is a new operation, if so change the union tag. -Else if escape character value set the escape flag. -Else push the data to the buffer, increment bytes read. Also call a check finished function for the current operation(switch statement). If finished, handle the packet and reset the control structure. If not, nothing happens.

void DispRead1B(void)
{
    static uint8_t ReadByte;

    ReadByte = (uint8_t)UARTCharGet(DISP_BASE);

    if(DisplayPacket.EscapeNext)
    {
        DisplayPacketPushByte(ReadByte);
        DisplayPacket.EscapeNext = false;
    }
    else
    {
        if(ReadByte == RECEIVE_ESCAPE)
        {
            DisplayPacket.EscapeNext = true;
        }
        else if((ReadByte < RECEIVE_ESCAPE) && (ReadByte > RECEIVE_NO_OPERATION))
        {
            DisplayPacketChangeOperation(ReadByte);
        }
        else
        {
            DisplayPacketPushByte(ReadByte);
        }
    }
}

void DisplayPacketChangeOperation(uint8_t Operation)
{
    DisplayPacket.CurrentOperation = Operation;
    DisplayPacketResetBytesRead();
    DisplayPacket.EscapeNext = false;
    DisplayPacketCheckOperationFinished();
}

void DisplayPacketPushByte(uint8_t Value)
{
    DisplayPacket.SerialBuffer[DisplayPacket.BytesRead] = Value;
    DisplayPacketIncrementBytesRead();
    DisplayPacketCheckOperationFinished();
}

void DisplayPacketIncrementBytesRead(void)
{
    DisplayPacket.BytesRead += 1;
    if(DisplayPacket.CurrentOperation == RECEIVE_NO_OPERATION)
    {
        DisplayPacket.BytesRead = 0;
    }
}

bool DisplayPacketCheckOperationFinished(void)
{
  bool PacketFinished = false;
  switch (DisplayPacket.CurrentOperation)
  {
      case ACK:
          PacketFinished = DisplayPacketCheckAckFinished(); break;
      case VERSION_REQUEST:
          PacketFinished = DisplayPacketCheckVersionRequestFinished(); break;
      case PASSWORD:
          PacketFinished = DisplayPacketCheckPasswordFinished(); break;
      default:
          return false;
  }
  if(PacketFinished)
  {
      DisplayPacketChangeOperation(RECEIVE_NO_OPERATION);
  }
  return PacketFinished;
}

bool DisplayPacketCheckAckFinished(void)
{
    if(DisplayPacket.BytesRead == ACK_PACKET_BYTES)
    {
        if(CheckSysFlagNotSet(KeepAliveAckReceived))
        {
          UpdateSysFlag(KeepAliveAckReceived,true);
          StartAliveTimer();
        }
        else
        {
          UpdateSysFlag(ConnectionError,false);
          ReloadAliveTimer();
        }
        return true;
    }
    return false;
}

bool DisplayPacketCheckVersionRequestFinished(void)
{

    if(DisplayPacket.BytesRead == VER_REQ_PACKET_BYTES)
    {
      UpdateSysFlag(FirmwareVerRequested,true);
      return true;
    }
    return false;
}

bool DisplayPacketCheckPasswordFinished(void)
{
    if(DisplayPacket.BytesRead == PASSWORD_PACKET_BYTES)
    {
        PasswordPacket = DisplayPacket.PasswordPacket;
        UpdateSysFlag(PasswordReceived,true);
        return true;
    }
    return false;
}

With this approach there is no serialization necessary because the union handles it. Also, syntax for copying packets is easy if there needs to be a backed up copy of the data. ie. PacketType = CommUnion.PacketType;

When a new packet is added. A struct is created and added to the union, new enum field is added and a case to the check finished switch statement that calls the function for this packet is added.

This requires the source code of the library to change on multiple lines which I am not fond of.

Is there a way to implement a similar functionality in a simpler way that requires external change while the compiled comm structure could stay the same?

Second Approach In Mind(Using Function Pointers):

• Get rid of the union. The control structure still stays because we need to keep track of current packet type and bytes read, escape etc.
• Get rid of enum method to create packet values. Create functions to register check finished functions for different packets and assign them values like the enum method. The register function is an array of function pointers to check functions.
• On startup all the packet structures are initialized.(Change in main program or project specific module not on the library.)
• Then all the packets are registered using the register function. This way the number of packets and the value of the escape character can be computed on runtime (or compile time if the compiler is smart).
• When you push a data byte the Check finished function pointer is used to call the function. This function serializes (or calls such function.) and handles the packet if the packet is fully received.

The benefit of this method is the instance of the packet is created (which needs to be done either way) and register is called. I feel like there is less things to change which should reduce human errors(ideally).

If anyone would discuss the advantages or disadvantages of these two methods with me or point me to a more streamlined way of doing things, I would appreciate it.

Answer 1

Data Structure Design

With the union, you will have to force it to bit wise (#pragma pack) to be sure that the data is correct. With this fix, then I think the union approach works fine since I have worked on similar implementations

But there are other things that I would be concerned about.

Variable Length Packets

There needs to be distinct packets that carry variable length information. Each type of packet is interpreted differently by the processor receiving it.

It will be MUCH easier for you to implement a fixed length packet where you ignore the trailing bits. If you do variable packet lengths, depending on the peripheral you are using, but I am assuming UART, you will have configure the driver to be able to determine when the packet has been completely received (I am assuming your EscapeNext flag). Otherwise, you will not know if the data received is valid or not.

With the escape flag, you have to be sure that your main payload will never have that same byte data, otherwise your driver is going to incorrectly assume that the packet has been completely received.

You can handle variable lengths with some type of peripheral receive time out that is triggered if too much time has passed between bits, but there are still many variables that can play with that type of implementation

** EDIT If you intend to still use variable length, then you definitely need to have a portion of your packet represent the length of the packet.

Using CRC vs Using Parity

To confirm integrity of your packet overall, you would need to use CRC. CRC can confirm the overall validity of your packet since you may not know if you are missing a single byte in the overall packet.

Parity is only used at the byte level and since the peripheral doesnt care if the data it needs to send out is meant to be garbage or not, it will not know if it screwed up the packet as a whole.

Protip: fixed length packets = less debug time

Using faster baud rate and fixed length packets has the benefit of being less complicated and therefore takes less time to debug. More than likely this will outweigh the benefits you get from using variable length packets in terms of packet transmission time . At 921k baud rate, you should be able to transmit a byte every 8 or 9 microseconds. If you are dealing with less than 10 bytes, then you will save .1 milliseconds per packet or less.

Protip: DMA

I do not know what MCU or library you are using, but ideally you could setup DMA with a fixed length packet and configure a DMA Rx Complete interrupt. The DMA will allow you move the bytes to another memory location and let you know once it is full (again depending upon the MCU). So there is less to monitor, but more upfront configuration.