In need for some help with programming multiple if-statements

Hi there,

I recently started with Arduino.
It takes a while to understand the basics of the programming, but I can manage…until now.

I’m trying to compile a code for a sign post…that is…I promised my grandson that I could make him one.
So…here I am… :smiley: :smiley:

The electrical setup consists out of 3 momentary push buttons and 3 leds (green, orange and red).

What the code should do:

At startup only the green LED should be ON.

When button1 is pushed, the green LED has to blink.
If button1 is pressed again, the green LED should be ON.

When button2 is pressed, the other LED (green or red) should go OFF and the orange LED goes ON.
If button2 is pressed again, the orange LED has to blink.
If button2 is pressed again, the orange LED should be ON.

When button3 is pressed, the other LED (green or orange) should go OFF and the red LED goes ON.

The code I wrote is assembled out of some example codes found in the example library of Arduino IDE.

const int button1 = 2;                    //the green pushbutton attached to pin 2
const int button2 = 3;                    //the orange pushbutton attached to pin 3
const int button3 = 4;                    //the orange pushbutton attached to pin 4

const int led1 = 8;                       //the green LED attached to pin 8
const int led2 = 9;                       //the orange LED attached to pin 9
const int led3 = 10;                       //the orange LED attached to pin 10

const long interval1 = 750;               //the interval at which the green LED has to blink
const long interval2 = 750;               //the interval at which the orange LED has to blink

int buttonPushCounter1 = 0;               //counter for number of presses on the green pushbutton
int buttonState1 = 0;                     //current state of the green pushbutton
int lastButtonState1 = 0;                 //previous state of the green pushbutton
int ledState1 = 0;                        //current state of the green LED

int buttonPushCounter2 = 0;               //counter for number of presses on the orange pushbutton
int buttonState2 = 0;                     //current state of the orange pushbutton
int lastButtonState2 = 0;                 //previous state of the orange pushbutton
int ledState2 = 0;                        //current state of the orange LED

int buttonState3 = 0;                     //current state of the red pushbutton
int lastButtonState3 = 0;                 //previous state of the red pushbutton
int ledState3 = 0;                        //current state of the red LED

unsigned long previousMillis1 = 0;        //will store the last time the green LED was updated
unsigned long previousMillis2 = 0;        //will store the last time the orange LED was updated

void setup() {

  Serial.begin(9600);                     //start the serial communication at 9600 bits per second

  pinMode(button1, INPUT_PULLUP);         //configure the green pushbutton as an input and enable the internal pull-up resistor
  pinMode(led1, OUTPUT);                  //configure the green LED as an ouput

  pinMode(button2, INPUT_PULLUP);         //configure the orange pushbutton as an input and enable the internal pull-up resistor
  pinMode(led2, OUTPUT);                  //configure the orange LED as an ouput

  pinMode(button3, INPUT_PULLUP);         //configure the red pushbutton as an input and enable the internal pull-up resistor
  pinMode(led3, OUTPUT);                  //configure the red LED as an ouput

}

