Switch State with debounce and Switch Cases giving erratic results

[hughosulliivan]

I am trying to replicate what I believe is the simplest type of code and setup you’d need to have it so that one push button can turn on a traffic light of LEDs in sequence, starting with the Red, then yellow, then Green, then all off, and then repeat from the top.

In short, where each button press indicates the next switchcase.

This does work to a degree, but each press of the button does not automatically mean the next switchcase is chosen. In practice I have to either hammer the button awfully fast for it to register, or press once, wait, then press again X seconds later, its sporadic and unreliable.

Here is the code

// using combo of arduino example codes for debounce (https://docs.arduino.cc/built-in-examples/digital/Debounce/)
// and edge detection (https://docs.arduino.cc/built-in-examples/digital/StateChangeDetection/)
// and then switch cases for good measure
const int ButtonPin = 2;
const int RedLEDPin = 11;
const int YellowLEDPin = 9;
const int GreenLEDPin = 5;
int RedLEDState = LOW;
int YellowLEDState = LOW;
int GreenLEDState = LOW;
int ButtonState = LOW;
int lastButtonState = LOW;
int ButtonPushCounter = 0;
unsigned long lastDebounceTime = 0;
unsigned long debounceDelay = 100;

void setup() {
  pinMode(ButtonPin, INPUT);
  pinMode(RedLEDPin, OUTPUT);
  pinMode(YellowLEDPin, OUTPUT);
  pinMode(GreenLEDPin, OUTPUT);
  Serial.begin(9600);
}

void loop() {                            // start by pulling from the debounce script,
  int reading = digitalRead(ButtonPin);  // read the state of the switch into a local variable, check the see if the button is pressed
  if (reading != lastButtonState) {      //compare the buttonstate to its previous, if the switch is changed due to noise or pressing:
    lastDebounceTime = millis();         // reset the debounce timer.
  }
  if ((millis() - lastDebounceTime) > debounceDelay) {  // whatever the reading is at, it has been there for longer than the debounce delay, so take it as the actual current state
    if (reading != ButtonState) {                       // if the button state has changed
      ButtonState = reading;                            // change it back to the reading.
    }
    ButtonState = digitalRead(ButtonPin);  // assuming debounce is cleared, read the button input pin again
    if (ButtonState != lastButtonState) {  // compare it to its previous state
      if (ButtonState == HIGH) {           // if it has changed, increment the counter
        ButtonPushCounter++;
        Serial.println("On");
        Serial.print("Number of Button Presses  ");
        Serial.println(ButtonPushCounter);
        Serial.print(millis() - lastDebounceTime);
        Serial.println("  Last Debounce Time");
        if (ButtonPushCounter >= 4) {  //try to get it to reset the counter so it loops back to 0 then 1 2 and 3 again
          ButtonPushCounter = 0;
        } else {
          Serial.println("Off");
        }
      }
    }
  }
  lastButtonState = ButtonState;  // save the current sate as the last state, for next time through the loop.
  switch (ButtonPushCounter) {
    case 1:
      digitalWrite(RedLEDPin, HIGH);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, LOW);
      break;
    case 2:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, HIGH);
      digitalWrite(GreenLEDPin, LOW);
      break;
    case 3:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, HIGH);
    case 4:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, LOW);
      break;
  }
}

I initially thought the debounceDelay was the cause of the problem, but adjusting it between 25, 50, 100 and more millis didn’t produce reliable results.

This is my schematic

An example of the Serial Monitor output

