Hi all, I am trying to program a tiny85 with a code that activates two LEDs, one after the other, on a button push. I.E, the first button press will light LED #1, and after #1 has been lit the button will activate #2. this is just part of my code but its driving me crazy as every time it turns on LED#1. >:(
The button is using the built-in pull-up resistor
int buttonPin = 12;
char* one = "off";
char* two = "off";
char* three = "off";
void setup() {
pinMode(1, OUTPUT);
pinMode(2, OUTPUT);
pinMode(3, OUTPUT);
pinMode(buttonPin, INPUT_PULLUP);
}
void loop(){
int buttonValue = digitalRead(buttonPin);
if ((buttonValue == LOW)&&(one="off")){
digitalWrite(2, HIGH);
delay(2000);
digitalWrite(2, LOW);
one = "on";
} else {
if(two="off");
digitalWrite(4, HIGH);
delay(2000);
digitalWrite(4, LOW);
two="on";
Come on...14 posts and you still don't know how to post source code correctly on this site. Read Nick Gammon's posts at the top of this Forum before you post anything else.
Here's some code that will hopefully work for you. Keep in mind this could be done many ways (if/else) but I chose to use switch case.
#define Button 8 //Your button
#define FirstLED 13 //LED number 1
#define SecondLED 9 //LED number 2
boolean ButtonState; //Current state of the button
boolean LastButtonState = 0; //Previous state of the button (for comparison)
byte ButtonCounter = 0; //A counter so we know how many times button is pressed
void setup() {
pinMode(Button,INPUT_PULLUP);
pinMode(FirstLED,OUTPUT);
pinMode(SecondLED,OUTPUT);
}
void loop(){
ButtonState = digitalRead(Button); //Read state of button (pressed or not)
if (ButtonState != LastButtonState){ //If button is pressed
delay(10); //Delay for debounce
ButtonState = digitalRead(Button); //Then read to double-check current state
if (ButtonState == LOW){
ButtonCounter++; //If button press, count up 1
}
}
//Replacement for using "if/else" statements//
switch (ButtonCounter){ //What we want to happen depending on how many times we press the button
case 1: //If button is pressed 1 time
digitalWrite(FirstLED,HIGH); //Do my stuff
break; //Move on
case 2: //If button is pressed 2 times
digitalWrite(SecondLED,HIGH); //Do my stuff
break;
case 3:
digitalWrite(FirstLED,LOW);
digitalWrite(SecondLED,LOW);
ButtonCounter = 0; //After all LEDs lit, restart the sequence
break;
}
}
A bit of constructive criticism on that suggested code if I may. (Good intent and good to point at the docs)
Mr_Laggy:
boolean LastButtonState = 0; //Previous state of the button (for comparison)
I Would suggest to educate with good practice in mind: Boolean conceptually is a logic state that can be either true or false. There are such literals in the programming language and thus rather than using 0 as the initialisation value, I would highly recommend to use false. also in that specific case buttonState is actually conceptually a pin status, which can be HIGH or LOW. So I would not declare that as a Boolean and set it to LOW not 0. Same for buttonSate of course (See constants in arduino)
if (ButtonState != LastButtonState){ //If button is pressed
The comment is highly misleading (more on this below). Good comments make good code and are a best practice.
Indenting correctly (see Auto Format) is also important for readability by the way, just press cmd-t or ctrl-t in the IDE to get better looking code.
Of course when giving code to new comers, would be better to test if possible (I understand it's not always possible, I'm typing on my tablet without an arduino at hand). In the code above you forget to change the LastButtonState value and thus don't wait for the button going back up and the code will likely fly through 3 consecutive presses much faster than a blink of an eye and thus won't meet the spec.
I would also argue that the code is not optimized as you keep lighting the same LED if no button press has occurred. That's un-necessary, would be best to have the switch case within the if button has been pressed.
In this case it's not a massive issue but in time critical apps ensuring you have an ability to come back to the top of the loop without perform needless actions is a good practice.
if (ButtonState != LastButtonState){ //If button is pressed
LastButtonState wasn't supposed to be changed. It's purpose was always to be 0 (LOW) so that it could be compared to the buttons pressed state (HIGH).
So you're saying not to do that?
Should LastButtonState be removed and then " if (ButtonState != LastButtonState)" be simply replaced with "if (ButtonState == HIGH)" ?
have you connected your button to read +5V when pressed?
yes, I've attached a pic of how my button is connected. I'm testing on a real breadboard but didn't want to take a picture of that as its a big board with other wires from other projects and would look confusing.
OK so then instead of being zooming through the LEDS I think it won't do anything
let me walk "manually" through the code you posted:
boolean LastButtonState = 0; //Previous state of the button (for comparison)
...
ButtonState = digitalRead(Button); //Read state of button (pressed or not)
if (ButtonState != LastButtonState){ //If button is pressed
delay(10); //Delay for debounce
ButtonState = digitalRead(Button); //Then read to double-check current state
if (ButtonState == LOW){
ButtonCounter++; //If button press, count up 1
}
}
Button is not pressed, so reads HIGH according to your wiring
because (HIGH != LOW) is true, you enter the first if and do a 10 ms debounce
you read again, button is still not touched so ButtonState = HIGH
your next test is HIGH == LOW, it's false so you don't increase ButtonCounter which is still 0
the rest of the switch is not triggering anything because you don't have a case for 0
that stays this way as long as I don't touch the button.
Say I now press the button and maintain it pressed, ButtonState becomes LOW
the first if (LOW != LOW) is false, so you don't enter the if and go straight to the switch clause. Since nothing has changed the ButtonCounter, no case is matching and you go back to the top of the loop.
because you did not memorize in LastButtonState the fact the button was pressed you will test again if (LOW != LOW) which still won't be true... so you won't enter the test... and nothing works.
I maintain you need a LastButtonState = ButtonState;after the first if block for this to work.