void loop() {     //begin void loop()
  // put your main code here, to run repeatedly:

  int sensorVal1 = digitalRead(button1);  //read the green pushbutton value into a variable
  int sensorVal2 = digitalRead(button2);  //read the orange pushbutton value into a variable
  int sensorVal3 = digitalRead(button3);  //read the red pushbutton value into a variable

  Serial.println(sensorVal1);             //print out the value of the green pushbutton to the serial monitor
  Serial.println(sensorVal2);             //print out the value of the orange pushbutton to the serial monitor
  Serial.println(sensorVal3);             //print out the value of the red pushbutton to the serial monitor


  //Green
  //if button1 is pushed uneven times   =>  LED green:  ON
  //                                        LED orange: OFF
  //                                        LED red:    OFF
  //
  //if the button1 is pushed even times =>  LED green:  BLINK
  //                                        LED orange: OFF
  //                                        LED red:    OFF
  
  if (sensorVal1 != lastButtonState1) {   //compare the state of the green pushbutton to its previous state

    if (sensorVal1 == 0) {                //if the current state of the green pushbutton is LOW (0) then the button went from "OFF" to "ON"

      buttonPushCounter1++;               //if the state of the green pushbutton has changed, increment the counter for the green pushbutton
      Serial.println("ON");
      Serial.print("Number of pushes on the green pushbutton: ");
      Serial.println(buttonPushCounter1);
    } else {

      Serial.println("OFF");              //if the current state of the green pushbutton is HIGH (1) then the button went from "ON" to "OFF"
    }
  } 
    delay(50);                            //delay a little bit for stability

    lastButtonState1 = sensorVal1;        //save the current state of the green pushbutton as the last state, for the next time through the loop

  if (buttonPushCounter1 % 2 == 0) {

    digitalWrite(led1, 1);
  } else {

    unsigned long currentMillis1 = millis();

    if (currentMillis1 -previousMillis1 >= interval1) {

      previousMillis1 = currentMillis1;

      if (ledState1 == 0) {
        ledState1 = 1;
        
      } else {
        ledState1 = 0;
      }

      digitalWrite(led1, ledState1);
    }
  }


  //Orange
  //if button2 is pushed uneven times =>  LED green:  OFF
  //                                      LED orange: ON
  //                                      LED red:    OFF
  //
  //if  button2 is pushed even times  =>  LED green:  OFF
  //                                      LED orange: BLINK
  //                                      LED red:    OFF
  
  if (sensorVal2 != lastButtonState2) {   //compare the state of the orange pushbutton to its previous state

    if (sensorVal2 == 0) {                //if the current state of the orange pushbutton is LOW (0) then the button went from "OFF" to "ON"

      buttonPushCounter2++;               //if the state of the orange pushbutton has changed, increment the counter for the orange pushbutton
      Serial.println("ON");
      Serial.print("Number of pushes on the green pushbutton: ");
      Serial.println(buttonPushCounter2);
    } else {

      Serial.println("OFF");              //if the current state of the orange pushbutton is HIGH (1) then the button went from "ON" to "OFF"
    }
  } 
    delay(50);                            //delay a little bit for stability

    lastButtonState2 = sensorVal2;        //save the current state of the orange pushbutton as the last state, for the next time through the loop

  if (buttonPushCounter2 % 2 == 0) {

    digitalWrite(led2, 1);
  } else {

    unsigned long currentMillis2 = millis();

    if (currentMillis2 -previousMillis2 >= interval2) {

      previousMillis2 = currentMillis2;

      if (ledState2 == 0) {
        ledState2 = 1;
        
      } else {
        ledState2 = 0;
      }

      digitalWrite(led2, ledState2);
    }
  }

  //Red
  //if  button3 is pushed once =>  LED green:  OFF
  //                               LED orange: OFF
  //                               LED red:    ON
  
  if (sensorVal3 != lastButtonState3) {

    if (sensorVal3 == 0) {

      digitalWrite(led3, 1);
    }
  }

  lastButtonState3 = sensorVal3;

}     //end of void loop()

I can’t find what I’m doing wrong. I’ve read various posts and tried many variations in the coding.
Still no succes. I hope that you can help me figure it out.

Thanks in advance.

Cheers.



You are using the internal pullups, but the switches are wired to Vcc. That will not work. Wire the side of the switches that is now connected to Vcc to ground, instead. The swich inputs will, then, read LOW when pressed.

groundFungus: You are using the internal pullups, but the switches are wired to Vcc. That will not work. Wire the side of the switches that is now connected to Vcc to ground, instead. The swich inputs will, then, read LOW when pressed.