14:56:46.412 -> On
14:56:46.412 -> Number of Button Presses  1
14:56:46.412 -> 550  Last Debounce Time
14:56:46.447 -> Off
14:56:51.403 -> On
14:56:51.403 -> Number of Button Presses  2
14:56:51.436 -> 162  Last Debounce Time
14:56:51.473 -> Off
14:56:52.488 -> On
14:56:52.488 -> Number of Button Presses  3
14:56:52.488 -> 124  Last Debounce Time
14:56:52.521 -> Off
14:58:29.881 -> On
14:58:29.881 -> Number of Button Presses  4
14:58:29.917 -> 140  Last Debounce Time
14:58:34.560 -> On
14:58:34.560 -> Number of Button Presses  1
14:58:34.593 -> 4819  Last Debounce Time
14:58:34.628 -> Off
14:58:39.713 -> On
14:58:39.754 -> Number of Button Presses  2
14:58:39.754 -> 1074  Last Debounce Time
14:58:39.787 -> Off
14:58:47.385 -> On
14:58:47.385 -> Number of Button Presses  3
14:58:47.428 -> 227  Last Debounce Time
14:58:47.428 -> Off
15:03:59.520 -> On
15:03:59.563 -> Number of Button Presses  4
15:03:59.591 -> 195  Last Debounce Time

I’ll also include a physical picture of the setup, as I’m not against the idea that perhaps somewhere in construction I’ve enabled a completely garbled signal between push button and the arduino, in which case I’d shoot off to Hardware.

[alto777]

First, it looks like the switch variabke goes 0 .. 3, not matching the cases which handle 1 .. 4.

More important is the debouncing, which looks plausible. You should only read the input once per loop, and use that reading in lieu of digitaReading the button again.

Separated by only a few lines of code, a button can read different to what it just read.

Pseudocode a different way to debounce

    if it's been more than 50 ms
        take  reading
        if the reading has changed

              if the reading is HIGH (or whatever means act)
                   do the thing we are debouncing

              remember the reading to compare to next time
              note the time we did this so we don't until time has gone by

A quick way to debounce as a debugging technique is to simply slow the loop down.

