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 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
}
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.
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
doesn't do anything. It's probably a leftover. It should be deleted.
Use more meaningful names for variables. For example:
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.
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).
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).
It's not going to affect the integrity of the program, but this is rather amusing:
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.
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!