Arduino Create and send data to Serial Port (Software Serial function)

Question

I have this code written and functioning that will write out the chars in cmd to a serial port for an input of two int values. As the majority of the byte values fed are constant, I created the following code that only requires receiving the values that change frequently.

void Create_Message(int Address, int Data)
{
 //Local Vars
 Address--;
 byte SvAdd = 1;
 byte FuCode = 6;
 byte AddHi = Address>>8;
 byte AddLo = Address;
 byte DatHi = Data>>8;
 byte DatLo = Data;
 byte AdDa[] = {SvAdd,FuCode,AddHi,AddLo,DatHi,DatLo};
 unsigned int crc=CRC(AdDa,sizeof(AdDa)/sizeof(AdDa[0]));
 byte CrcHi = crc & 0xFF;
 byte CrcLo = crc >> 8;
 /*U8 Slave Address, U8, Function Code, U16 Address, U16 Data, U16 CRC
 * to
 *[U8 Address Split High, U8 Address Split Low, U8 Data Split High, U8 Data Split Low, U8 Function Code]
 */
 //Send Data
 unsigned char cmd[8]={SvAdd,FuCode,AddHi,AddLo,DatHi,DatLo,CrcHi,CrcLo};
 digitalWrite(SSerialTxControl, RS485Transmit); // Enable RS485 Transmit
 lcd.setCursor(0, 1); 
 for (int i=0; i< 8; i++)
 {
 RS485Serial.write(cmd[i]); // Send string someplace
 /*//Serial.print(cmd[i]); //echo back (debug)
 if (i>1)
 {
 lcd.print(cmd[i]);
 lcd.print(" ");
 }*/
 }
 digitalWrite(SSerialTxControl, RS485Receive); // Disable RS485 Transmit 
}
unsigned int CRC(byte AdDa[], int Size )
{
 unsigned int crc16 = 0xFFFF;
 int pos = 0;
 int i = 0;
 /*Write CRC*/
 for (pos = 0; pos < Size; pos++) 
 {
 crc16 ^= AdDa[pos]; // XOR byte into least sig. byte of crc
 for (i = 8; i != 0; i--) // Loop over each bit
 { 
 if ((crc16 & 0x0001) != 0) // If the LSB is set
 {
 crc16 >>= 1; // Shift right and XOR 0xA001
 crc16 ^= 0xA001;
 }
 else // Else LSB is not set
 crc16 >>= 1; // Just shift right
 }
 }
 /*Note, this number has low and high bytes swapped,
 so use it accordingly (or swap bytes)*/
 return crc16;
}

I'm pretty new to writing code, but want to try and make this as efficient as possible. Any pointers as to how I could better optimize this function would be greatly appreciated.

Answer 1

• CreateMessage is a misnomer. The message it creates vanishes as soon as the function returns. Its real purpose it to send a message. So call it appropriately.

• Too many comments duplicating the functionality. For example, any time you feel you need a comment as in

  digitalWrite(SSerialTxControl, RS485Transmit); // Enable RS485 Transmit
  

  it means that you really need a function

  void enableRS485Transmit(....) {
   digitalWrite(SSerialTxControl, RS485Transmit);
  }
  

• The inner loop of CRC computation can be simplified. Notice that no matter what, the first thing you do is crc16 >>= 1. Factor it out:

   for (i = 8; i != 0; i--) {
   crc16 >>= 1;
   if (crc16 & 0x0001) {
   crc16 ^= 0xA001;
   }
   }
  

  Granted, the compiler will likely notice that. Still it is good for the reviewers and maintainers.

• The function takes 24 bytes of stack space. Seems like a bit too much. Granted, the compiler would likely optimize out most of it. Still, consider

   enum { Slave, Function, AddHi, AddLo, DataHi, DataLo, CrcHi, CrcLo, MsgSize};
   byte message[MsgSize] = {1, 6, Address>>8, Address, Data>>8, Data};
   unsigned int crc = CRC(message, CrcHi);
   message[CrcHi] = crc;
   message[CrcLo] = crc >> 8;
  

• If your compiler has <stdint.h>, I strongly recommend using an explicit uint16_t rather then unsigned int (of unspecified bit width).

• ATE-ENGE: Wanted to come back and point out that The inner loop of CRC computation can be simplified. Notice that no matter what, the first thing you do is crc16 >>= 1. Factor it out: Won't work because you need to test before you do the shift.