Critique my simple sketch

Forum 2005-2010 (read only) Software Syntax & Programs
1

I've had my arduino packed a way for a while and tonight decided to get it out and learn. I modified the fade example sketch to accept commands from a serial connection - so an LED is connected with a resistor to pin 9 on the arduino, sending a '1' over the serial connection turns it on, sending a '0' turns it off.

I have been reading over the forums and am really impressed by the knowledge that most people have here, so I would really appreciate it if I could get your thoughts on my sketch. I just want to see if I am doing things correctly so far or if there are easier ways of doing things. Thanks very much in advance.

int command = '2';
int oneLed = 9;
int ledState = 0;

int fadeOutFunction(int pin){
  for (int fadeValue = 255 ; fadeValue >= 0; fadeValue -=5){
      analogWrite(oneLed, fadeValue);
      delay(30);
      }
      ledState = 0;
    }
    
int fadeInFunction(int pin){
  for (int fadeValue = 0 ; fadeValue <= 255; fadeValue +=5){
      analogWrite (oneLed, fadeValue);
      delay(30);
      }
      ledState = 1;
    }
    
void setup()
{
  Serial.begin(9600);
  digitalWrite (oneLed, LOW);
}

void loop()
{
  while (Serial.available() > 0) {
    command = Serial.read();
    if (command=='1') 
    {
      if (ledState == 1) 
      {
        Serial.println("LED was already on, doing nothing");
      } else {
      Serial.println("Switching LED on");
      fadeInFunction(oneLed);
      }
    }
      else if (command=='0') 
      {
         if (ledState == 0) 
      {
        Serial.println("LED was already off, doing nothing");
      } else {
      Serial.println("Switching LED off");
      fadeOutFunction(oneLed);
      }}
      else 
      {if (ledState == 0){
        Serial.println("Unknown command - leaving LED off");
      } else {
       Serial.println("Unknown command - turning off LED");
       fadeOutFunction(oneLed);
      }}
  }
}
2

Clean and easily-followed logic, but I think you can improve your indentation. As is now, it makes your sketch hard to read.

:slight_smile:

3

Agree with AlphaBeta about the indentation, just a few other things:

  1. "command" is only used within "setup", there's no need to give it global scope.

  2. "oneLed" is a constant, may as well declare it as one.

  3. you have a couple of "delay(30)" statements - it is good practice to declare a constant for the delay at the top of the program, or in a header, so it is a simple matter to change it at a single point. No big deal in a short sketch like this, but it's a good habit to acquire.

4

Thanks guys - I'll sort out the indentation.

AWOL: good tip about oneLed being a constant, will change that now. Although I don't understand your first point - 'command' is actually used in the loop, so not sure why you say it shouldn't be given a global scope? Can you explain?

Thanks again!

5

Where command is declared right now, it has global scope, so for repeated calls of "loop", the last assigned value of "command" will survive.

However, you're renewing the value of "command" each time through "loop", and you're not using it anywhere else, unlike "ledState", so you don't need it to have global scope.

Keeping scope to the minimum required is a good habit to acquire, and can save much head-scratching when sketches get bigger.

In fact, "command" is not used outside of the initial "while", so even better would be:

void loop()
{
  while (Serial.available() > 0) {
    [glow]int[/glow] command = Serial.read();
// etc

[edit]: the IDE has a handy "auto-format" tool, which sometimes messes up array declations (they're still legal, but they look awful), but can sort out indentation a treat.[/edit]