I am trying to cycle outputs based on a single button press. I have it working but is this the best way to code it? it seems messy.
Here is the logical description of my project:
Buttonpress <pin 8 LOW> Buttonpress <pin 9 LOW> Buttonpress <pin 10 LOW> Buttonpress <pin 10 HIGH> Buttonpress <pin 9 HIGH> Buttonpress <pin 8 HIGH> Repeat
here is my code:
int inpin= 3;
int x=7;
int y=11;
void setup() {
Serial.begin(9600);
pinMode(inpin, INPUT_PULLUP);
pinMode(8, OUTPUT);
pinMode(9, OUTPUT);
pinMode(10, OUTPUT);
digitalWrite(8,HIGH);
digitalWrite(9,HIGH);
digitalWrite(10,HIGH);
}
void loop() {
if (x==13){ //test x
x=7;
y=11;
}
if (digitalRead(inpin) == LOW) {
delay (100);
if (digitalRead(inpin) == LOW) { //confirm button press actually happened
Serial.println("Button press confirmed");
x=x++;
Serial.print("x= ");
Serial.println(x);
if (x<11){
digitalWrite (x,LOW); //turn output off based on value of X
}
if (x>10){ //if outputs are off turn them on based on value of Y
y=y-1; // subtracting instead of adding Y so they go in reverse order
Serial.print("y= ");
Serial.println(y);
digitalWrite(y,HIGH);
}
}
}
}
I have it working but is this the best way to code it? it seems messy.
@JMS:
You are asking a very open-ended question ... Your code works, so commenting is a sole educational response. But, from that prospective, the code beginning with
if (digitalRead(inpin) == LOW) {
delay (100);
Is considered poor programming practice for button denouncing. Consider that the uC is a single core processor and that only 1-thing at a time can be done; then the use of delay100) in the main loop() essentially wastes 10% of the processor bandwidth for nothing! This is like the 10% of alcohol the U.S. Fed gov. forces fuel distributors to put into gasoline- such a waste (opinion!)
What you want to do is to avoid delay() in your loop(). Use state variables and millis() to determine timelapses. You can study the example sketch 'BlinkWithoutDelay' for the concept or look into using the library abstraction Arduino Playground - HomePage
Once you get a grasp on the concept, the delay() function use within loop() will occur infrequently.
I have it working but is this the best way to code it?
No.
int x=7;
int y=11;
What are these names supposed to be telling me? What are these magic numbers supposed to be telling me?
if (x==13){ //test x
x=7;
y=11;
}
I still don't have a clue.
You registered with the forum with a name that is unique, and presumably means something to you. Your variable names should have meaning, too.
if (digitalRead(inpin) == LOW) {
delay (100);
if (digitalRead(inpin) == LOW) { //confirm button press actually happened
Serial.println("Button press confirmed");
100 milliseconds is a lllooonnnggg time for a switch to bounce. If yours really bounce that long, get better switches. 10 milliseconds is more normal.
This code is NOT looking for a change in state. That is, it simply tells you that at two times, not too far apart, the switch was pressed. It tells you nothing about whether the switch was pressed and released. Look at the state change detection example.
x=x++;
x++ is equivalent to x = x + 1. So, your code is equivalent to x = x = x + 1. Doesn't look right, that way, does it?
There are lots of different ways to implement this. Which is best for you depends on what else you need your sketch to do or might want it to do in future.
The very simplest solution, which also has least scope for extension to do anything else, is to use a blocking approach. Here, 'wait for keypress' would be a call to a function that loops until a keypress was detected:
wait for button press
set pin 8 low
wait for button press
set pin 9 low
wait for button press
set pin 10 low
wait for button press
set pin 10 high
wait for button press
set pin 9 high
wait for button press
set pin 8 high
Since you seem to be basically counting up to three and then down again, an alternative would be to maintain a counter which you increment and decrement based on button presses (i.e. using a variable to record whether you're currently incrementing or decrementing) and write a function to turn the outputs high and low based on the current counter value. It would make sense then to put your pin numbers in an array. This would be significantly more complex that the first approach but would enable you to change the number of LEDs or LED/pin relationship very easily and would also make it possible for your sketch to do other things at the same time. But if none of that matters to you and you just want the easiest, simplest, quickest solution then take the blocking approach and just code it how you described it.