Help with some code

Hi all,

Below is some code that I have written with the aid of some people here and over on AVRFreaks. Everything above the line that says //EVERYTHING ABOVE THIS LINE WORKS works (funny about that).

I am having trouble with whats below that line.

What I am trying to achieve is to read a serial "packet", 8 bytes consisting of 7 bytes + one checksum byte. Each byte is RXed 5ms apart and then ~60ms before the next packet arrives. Baud rate is 7800 but for testing I have set it to 9600. 8N1. I have created a test TXer on a Pro Micro and it does exactly as it should.....Now. So the data is TXed into the UNO but thats where the fun starts. It should then echo these bytes out again BUT replace byte 5 with the current gear reading. Echo byte 6 and then recalculated byte 7 checksum.

Could someone please go over this code and slap me around the head a few times so it sinks in as to what I have done wrong.

//Wondatre R1.0

#include "Wire.h"
#include <EEPROM.h>
#include <SoftwareSerial.h>

SoftwareSerial MySerial(2, 3); // RX, TX

uint8_t indx;
uint8_t buf[8];
uint8_t bufIdx;
uint8_t new_byte;
uint8_t x;
uint8_t gear_in;
uint8_t Gear;
int sensorVal;
uint8_t out1 = 5;
uint8_t out2 = 6;
uint8_t rxd = 13;
uint8_t txd = 14;
uint8_t gearN = 0x00;
uint8_t gear1 = 0x20;
uint8_t gear2 = 0x40;
uint8_t gear3 = 0x60;
uint8_t gear4 = 0x80;
uint8_t gear5 = 0xA0;
uint8_t ECU_byte;
int checksum;
int c;

void setup()

//stolen from PIGPI

{

  delay(500); //wait to settle
  pinMode(out1, OUTPUT);
  pinMode(out2, OUTPUT);
  pinMode(rxd, INPUT_PULLUP);
  pinMode(txd, OUTPUT);
  pinMode(A0, INPUT);

  // Read sensor value

  sensorVal = analogRead (A0);

  //Save Gear to EEPROM

  if (sensorVal < 665 )
  {
    // No Change
    Gear = EEPROM.read(0); //Load gear from EEPROM
  }
  else if  (sensorVal > 665 and sensorVal <= 800)
  {
    Gear = 1; //4th Gear
  }
  else if  (sensorVal > 800 and sensorVal <= 905)
  {
    Gear = 2; //5th Gear
  }

  EEPROM.update(0, Gear); // Only writes if different

  switch (Gear)
  {
    case 1:  digitalWrite(out1, HIGH); break;
    case 2:  digitalWrite(out2, HIGH); break;
  }

  Serial.begin(9600);
  MySerial.begin(9600);

}

void loop()
{
  sensorVal = analogRead (A0);

  Gear = EEPROM.read(0); //Load gear from EEPROM

  if (sensorVal > 300)

    switch (Gear)
    {
      case 1:  digitalWrite(out1, HIGH); break;
      case 2:  digitalWrite(out2, HIGH); break;
    }

  gear_in = analogRead (A0);      // read gear
  /*  Serial.println(Gear);           // For testing
    Serial.println(gear_in);        // For testing
    delay(1000);                    // For testing
  */

  if (gear_in <= 300)
  {
    new_byte = gearN;
  }

  else if (gear_in > 300 and gear_in <= 420)
  {
    new_byte = gear1;
  }

  else if (gear_in > 420 and gear_in <= 540)
  {
    new_byte = gear2;
  }

  else if (gear_in > 540 and gear_in <= 665)
  {
    new_byte = gear3;
  }

  else if (gear_in > 665 and gear_in <= 800)
  {
    new_byte = gear4;
  }

  else if (gear_in > 800 and gear_in <= 905)
  {
    new_byte = gear5;
  }



  //EVERYTHING ABOVE THIS LINE WORKS




  // This bit is to check every 100ms for the next 8 bytes. Will work this out later.


  //wait for start of 8 byte packet.
  findRxSilence();


  //next 8 bytes should be a 'packet'

  //first 5 just echo out
  for ( uint8_t i = 0; i < 5; i++ ) echoRx();

  //byte 6 replace with gear, calc checksum adjust

  getRx();
  uint8_t checksumAdjust = c - new_byte;
  MySerial.write(checksumAdjust);

  //byte 7 echo
  echoRx();

  //byte 8 replace checksum
  Serial.write( c + checksumAdjust );

  //done, do again

}
void getRx()
{
  int c;
  while (c = MySerial.read(), c == -1 ) {}
  return c;
}

void echoRx()       //wait for rx, echo it, blocking
{
  getRx();
  MySerial.write( c );
}


void findRxSilence()
{
  //wait for start of 8 byte packet, 30ms of silence should do
  // [8x5ms=40ms...][60ms silence]...
  //9600baud is ~1ms+ per byte, using delay(1) will be fine

  for ( uint8_t i = 30; i; i--, delay(1) ) {
    if ( MySerial.read() == -1 ) continue;
    i = 30; //start over if received something
  }
}

I've never seen this syntax:

while (c = MySerial.read(), c == -1 )