Already changed the wiring to GND. Forgotten to update the diagram :-[

That leaves me with when pressing a random button, that the LEDs won't go OFF.

Hello TinusNL, Welcome to the forum. Too many new people on here dive in without posting their code or circuit diagram properly, if at all, so ++Karma from me for doing so.

Where to start... I'm not the best when it comes to helping with programming questions but I admire that you are making a real effort so I'll try.

I think the thing you need to work on is separating your code into functions, trying to do everything in the main loop just makes for code that's difficulty to understand and too easy to make mistakes in. Concentrate on a function that controls 1 LED, it should be called from the main loop, check to see if anything needs to be done then return to the main loop. As each LED can be in one of 3 states (on, off, flashing) consider a state machine for each LED (now I have to find where the tutorial is for state machines). If you get 1 LED doing what you want it will be easy to add in the others.

EDIT: State machine tutorial

PerryBebbington: I think the thing you need to work on is separating your code into functions, trying to do everything in the main loop ljust makes for code that's difficulty to understand and too easy to make mistakes in. Concentrate on a function that controls 1 LED, it should be called from the main loop, check to see if anything needs to be done then return to the main loop. As each LED can be in one of 3 states (on, off, flashing) consider a state machine for each LED (now I have to find where the tutorial is for state machines). If you get 1 LED doing what you want it will be easy to add in the others.

Hi Perry,

Thanks for the reply.

Sometimes the answer is so obvious that one doesn't think of it. I will certainly give it a try and also will look for that tutorial you've mentioned.

The updated electrical diagram.

If you look at Using Nextion displays with Arduino you will see an example of how I break everything into functions. The only thing in the main loop is calls to other functions.

That's a reasonable description of what the code is supposed to do but, unless I missed it, you haven't actually given any detail of what the code does now. That's useful information to have.

Steve

PerryBebbington: EDIT: State machine tutorial

Thanks Perry.

I already took a quick look at it. Maybe the answer to my question lies indeed within that post.

Another good tutorial is Several Things at a Time. it covers using functions and millis() for timing.

slipstick: That's a reasonable description of what the code is supposed to do but, unless I missed it, you haven't actually given any detail of what the code does now. That's useful information to have.

Steve

Once the board is start up, the follow happens:

The green and orange LED are ON, red is OFF.

When pressed on the green push button: the green LED blinks, the orange LED stays ON, the red LED stays OFF. When pressed again on the green push button, the green LED is ON, the orange LED stays ON, red LED stays OFF.

When pressed on the orange push button: the orange LED blinks, the green LED stays ON, the red LED stays OFF When pressed again on the orange push button: the orange LED is ON, the green LED stays ON, the red LED stays OFF.

When pressed on the red push button: the red LED goes ON, the green LED stays ON, the orange LED stays ON.

It would be a lot easier to follow the code if you at least split the LED blinking out into separate functions you called from the main loop.

Other than that the main thing seems to be that switching the various LEDs OFF is not handled by the other LED routines. E.g. in the button1, button2, button3 routines they only handle their own LEDs. They don't sort out switching the others off. As a result, just as one example, there's no code anywhere that will ever switch the red off. Once it's been switched on it's on forever.

Steve

slipstick: It would be a lot easier to follow the code if you at least split the LED blinking out into separate functions you called from the main loop.

That is what I'm going to de in the new code. Seperate functions for green, orange and red.

slipstick: As a result, just as one example, there's no code anywhere that will ever switch the red off. Once it's been switched on it's on forever.

If I understand it correctly, what you're saying is that there's no line of coding that can perform the tasks I require for my setup?

Of course I'm a newbie with Arduino and coding.....but if we, as they say, managed to put a man on the moon. It most certainly will be possible to write a code for my setup. At least I'm not giving up on it.

Nevertheless, I appreciate your reply.

Basically if you have a digitalWrite(pin, 1) to switch something on I would expect to find at least one corresponding digitalWrite(pin, 0) somewhere to switch it off again. I can’t see those for any of your LEDs.

BTW you should really use HIGH and LOW rather than 1 and 0 for on and off. It’s better coding and also much easier to read.

Also if you want the program to start in a known state you should really set that up in the well-named setup() section. E.g. after the all pinMode()s do a digitalWrite() HIGH or LOW to each LED so you know they’re all in exactly the state you want.

Steve

Tried another option for coding, still no succes…

But, I feel that I’m getting there… :wink:

#define pressed   LOW
#define released  HIGH

#define ledOn1    HIGH
#define ledOn2    HIGH
#define ledOn3    HIGH

#define ledOff1   LOW
#define ledOff2   LOW
#define ledOff3   LOW

const byte button1 = 2;     //  green push button on pin 2
const byte button2 = 3;     //  orange push button on pin 3
const byte button3 = 4;     //  red push button on pin 4

const byte led1 = 8;        //  green LED on pin 8
const byte led2 = 9;        //  orange LED on pin 9
const byte led3 = 10;       //  red LED on pin 10

byte buttonState1;          //  current state of the green push button
byte lastButtonState1;      //  previous state of the green push button
byte buttonPushCounter1;    //  counter for the green push button

byte buttonState2;          //  current state of the orange push button
byte lastButtonState2;      //  previous state of the orange push button
byte buttonPushCounter2;    //  counter for the orange push button

byte buttonState3;          //  current state of the red push button
byte lastButtonState3;      //  previous state of the red push button

unsigned long blinkMillis1;
unsigned long blinkMillis2;

unsigned long switchMillis;

void setup()
{
  pinMode(button1, INPUT);          //  set the green push button as an input
  digitalWrite(button1, HIGH);      //  enable the internal pullup for the green push button

  pinMode(button2, INPUT);          //  set the orange push button as an input
  digitalWrite(button2, HIGH);      //  enable the internal pullup for the orange push button

  pinMode(button3, INPUT);          //  set the red push button as an input
  digitalWrite(button3, HIGH);      //  enable the internal pullup for the red push button

  pinMode(led1, OUTPUT);            //  set the green LED as an output
  digitalWrite(led1, ledOff1);      //  as default the green LED is OFF

  pinMode(led2, OUTPUT);            //  set the orange LED as an output
  digitalWrite(led2, ledOff2);      //  as default the orange LED is OFF

  pinMode(led3, OUTPUT);            //  set the red LED as an output
  digitalWrite(led3, ledOff2);      //  as default the red LED is OFF
}

void loop()
{
  ledBlink1();
  ledBlink2();


  if (millis() - switchMillis >= 50ul)      //  check the buttons every 50ms
  {
    switchMillis = millis();                //  reset the timing

    if (checkButton1(button1, lastButtonState1) == true)     //  check the green push button
    {
      digitalWrite(led2, ledOff2);
      digitalWrite(led3, ledOff3);
      buttonPushCounter1++;     //  increment the counter for the green push button
      delay(100);
      digitalWrite(led1, ledOn1);
      
    }
    
    if (checkButton2(button2, lastButtonState2) == true)     //  check the orange push button
    {
      digitalWrite(led1, ledOff1);
      digitalWrite(led3, ledOff3);
      buttonPushCounter2++;     //  increment the counter for the orange push button
      delay(100);
      digitalWrite(led2, ledOn2);
    }

    if (checkButton3(button3, lastButtonState3) == true)     //  check the green push button
    {
      digitalWrite(led1, ledOff1);
      digitalWrite(led2, ledOff2);
      delay(100);
      digitalWrite(led3, ledOn3);
    }
  }

  if (buttonPushCounter1 % 2 == 0)
  {
    digitalWrite(led1, ledOn1);
  }

  else
  {
    ledBlink1();
  }

  if (buttonPushCounter2 % 2 == 0)
  {
    digitalWrite(led2, ledOn2);
  }

  else
  {
    ledBlink2();
  }
}

void ledBlink1()
{
  if (millis() - blinkMillis1 < 500ul)
  {
    //nothing to do yet
    return;
  }

  blinkMillis1 = millis();      //  reset the timing

  digitalWrite(led1, !digitalRead(led1));
}

void ledBlink2()
{
  if (millis() - blinkMillis2 < 500ul)
  {
    //nothing to do yet
    return;
  }

  blinkMillis2 = millis();      //  reset the timing

  digitalWrite(led2, !digitalRead(led2));
}

bool checkButton1(byte button1, byte &lastState)
{
  byte thisSwitchState = digitalRead(button1);

  if (thisSwitchState != lastState)
  {
    lastState = thisSwitchState;

    if (thisSwitchState == pressed)
    {
      return true;
    }

    else
    {

    }
  }

  return false;

}

bool checkButton2(byte button2, byte &lastState)
{
  byte thisSwitchState = digitalRead(button2);

  if (thisSwitchState != lastState)
  {
    lastState = thisSwitchState;

    if (thisSwitchState == pressed)
    {
      return true;
    }

    else
    {

    }
  }

  return false;

}

bool checkButton3(byte button3, byte &lastState)
{
  byte thisSwitchState = digitalRead(button3);

  if (thisSwitchState != lastState)
  {
    lastState = thisSwitchState;

    if (thisSwitchState == pressed)
    {
      return true;
    }

    else
    {

    }
  }

  return false;

}

I'm glad you're getting there but if you want more help you're going to have provide more details about what your code currently does and what else it needs to do. "Still no success" isn't a very helpful problem description.

Steve

  pinMode(button1, INPUT);          //  set the green push button as an input
  digitalWrite(button1, HIGH);      //  enable the internal pullup for the green push button

This is very old code. The modern way to do it is

  pinMode(button1, INPUT_PULLUP);
void loop()
{
  ledBlink1();
  ledBlink2();

This means they will always blink. You can't turn the LED on or off if you keep calling the blink function thousands of times per second.

  if (millis() - switchMillis >= 50ul)      //  check the buttons every 50ms

Where does that come from? It is an inefficient and mostly useless method of debouncing. Your code will work better without it.

Instead of the button code doing a digitalWrite() the buttons should set a global variable for the LED state. Then the output functions can look at those to decide to blink the LED or turn it on or off.

slipstick:
I’m glad you’re getting there but if you want more help you’re going to have provide more details about what your code currently does and what else it needs to do. “Still no success” isn’t a very helpful problem description.

Steve

What the code currently does:

At start-up the green and orange LED light up, the red LED stays off.

Pressing the green push button makes the green LED blink, the orange LED blinks shortly when the green push button is pressed. Nothing happens with red LED.
Pressing the green push button again makes the green LED light up, the orange LED blinks shortly when the green push button is pressed. Nothing happens with red LED.

Pressing the orange push button makes the orange LED blink, the green LED blinks shortly when the orange push button is pressed. Nothing happens with red LED.
Pressing the orange push button again makes the orange LED light up, the green LED blinks shortly when the orange push button is pressed. Nothing happens with red LED.

Pressing the red push button makes the red LED light up. The green and orange LED stay on or keep blinking, whatever state they are in.

Pressing the green of orange push button, makes the red LED turn off and blink or light up the corresponding LED.

What the is supposed to do.

At start-up the green LED should light up, orange and red LED are off.

Pressing the green push button: blink green LED, orange and red LED go/stay off
Pressing the green button again: light up green LED, orange and red LED go/stay off.

Pressing the orange push button: light up orange LED, green and red LED go/stay off.
Pressing the orange push button again: blink orange LED, green and red LED go/stay off

Pressing the red push button: red LED light up, green and orange LED go/stay off.

In a nutshell, pressing any push button will make the corresponding LED to light up and other LEDs to off.
Pressing the green or orange push button a second time will make the corresponding LED blink at an interval of 750 ms.

MorganS: This is very old code. The modern way to do it is

  pinMode(button1, INPUT_PULLUP);

I'll try any code in search for the solution to this challenge. I can always adapt it to the current standards ;)

MorganS: This means they will always blink. You can't turn the LED on or off if you keep calling the blink function thousands of times per second.

That makes a lot of sense....back to the drawing board

MorganS: Where does that come from? It is an inefficient and mostly useless method of debouncing. Your code will work better without it.

I've tried to fit in the code of debouncing ( https://www.arduino.cc/en/Tutorial/Debounce ) but was a bit complicated for me to make it fit the wright way.

MorganS: ...the buttons should set a global variable for the LED state. Then the output functions can look at those to decide to blink the LED or turn it on or off.

Can you give an example of how that can be done?

I've tried to fit in the code of debouncing ( https://www.arduino.cc/en/Tutorial/Debounce ) but was a bit complicated for me to make it fit the wright way.

That explains the un-used variables in your code. It does not explain where you got that other wrong code from.

You are using the buttonPushCounter1 variable to control the blinking on LED1. When you push some other button, you don't update counter1, so it keeps blinking.

I suggest you separate the counting of button pushes from the LED states. Do you need to count at all?