Create an a message from two inputs[Programming Help] [closed]

Question

I'm a bit new to this and I have the following function that I'm having trouble getting it to compile. The intent is to create a message to write out to a software serial port. Inputs are two int values of Function address and the data passed to the device while the output is a series of char characters

void Create_Message(int Address, int Data)
{
  byte S1Add = 1;
  byte FuCode = 2;
  byte AddHi = Address>>8;
  byte AddLo = Address;
  byte DatHi = Data>>8;
  byte DatLo = Data;
  byte AdDa[] = {AddHi,AddLo,DatHi,DatLo};
  unsigned int crc=CRC(AdDa,4);
  byte CrcHi = crc >> 8;
  byte CRCLo = crc & 0xFF;
 /*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]
 */
 unsigned char cmd[]={S1Add,FuCode,AddHi,AddLo,DatHi,DatLo,CrcHi,CRCLo};

  digitalWrite(SSerialTxControl, RS485Transmit);  // Enable RS485 Transmit   
  for (int i=0; i< clen; i++)
  {
    RS485Serial.write(cmd[i]);   // Send string someplace
    Serial.write(cmd[i]); //echo back
  }
  digitalWrite(SSerialTxControl, RS485Receive);  // Disable RS485 Transmit    
}

byte 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)*/
  byte crc[]={crc16>>8,crc16};
  return crc;
}

Any help with optomizition would also be greatly appreciated :)

Answer 1

If you want help with optimisation then "Code Review" would be the better forum.

Its very difficult to optimise code that's hard to read. You have used an inconsistent formatting style (which might be because of the post but your failure to fix it indicates a lack of interest in the answers.

You passed all you function arguments by value, you should pass them by constant reference.

You are doing a number of implicit casts, because you have mismatched types.

You are sending a byte to serial.write, what happens if the value is outside of the printable range, it would be better to print this as a hex number.

I assume someone has already pointed out CRC should return a byte*.

As for the algorithm, the code isn't readable enough to comment on that.