void loop() {

  delay(25);  // poor man's debounce!

// all the rest of the loop below here will never be able to see a bounce!

Sometimes this is acceptable, if you've nothing else going on. A slight adjustment woukd use a standard millis()-based timer to read the buttons only every 25 ms, so the rest of the loop,would run at full speed.

HTH

a7

[gilshultz]

Your schematic is bad, it if full of mistooks. For example your resistor is shorted out. There is no power to operate. My guess is you short the supply when pressing the button and it comes back changing state.

[gcjr]

please look this over

const int PinRed = 11;
const int PinYel = 9;
const int PinGrn = 5;
const int PinBut = 2;


int lastButtonState = LOW;

enum { Off = HIGH, On = LOW };
enum { S_Red, S_Yel, S_Grn, S_Last };
int state;

// -----------------------------------------------------------------------------
void setLeds (
    int  red,
    int  yel,
    int  grn )
{
    digitalWrite (PinRed, red);
    digitalWrite (PinYel, yel);
    digitalWrite (PinGrn, grn);
}

// -----------------------------------------------------------------------------
void loop ()
{
    byte but = digitalRead (PinBut);
    if (lastButtonState != but)  {
        lastButtonState  = but;
        delay (50);                 // ignore bounces

        if (LOW == but)  {          // pressed
            if (S_Last <= ++state)
                state = 0;
            Serial.println (state);
    
            switch (state)  {
            case S_Red:
                setLeds (On, Off, Off);
                break;
    
            case S_Yel:
                setLeds (Off, On, Off);
                break;
    
            case S_Grn:
                setLeds (Off, Off, On);
                break;
            }
        }
    }
}

// -----------------------------------------------------------------------------
void setup ()
{
    Serial.begin (9600);

    pinMode (PinBut, INPUT);
    lastButtonState = digitalRead (PinBut);

    pinMode (PinRed, OUTPUT);
    pinMode (PinYel, OUTPUT);
    pinMode (PinGrn, OUTPUT);

    setLeds (On, Off, Off);
}

[gcjr]

if you insist on using a timer, what about this approach

const int PinRed = 11;
const int PinYel = 9;
const int PinGrn = 5;
const int PinBut = 2;

int lastButtonState = LOW;

enum { Off = HIGH, On = LOW };
enum { S_Red, S_Yel, S_Grn, S_Last };
int state;

const unsigned long MsecDebounce = 50;
unsigned long msecBut;

// -----------------------------------------------------------------------------
void setLeds (
    int  red,
    int  yel,
    int  grn )
{
    digitalWrite (PinRed, red);
    digitalWrite (PinYel, yel);
    digitalWrite (PinGrn, grn);
}

// -----------------------------------------------------------------------------
void
butChk ()
{
    unsigned long msec = millis ();

    byte but = digitalRead (PinBut);
    if (lastButtonState != but)  {
        if (msec - msecBut <= MsecDebounce)
            return;

        msecBut          = msec;
        lastButtonState  = but;

        if (LOW == but)  {          // pressed
            if (S_Last <= ++state)
                state = 0;
            Serial.println (state);
    
            switch (state)  {
            case S_Red:
                setLeds (On, Off, Off);
                break;
    
            case S_Yel:
                setLeds (Off, On, Off);
                break;
    
            case S_Grn:
                setLeds (Off, Off, On);
                break;
            }
        }
    }
}

// -----------------------------------------------------------------------------
void loop ()
{
    butChk ();
}

// -----------------------------------------------------------------------------
void setup ()
{
    Serial.begin (9600);

    pinMode (PinBut, INPUT);
    lastButtonState = digitalRead (PinBut);

    pinMode (PinRed, OUTPUT);
    pinMode (PinYel, OUTPUT);
    pinMode (PinGrn, OUTPUT);

    setLeds (On, Off, Off);
}

[alto777]

I can't tell if the very last change I made would have solved the problem with @hughosulliivan's code.

It is a debouncing pattern I do not like and do not use; by the time I got it working as intended I had made some changes just to declutter the logic so I could focus on the baffling behaviour of the original. Which I admit also included realizing my button was not being pulled either up or down when open...

I found the code you must have been riffing off without having a deep enough understanding of how it functions. Here's your code chopped down with the "reset timer on bounces" pattern you chose to use working correctly. See comments //... for where I made changes.

Try it in the simulator

debounceThis - Wokwi ESP32, STM32, Arduino Simulator

// https://wokwi.com/projects/439190331847941121

const int ButtonPin = 2;

int ButtonState = LOW;
int lastButtonState = LOW;
int ButtonPushCounter = 0;

unsigned long lastDebounceTime = 0;
unsigned long debounceDelay = 25;   // 100? get better switches!

void setup() {
//... switch wired to ground, so
  pinMode(ButtonPin, INPUT_PULLUP);

  Serial.begin(115200);
}

void loop() {                            // start by pulling from the debounce script,
  int reading = digitalRead(ButtonPin);
 
  if (reading != lastButtonState) { 
    lastDebounceTime = millis();
  }

  if (millis() - lastDebounceTime > debounceDelay) {
    if (reading != ButtonState) {
      
      ButtonState = reading;
    
      if (ButtonState == LOW) {   // iincrement the counter. on press (LOW for my hardware)
        ButtonPushCounter++;
        if (ButtonPushCounter >= 4) {
          ButtonPushCounter = 0;
        }
      }
    }
  }

//...
  lastButtonState = reading;

  static int lastPrinted = -1;  // not for long!
  if (lastPrinted != ButtonPushCounter) {
    Serial.print(" counter is ");
    Serial.println(ButtonPushCounter);
    lastPrinted = ButtonPushCounter;
  }
}

HTH

a7

[Dave_Lowther]

@gilshultz

Your schematic is bad, it if full of mistooks.

Is there more than one mistake? I see the highlighted wire as a mistake, but I don’t see any other mistakes.

[6v6gt]

@Dave_Lowther

Is there more than one mistake?

This is getting quite pedantic, but here is another. Of course it is obvious what is intended:

[gcjr]

@6v6gt

image715×154 10.9 KB

why not used internal pullup, config pin as INPUT_PULLUP and wire button between pin and gnd?

  pinMode(ButtonPin, INPUT_PULLUP);

[hughosulliivan]

Rejiggered the schematic to clean it up, should be better now

(Also swapped resistors from IEC to US because thats what I was taught with back in school, oddly enough for the UK)

[hughosulliivan]

Solution has been found

Add a bool function to if the button has been pushed or not.

If the button is pushed, and it is pushed for longer than our debug time (which can go up to 100ms without issue), the bool makes the code acknowledge its intentionally pressed with ButtonPushed.

Then when the button is released the reading will be high, so if the reading is high and the button has been pushed and acknowledged as intentional, then the code will iterate the counter and set the button as unpushed, ready for the next case.

// using combo of arduino example codes for debounce (https://docs.arduino.cc/built-in-examples/digital/Debounce/)
// and edge detection (https://docs.arduino.cc/built-in-examples/digital/StateChangeDetection/)
// and then switch cases for good measure
const int ButtonPin = 2;
const int RedLEDPin = 11;
const int YellowLEDPin = 9;
const int GreenLEDPin = 5;
int RedLEDState = LOW;
int YellowLEDState = LOW;
int GreenLEDState = LOW;
int ButtonState = LOW;
int lastButtonState = LOW;
int ButtonPushCounter = 0;
unsigned long lastDebounceTime = 0;
unsigned long debounceDelay = 100;
bool ButtonPushed = false;

void setup() {
  pinMode(ButtonPin, INPUT);
  pinMode(RedLEDPin, OUTPUT);
  pinMode(YellowLEDPin, OUTPUT);
  pinMode(GreenLEDPin, OUTPUT);
  Serial.begin(9600);
}

void loop() {                            // start by pulling from the debounce script,
  int reading = digitalRead(ButtonPin);  // read the state of the switch into a local variable, check the see if the button is pressed
  if (reading != lastButtonState) {      //compare the buttonstate to its previous, if the switch is changed due to noise or pressing:
    lastDebounceTime = millis();         // reset the debounce timer.
  }
  if ((millis() - lastDebounceTime) > debounceDelay) {  // whatever the reading is at, it has been there for longer than the debounce delay, so take it as the actual current state
    if (reading == LOW) {                               // if the button state has changed
      ButtonPushed = true;                              // change it back to the reading.
    }
  }
  if (ButtonPushed && reading == HIGH) {
    ButtonPushCounter++;
    Serial.println("On");
    Serial.print("Number of Button Presses  ");
    Serial.println(ButtonPushCounter);
    Serial.print(millis() - lastDebounceTime);
    Serial.println("  Last Debounce Time");
    if (ButtonPushCounter >= 4) {  // try to get it to reset the counter so it loops back to 0 then 1 2 and 3 again
      ButtonPushCounter = 0;
    } else {
      Serial.println("Off");
    }
    ButtonPushed = false;
  }
  lastButtonState = ButtonState;  // save the current sate as the last state, for next time through the loop.
  switch (ButtonPushCounter) {
    case 1:
      digitalWrite(RedLEDPin, HIGH);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, LOW);
      break;
    case 2:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, HIGH);
      digitalWrite(GreenLEDPin, LOW);
      break;
    case 3:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, HIGH);
    case 4:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, LOW);
      break;
  }
}

