2
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$
  • 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).

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – ATE-ENGE May 30 '17 at 17:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.