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