LEDs and Pushbutton

Hi, I can’t seem to get case 3 and default case to work with the button press, any help would be grateful https://pastebin.com/XWS4sYD9

1st Press all LEDs should light
2nd Press All LEDs should blink together
3rd Press All LEDs should blink one by one
4th Press All LEDs should be off. Button press count should reset to zero.

I’m using tinkercad and I am new to arduino.

Any help is much appreciated.

int buttonState = 0;
//button is attached to pin 2
int button = 2;

//LEDs are attatched to pins 13,12,11
int ledPin1 = 13;
int ledPin2 = 12;
int ledPin3 = 11;

//interger to hold current state

int state = 0;

//int to hold last state

int old = 0;
  
//int to hold button state
  
int buttonPoll = 0;

void setup(){


  pinMode(button,INPUT_PULLUP);     

  //set the LEDs pins as outputs
  pinMode(13, OUTPUT); 
  pinMode(12, OUTPUT); 
  pinMode(11, OUTPUT);
  
  //initial LED state is set to off
  
  digitalWrite(13, LOW);
  digitalWrite(12, LOW);
  digitalWrite(11, LOW);

} 


void loop() {
  
    buttonPoll = digitalRead(button);
  if (buttonPoll == 1){
    delay(50);
   buttonPoll = digitalRead(button);
    if (buttonPoll == 0){
      state = old + 1;
      
    }}
    else{
      delay(100);
    }
    
    switch (state) {
      
      //All LEDs ON if button pressed once
      case 1:
        digitalWrite(13, HIGH);
        digitalWrite(12, HIGH);
        digitalWrite(11, HIGH);
        old = state;
        break;
      
      
      //All LEDs should blink together if the button is pressed second time
      case 2:
         
    digitalWrite(13, LOW);
    digitalWrite(12, LOW);
    digitalWrite(11, LOW);
    delay(500); // Wait for 500 millisecond(s)
    digitalWrite(13, HIGH);
    digitalWrite(12, HIGH);
    digitalWrite(11, HIGH);
    delay(500); // Wait for 500 millisecond(s)
    digitalWrite(13, LOW);
    digitalWrite(12, LOW);
    digitalWrite(11, LOW);
    old = state;
    break;
      
      
      
    //All LEDs should blink but one by one if the button is pressed third time  
    case 3:
      
     
    digitalWrite(13, HIGH);
    delay(800); // Wait for 800 millisecond(s)
    digitalWrite(13, LOW);
    delay(800); // Wait for 800 millisecond(s)

    digitalWrite(12, HIGH);
    delay(800); // Wait for 800 millisecond(s)
    digitalWrite(12, LOW);
    delay(800); // Wait for 800 millisecond(s)

    digitalWrite(11, HIGH);
    delay(800); // Wait for 800 millisecond(s)
    digitalWrite(11, LOW);
    delay(800); // Wait for 800 millisecond(s)
    old = state;
    break;
      
      
      
      
    //All LEDs should be off if the button is pressed fourth time. The button press count should be reset to zero after the button has been pressed four times 
     default:
      
    digitalWrite(13, LOW);
    digitalWrite(12, LOW);
    digitalWrite(11, LOW);
    old = 0;
    break;
      
    }
  }

Blights.ino (2.39 KB)

Welcome to the forum. Please read the first post on how to post your code using tags </> rather than attach it. It makes it easier for people to help you.

You should check of the “Delay using milliseconds()” so see a better approach to your problem. While you are delaying in case in case 2 and 3, your button presses will be ignored. At the beginning of loop(), you check the button, and even if nothing has changed, you continue into your case statement which again goes into lots of delays().

Hi, thank you for the response, I have now attached the code to the original post :smiley: I had a read at the pinned post and watch several videos; unfortunately I don't think i'm grasping the concept, the whole incorporating the push button may be throwing me off a little but it is important that I use one.

My use of delays using millis are likely formatted incorrectly as i am still learning, I ask for help before I confuse myself any further, here's my current code, only 2/3 cases added and not working correctly; thanks again for any help in advance.

int buttonState = 0;

//button is attached to pin 2
const byte button = 2;

int ledState = LOW;

unsigned long startMillis = 0;
unsigned long currentMillis = millis();
const long interval = 100;

//LEDs are attatched to pins 13,12,11
const int ledPin1= 13;
const int ledPin2 = 12;
const int ledPin3 = 11;

//interger to hold current state

int state = 0;

//int to hold last state

int old = 0;
  
//int to hold button state
  
int buttonPoll = 0;

void setup(){


  pinMode(button, INPUT_PULLUP);

  //set the LEDs pins as outputs
  pinMode(13, OUTPUT); 
  pinMode(12, OUTPUT); 
  pinMode(11, OUTPUT);
  
  //initial LED state is set to off
  
  digitalWrite(13, LOW);
  digitalWrite(12, LOW);
  digitalWrite(11, LOW);
  
  startMillis = millis();

} 


