Go Down

Topic: first program, could probably be better..... (Read 661 times) previous topic - next topic

Goofballtech

So i just got my first Arduino Board in the other day and i have been playing. I got the protoshield with it from adafruit and so far i have been having a lot of fun with it. I wrote my first program this morning that will eventually turn into a project on my truck. It will control some LED's behind the door handles on my truck.

It's going to be one of those 'just because' projects but i think it will look cool.

Anyway, i would really apreciate it if you could give me some opinons on my coding, there may be a more elegant way to do what i want... but my coding expierence is limited to the last 3 or 4 days so this is what i came up with.

Code: [Select]


/* This program is a rough draft of one that will eventually power 6 LED's
that will fade in and out when a vehicle alarm system is armed and when
the vehicle's interior lights come on it will stop the Blue pulse and fade
into a green LED

The LED's are the RGB ones available from adafruit industries. */

int Blue = 6; //Just two LEDs for testing
int Green = 5;

int Pwr = 4; //using two digital pins for power to clean up the wiring on the breadboard
int Pwr2 = 8;

int button = 11; //button to simulate alarm system's 'Ground when armed'
int input2 = 12; //button to simulate trucks interior lighting

int pullUp = 1; //Pull up pins for buttons
int pullUp2 = 0;

void setup () {
 
pinMode (Blue, OUTPUT); //Just two LEDs for testing
pinMode (Green, OUTPUT);
digitalWrite (Blue, HIGH);
digitalWrite (Green, HIGH);

pinMode (Pwr, OUTPUT);//using two digital pins for power to clean up the wiring on the breadboard
pinMode (Pwr2, OUTPUT);
digitalWrite (Pwr, HIGH);
digitalWrite (Pwr2, HIGH);

pinMode (button, INPUT);  //button to simulate alarm system's 'Ground when armed'
pinMode (input2, INPUT);  //button to simulate trucks interior lighting

pinMode (pullUp, INPUT); //Pull up pins for buttons
pinMode (pullUp2, INPUT);
digitalWrite (pullUp, HIGH);
digitalWrite (pullUp2, HIGH);

}

void loop () {
 
int btInput = digitalRead(button); //Check Buttons
int input2st = digitalRead(input2);

if (btInput == LOW) { //if alarm is armed go to blue fade cycle
   alarmFade ();
   
}

else if (input2st  == LOW) { //if dome lights are on go to turn green LED
 domeLight ();
 
}
}


void alarmFade () {
 
   for(int fadeValueB = 255 ; fadeValueB >=0; fadeValueB-=4) { // fade in from off to on
   analogWrite(Blue, fadeValueB); //write to Blue LED pin      
   delay(50);   //Slow Fade
 }

 int input2st = digitalRead(input2); //Check dome light pin in case of a change

 if (input2st  == LOW) { //if dome light is on go to proper loop
 domeLight ();
 
 }
 
 for(int fadeValue = 0 ; fadeValue <=255 ; fadeValue +=5) { // fade out from on to off
   analogWrite(Blue, fadeValue);  //write to Blue LED pin  
   delay(40); //Slow Fade                          
 }
 
 input2st = digitalRead(input2); //Check dome light pin in case of a change
 
 if (input2st  == LOW) {  //if dome light is on go to proper loop
 domeLight ();
 
 }
 
 delay (1500); //pause with LED off for 1.5 seconds
 
 input2st = digitalRead(input2); //Check dome light pin in case of a change
 
 if (input2st  == LOW) { //if dome light is on go to proper loop
 domeLight ();
 
 }
}

