Function not working after upgrading from Uno to Mega

Hello everyone,

I am new to Arduino, I started out with an Uno to get my feet wet. After running out of Digital I/O pins I decided to upgrade to a Mega. I had to modify my Sparkfun Xbee Shield to get the Xbee serial to work because I found out SoftwareSerial doesn’t work with the Mega 2560. I soldered the DIN and DOUT from the Xbee Shield directly to TX3 and RX3 on the Mega. After the modification I was able to communicate back and forth between Xbees. I also modified my program to replace the Xbee.serial (software serial) with Serial3. After this I found that my function readXbeeByte() no longer works. Someone please take a look at my logic. I am passing 4 values in byte which are “255,x,y,sum of x and y”

void GetCommand() {
int val[3], checksum=0, incoming=0;
if(Serial3.available() > 0) {
//Serial.print(’\n’);Serial.print(Serial3.read());Serial.print(’*’); If I remove the comments, I can see all 4 values coming in.
if (Serial3.read()==255) { // read the next 3 bytes

val[0]=readXbeeByte()-125; // readXbeeByte() return zeros
val[1]=readXbeeByte()-125;
val[2]=readXbeeByte();

incoming=2;
}
}
if (incoming>0) {
//do something else
}
}

int readXbeeByte(){
unsigned long delaytime=millis();
while (Serial3.available() > 0 && millis()-delaytime < 250) { // This loop for some reason is always false.
return Serial3.read();
}
}

TankStyleSweep_v2.ino.ino (3.76 KB)

#include <SoftwareSerial.h>
#include <SabertoothSimplified.h>

// Mixed mode is for tank-style diff-drive robots.
// Only Packet Serial actually has mixed mode, so this Simplified Serial library
// emulates it (to allow easy switching between the two libraries).

SabertoothSimplified ST(Serial2); 
int deadmanbuttonPin = 9;  
int panic;
unsigned long currentTime;
unsigned long timeOfLastGoodPacket = 0;
unsigned long timeOfLastTTL = 0;
//SabertoothSimplified ST; // We'll name the Sabertooth object ST.
                         // For how to configure the Sabertooth, see the DIP Switch Wizard for
                         //   http://www.dimensionengineering.com/datasheets/SabertoothDIPWizard/start.htm
                         //   http://www.dimensionengineering.com/datasheets/SabertoothDIPWizard/nonlithium/serial/simple/single.htm
                         // Be sure to select Simplified Serial Mode for use with this library.
                         // This sample uses a baud rate of 9600.
                         //
                         // Connections to make:
                         //   Arduino TX->13  ->  Sabertooth S1
                         //   Arduino GND     ->  Sabertooth 0V
                         //   Arduino VIN     ->  Sabertooth 5V (OPTIONAL, if you want the Sabertooth to power the Arduino)
                         //
void setup() {
  pinMode(deadmanbuttonPin, INPUT);
  Serial2.begin(9600);                              // This is the baud rate chosen for the DIP switches above.
  Serial3.begin(9600);
  Serial.begin(9600);
  delay(2000);
  allStop();                                             
}                                                     

void allStop() {                                      // The Sabertooth won't act on mixed mode until
  ST.drive(0);                                        // it has received power levels for BOTH throttle and turning, since it
  ST.turn(0);                                         // mixes the two together to get diff-drive power levels for both motors.
  Serial.print('\n'); Serial.write("All STOP +++++"); // So, we set both to zero initially.
}

void timeout() {
  if (currentTime > (timeOfLastGoodPacket + 200)) {   // Stop robot if no good packets received within 200th of a seconds
    allStop();
    timeOfLastGoodPacket = currentTime;
  }
  if (currentTime > (timeOfLastTTL +1000)) {          // send stay alive command to receiver.
    Serial3.write("100");
    timeOfLastTTL=currentTime;
  }
}

void GetCommand() { 
  int val[3], checksum=0, incoming=0;
  if(Serial3.available() > 0) {
      Serial.print('\n');Serial.print(Serial3.read());Serial.print('*');  
    if (Serial3.read()==255) {  
      
      val[0]=readXbeeByte()-125;    // readXbeeByte() return zeros 
      val[1]=readXbeeByte()-125;
      val[2]=readXbeeByte();
      
      incoming=2;
    } 
  }
  if (incoming>0) {  
    Serial.print('\n');Serial.print(val[0]);Serial.print(',');Serial.print(val[1]);Serial.print(',');Serial.print(val[3]);Serial.print('%');
    // debugging to see what readXbeeByte return.
    if ((val[0]<0 || val[1]<0) && val[2]>125) {
      val[2]-=256;}
    else if ((val[0]<0 && val[1]<0)) {
      val[2]-=256;
    }
  }
  checksum=val[0]+val[1];

  if (checksum==val[2]){
    timeOfLastGoodPacket = currentTime;
    ST.drive(val[1]);
    ST.turn(val[0]);
    Serial.print('\n');
    Serial.print(val[0]);Serial.print(',');
    Serial.print(val[1]);Serial.print(',');
    Serial.print(val[2]);
  }
}

