Don't stick the output of millis() into an int

[pneumatic]

I just tracked down a tricky bug, and I thought I'd share what I learned.

I had been doing button debounce using a variable called btn_debounce_timer, like so:

Code: Select all

#define DEBOUNCE_DELAY 50
int btn_debounce_timer = 0;
[...]
boolean btn_read(boolean *btn_dwn, int btn_pin) {
  if ( (millis() - btn_debounce_timer) <= DEBOUNCE_DELAY ) return false;  // too soon to read again

  if ( digitalRead(btn_pin) == 0 ) {  // the button is pressed
    if ( ! *btn_dwn ) {  // the state has changed
      btn_debounce_timer = millis();
      *btn_dwn = true;
      return true;
    }
  } else {
    if ( *btn_dwn ) {  // the state has changed
      btn_debounce_timer = millis();
      *btn_dwn = false;
      return true;
    }
  }
  
  return false;
}

The function is supposed to return true only if the state changes, and store the button state in the boolean btn_dwn.

The bug was the it seemed to work fine for a while, then it would stop debouncing properly. The problem is that on the arduino/ATmega, the "int" type is a signed 16 bit integer, so after 32767ms (about half a minute) the value of millis() overflows the 16 bit int, and you get weird behavior. Changing the type of btn_debounce_timer to uint32_t fixed all the problems. I've been too used to assuming that an int is 32 bits (or greater).

[adafruit_support_mike]

Nice catch!

The possibility for confusion between signed and unsigned is one of the reasons C has been called "a language with all the programming power of machine code, and all the readability of machine code." ;-)

[adafruit_support_bill]

"a language with all the programming power of machine code, and all the readability of machine code."

Exactly my thoughts when I was first exposed to it years ago. It borrows an awful lot from Macro-11.