void domeLight () {
 
 int btInput = digitalRead(button);  //Check buttons for a change
 int input2st = digitalRead(input2);

 if (btInput == HIGH && input2st == LOW){ //if alarm is off and dome light is on turn on green led
   for(int fadeValueG = 255; fadeValueG >=0; fadeValueG -=5) { // fade in from off to on
   analogWrite(Green, fadeValueG);  //write to Green LED pin          
   delay(30);  //Fade
   }
  while (btInput == HIGH && input2st == LOW) { //maintain the Green LED being on until either th alarm is armed or the dome light turns off
 
    digitalWrite (Green, LOW);
   
  btInput = digitalRead(button);
  input2st = digitalRead(input2);
   
  }

   for(int fadeValueG = 0; fadeValueG <=255 ; fadeValueG +=5) { //Fade out from on to off
   analogWrite(Green, fadeValueG); //   write to Green LED pin  
   delay(30);//Fade
  }
 }
   
 }  

Fjornir

I'm impressed. If that is really from 3-4 days experience then you are well on the right track. My critique would consist of two points


  • If it isn't a variable don't declare it as a variable. Use a constant or a #define.
    Code: [Select]

    int Blue = 6; //Just two LEDs for testing
    int Green = 5;


    Your program isn't going to be replugging the LEDs to connect to different pins is it? ;)

    Code: [Select]

    #define BLUE 6
    #define GREEN 5
    // -or-
    const int blue = 6;
    const int green 5;


  • Factor out repetitive code

    Just like when you took algebra and factored (4x+2x)   as 2(2x+x)   so should you factor out common terms from your functions. You have got several loops like:

    Code: [Select]

       for(int fadeValueG = 255; fadeValueG >=0; fadeValueG -=5) { // fade in from off to on
       analogWrite(Green, fadeValueG);  //write to Green LED pin          
       delay(30);  //Fade
       }


    -- since they're so small this is a ridiculous nitpick (especially for someone as new to coding as you) but you could make your code a bit more expressive by making a generic fadeLED() function (have it take parameters of the pin of the led, start val, stop val, and step value) and then replace your loops with calls to fadeLED().

    For a two-line loop it probably isn't worth it. But going forward you might see yourself doing copy&paste on something bigger and have it repeated 20 times in your code and then you find a bug and you need to go and..fix...it..20...times...



Lookin' good!  ;D

Goofballtech

what changes by me defineing something as a variable or a contant other than the obvious?

i have been going off of examples found on the web and modeling them for my purposes and have seen the 'const int' and '#define' but don't really understand what the true difference would be.

Fjornir

Quote
what changes by me defineing something as a variable or a contant other than the obvious?

i have been going off of examples found on the web and modeling them for my purposes and have seen the 'const int' and '#define' but don't really understand what the true difference would be.


There are three benefits.
1. It makes your code more readable and maintainable. If you declare something as a constant then when you read it two or three months from now you're going to know "oh, yeah. That's a constant!" and you won't waste brain-cycles looking for the code that changes that pin

2. RAM. If you go ahead and declare it as a variable then the compiler will oblige you and eat up some RAM storing that variable in memory. On a microcontroller RAM is a very finite resource. By going the #define route the compiler can use the constant instead of the variable when it makes the machine-code to run on the processor.

3. Accidents. By making a constant a constant you gain the advantage of having the compiler scream at you if you try to change it. Consider:

Code: [Select]

int green = 5;

// And then later you typo:
if( green = currentPin ) //  I meant: if( green == currentPin  )
{
}


...this introduces a subtle and hard to debug bug into your code; especially if the buggy code above is invoked infrequently. You could blow a ton of time fixing that. Contrast that with:

Code: [Select]

#define GREEN 5

//and somewhere later
if( GREEN = currentPin )  // This won't even compile. You will know where the problem is immediately and save yourself time and pain
{
}

Goofballtech

awesome.

Those are the kind of things you don't get off the reference page on the arduino site. (Which has been invaluable along with this forum the last few days)

Thanks for your thoughts, i will change those around, i still have to expand it a bit to run all the LED's i want to throw in there so i may go ahead and try to define the Fade variable too.

Thanks for your help.

Udo Klein

Hi Goofball,

I recommend to read "Code Complete" by Steve Mc Connell. The best book on software construction I have ever read. To be more precise: the only book on the subject I know at all  ;)

Cheers, Udo
Check out my experiments http://blog.blinkenlight.net

Go Up