Debouncing a limit switch in Arduino ISR with delays

Question

I have a limit switch attached to an arduino Mega 2650 for motion control. The limit switch's two Normally Open contacts are connected to an Arduino Pin and ground, such that when the Limit Switch is engaged, the Arduino Pin gets short circuited to ground.

As expected, I have bouncing issues with this setup. I confirmed it using counters in my ISRs. Finally, I wrote the following code that seems to reliably identify whether my limit switch is engaged or disengaged at any given point in time.

const int lsOuterLeftIn = 18; // lsOuterLeftIn is my Limit Switch
const int LED = 9;
volatile bool lsEngaged = false; // flag for limit switch engaged
void setup() {
    pinMode(lsOuterLeftIn, INPUT_PULLUP);
    pinMode(LED, OUTPUT);
    attachInterrupt(digitalPinToInterrupt(lsOuterLeftIn), ISR1, FALLING);
    attachInterrupt(digitalPinToInterrupt(lsOuterLeftIn), ISR2, RISING);
}
void  loop() {
    if (lsEngaged) digitalWrite(LED, HIGH);
    else digitalWrite(LED, LOW);
}
void ISR1(){
    delay(100);
    lsEngaged = (digitalRead(lsOuterLeftIn));
}
void ISR2(){
    delay(100);
    lsEngaged = (digitalRead(lsOuterLeftIn));
}

But, here is my problem. I came upon this Arduino documentation page, and it says

"Since delay() requires interrupts to work, it will not work if called inside an ISR. "

But, I do make use of delay() inside ISRs and it seems to work, what is going on? Do I have a situation where things are working at the moment, but could break easily because the delay() function could malfunction on me as the documentation says?

Answer 1

TomKeddie's answer looks right: you won't have any problems. Anyway your code is, in my opinion, conceptually wrong for at least two reasons. Now I'll explain you why.

There are two kind of inputs: the ones to which you have to answer IMMEDIATELY and the ones you have to answer but are not immediate threats. For instance usually a security endstop falls in the first group, since as soon as you hit it you need to stop the actuator. UI buttons, on the other side, fall in the second group, since you don't need to answer it immediately.

NOTE: in a well-done program you typically can answer to the second kind of inputs within tenth of milliseconds, so a user will never see delays.

Now, if your input falls in the second group of inputs, you shall NOT use an ISR to read it, since you may block something more important. Instead read it in the main loop, properly debouncing it. For instance you can use the Bounce library, or implement it by yourself:

#define CHECK_EVERY_MS 20
#define MIN_STABLE_VALS 5

unsigned long previousMillis;
char stableVals;

...

void  loop() {
    if ((millis() - previousMillis) > CHECK_EVERY_MS)
    {
        previousMillis += CHECK_EVERY_MS;
        if (digitalRead(lsOuterLeftIn) != lsEngaged)
        {
            stableVals++;
            if (stableVals >= MIN_STABLE_VALS)
            {
                lsEngaged = !lsEngaged;
                stableVals = 0;
            }
        }
        else
            stableVals = 0;
    }

    ...
}

This will check every 20ms if the value changed. The value, however, is updated only if it is stable for more than 5 cycles (i.e. 100 ms).

This way you are not blocking your main program with that task.

If on the other hand, your input represents a severe threat for your device (e.g. an endstop), you NEED to answer as soon as you can. If this is your case, you are waiting for 100ms before answering, and this is against the need for quickness of your input.

Of course you CAN'T debounce such an input, since debouncing presents delays. You can, however, privilege one state over the other. In the connected-to-ground-endstop case, the severe threat is when the input state is ground. So I suggest you to set your variable in such a way that:

1. when the pin goes down you immediately set it to 0
2. when the pin goes up you wait for 100 ms (IN THE MAIN LOOP) and then set it up.

The code to do this is something like:

#define CHECK_EVERY_MS 20
#define MIN_STABLE_VALS 5

unsigned long previousMillis;
char stableVals;

attachInterrupt(digitalPinToInterrupt(lsOuterLeftIn), ISR1, FALLING);

...

void  loop() {
    if ((millis() - previousMillis) > CHECK_EVERY_MS)
    {
        previousMillis += CHECK_EVERY_MS;
        if ((digitalRead(lsOuterLeftIn) == HIGH) && (lsEngaged == LOW))
        {
            stableVals++;
            if (stableVals >= MIN_STABLE_VALS)
            {
                lsEngaged = HIGH;
                stableVals = 0;
            }
        }
        else
            stableVals = 0;
    }

    ...
}

void ISR1()
{
    lsEngaged = LOW;
}

As you can see the ONLY interrupt is the falling one, and MOST IMPORTANT, it is very short.

If you need to perform other instructions, such as halting the motor, you can in the ISR1 function (IF they are quite short).

Just remember: ISRs MUST be as short as possible, since when the microcontroller is in one of them it becomes blind to everything else

Answer 2

On AVR, delay() is implemented as below. There are no interrupts involved (micros() returns the timer0 count value, yield() refers to the scheduler which would not be used in your simple sketch).

I think that comment is there for portability, you're using an environment that works on more and more platforms. What you're doing is fine on the AVR, not so much on another platform perhaps.

I suggest spin waiting with a simple for loop. The cpu isn't doing anything else, unless power consumption is a concern, but that is beyond the scope here.

From https://github.com/arduino/Arduino/blob/79f5715c21a81743443269a855979a64188c93df/hardware/arduino/avr/cores/arduino/wiring.c

void delay(unsigned long ms)
{
    uint16_t start = (uint16_t)micros();

    while (ms > 0) {
        yield();
        if (((uint16_t)micros() - start) >= 1000) {
            ms--;
            start += 1000;
        }
    }
}

Answer 3

From your debouncing code, it looks like you can spare 100ms of reaction time to the switch engaging.

So, if you don't really need to react within microseconds of the event, consider just polling the input every say 10ms (e.g. from a timer ISR).

(There are only two reasons to use an external interrupt: 1. you need to react to a signal really fast (µs!), or 2. you need to be woken up from a deep power save mode where timers are not active. For everything else you can go for timer-based polling.)

Pseudocode:

#define STABLE_SIGNAL_DURATION 5

uint8_t button_time_on = 0;

volatile bool button_is_pressed = false;

...
// Every 10ms do (can be done in a timer ISR):

if ( read_button_input() == ON ) {

  if ( button_time_on >= STABLE_SIGNAL_DURATION ) {

    button_is_pressed = true;

  } else {
     button_time_on++;
  }

} else {
  button_time_on = 0; // button not pressed (any more).
  button_is_pressed = false;
}

...

and in main():

bool button_press_handled = false;

while(1) {
  // do your other main loop stuff...

  button_press_handled = button_press_handled && button_is_pressed;

  if ( !button_press_handled && button_is_pressed ) {

    // Handle press of the button

    // ...

    // Note that we handled the event for now:
    button_press_handled = true;
  }
}

Answer 4

easier than using timestamps

volatile bool buttonDirty = false;

void setup() {
    attachInterrupt(digitalPinToInterrupt(buttonPin), buttonPress, FALLING);
}


void loop() {
  while(1){
    readButtons();
  }
}

void buttonPress(){
  if(buttonDirty) return;
  buttonDirty = true;  
}

void readButtons(){
  if(!buttonDirty) return;
  delay(100);
  ...........

}