This works, with quick modifications, both with the button on 5V and on GRND using input_pullup

[gcjr]

@hughosulliivan

@hughosulliivan

    if (reading == LOW) {                               // if the button state has changed
      ButtonPushed = true;  

if the button pulls the pin HIGH, why does the code recognize the pin being LOW as being pressed

[alto777]

@hughosulliivan

This works,

You've added some kludge to a well-known pattern that you failed to implement correctly in the first place. Good work, but if you go further in this hobby, that development technique will fail you sooner later.

This

if (ButtonPushCounter >= 4) {  // try to get it to reset the counter so it loops back to 0 then 1 2 and 3 again
      ButtonPushCounter = 0;
    }

means ButtonPushCounter is never 4 for very long, certainly not long enough to reach the case in the code for 4.

Except case 3 is totally ineffective, as you run right into case 4 even though the counter is 3

    case 3:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, HIGH);

// something missing here! means case 4 code runs also

    case 4:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, LOW);

@gcjr

why does the code recognize the pin being LOW as being pressed

Who cares? @hughosulliivan may just be a fan of action on mouse up, not mouse down.

a7

[gcjr]

don't you mean button-released, not mouse up?

@alto777

@gcjr

why does the code recognize the pin being LOW as being pressed

Who cares?

because it is HIGH when pressed, hence the kludge.