Is this legal?

Why not:

while ( MySerial.read() == -1 );

?

No idea.

I am in the middle of some laser etching so I will give it a try when it finishes.

Thanks.

I looked at your OP but didn't see the actual problem you're having; you describe what you want it to do but I didn't see you describe what it actually does (or am I just blind and/or too tired...?)

Sorry. I am half asleep at the moment.

It doesn't seem to do anything. I have tried printing the output but it is all zeros. I will add the print stuff later and post the output of that.

When I compile it I get a warning:

void getRx()
{
    int c;
    
    while (c = MySerial.read(), c == -1 ) {}
    return c;
}

getRx() returns void but you're trying to return the character read.

Try int getRx() instead.

This is nonsense:

while (c = MySerial.read(), c == -1 ) {}

I'm thinking it will never exit the while loop.
See definition #1 here: Comma Operator

I suppose this is what happens when you rely on experts. I appreciate all there help but it is only help if it helps.

I still have 3 hours for this laser to finish what it is doing then I can have a play.

See definition #1 here: Comma Operator

Very possible that I'm missing something here, but what's the point of an operator that

evaluates its first operand and discards the result

?

https://forum.arduino.cc/index.php?topic=396450

Blackfin:
I've never seen this syntax:

while (c = MySerial.read(), c == -1 )

Is this legal?

Why not:

while ( MySerial.read() == -1 );

?

I changed this as I too thought it didn't look right.

Question. This line has == -1 but I have seen >0 used in a similar way with serial.read. What is the difference?

Blackfin:
When I compile it I get a warning:

void getRx()

{
    int c;
   
    while (c = MySerial.read(), c == -1 ) {}
    return c;
}




getRx() returns void but you're trying to return the character read.

Try int getRx() instead.

Did this too. Now compiles. I also changed "c" to RxByte so it was a bit easy to read, for me at least.

I hooked up logic analyser and it confirms there is no output on MySerial. I shall keep playing.

windoze_killa:
I changed this as I too thought it didn't look right.

Question. This line has == -1 but I have seen >0 used in a similar way with serial.read. What is the difference?

Serial.read returns -1 if the receive buffer is empty, but if there is something in the receive buffer, Serial.read will return that character value.
The character value could be zero, so I would suggest that "> 0" may be incorrect.

I suppose this is what happens when you rely on experts.

I don't know what to make of that - it makes you sound like Trump

Blackfin:
I've never seen this syntax:

while (c = MySerial.read(), c == -1 )

Is this legal?

Yup. Weird, but legal. Equivalent to

while ((c = MySerial.read()) == -1 )

Blocks until a character is read from serial.

PaulMurrayCbr:
Yup. Weird, but legal. Equivalent to

while ((c = MySerial.read()) == -1 )

Blocks until a character is read from serial.

Ahhh ... good observation. I missed the equality check on my first read of it. Saw it as a single '=' assignment. That would cause the while() loop to never exit.

TheMemberFormerlyKnownAsAWOL:
Serial.read returns -1 if the receive buffer is empty, but if there is something in the receive buffer, Serial.read will return that character value.
The character value could be zero, so I would suggest that "> 0" may be incorrect.
I don't know what to make of that - it makes you sound like Trump

Thanks for the explanation. Makes sense.

Also ease up on the Trump insults, I would prefer to be akin to Hitler. :wink:

Well. I have been reading over this and I have decided I might save this somewhere so I can come back to it and work on my original code that worked a bit better. What is in the posted code is confusing me and I need to reorg the comments so I, and everyone else, can read it. I might have a bit of a play with it before I do but I still don't like it.

Thanks for all your comments and help, especially the PM I got.

I know I will probably get chastised for posting this but I am going away for a few days on a well earned holiday to Port Douglas.

This is my original code before I modified it to suit what I was given on AVR freaks.

Currently it is not reading the ECU_byte (incoming bytes) I have a whole lot of prints so I can see what is happening but that bit is not. Also the checksum bit isn't working either, Ithought it was but its not,

Can someone please point me in the right direction.

//Wondatre R1.0

#include "Wire.h"
#include <EEPROM.h>
#include <SoftwareSerial.h>

SoftwareSerial MySerial(2, 3); // RX, TX

uint8_t indx;
uint8_t buf[8];
uint8_t bufIdx;
uint8_t new_byte;
uint8_t x;
int gear_in;
uint8_t Gear;
int sensorVal;
uint8_t out1 = 5;
uint8_t out2 = 6;
uint8_t rxd = 13;
uint8_t txd = 14;
uint8_t gearN = 0x00;
uint8_t gear1 = 0x20;
uint8_t gear2 = 0x40;
uint8_t gear3 = 0x60;
uint8_t gear4 = 0x80;
uint8_t gear5 = 0xA0;
uint8_t ECU_byte;
int checksum;

void setup()

//stolen from PIGPI

