arduino / arduino-examples

Arduino IDE bundled examples
Creative Commons Zero v1.0 Universal
92 stars 42 forks source link

Undefined behaviour (bug) in button debouncing tutorial code #10

Open mrexodia opened 7 years ago

mrexodia commented 7 years ago

The documentation page: https://www.arduino.cc/en/Tutorial/Debounce has an error in the code.

The variable buttonState at line 40 is uninitialized which will cause undefined behaviour the first time line 75 if (reading != buttonState) is executed.

Usually (based on testing) buttonState is not equal to reading which results in detection of a button press on startup that did not happen. Since reading is HIGH when the button is not pressed, the correct initial value for buttonState is also HIGH. When changing this, the example functions correctly and the LED is on after the program started.

per1234 commented 7 years ago

The variable buttonState at line 40 is uninitialized which will cause undefined behaviour

Global variables are always zero initialized so there's no undefined behaviour.

reading is HIGH when the button is not pressed

 * pushbutton attached from pin 2 to +5V
 * 10K resistor attached from pin 2 to ground

So reading is actually LOW when the button is not pressed. LOW happens to be defined as 0 but it's probably safer to not assume this and explicitly initialize buttonState to LOW.

mrexodia commented 7 years ago

Ah yes, my schematics were the other way around. Thanks for your comment! Is there a reference that explains the initialization of global variables? I'm new to Arduino so I didn't know about that.

per1234 commented 7 years ago

Unfortunately I'm not aware of a reference for this other than the C++ standard (section 3.6.2). The free draft version closest to the C++11 standard (which is not free) is: https://github.com/cplusplus/draft/raw/master/papers/n3337.pdf but that's not really the most fun reading material.

I'm not sure this issue should be closed. I don't see that the zero initialization of buttonState actually causes a problem but it's essentially a magic number so initializing it to LOW would be better.

mrexodia commented 7 years ago

Here is my class that I wrote that works for both the pull up and pull down configuration (pull up actually makes more sense as a default because analog pins provide INPUT_PULLUP). It might be useful for someone who sees this issue.

//https://www.arduino.cc/en/Tutorial/Debounce
struct DebouncedButton
{
    DebouncedButton(byte buttonPin, byte unpressedRead)
      : buttonPin(buttonPin),
      unpressedRead(unpressedRead),
      buttonState(unpressedRead),
      lastButtonState(unpressedRead) { }

    // call this function in your setup()
    void setup(byte inputMode = INPUT) {
      pinMode(buttonPin, inputMode);
    }

    // call this function in your loop() before you want to use IsPressed
    void loop() {
      // read the state of the switch into a local variable:
      byte reading = digitalRead(buttonPin);
      unsigned long currentMillis = millis();

      // the button is not pressed
      buttonPressed = false;

      // check to see if you just pressed the button
      // (i.e. the input went from LOW to HIGH),  and you've waited
      // long enough since the last press to ignore any noise:

      // If the switch changed, due to noise or pressing:
      if (reading != lastButtonState) {
        // reset the debouncing timer
        lastDebounceTime = currentMillis;
      }

      if (currentMillis - lastDebounceTime >= debounceDelay) {
        // whatever the reading is at, it's been there for longer
        // than the debounce delay, so take it as the actual current state:

        // if the button state has changed:
        if (reading != buttonState) {
          buttonState = reading;

          // only toggle the LED if the new button state is HIGH
          if (buttonState == !unpressedRead) {
            buttonPressed = true;
          }
        }
      }

      // save the reading.  Next time through the loop,
      // it'll be the lastButtonState:
      lastButtonState = reading;
    }

    bool IsPressed() {
      return buttonPressed;
    }

  private:
    byte buttonPin;               // the number of the pushbutton pin
    byte unpressedRead;           // the value read when the button is not pressed
    byte buttonState;             // the current reading from the input pin
    byte lastButtonState;         // the previous reading from the input pin
    bool buttonPressed = false;   // whether the button is pressed

    unsigned long lastDebounceTime = 0;  // the last time the output pin was toggled
    const int debounceDelay = 20;        // the debounce time; increase if the output flickers
};

Usage:

DebouncedButton button1(A0, HIGH);

void setup() {
  button1.begin(INPUT_PULLUP);
  Serial.begin();
}

void loop() {
  button1.loop();

  if(button1.IsPressed())
    serial.println("Button 1 pressed");
}
DavidEGrayson commented 7 years ago

The Pushbutton library can also be used. It can be used with any type of button wiring and it handles debouncing.

mrexodia commented 7 years ago

Yeah I am not allowed to use libraries for debouncing in this assignment so I had to write my own...