void loop() {
  


       buttonPoll = digitalRead(button); //poll the state of the button
   if (buttonPoll == 1){ //check if button has been pressed
                    

      
        buttonPoll = digitalRead(button);//poll button again
    if (buttonPoll == 0){ //check button state
      
        state = old + 1; //increase state by 1
      }}
          
  
 
          
      
    
  switch (state) {
 
    case 1:
            currentMillis = millis();
        if (currentMillis - startMillis >= interval)
      
        digitalWrite(13, HIGH);
        digitalWrite(12, HIGH);
        digitalWrite(11, HIGH);
        startMillis = currentMillis;
        old = state; 
         break; 
    
      case 2:
    
    currentMillis = millis();
    if (currentMillis - startMillis >= 100)
             
    digitalWrite(13, LOW);
    digitalWrite(12, LOW);
    digitalWrite(11, LOW);
    startMillis = currentMillis;
    
    
    currentMillis = millis();
    if (currentMillis - startMillis >= 100)
      
    digitalWrite(13, HIGH);
    digitalWrite(12, HIGH);
    digitalWrite(11, HIGH);
    startMillis = currentMillis;
    
    
    currentMillis = millis();
    if (currentMillis - startMillis >= 100)
    digitalWrite(13, LOW);
    digitalWrite(12, LOW);
    digitalWrite(11, LOW);
    startMillis = currentMillis;
    old = state;
    break;
        }
    }
int buttonState = 0;

//button is attached to pin 2
const byte button = 2;

You put state in the name of the variable holding the state. Why not put pin in the name of the variable holding the pin number? Seeing button later on doesn’t give a clue as to whether the variable hold a pin number, a state, or a shoe size.

//interger to hold current state

int state = 0;

Like Pennsylvania? The state of what?

       buttonPoll = digitalRead(button); //poll the state of the button
   if (buttonPoll == 1){ //check if button has been pressed
                   

     
        buttonPoll = digitalRead(button);//poll button again
    if (buttonPoll == 0){ //check button state
     
        state = old + 1; //increase state by 1
      }}

EVERY } should be on a line BY ITSELF.
EVERY { should be on a line BY ITSELF.

What are the odds that reading the state of the pin a few microseconds later will result in a different reading?

That is NOT how the state change detection example works (and it DOES work).

PaulS:
EVERY } should be on a line BY ITSELF.

Absolutely !!

