Serial Sadness

Hi everybody,

I'm new to writing code for the Arduino and I'm having some trouble. I have set up two LEDs (one blue, one yellow) and I'm using them to simulate a motor. The yellow LED indicates direction. On for forwards and off for backwards. The blue LED I want to pulse as an indication of speed.

I want to be able to control it all over the serial port. This is what I have so far:

int MotorDir = 12;
int MotorSpd = 11;
int incomingByte = 0;
int Spd = 0;

void flash(int S)
{
  Serial.println(S, BYTE);
  digitalWrite(MotorSpd, HIGH);
  delay(1000 - S*100);
  digitalWrite(MotorSpd, LOW);
  delay(1000 - S*100);
}

void setup()
{
  pinMode(MotorDir, OUTPUT);
  pinMode(MotorSpd, OUTPUT); 
  Serial.begin(9600);           // set up Serial library at 9600 bps
}

void loop()
{
  if (Serial.available() > 0)
  {
    incomingByte = Serial.read();

    Serial.print("I received: ");
    Serial.println(incomingByte, BYTE);

    if (incomingByte == 'Y') {
      incomingByte = Serial.read();
      if (incomingByte == '1') {
        digitalWrite(MotorDir, HIGH);
      }
      if (incomingByte == '0') {
        digitalWrite(MotorDir, LOW);
      }
    }

    if (incomingByte == 'B') {
      Spd = Serial.read();
      Serial.println(Spd, BYTE);
    }
  }
  flash(Spd);
}

Turning the yellow LED on and off works fine. I.e. if I type Y1 into the serial monitor it turns on and if I type Y0 it turns off.

When the program starts, the blue LED flashes at the required speed but if I type B2 (for example) into the serial monitor the blue LED turns on, stays on and the Arduino wont accept any more commands. I think it may be getting stuck in the "flash" function but I'm not sure how or why.

Can anybody help or offer a suggestion?

Thanks :slight_smile:

Your flash uses nasty "delay"s.
Check out the "blink without delay" tutorial.

Also,

if (incomingByte == 'B') {
      Spd = Serial.read();
      Serial.println(Spd, BYTE);

you need to use "Serial.available" the check that there is something there to read before you read it, otherwise "Serial.read" returns "-1".

The flash function gets called every time through loop. It prints out the value that it was called with. What value do you see in the serial monitor when the function fails to return?

Note that you are checking to see that there is at least one character available to read. If there is one, you read two ('Y', '0' or 'Y', '1', or 'B', '3').

Also note that you are reading '1', or '4', not 1 or 4. You pass the ascii value of a number to flash, not the number. To pass it the number, you would need to subtract '0' from Spd.

      Spd = Serial.read();

This line is setting Spd to be the ASCII value of the speed data - ie a number between 48 and 57. That might not be what you want. Try this instead:

      Spd = Serial.read() - '0';

This should set Spd to a value from 0 to 9.

Hey, thanks guys.

This worked a treat:

Spd = Serial.read() - '0';

Is that a standard way to convert an incoming byte into an int? I would never have thought of that.

PaulS: The value that I saw on the serial monitor each time the flash function ran was just blank, or null. The only way I knew it was receiving something was that the scroll bar would re-size.

Groove: Good call with the 'blink without delay' tip. That's a much better way of doing things.

I'm glad that it is now working for you. I'd like to make a couple of suggestions, though, that might ease future debugging issues.

When outputting data for debugging purposes, write out some descriptive text, too. Instead of this:

  Serial.println(S, BYTE);

try this:

  Serial.print("In flash, S is [");
  Serial.print(S, BYTE);  Serial.println("]");

This way, you know what value is being printed, and where that value starts and ends.

When calling a function, like Serial.print (or Serial.println), understand what all the arguments are, and how they influence the function being called. Read the reference document on the function you are using. As an exercise, read the documentation for the Serial.print function, and understand what the BYTE argument is doing, and see if you can understand why using it in the flash function caused the (incorrect/unexpected) behavior that you saw.

There are a couple of other things that make your code differ from the ideal. They might seem nit-picky, but I'm going to mention them, anyway.

Names that are all capital letters are generally reserved for #defined values (HIGH, LOW, INPUT, OUTPUT, BYTE, DEC, HEX, BIN, etc.). The argument to the flash function is all caps.

One letter variable names are generally reserved for throw-away variables, like those used in a for loop (for(int i=0; i<256; i++)). Longer names more clearly define the purpose of a variable. The argument to the flash is a single letter name. Spelling out Speed would not have been that much more effort.

Variable names are generally lower case. The argument for the flash function should have been speed, not Speed or S.

Variable names should accurately reflect their purpose. The variables MotorSpd and MotorDir do not. Names like motorSpdPin and motorDirPin more accurately reflect their purpose.

Thanks for the follow up Paul.

I have removed the flash function now and I'm using the method that Groove mentioned.

I have taken your comments on board and will update my code. :slight_smile: