problem with sending serial commands via USB through the arduino serial monitor

Hi All,

i'm having a problem with sending serial commands via USB through the arduino serial monitor or xbee v2 module connected to an xbee shield. the problem is that when i send the serial command some times the command is pickup right away and the function is executed which is great but most of the time i'll send the serial command and nothing happens so i'll send it again and again and again and eventually the function will execute. at first i was sending the command via xbee modules but i thought i would remove that as a point of failure and decided to use the serial monitor instead via USB. the code uploads fine and like i said it works but it is very intermittent on when it wants to work. i've loaded the code on my uno r3 and my mega r3 and the problem is the same. i'm using a windows 8 laptop with arduino 1.0.2 or 1.0.3 i think. please see code below any help would be appreciated. also i understand my coding skills are not up to par so please be gentle when with me on my code as it's been put together from multiple sources and then modified by me to do what i need it to do.

below is the code that is going onto the receiving end of the xbee module which has be tried on a uno and a mega with the same results

#include "LPD8806.h"
#include "SPI.h"

// Example to control LPD8806-based RGB LED Modules in a strip

/*****************************************************************************/

// Number of RGB LEDs in strand:
int nLEDs = 170;
// int nLEDs2 = 170;

// Chose 2 pins for output; can be any valid output pins:
int dataPin  = 11;
int clockPin = 13;

// First parameter is the number of LEDs in the strand.  The LED strips
// are 32 LEDs per meter but you can extend or cut the strip.  Next two
// parameters are SPI data and clock pins:
LPD8806 strip = LPD8806(nLEDs, dataPin, clockPin);

// You can optionally use hardware SPI for faster writes, just leave out
// the data and clock pin parameters.  But this does limit use to very
// specific pins on the Arduino.  For "classic" Arduinos (Uno, Duemilanove,
// etc.), data = pin 11, clock = pin 13.  For Arduino Mega, data = pin 51,
// clock = pin 52.  For 32u4 Breakout Board+ and Teensy, data = pin B2,
// clock = pin B1.  For Leonardo, this can ONLY be done on the ICSP pins.
//LPD8806 strip = LPD8806(nLEDs);
//LPD8806 strip2 = LPD8806(nLEDs2);

void setup() {
  // Start up the LED strip
  strip.begin();
  Serial.begin(9600);

  // Update the strip, to start they are all 'off'
  strip.show();
}
void loop() {
  int i, t, r, g, b;
  if (Serial.available() > 0){
    while (Serial.read() == 'D'){
      b = 0;
      colorCrawl(strip.Color(0,   127,   0), 50);  // BLUE
    while (b < 127){
      colorEntry (strip.Color(0,   b,   0), 25 );  // BLUE
      b = b + 5;
    }
      r = 0;
      g = 0;
      while (r < 127 && g < 127){
      overheadlight (strip.Color(r , 127, g), 10); // White
      r = r + 2;
      g = g + 2;
    }
   colorWipe(strip.Color(0,   0,   0), 75);  // off 
   }
  }
   if (Serial.available() > 0){
    while (Serial.read() == 'E'){
      rainbowCycle(10);
      colorWipe(strip.Color(0,   0,   0), 25);  // off
   }
  }
   if (Serial.available() > 0){
    while (Serial.read() == 'F'){
      t = 0;
     while(t < 5){
       r = random(1, 25);
       g = random(1, 25);
       b = random(1, 128);
      colorCrawl(strip.Color(r,   b,   g), 50);  // random color
      t++;
      Serial.print(r);    // shows random values for testing purpose only
      Serial.println("");
      Serial.print(g);
      Serial.println("");
      Serial.print(b);
      Serial.println(""); 
     }
   }
  } 
 }  


/***************************************************************************/
/************************** FUNCTION SECTION *******************************/
/***************************************************************************/

// makes the rainbow wheel equally distributed 
// along the chain
void rainbowCycle(uint8_t wait) {
  uint16_t i, j;
  
  for (j=0; j < 384 * 5; j++) {     // 5 cycles of all 384 colors in the wheel
    for (i=0; i < strip.numPixels(); i++) {
      // tricky math! we use each pixel as a fraction of the full 384-color wheel
      // (thats the i / strip.numPixels() part)
      // Then add in j which makes the colors go around per pixel
      // the % 384 is to make the wheel cycle around
      strip.setPixelColor(i, Wheel( ((i * 84  / strip.numPixels()) - j) % 384) );
    }  
    strip.show();   // write all the pixels out
    delay(wait);
  }
}
          
          /*********************************************/
          
