If this works and meets your system's requirements then it's fine. But there are several ways it could be improved.
Receive_Resp()
andUSCIA0RX_ISR()
are tightly coupled, which is undesirable. They both manipulateUART_RX_Index
for the other (USCIA0RX_ISR()
increments it andReceive_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 whileReceive_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 thatUART_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.