Strange bug registering button input

Hi,

In the sketch below, I’m having a problem where my button debouncing code is registering 2 separate button presses. One when the switch is pressed and another when the switch is released. My sketch is below. I’m running this on a Wemos D1 Mini and using platformio as my IDE.

#include <Arduino.h>

// Switches between 3 Blinking LED's.

class BlinkLed{
    int ledPin; //LED Pin to blink
    int ledState; // current state of the led
    unsigned long previousMillis; // last the led was updated in milliseconds
    unsigned long currentMillis; // Current time the led was updated in milliseconds
    unsigned long blinkInterval; // rate at which to blink the led in milliseconds
    
    // constructor
    public:BlinkLed(int ledpin, unsigned long interval){
      ledPin = ledpin;
      blinkInterval = interval;
      pinMode(ledPin, OUTPUT); // set led pin as an output
      ledState = LOW; // Start with led turned off
      Serial.begin(9600);
    }

    void Blink(){
        currentMillis = millis(); //store the current time
        
        //blink the led at the frequency defined in interval if the led is marked as active (ledActive is true)
        if( (currentMillis - previousMillis > blinkInterval)) {
          previousMillis = currentMillis; // Save last time we blinked the LED

          // If LED is off turn it on and visa-versa
          if(ledState == LOW){
            ledState = HIGH;
          }else{
            ledState = LOW;
          }
        
          // write state of led to the relevent pin
          digitalWrite(ledPin, ledState);
        }
    }

    void DeactivateLed(){
      // set led pin to LOW (Deactivate the LED)
      digitalWrite(ledPin, LOW);
    }

};

class Switch{
    int switchPin; //Pin the switch is connected to
    int switchState; // current state of the switch
    int lastSwitchState; // last known state of the switch
    unsigned long debounceDelay; // Number of milliseconds to wait before considering a button press valid 
    unsigned long lastDebounceTime; // last time stored in the debounce timer in milliseconds
    unsigned long previousMillis; // last the led was updated in milliseconds
    unsigned long currentMillis; // Current time the led was updated in milliseconds
    int ButtonCounter; // Counts how many times the button has been pressed

    public:Switch(int pin, unsigned long debouncedelay){
      switchPin = pin;
      debounceDelay = debounceDelay;
      previousMillis = 0;
      currentMillis = 0;
      lastSwitchState = LOW;
      ButtonCounter = 0;
    }

    void CheckSwitchState(){
        // read the state of the switch pin into a local variable
        int reading = digitalRead(switchPin);

        // check to see if the button was pressed (LOW to HIGH transition)
        // and we've waited long enough since the last press to avoid any noise
        if(reading != lastSwitchState){
          //reset the debouncing timer
          lastDebounceTime = millis();
        }

        if ( (millis() - lastDebounceTime) > debounceDelay){
            
              //lastDebounceTime = millis(); // store the last debounce time value
              
              //whatever the reading is at, its been there longer than than the debounce delay
              //so this will be its actual state
              //Serial.println("millis - lastdebouncetime is more than debounce delay");
              //if the button has changed
              if(reading != switchState){
                //Serial.println("increment the counter");                
                switchState = reading;
                ButtonCounter = ButtonCounter + 1;
                //ButtonCounter = ButtonCounter - 1; 
                //Serial.println("switchstate set equal to reading");
              }
                     
          }
              // save the switch reading. next time through the loop it will be the lastSwitchState  
              lastSwitchState = reading;
 
    }         

    bool ButtonPressed(){
        CheckSwitchState();
        if(switchState == HIGH){
          return true;
        }else{
          return false;
        }

    }

    int GetButtonCounter(){
      //CheckSwitchState();
      return(ButtonCounter);
    }

    void ResetButtonCounter(){
      ButtonCounter = 0;
    }

};

// initialise objects
BlinkLed led1(5, 1000);
BlinkLed led2(4, 1000);
BlinkLed led3(15, 1000);
Switch switch1(16, 150);

void setup() {
  // put your setup code here, to run once:
  Serial.begin(9600);
}

void loop() {
  // put your main code here, to run repeatedly:
  
  
  switch1.ButtonPressed(); // check for a button press

  //Serial.println(switch1.GetButtonCounter());

  
  if((switch1.GetButtonCounter() == 1)){
    led1.Blink();
    led2.DeactivateLed();
    led3.DeactivateLed();
  }

  if (switch1.GetButtonCounter() == 2){
    led1.DeactivateLed();
    led2.Blink();
    led3.DeactivateLed();
  }
  
  if(switch1.GetButtonCounter() == 3){
    led1.DeactivateLed();
    led2.DeactivateLed();
    led3.Blink();
  }

  if(switch1.GetButtonCounter() > 3){
    switch1.ResetButtonCounter();
  }

}

As a result, ButtonCounter is being incremented by 2 on each button press instead of 1.

My question is, why is this happening? It must be a mistake I’m making somewhere, but I can’t seem to spot what/where it is.

looks like you increment ButtonCounter whenever there is a change in switchPin state, not when the button is pressed

why don't you use a timeout to not check the button after a state change instead of when there is? you don't need both switchState and lastSwitchState.

Thank you for your reply. Would I need a check for if the switch is HIGH inside of

if ( (millis() - lastDebounceTime) > debounceDelay){

}

or would the check need to be somewhere else?

I think I've solved it now. Thank you very much for your advice.

I changed my code to the following

if ( (millis() - lastDebounceTime) > debounceDelay){
           
              //lastDebounceTime = millis(); // store the last debounce time value
             
              //whatever the reading is at, its been there longer than than the debounce delay
              //so this will be its actual state

              //if the button state has changed
              if(reading != switchState){               
                switchState = reading;
                // Check if the switch is currently HIGH and its last known state was also HIGH (i.e The button has been pressed)
                if((switchState == HIGH) && lastSwitchState == HIGH){
                   ButtonCounter = ButtonCounter + 1;
                   Serial.println("increment the counter");
                }
               
                //ButtonCounter = ButtonCounter - 1;
                //Serial.println("switchstate set equal to reading");
              }
                     
          }

counter now increments by 1 each time the button is pressed and released.

              if((switchState == HIGH) && lastSwitchState == HIGH){

no need for lastSwitchState, you already know there is a change in state because of switchState

need to capture lastDebounceTime. i usually capture msec = millis() once instead of repeated calls to millis().

Thank you for your advice. Have implemented your advice and the button reading method is working perfectly now.