{

  delay(500); //wait to settle
  pinMode(out1, OUTPUT);
  pinMode(out2, OUTPUT);
  pinMode(rxd, INPUT_PULLUP);
  pinMode(txd, OUTPUT);
  pinMode(A0, INPUT);

  // Read sensor value

  sensorVal = analogRead (A0);

  //Save Gear to EEPROM

  if (sensorVal < 665 )

  {
    // No Change
    Gear = EEPROM.read(0); //Load gear from EEPROM
  }
  else if  (sensorVal > 665 and sensorVal <= 800)
  {
    Gear = 1; //4th Gear
  }
  else if  (sensorVal > 800 and sensorVal <= 905)
  {
    Gear = 2; //5th Gear
  }

  EEPROM.update(0, Gear); // Only writes if different

  switch (Gear)
  {
    case 1:  digitalWrite(out1, HIGH); break;
    case 2:  digitalWrite(out2, HIGH); break;
  }

  Serial.begin(9600);
  MySerial.begin(9600);

}

void loop()
{

  gear_in = analogRead (A0);      // read gear
  Serial.println(Gear);           // For testing....working
  Serial.println(gear_in);        // For testing
  delay(1000);                    // For testing


  if (gear_in <= 300)
  {
    new_byte = gearN;
  }

  else if (gear_in > 300 and gear_in <= 420)
  {
    new_byte = gear1;

  }

  else if (gear_in > 420 and gear_in <= 540)
  {
    new_byte = gear2;
  }

  else if (gear_in > 540 and gear_in <= 665)
  {
    new_byte = gear3;
  }

  else if (gear_in > 665 and gear_in <= 800)
  {
    new_byte = gear4;
  }

  else if (gear_in > 800 and gear_in <= 905)
  {
    new_byte = gear5;
  }

  delay(1000);                      // For testing



  //EVERYTHING ABOVE THIS LINE WORKS
  /*-----------------------------------------------------------------------*/

  findRxSilence();
  read_data_replace_byte5();
  calculate_checksum();
  //  write_bytes();

}

// This bit is to check every 100ms for the next 8 bytes.

void findRxSilence()                        // Seems to be working
{
  //wait for start of 8 byte packet, 40ms of silence should do
  // [8x5ms=40ms...][60ms silence]...
  //9600baud is ~1ms+ per byte, using delay(1) will be fine


  for ( uint8_t i = 40; i; i--, delay(1) ) {
    if ( MySerial.read() == -1 ) continue;
    i = 40; //start over if received something
  }
}


void read_data_replace_byte5() { //Read data from ECU and store in buf. Replace the 5th byte. Not working. 

  /*  if (MySerial.available() > 0) {    //Previous attempt.
      ECU_byte = MySerial.read();

      if (indx < sizeof buf-1)
        buf [indx++] = ECU_byte;
    }

  */

  int  numBytes = MySerial.available();
  for (int n = 0; n < numBytes; n++) {     //numBytes is always 0.
    buf[n] = MySerial.read();
  }
  
  Serial.print ("ECU_Byte = ");       // For testing
  Serial.println (ECU_byte, HEX);   // For testing
  Serial.print ("New_Byte = ");       // For testing
  Serial.println (new_byte, HEX);   // For testing

  if (indx = 5, buf[5] = new_byte);     //check for byte 5 and replace with new_byte....this bit working

  Serial.print ("indx =  ");         // For testing
  Serial.println (indx);            // For testing
  Serial.print ("buf 0 =  ");         // For testing
  Serial.println (buf[0], HEX);            // For testing
  Serial.print ("buf 1 =  ");         // For testing
  Serial.println (buf[1], HEX);            // For testing
  Serial.print ("buf 2 =  ");         // For testing
  Serial.println (buf[2], HEX);            // For testing
  Serial.print ("buf 3 =  ");         // For testing
  Serial.println (buf[3], HEX);            // For testing
  Serial.print ("buf 4 =  ");         // For testing
  Serial.println (buf[4], HEX);            // For testing
  Serial.print ("buf 5 =  ");         // For testing
  Serial.println (buf[5], HEX);            // For testing
  Serial.print ("buf 6 =  ");         // For testing
  Serial.println (buf[6], HEX);            // For testing

}

void calculate_checksum() {  // Calculate checksum - Appears not to be working (this will be something simple)

  checksum = 0;
  for (x = 0; x <= 6; x++); {
    checksum = checksum + buf[x];
    Serial.print ("Checksum =  ");    // For testing
    Serial.println (checksum, HEX);   // For testing

    buf[7] = 0xff - checksum;         // Replace byte 7 with new checksum
  }

  Serial.print ("buf 7 =  ");         // For testing
  Serial.println (buf[7], HEX);       // For testing

}



void write_bytes()   //write Data to display. Haven't checked this bit yet.
{

  byte write_byte;
  bufIdx = 0;
  while ( bufIdx < 8 ) write_byte;
  buf[bufIdx++];
  MySerial.write(buf, 7);
  delay(5);      // pauses for 5 milliseconds before sending the next byte

}

Hi All,

I have been stuffing around with this for the last few days and my head hurts. I have read and reread over serial.available and serial .read a dozen times and I still can't get data frothe input. I know its there because the CRO and logic analyser shows it.

If I could just get the buf to contain data it would be helpful and I "should" be able to stumble through the rest.

Any ideas much appreciated.