PaulS:
EVERY { should be on a line BY ITSELF.

No. It should be followed by an indentation. (so opinions vary)

   if (digitalRead(button)){    //check if button has been pressed
     while (digitalRead(button)){ //wait till it's released
        delay(1);        
     }
     state = old + 1; //increase state by 1
   }

Let's get rid off unneeded code.
btw i always wonder is "delay(1); " sufficient for the condition to be tested again ?

and shouldn't it be ?

   if (digitalRead(button)){    //check if button has been pressed
     while (digitalRead(button)){ //wait till it's released
        delay(1);        
     }
     state++; //increase state by 1
     if (state>3) state=0;
   }

Here is where i am currently, i hope this shows a bit more progress and understanding, also thank you very much for the feedback. Button pressed now work and LEDs change on each press, but don't act as i have programmed them to, here is what is happening:

LEDs turn on with 1 button press - This is supposed to happen
LEDs don't do anything on 2nd press - LEDs should all blink together
LEDs(12,11) turn off on 3rd press - All LEDs should blink one by one
LEDs (ALL) off on 4th press and button press count has reset to zero This is supposed to happen

//button is attached to pin 2
const byte button = 2;


unsigned long startMillis = 0;
unsigned long currentMillis = millis();
const long interval = 300;

//LEDs are attatched to pins 13,12,11
const int redLED = 13;
const int orangeLED = 12;
const int greenLED = 11;


//interger to hold current state

int buttonState = 0;

//int to hold last state

int buttonOld = 0;
  
//int to hold button state
  
int buttonPoll = 0;

void setup(){


  pinMode(button, INPUT_PULLUP);

  //set the LEDs pins as outputs
  pinMode(13, OUTPUT); 
  pinMode(12, OUTPUT); 
  pinMode(11, OUTPUT);
  
  //initial LED state is set to off
  
  digitalWrite(13, LOW);
  digitalWrite(12, LOW);
  digitalWrite(11, LOW);
  
  startMillis = millis();

} 


void loop() {
  
  currentMillis = millis();

   if (digitalRead(button)){    //check if button has been pressed
     while (digitalRead(button)){ //wait till it's released
        delay(1);        
     }
    buttonState++; //increase state by 1
     if (buttonState>3) buttonState=0;
   } 
  
 
          
      
    
  switch (buttonState) {
    
    //All LEDs ON if button pressed once
    case 1:
        currentMillis = millis();

      
        digitalWrite(13, HIGH);
        digitalWrite(12, HIGH);
        digitalWrite(11, HIGH);
        startMillis = currentMillis;
        buttonOld = buttonState; 
        break;
    
    
    //All LEDs should blink together if the button is pressed second time
    case 2:
    
    currentMillis = millis();
    
    if (currentMillis - startMillis >= interval)
    digitalWrite(13, LOW);
    digitalWrite(12, LOW);
    digitalWrite(11, LOW);
    startMillis = currentMillis;
    
    

    if (currentMillis - startMillis >= interval)     
    digitalWrite(13, HIGH);
    digitalWrite(12, HIGH);
    digitalWrite(11, HIGH);
    startMillis = currentMillis;
    

    if (currentMillis - startMillis >= interval)
    digitalWrite(13, LOW);
    digitalWrite(12, LOW);
    digitalWrite(11, LOW);
    startMillis = currentMillis;
    buttonOld = buttonState;
    break;
    
    
    //All LEDs should blink but one by one if the button is pressed third time  
    case 3:
    
    currentMillis = millis();
    
    if (currentMillis - startMillis >= interval)
    digitalWrite(13, HIGH);
    startMillis = currentMillis;
    
    if (currentMillis - startMillis >= interval)
    digitalWrite(13, LOW);
    startMillis = currentMillis;
    

    if (currentMillis - startMillis >= interval)
    digitalWrite(12, HIGH);
    startMillis = currentMillis;
    
    if (currentMillis - startMillis >= interval)
    digitalWrite(12, LOW);
    startMillis = currentMillis;

    
    if (currentMillis - startMillis >= interval)
    digitalWrite(11, HIGH);
    startMillis = currentMillis;
    
    if (currentMillis - startMillis >= interval)
    digitalWrite(11, LOW);
    startMillis = currentMillis;
    
    buttonOld = buttonState;
    break;
      
    
    
    default:   //if state is not 1,2,3 All LEDS off
    
    digitalWrite(13, LOW);
    digitalWrite(12, LOW);
    digitalWrite(11, LOW);
    buttonOld = 0;
    break;
    
    
    
   
    
    
        }
    }
//interger to hold current state
int buttonState = 0;

//int to hold button state
int buttonPoll = 0;

Which ONE holds the (current) pin state?

     while (digitalRead(button)){ //wait till it's released
        delay(1);       
     }

The delay() is useless. You are not paying for each iteration of the while loop. It doesn’t matter if it iterates once or a million times.

    buttonState++; //increase state by 1
     if (buttonState>3) buttonState=0;

Oh, so buttonState is NOT the state of a button (pin). Stupid name, then.

   }
 
 
         
     
   
  switch (buttonState) {

What IS the value of buttonState between the points? Why don’t you know?

        buttonOld = buttonState;

This is where the stupid name is biting you in the butt. The value to be stored in the buttonOld variable is NOT the number of times the switch was pressed.

PaulS:

     while (digitalRead(button)){ //wait till it's released

delay(1);     
    }



The delay() is useless. You are not paying for each iteration of the while loop. It doesn't matter if it iterates once or a million times.

Yeah i figured that as well, it is not about the amount of times the iteration is called, but about making sure that the condition is checked again, i once created an eternal loop calling a function (i think it was milliis() but not on an Arduino) in a condition and it got optimized into 'no need to check again, i have never recreated it, but it is still imprinted in my mind. Anyway    while (digitalRead(button)); //wait till it's released it is !

PaulS:
Oh, so buttonState is NOT the state of a button (pin). Stupid name, then.

it is the state of the program as a result of a press of the button ach.. not to bad, maybe a little foolish,
buttonOld = 0; doesn't do anything anymore as far as i can tell.

if (currentMillis - startMillis >= interval)
    digitalWrite(13, LOW);
    digitalWrite(12, LOW);

if you want several lines of code to be conditionally executed you should enclose them with curly braces, i think there is your fault(s), you did this more than once.

it is the state of the program

So, why not use a name that reflects that, like, oh, I don't know, maybe programState?

doesn't do anything anymore as far as i can tell.

Sure it does. Not what you want, or anything useful. Tell us WHAT you are trying to save the old value of. Seems to me that it is the CURRENT state OF THE SWITCH that you want to save as the PREVIOUS state of the switch.

If you used names that made sense, like currentState and previousState, for the switch states, then it would be perfectly obvious what should be on the right of the equal sign when previousState occurs on the left.

Paul, can you please pay attention a bit better, storing the old-state of the program is pointless in the current program, nothing is done with the value stored in the variable, there is no need to store it, it has become redundant (the variable) and please if you were not so pre-occupied with the names you would also have seen that he forgot to enclose the digital writes to the pins after his 'if' in curly braces, so only the first line right after gets executed conditionally, the other lines get executed all the time resulting in superfast flashing of the leds, which then 'appear' to be on. and maybe you would have noticed that i am the author of the program, so don't quote me and respond as if i am. please.