// Chase one dot down the full strip.
void colorChase(uint32_t c, uint8_t wait) {
  int i;

  // Start by turning all pixels off:
  for(i=0; i<strip.numPixels(); i++) strip.setPixelColor(i, 0);

  // Then display one pixel at a time:
  for(i=0; i<strip.numPixels(); i++) {
    strip.setPixelColor(i, c); // Set new pixel 'on'
    strip.show();              // Refresh LED states
    strip.setPixelColor(i, 0); // Erase pixel, but don't refresh!
    delay(2);
  }

  strip.show(); // Refresh to turn off last pixel
}
          
          /*********************************************/
          
void colorWipe(uint32_t c, uint8_t wait) {
  int i;

  for (i=0; i < strip.numPixels(); i++) {
      strip.setPixelColor(i, c);
      strip.show();
      delay(wait);
  }
}
          
/*******************************************************************************/
/*******************************************************************************/
          
void colorEntry(uint32_t c, uint8_t wait) {
  int i;

  for (i=0; i < strip.numPixels(); i++) {
      strip.setPixelColor(i, c);
      strip.show();
  }
}
          
          /*********************************************/
/*          
void colorOff(uint32_t c, uint8_t wait) {
  int i;

  for (i=0; i < strip.numPixels(); i++) {
      strip.setPixelColor(i, c);
      strip.show();
    // delay(wait);
  }
}
  */        /*********************************************/
          
void overheadlight(uint32_t c, uint8_t wait) {
  int i;

  // Start by turning all pixels off:
  for(i=0; i<strip.numPixels(); i++) strip.setPixelColor(i, 0);
  
  // Then display one pixel at a time:
  for(i=0; i<strip.numPixels(); i++) {
   
    strip.setPixelColor(i, c);
    strip.setPixelColor(1, c); // Set new pixel 'on'
    strip.setPixelColor(3, c);
    strip.setPixelColor(5, c);
    strip.setPixelColor(7, c);
    strip.setPixelColor(9, c);
    strip.setPixelColor(11, c);
    strip.setPixelColor(13, c);
    strip.setPixelColor(15, c);
    strip.setPixelColor(17, c);
    strip.setPixelColor(19, c);
    strip.setPixelColor(21, c);
    strip.setPixelColor(23, c);
    strip.setPixelColor(25, c);
    strip.setPixelColor(27, c);
    strip.setPixelColor(29, c);
    strip.setPixelColor(41, c);
    strip.setPixelColor(43, c);
    strip.setPixelColor(45, c);
    strip.setPixelColor(47, c);
    strip.setPixelColor(49, c);
    strip.setPixelColor(51, c);
    strip.setPixelColor(53, c);
    strip.setPixelColor(55, c);
    //delay(10);
    
     
    strip.show();              // Refresh LED states
    strip.setPixelColor(i, 0); // Erase pixel, but don't refresh!
    //delay(wait);
 }
  strip.show(); // Refresh to turn off last pixel
   
}

          
          /*********************************************/
          
// Chase down the full strip with random tail lenght.
void colorCrawl(uint32_t c, uint8_t wait) {
  int i, l;
  l = random(2, 30);
     
  // Start by turning all pixels off:
  for(i=0; i<strip.numPixels(); i++) strip.setPixelColor(i, 0);

  // Then display one pixel at a time:
  for(i=0; i<strip.numPixels(); i++) {
    
    strip.setPixelColor(i, c); // Set new pixel 'on'
    strip.show();    // Refresh LED states
    delay(10);
    strip.setPixelColor(i - l, 0); // Erase pixel, but don't refresh!
    //delay(10);
  }

  strip.show(); // Refresh to turn off last pixel
}

/* Helper functions */

//Input a value 0 to 384 to get a color value.
//The colours are a transition r - g -b - back to r

uint32_t Wheel(uint16_t WheelPos)
{
  byte r, g, b;
  switch(WheelPos / 128)
  {
    case 0:
      r = 127 - WheelPos % 128;   //Red down
      g = WheelPos % 128;      // Green up
      b = 0;                  //blue off
      break; 
    case 1:
      g = 127 - WheelPos % 128;  //green down
      b = WheelPos % 128;      //blue up
      r = 0;                  //red off
      break; 
    case 2:
      b = 127 - WheelPos % 128;  //blue down 
      r = WheelPos % 128;      //red up
      g = 0;                  //green off
      break; 
  }
  return(strip.Color(r,b,g));
}

what i'm trying to do is that when the arduino receives a serial command (eg D) whether via serial monitor or over the xbee module that it run the function and as i said it does do that but very inconsistently.

please let me know if there is any other information you'd like again i'm not sure what info to include.

