Debounce Code not working

I’m using multiple buttons for my Arduino project, and I thought it might be easier to debounce the values if I used an if statement, so, most of my code looks weird. In case it’s important, I also have some through hole neopixels hooked up.

As for the circuit, I can guarantee it works, because the buttons (mostly) work.

int buttonLeft = 11;
int buttonRight = 12;
int pixelsPin = 6;

int buttons[] = {11, 12};

int playerPin = 0;

long debounceDelay[] = {50, 50}; 

long lastDebounceTime[] = {0, 0};
int lastButtonValue[] = {0, 0};
int buttonState[] = {false, false};
int buttonValue[] = {0, 0};

bool isTimerOn[] = {false, false};
long timerStartTime[] = {0, 0};

#include <Adafruit_NeoPixel.h>

void setup() {
  Serial.begin(9600);
  Adafruit_NeoPixel pixels = Adafruit_NeoPixel(5, pixelsPin, NEO_RGB + NEO_KHZ800);
  pinMode(11, INPUT);
  pinMode(12, INPUT);
  pinMode(6, OUTPUT);
}

void loop() {
  for(int i = 0; i < 2; i++) {
    buttonValue[i] = digitalRead(buttons[i]);
    if(buttonValue[i] != lastButtonValue[i]) {
      lastDebounceTime[i] = millis();
    }

    [color=red]//If I print right here, the buttonValue is correct.[/color]

    if((millis() - lastDebounceTime[i]) > debounceDelay[i]) {

      [color=red]//If I print right here, buttonValue is always a 0.[/color]

      if(buttonValue[i] != buttonState[i]) {
        buttonState[i] = buttonValue[i];
      }

    [color=red]//I have a bit more after this, but I don't show it because it is redundant. The code basically stops working here[/color]

My guess here is that the if() statement basically ‘filters out’ the 1s.
Any help is appreciated, thanks.

   if(buttonValue[ i ] != [color=red]lastButtonValue[/color][ i ]) {

You forgot to maintain lastButtonValue

Yeah, sorry, I should have mentioned, at the end of the program, I DID update last button value.

Edit: Oh my gosh, thank you! You are a life saver! I rechecked where I put "lastButtonValue = buttonValue*;", and it was actually wrong, it works perfectly now! Thank you!*

Good :)

SpencerWhite: Yeah, sorry, I should have mentioned, at the end of the program, I DID update last button value.

Edit: Oh my gosh, thank you! You are a life saver! I rechecked where I put "lastButtonValue = buttonValue*;", and it was actually wrong, it works perfectly now! Thank you!*

Diffuse logic is evil.

There is absolutely no reason that your switch bounce filtering code should be entwined with the logic of your program. When the logic for a single feature is strewn all over the place, it makes debugging a PITA. In this case, you included most of the debouncing logic, but unhelpfully omitted the part that was actually incorrect because you guessed wrong about where the error is. This wastes our time.

There is a ready-made Bounce library you can download, but re-implementing a common and simple thing like this can be good practice for you. I recommend you do that.

C and C++ have a wonderful feature called structures that let you package multiple different related variables of disparate types into a single object to perform a function. Structures can also have member functions that can automatically process the structs variables and return a result. The Serial object is a perfect example of this. You usually don't need to know or care much about the exact details of how it handles the read, and write buffers and gets the data to and from the UART peripheral on the chip, you just use the print(), read(), and available() functions and the object itself handles all the rest internally.

For buttons, if I have variables in the button struct called _debounced_level and _private_debounced_level, I can create these functions to handle level and edge logic:

    button_level level() const {return _debounced_level;}
    bool edge() const {return _debounced_level != _previous_debounced_level;}
    bool is_low() const {return level()==LOW;}
    bool is_high() const {return level()==HIGH;}
    bool rising_edge() const {return high_level() && edge();}
    bool falling_edge() const {return low_level() && edge();}

By packaging all the needed variables into a single struct, you don't have to have a gazillion parallel arrays to hold all the individual values needed, you can just declare a single array to hold a bunch of the structure objects, or multiple individual objects with a meaningful name:

button b_play_pause;
button b_stop;
button b_dpad[4];

Then every time through loop you call the debounce() or check(), or whatever you call it function poll the button and perform the debounce filtering. Then you can use simple member functions like the ones I posted above to process those internal variables into meaningful values.

Get into the habit of packaging up related code and variables into structures from the very beginning, and it'll make your life a lot easier, especially if you want to reuse the code in another sketch. Reusing a structure's code is much easier if it's all packaged in one place, instead of having to search for it within the jungle of your code.

@jiggy

You are conceptually correct in the approach but what you pitch is beyond basics and out of reach for beginners in programming. Using a button library encourages a good practice at the expense of using someone else code you might not understand and bloating your own code.

Arduino is also for experimenting and learning, the OP has been trying something and he tried to expand from the debounce article clearly in the arduino doc which works exactly the way he did but with only one button.

So we could argue that the doc in arduino.cc in the learning section pointing at debounce is a bad practice, misleading beginners... My view is that it's a right place to start, and only as you understand more about C++ and can read libraries and code from other then you go do more complex stuff...

it was totally obvious from his post that the pb was what I stated. It took me 5 sec to guess, and the guess was right...

@ J-M-L

structs are more "advanced" than a half-dozen parallel arrays? No. They aren't that advanced. They're probably less confusing than pointers for most people. There's a place in these topics not just for answering the immediate question, but suggesting a better way to do it.

I did not recommend using someone else's library, I recommended Spencer make his own version of it. Debouncing buttons, probably the most common non-trivial function a microcontroller needs to do, is the perfect kind of thing that everyone should practice making a library for.

Good practices exist for a reason, because if you learn them and discipline yourself to follow them they tend to make your life easier. That's why we say to use named constants instead of magic numbers; that's why we say to use functions, for and while loops, and if branches instead of goto. It isn't wrong to use magic numbers and goto, it just makes your job more difficult than it has to be.

If I'm doing quick-and-dirty testing of a single thing that I haven't tried before, I'll do the "strewn about all over the place" approach for the test sketch. But before trying to integrate it into something larger I will reimplement it into an enclosed package so that I don't have to copy and paste a dozen different things into 10 different places.

I don't pretend that I'm a master at this stuff. I'm not old (25), nor am I a programmer as my primary profession. My programming sophistication is still no where near the level of something like the Standard Template Library, and I'm pretty sure when I was just starting out I implemented multiple buttons the exact same way Spencer just did.

If you continue to program though, once you have to fix a stupid issue like this for the dozenth time, you look for ways to fix it, and that's when you start to understand why these practices exist. With more practice you'll get an eye for planning ahead on the kind of things you'll need to make to simplify the project. For example, I'm planning currently planning a project that will involve controlling 3 servo motors. Since it's a big, heavy, and relatively expensive thing they'll be swinging around, an important part of the software will be limiting the rate of change of the servo setting. For that, I have already created a creeping_integer class that, after changing the setpoint, will creep to the new value as a speed set by a delta_value and delta_time slope. This class will be used to generate the control setting for the servos so they gentle move for position to position, rather than jerking around.

That sort of thing comes with experience, so the only thing you can do is to keep doing and keep trying to improve.

I was able to guess what the problem in this post might be too, because I know what's needed for debouncing and I saw what was missing. It was just luck that there was only one piece of the logic missing from the post, and that debouncing has such a standard pattern that it was easy to tell what was missing.

As mentioned fully Agreed your suggestion is the way to go longer term but reading his short code and question, I felt he is at square one and struct, member functions etc are as remote as everything else probably to him and probably not within immediate grab. when dealing with multiple things repetitively an array and a loop comes to mind immediately and that's what he was doing based on the arduino learning exercise.

As you can see my answer was kinda a one liner pointing him to where the error was likely to be and for him to explore. He noticed he forgot to mention that - probably learnt a lesson there - and then went to check and found out this was his issue, learnt a second lesson... And It took me literally 5 seconds...

Might be just different personality or cultural thing, but I feel it's not helpful to shout at a newby and tell him he is wasting our time. Don't we come here and participate in the forum in the spirit of helping out and by definition "wasting" or investing some of our time for the benefit of others? Let him suffer reinventing the wheel 12 times then he will know and understand the power of struct, classes, abstractions, etc...

PS / your project looks cool.

I do know a bit about OOP, and I've used it in the past(Not alot, I'm somewhat new to it). I didn't think about using it for my code, though...Thanks, that's a great idea!

Messy, and ameteur, but it is finished. This is my class for the button:

class Button { long lastDebounceTime; int lastButtonValue; int buttonValue; int buttonState; int pin;

public: Button(int Pin) { pin = Pin; } int debounce() { buttonValue = digitalRead(pin); if(buttonValue != lastButtonValue) { lastDebounceTime = millis(); } if((millis() - lastDebounceTime) > 50) { if(buttonValue != buttonState) { buttonState = buttonValue; } } return buttonValue; } };

SpencerWhite: Messy, and ameteur, but it is finished. This is my class for the button:

class Button { long lastDebounceTime; int lastButtonValue; int buttonValue; int buttonState; int pin;

public: Button(int Pin) { pin = Pin; } int debounce() { buttonValue = digitalRead(pin); if(buttonValue != lastButtonValue) { lastDebounceTime = millis(); } if((millis() - lastDebounceTime) > 50) { if(buttonValue != buttonState) { buttonState = buttonValue; } } return buttonValue; } };

First, code tags.

Second, you have one huge bug in debounce() that will stop this from working properly.

Also, magic numbers are bad. Have a parameter in the constructor to set a debounceInterval variable. 50 ms is usually more than enough for a little clicky tactile button, but other buttons (like the big ones on arcade machines) may require even more than that. When you're going through the trouble of making a struct, it's best to make it as generic as possible.

What is the difference between buttonState and buttonValue? There are no comments, so it's not obvious at all.

This struct only lets you get the button's level from debounce's return value. There's no way to detect a change in the button's state.

lastDebounceTime is the wrong type. It must be unsigned.

Finally, an additional feature you might not have known about: you can use typedef inside classes and structs to provide a local typename. You can even use decltype() to get the return value of a function.

class Button {
public:
  typedef decltype(millis()) time_type;
  typedef uint8_t pin_no_type;
  typedef uint8_t level_type;
private:
  time_type lastDebounceTime;
  level_type lastButtonValue;
  level_type buttonValue;
  level_type buttonState;
  pin_no_type pin;
  
  public: Button(pin_no_type Pin) {
    ...
  }
  level_type debounce() {
    ...
  }
};

See that first typedef? I don't need to look up what return type millis() is when I can make the compiler do it for me. :D

You can use them outside the class too. If I need a local variable in my sketch to store a button level from this class and I don't want to look up the type, I can do:

Button::level_type play_button_level = HIGH;

You won't get it perfect right away, of course. Just make sure you test it.