-3

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

  • 2
    What errors and warnings does the compiler give? Which lines are we looking at? – Mark Smith May 16 '17 at 21:51
  • 2
    I downvoted this question because it does not incorporate suggestions from comments and answer at byte Array to CRC calculation, your previous question. – James Waldby - jwpat7 May 17 '17 at 4:48
  • 2
    I'm voting to close this question as off-topic because it is not an Arduino specific question, it is asking for basic optimisation pointers that would be better answered elsewhere. – Code Gorilla May 17 '17 at 12:11
  • This question is better suited for the code review SE site. – sa_leinad May 17 '17 at 14:39
2

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.

  • Ok, I'll check there, Also I guess I missed something from my previous question as well. In terms of formatting, where can I find style guidelines that would improve my code readability? – ATE-ENGE May 17 '17 at 12:17

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