thank you all for any help you maybe able to give.

K Jones

Your code reduced to the essential:

  if (Serial.available() > 0){
    while (Serial.read() == 'D'){
... // D-instructions
    }
  }
  if (Serial.available() > 0){
    while (Serial.read() == 'E'){
... // E-instructions
    }
  }
  if (Serial.available() > 0){
    while (Serial.read() == 'F'){
...  // F-instructions
    }
  } 
}

The problem is: suppose that at the beginning of the loop you are reading an 'E'. The first thing that is checked in your loop is a 'D', so the D-instructions are not executed. But the next control (for 'E') takes a fresh byte from Serial. Your previous 'E' is lost.

Solution: use else, and read at most one character for each loop.

Hi Spatula thank you for your reply. so i should try and implement something like this

if (Serial.available() > 0){
    while (Serial.read() == 'D'){
... // D-instructions
    }
  }
  else if (Serial.available() > 0){
    while (Serial.read() == 'E'){
... // E-instructions
    }
  }
  else (Serial.available() > 0){
    while (Serial.read() == 'F'){
...  // F-instructions
    }
  } 
}

please forgive my coding ignorance

also in not sure what you mean by "and read at most one character for each loop." i thought that was what i was doing but again please forgive my poor coding knowledge it's been a long time since I've had to code anything and when i did it was in pascal about 20 years ago

also would a switch case help in this situation just wondering

thanks for your help
K Jones

kjonesunltd:

if (Serial.available() > 0){                // each time the loop is executed, check if there is an incoming message on serial - OK

while (Serial.read() == 'D'){          // this while() means that you might be expecting more than one 'D' - DOUBTFUL
                                                  // additionally, you have now taken the character out of the serial buffer, but
                                                  // you will only do something if it's a 'D' - BAD
... // D-instructions
   }
 }
 else if (Serial.available() > 0){        // this else is not about the character being != 'D', but about the previous available() - BAD
   while (Serial.read() == 'E'){          // same as before
... // E-instructions
   }
 }
 else (Serial.available() > 0){          // same as before, plus missing an 'if' (should be 'else if')
   while (Serial.read() == 'F'){          // same as before
...  // F-instructions
   }
 }
}

Not exactly what I had in mind :stuck_out_tongue: I have inserted some comments to show what's wrong. Consider that a Serial.read() takes a char out of the serial buffer; if you don't process it, it will be lost the next time you do a Serial.read(), or in the next loop(). So, once you have read() a char, you need to keep it long enough to see if it is D, else E, else F.

void loop()
{
  char ch;

  ch = Serial.read();
  if (ch == 'D') 
  {
  } 
  else if (ch == 'E')
  {
  } 
  ...
}

As you can see, Serial.read() is called only once within loop(). If there are more characters to read, you'll do that in the next loop().
Now you may ask, shouldn't I check for Serial.available() before reading from Serial? Yes. It's not strictly necessary in your case, because Serial.read() will return -1 if there are no characters to be read, but it is good practice to do so because Serial.read() is more expensive than Serial.available(), so you want to avoid it if there's nothing to read. But as in the case of read() you need to check available() only once.

void loop()
{
  char ch;

  if (Serial.available() > 0) { 
    ch = Serial.read();
    if (ch == 'D') { 
  ... // all the rest as in previous example
    }
  }
}

And yes, you can replace the if ... else construct with a switch statement.

Thank you so very much Spatula it works like a charm now.

your help was very much appreciated now I can reconnect my xbee modules and try sending the serial commands that way.

again thank you very much

K Jones

but it is good practice to do so because Serial.read() is more expensive than Serial.available(), so you want to avoid it if there's nothing to read.

Not really. The call to Serial.available() has to subtract one value (the tail pointer) from another (the head pointer). If the result is zero, there is nothing to read, so it returns. The call to Serial.read() also has to determine if there is something to read. If there is, it copies one byte from the buffer and increments the tail pointer. So, not more than a couple of clock cycles longer to do the assignment (the read part) and the addition and assignment (the increment part).

PaulS:

but it is good practice to do so because Serial.read() is more expensive than Serial.available(), so you want to avoid it if there's nothing to read.

Not really. The call to Serial.available() has to subtract one value (the tail pointer) from another (the head pointer). If the result is zero, there is nothing to read, so it returns. The call to Serial.read() also has to determine if there is something to read. If there is, it copies one byte from the buffer and increments the tail pointer. So, not more than a couple of clock cycles longer to do the assignment (the read part) and the addition and assignment (the increment part).

Glad to know; honestly I never liked the prevalence of available() in so many examples but I felt like I had to conform.