I'm using a Nano. The code below is a simplified workup of my problem code.
The point of it is asynchronous debounced button press detection. The actual code sets a volatile bool to tell the main loop it saw a press, but this simplified version toggles the state of the built-in LED. Another LED follows the state of the pushbutton to make it possible to see whether the ISR is firing appropriately.
#define SHORT_PRESS_MS 50
#define PUSHBUTTON_PIN 2
#define BUILTIN_LED_PIN 13
#define OTHER_LED_PIN 12
volatile unsigned long fallTime = 0;
volatile int builtinLedState = LOW;
void isr_change()
{
unsigned long now = millis();
int pushbuttonState = digitalRead(PUSHBUTTON_PIN);
// show switch state with LED and 120 Ohms on D12
digitalWrite(OTHER_LED_PIN, pushbuttonState);
if ((pushbuttonState == LOW) || (fallTime == 0))
{ // change to low => falling edge
fallTime = now;
}
if (pushbuttonState == HIGH)
{ // change to high => rising edge
long pressMs = now - fallTime;
if (pressMs > SHORT_PRESS_MS)
{
builtinLedState = !builtinLedState;
digitalWrite(BUILTIN_LED_PIN, builtinLedState);
}
}
}
void setup()
{
pinMode(PUSHBUTTON_PIN, INPUT_PULLUP);
pinMode(BUILTIN_LED_PIN, OUTPUT);
pinMode(OTHER_LED_PIN, OUTPUT);
attachInterrupt(digitalPinToInterrupt(PUSHBUTTON_PIN), isr_change, CHANGE);
}
void loop() {}
When this runs, OTHER_LED slavishly follows the button. There is no perceptible lag and it never gets out of synch no matter how machine-gun my antics with the button.
However, toggling of the built-in LED is not reliable. That OTHER_LED remains in sync with the button state implies that the event is always firing and the state is always what I expect, so it seems like there is a problem with my attempt to measure time.
What is the right way to measure time inside an ISR?
Applying suggested diagnostics confirmed the hypothesis of the accepted answer. Having determined the nature of the problem, I rewrote the ISR as follows.
void isr_change()
{
static long fallTime = 0;
static int lastPushbuttonState;
int pushbuttonState = digitalRead(PUSHBUTTON_PIN);
if (lastPushbuttonState != pushbuttonState)
{
long now = millis();
if (pushbuttonState == LOW)
{ // change to low => falling edge
fallTime = now;
}
if (pushbuttonState == HIGH)
{ // change to high => rising edge
long pressMs = now - fallTime;
if (pressMs > SHORT_PRESS_MS)
{
digitalWrite(LED_BUILTIN, !digitalRead(LED_BUILTIN));
}
}
}
}
Testing reveals that the problem is resolved and the intended behaviour is occurring (reliably detect presses of minimum duration).
Also of interest is the use of static
to carry values between ISR invocations with local scope, rather than using global volatile variables.