Search code examples
cembeddedfirmwaremsp430

Embedded C UART conventions

I need advice on proper way of handling UART communication. I feel like I've done handling sending serial commands over UART well but I don't know if the way I'm parsing the response or receiving serial data is the best way to do it. Any tips are appreciated but I just want to know if there's a better more and elegant way to parse UART RX.

This is for an MSP430 uC by the way...

First I have these declared in the header file:

const unsigned char *UART_TX_Buffer;
unsigned char UART_TX_Index;
unsigned char UART_TX_Length;
unsigned char UART_TX_Pkt_Complete;

unsigned char UART_RX_Buffer[25];
unsigned char UART_RX_Pkt_Complete;
unsigned char UART_RX_Index;

Here is the function that is called once the flag UART_RX_Pkt_Complete is set in the ISR:

void Receive_Resp()
{
    switch (UART_RX_Buffer[UART_RX_Index - 3])
    {
    case 0x4B:
        break;
    case 0x56:
        P1OUT &= ~(tos_sel0 + tos_sel1);
        break;
    case 0x43:
        P1OUT |= tos_sel0;
        P1OUT &= ~tos_sel1;
        break;
    case 0x34:
        P1OUT |= tos_sel1;
        P1OUT &= ~tos_sel0;
        break;
    case 0x33:
        P1OUT |= tos_sel0 + tos_sel1;
        break;
    default:
        break;
    }
    UART_RX_Pkt_Complete = 0;
    UART_RX_Index = 0;
}

For reference here's the RX ISR:

#pragma vector=USCIAB0RX_VECTOR
 __interrupt void USCIA0RX_ISR(void)
 {
     UART_RX_Buffer[UART_RX_Index++] = UCA0RXBUF;

     if (UART_RX_Buffer[UART_RX_Index - 1] == 0x0A)
    {
        UART_RX_Pkt_Complete = 1;
        _BIC_SR_IRQ(LPM3_bits);
    }
    IFG2 &= ~UCA0RXIFG;
 }

Also here's the TX ISR and send UART command routine:

    if (UART_TX_Index < UART_TX_Length)                                 // Check if there are more bytes to be sent
    {
        UCA0TXBUF = UART_TX_Buffer[UART_TX_Index++];
    }

    else                                                                // Last byte has been sent
    {
        UART_TX_Pkt_Complete = 1;                                       // Set flag to show last byte was sent
        _BIC_SR_IRQ(LPM3_bits);
    }
    IFG2 &= ~UCA0TXIFG;



void Send_CMD (const unsigned char *Data, const unsigned char Length)
{                                                       
    UART_TX_Buffer = Data;                                                  // Move into global variables
    UART_TX_Length = Length;
    UART_TX_Pkt_Complete = 0;                                               // Starting values
    UART_RX_Pkt_Complete = 0;
    UART_TX_Index = 0;

    UCA0TXBUF = UART_TX_Buffer[UART_TX_Index++];

    while(!UART_TX_Pkt_Complete)
    {
        Delay(5,'u');
    }

    while(!UART_RX_Pkt_Complete)
    {
        Delay(5,'u');
    }
}
Solution

  • If this works and meets your system's requirements then it's fine. But there are several ways it could be improved.

    • Receive_Resp() and USCIA0RX_ISR() are tightly coupled, which is undesirable. They both manipulate UART_RX_Index for the other (USCIA0RX_ISR() increments it and Receive_Resp() clears it) and they both rely on the other for part of the framing of each message (USCIA0RX_ISR() finds the end of the frame while Receive_Resp() interprets and resets for the next frame). It would be better if these routines were decoupled.
    • The character buffer should be a circular buffer with a head pointer (where characters get added) and a tail pointer (where characters get removed). The ISR should only add chars to the circular buffer and advance the head pointer. The ISR should also handle wrapping of the head pointer back to the beginning of the circular buffer. And the ISR should protect from overruns by making sure the head pointer doesn't pass the tail pointer.
    • The Receive routine should be responsible for framing the message. This includes pulling chars from the tail pointer and identifying the beginning and end of the message. The Receive routine increments and wraps the tail pointer. Typically the Receive routine is implemented as a state machine with states for identifying the start, body, and end of a frame.
    • For even less coupling you might have a separate function that interprets the content of the message (i.e., separate the framing from the interpretation of the message).
    • Your ReceiveResp() routine doesn't handle any errors such as a dropped character. It assumes that all three characters were received correctly. Maybe that's fine for your application and requirements. But typically there should be some error checking performed here. At the very least you should ensure that UART_RX_Index >= 3 before you subtract 3 from it. In other words, make sure the message length is sane. A more robust serial protocol would have a checksum or CRC in each frame to ensure the frame was received correctly but that is probably overkill for your application.
    • Your Transmit side could be improved with some of the same advice. Typically there is a circular buffer for transmit chars. The Transmit routine adds chars to the buffer and manages the head pointer. The TX ISR copies chars from the buffer to the UART and manages the tail pointer.

    Do the calls to Delay in Send_CMD() mean that your application is totally stalled while it's waiting to finish the transmission? Again, maybe that's OK for your application but typically that is undesirable. Typically you want the application to continue to function even while it's waiting for the UART to be ready to transmit. By using a circular transmit buffer, it would be possible for multiple messages to queue up in the transmit buffer and you wouldn't have to wait for the previous message to finish before queuing up another. But then you should add protection for a buffer overrun and this may complicate things unnecessarily for you. Which brings me back to my first point, if what you have works and meets your requirements then it is fine.