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).