int readXbeeByte(){
  unsigned long delaytime=millis();
  while (Serial3.available() > 0 && millis()-delaytime < 250) {
    return Serial3.read();
  }
}  

void loop()
{
  bool power=true;
  currentTime = millis();
  panic=digitalRead(deadmanbuttonPin);
  if (panic == HIGH ) { power=false; }
  if (power) { 
    GetCommand(); 
    timeout();
  }
}
  if(Serial3.available() > 0) {
      Serial.print('\n');Serial.print(Serial3.read());Serial.print('*');

If there is a byte to read, read and print it.

    if (Serial3.read()==255) {

Then, regardless of whether there is anything to read, or not, read another value.

Useless!

int readXbeeByte(){
  unsigned long delaytime=millis();
  while (Serial3.available() > 0 && millis()-delaytime < 250) {
    return Serial3.read();
  }
}

More crap code. What does this function return if there is nothing to read? In that case, you lied when you said that the function returned something.

That whole while statement is stupid. It appears that you want to wait for there to be serial data to read, but not more than a quarter of a second. That is not even close to what it does.

Thanks for the quick reply Paul. Go easy I am new at this.

Serial.print(’\n’);Serial.print(Serial3.read());Serial.print(’*’);

This is for debugging purposes. I want to see if it receives anything at all. Which it does when I send it 4 bytes “255,x,y,sum of x and y” This is line is deleted once I figure out why readXbeeByte() doesn’t work.

int readXbeeByte(){
unsigned long delaytime=millis();
while (Serial3.available() > 0 && millis()-delaytime < 250) {
return Serial3.read();
}
}

Why is this useless? If the first byte = 255, then I want to give it some time to read the next 3 byte without being stuck in an endless loop.???

The examples in serial input basics are simple and reliable - and don’t read non-existent bytes. And they don’t block the Arduino from doing other things while waiting for data.

Don’t use time as an indicator of bytes being available to read - it is too haphazard. Check how many there are and read that many and no more.

…R

This is for debugging purposes. I want to see if it receives anything at all. Which it does when I send it 4 bytes "255,x,y,sum of x and y" This is line is deleted once I figure out why readXbeeByte() doesn't work.

You need to read ans store the value, then print it and use it. When you read and print the 255, the next value, that you read even if it isn't there is x. The 255 is gone.

Why is this useless? If the first byte = 255, then I want to give it some time to read the next 3 byte without being stuck in an endless loop

For several reasons. Reading from the serial stream removes the data. You printed the 255, if that's what you read. That means that it is gone. The next value, that may not have arrived yet, is x, So, the readXbeebyte() method probably isn't called.

But, even if it is, that while loop will end as soon as there is nothing to read. It does not wait for there to be data to read (which I think is what you want).

Finally, only if there is actually something to read do you return a value. The rest of the time, the function returns junk.

I do believe that that is what you are seeing.

l4tran:

  while (Serial3.available() > 0 && millis()-delaytime < 250) {

return Serial3.read();
 }




Why is this useless? If the first byte = 255, then I want to give it some time to read the next 3 byte without being stuck in an endless loop.???

Your big problem here is you are returning inside the loop. If the condition is false (ie. no data available) it immediately exits the loop, and then returns an undefined value.

If the condition is true, it doesn’t loop because you have a return there. So in either case the while achieves nothing.

You want something more like:

 // wait 250 ms for data
 while ((Serial3.available() == 0) && (millis() - delaytime < 250)) {
    // do nothing
  }
 return Serial3.read ();

The read will return -1 if there was nothing there.

The examples in serial input basics are simple and reliable - and don't read non-existent bytes. And they don't block the Arduino from doing other things while waiting for data.

Thanks Robin, I will look into it more this weekend.

Paul after reading through Robin's example I have realized that my while loop is not at all efficient for reading in bytes. I believed I used 255 as the starting marker to prepare readXbeeByte() to get the remainder 3 bytes. You are definitely correct when you said I was getting garbage data, and I didn't even know why. For that reason I used the last byte as a checksum to filter out only good data. Does that mean that Robin's code will not generate any garbage data? Or do I still need to implement a checksum?

The thing that bugs me is why would this work on the UNO and not the MEGA?

l4tran: Does that mean that Robin's code will not generate any garbage data? Or do I still need to implement a checksum?

I'm pretty sure my code does not generate anything - especially garbage. If there is a possibility of your data being corrupted between the transmitter and receiver it would be a good idea to include a checksum. My code can retrieve all the data including the checksum and you can later use the checksum to verify the received data.

...R