[alto777]

@gcjr

don't you mean button-released, not mouse up?

I wrote mouse up and expected clever ppl to answer the questions that might raise all by themselves, as you have done.

It's not a kludge, just not what you do.

HIGH and LOW have nothing to do with introducing an extra variable where none is needed.

This is the code @hughosulliivan was going for, or informed whatever he did try to use:

https://docs.arduino.cc/built-in-examples/digital/Debounce/

It could be he didn't get it quite right, then made it worse trying to fix it, then made it even worse when he did finally. Get it working.

Like many examples the switch is pulled down. The original it was taken from used a pulled-up switch, as the article points out. Having switches read HIGH when pressed looks like it is just another part of the Arduino ppl's attempt to keep things simple.

a7

[hughosulliivan]

Right then

https://wokwi.com/projects/439365183505570817
const int ButtonPin = 2;
const int RedLEDPin = 11;
const int YellowLEDPin = 9;
const int GreenLEDPin = 5;
int RedLEDState = LOW;
int YellowLEDState = LOW;
int GreenLEDState = LOW;
int ButtonState = LOW;
int lastButtonState = LOW;
int ButtonPushCounter = 0;

unsigned long lastDebounceTime = 0;
unsigned long debounceDelay = 25;   // 100? get better switches!

void setup() {
//... switch wired to ground, so
  pinMode(ButtonPin, INPUT_PULLUP);
  pinMode(RedLEDPin, OUTPUT);
  pinMode(YellowLEDPin, OUTPUT);
  pinMode(GreenLEDPin, OUTPUT);
  Serial.begin(9600);
}

void loop() {                            // start by pulling from the debounce script,
  int reading = digitalRead(ButtonPin);
 
  if (reading != lastButtonState) { 
    lastDebounceTime = millis();
  }

  if (millis() - lastDebounceTime > debounceDelay) {
    if (reading != ButtonState) {
      
      ButtonState = reading;
    
      if (ButtonState == LOW) {   // iincrement the counter. on press (LOW for my hardware)
        ButtonPushCounter++;
        Serial.println("On");
        Serial.print("Number of Button Presses  ");
        Serial.println(ButtonPushCounter);
        Serial.print(millis() - lastDebounceTime);
        Serial.println("  Last Debounce Time");
        if (ButtonPushCounter >= 4) {
          ButtonPushCounter = 0;
        }
      }
    }
  }

//...
  lastButtonState = reading;
  switch (ButtonPushCounter) {
    case 1:
      digitalWrite(RedLEDPin, HIGH);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, LOW);
      break;
    case 2:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, HIGH);
      digitalWrite(GreenLEDPin, LOW);
      break;
    case 3:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, HIGH);
    case 4:
      digitalWrite(RedLEDPin, LOW);
      digitalWrite(YellowLEDPin, LOW);
      digitalWrite(GreenLEDPin, LOW);
      break;
  }
  static int lastPrinted = -1;  // not for long!
  if (lastPrinted != ButtonPushCounter) {
    Serial.print(" counter is ");
    Serial.println(ButtonPushCounter);
    lastPrinted = ButtonPushCounter;
  }
}

That worked.

Thanks but I’m concerned that relying on just Pull_Up and not using the 5V line limits any chance of this scaling up, say if i need other components to work alongisde the LEDs. So now I need to see if I can drive the LEDs from the positive line.

[gcjr]

@hughosulliivan

Thanks but I’m concerned that relying on just Pull_Up and not using the 5V line limits any chance of this scaling up, say if i need other components to work alongisde the LEDs

not sure what you're trying to say. sounds like you're mixing button press detection and driving LED.

most GPIOs have an optional pull-up resistor, if not also an optional pull-down, allowing a button switch to be connected to the pin and either ground or Vcc.

as for LEDs, the code i posted defines Off and On to clearly identify what the write does. Changing the enumerations allow the opposite approach. I believe it's more common for an output pin to turn on an LED by being set LOW

@gcjr

enum { Off = HIGH, On = LOW };