Go Down

Topic: Program Integrity (Read 1 time) previous topic - next topic

brewie

Hi - I'm new to  programming and using forums so I hope my etiquette is ok.
I have written a little bit of code to control some relays in a lighting box for a local mineral club to show off some of their specimens. It works the way I intend it to but I'd like to know if there's anything I need to do to it to improve it's integrity - I don't want to give it to these people only to have to look after it forever!  ( the word relay will replace the word led in the final draft)

#
//sketch to control relays for West Coast Gem and
//Minerals Club UV lighting Box

const int button = 7;   // button input pin
const int led = 9;      // led output pin
const int led1 = 10;      // led output pin
const int led2 = 11;      // led output pin
const int led3 = 13;    // led on arduino board
int value = 0;          // store digital value
int value1 = 0;         // value for reading analog 
int pin = 0;            //potentiometer on analog pin 0

void setup(){
 
  pinMode( button, INPUT);
  pinMode( led, OUTPUT);
  pinMode(led1, OUTPUT);
  pinMode(led2, OUTPUT);
  pinMode(led3, OUTPUT);
 
   
}

void loop(){
  value1 = analogRead(pin); // sets value equal to pin
  value1 /= 16;               // converts 0-1023 to 0-60 (approx)
  if (value1 < 5) value1 = 5; // minimum 5 count delay
  digitalWrite(led, HIGH);    //sets the first led at startup
  digitalRead(button);
  value = digitalRead(button);
  if (value == HIGH){
    digitalWrite(led, LOW);     //stop first led
    digitalWrite(led1, HIGH);   //start second led
    for (int i=0; i<value1; i++)   //count down value as set by
    {                              // potentiometer 
      digitalWrite(led3, HIGH);   // 1 second delay
      delay(500);
      digitalWrite(led3, LOW);
      delay(500);
    }
     digitalWrite(led1, LOW);    //stop second led
     digitalWrite(led2, HIGH);   //start third led 

 
   for (int i=0; i<value1; i++)
   {
     digitalWrite(led3, HIGH);
     delay(500);
     digitalWrite(led3, LOW);
     delay(500);
   }
     digitalWrite(led2, LOW);  //stop third led
     digitalWrite(led, HIGH);  //re-start first led
  }

}
T.Brewerton

Grumpy_Mike

Quote
I don't want to give it to these people only to have to look after it forever!

It's like having kids, a life sentence.
Code tends to be reliable once it is running, so there is no reliability problems with it. You could add some contact debounce on the digital button read in case the button develops bad debounce as it ages but otherwise it looks fine to me.

brewie

Thanks very much for your input - I never really thought of something aging and becoming a problem but it is of course mechanical so that's  something that is bound to happen at some part of it's life cycle
T.Brewerton

Nick Gammon

Please edit your post, select the code, and put it between [code] ... [/code] tags.

You can do that by hitting the # button above the posting area.
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

Nick Gammon

Looks reasonable to me. The delays will probably debounce it for you.
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

tuxduino

My 2 cents worth of suggestions...

This line:
Code: [Select]

digitalRead(button);

doesn't do anything. It's probably a leftover. It should be deleted.

Use more meaningful names for variables. For example:
Code: [Select]

const int btnPin = 7;
const int ledPin1 = 9;
const int ledPin2 = 10;
const int ledPin3 = 11;
const int ledPin4 = 13;
int btnState = 0;
int numBlinkCycles = 0;
int analogPin = 0;


Also, comments should not be a description of what the code line does, but rather why it does it or how it relates to the problem domain.

Use the handy map() function:
Code: [Select]

numBlinkCycles = analogRead(analogPin)
numBlinkCycles = map(numBlinkCycles, 0, 1023, 0, 60);


Use named constants:
Code: [Select]

const int MIN_DELAY_CNT = 5;

if (numBlinkCycles < MIN_DELAY_CNT) {
    numBlinkCycles = MIN_DELAY_CNT;
}


When one uses "good" variable and named constant names, comments often become superfluous. And that means the code is more readable. Which is a Good Thing (TM).

Quote
Looks reasonable to me. The delays will probably debounce it for you.


When value1 is 60, the sketch samples the button once in 2 minutes. IOW, the button pin has to read HIGH when the blink sequence ends and loop() restarts, or it won't be detected. This is sub optimal IMHO, however it could be OK in this particular application (it depends on the hardware context).

HTH

Nick Gammon

It's not going to affect the integrity of the program, but this is rather amusing:

Code: [Select]

const int led1 = 10;      // led output pin
const int led2 = 11;      // led output pin
const int led3 = 13;    // led on arduino board

...

  digitalWrite(led1, LOW);    //stop second led
  digitalWrite(led2, HIGH);   //start third led 


led1 is the second led and led2 is the third led. So what is led1? This is probably an example of comments confusing rather than helping.
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

brewie

Thanks to all above for your suggestions/comments - it's all valid stuff and this is a big learning curve for me. it's  one thing to write some code to make things go but another thing all together to write it efficiently and with readability. Back to the drawing board for a bit of a tidy up!
T.Brewerton

Go Up