[SOLVED] Is it acceptable to have an interrupt disable itself?

I've found no problems testing this. It provides an easy way to get instantaneous response to an input and also be immune to any bounce. That is the bounce itself doesn't flood the MCU with dozens of false interrupts because it's turned off until needed. Also, I think this would mean that volatile isn't needed because ISR variables would not change while being read in the main loop.

Here's the code. I just tested by using a wire connected to pin2 and touching GND (USB shell) on an Uno. Print monitor shows inputCount.

const byte inputPin = 2;
byte printFlag = 0;
word inputCount = 0;
const unsigned long interval = 1000; //ms
unsigned long currentMillis, previousMillis = 0;

void setup() {
  Serial.begin(115200);
  pinMode(inputPin, INPUT_PULLUP);
  attachInterrupt(digitalPinToInterrupt(inputPin), inputISR, FALLING);
}

void loop() {
  currentMillis = millis();
  if (currentMillis - previousMillis >= interval) {
    previousMillis += interval;
    EIFR |= bit(INTF0);   // clear INT0 interrupt flag
    EIMSK |= bit(INT0);   // enable INT0 interrupt
  }
  if (printFlag) {
    Serial.println(inputCount);
    printFlag = 0;
  }
}

void inputISR() {
  inputCount++;
  printFlag = 1;
  EIMSK &= ~bit(INT0);  // disable INT0 interrupt
  EIFR |= bit(INTF0);   // clear INT0 interrupt flag
}

volatile is always needed for variables shared between an ISR and other code otherwise the compiler is free to (correctly) optiimize the variable access away.

Thanks MarkT, I didn't know the compiler might do that, so I'll stick to using volatile. However, I would think, in this case, any volatile variable could be read safely in the main loop without wrapping it with noInterrupts() and interrupts().

dlloyd: Thanks MarkT, I didn't know the compiler might do that, so I'll stick to using volatile. However, I would think, in this case, any volatile variable could be read safely in the main loop without wrapping it with noInterrupts() and interrupts().

Wrong. I had to hunt for it, but I found the race condition.

Here's the sequence:

At the top of this loop iteration, printFlag is 1 because of a previous interrupt event. INT0 is disabled.

The time interval expires this time too, reenabling INT0.

Because printFlag is high, it goes to print inputCount, and another INT0 event happens in the middle of reading inputCount. Boom, there you go.

You might say "That's so unlikely!" That's besides the point. Race conditions are always unlikely things that depend on exactly the right sequence of events happened at exactly the wrong times. Because they happen in separate parts of the code and are a non-deterministic bug, they are hell to debug.

Guard against race conditions every time. I had to think of several different paths and conditions the code could follow before I found this one. Do not just think "I don't see a need, therefore there isn't a need."

Thank you Jiggy-Ninja for taking the time to investigate +1. I most definitely want to avoid any possible race condition and will think this through as what can be done to guard against it. I'm being too distracted at this time ... will report back later.

EDIT: Here's the fix

if (printFlag) {
  noInterrupts();
  word inputCountCopy = inputCount;
  interrupts();
  Serial.println(inputCountCopy);
  printFlag = 0;
}

Did some more research, looked through these References and Resources.

From here under "Enabling and Disabling Interrupts" is states:

"After being enabled, individual interrupt sources can be disabled and re-enabled as needed at any point in the program, and global interrupts can also be disabled and re-enabled at any point in the program."

The code within an ISR should be "any point in the program" but I'm thinking its safer to have ISR disable itself at the end of the routine.

Still not 100% sure if this answers my question.

Taking the Title on its own the direct answer is that it is perfectly acceptable and not at all unusual.

Whether it makes sense in a particular case is another matter entirely.

...R

Thanks Robin2, I'll mark this as [SOLVED] and take your response as a "good to go". A monster has been unleashed ...

dlloyd: if (printFlag) {   noInterrupts();   word inputCountCopy = inputCount;   interrupts();   Serial.println(inputCountCopy);   printFlag = 0; }

In this case I don't think it will matter but you really need to get in the habit of accessing ALL shared data in a critical section (printFlag = 0;).

dlloyd: Thank you Jiggy-Ninja for taking the time to investigate +1. I most definitely want to avoid any possible race condition and will think this through as what can be done to guard against it. I'm being too distracted at this time ... will report back later.

EDIT: Here's the fix

if (printFlag) {
  noInterrupts();
  word inputCountCopy = inputCount;
  interrupts();
  Serial.println(inputCountCopy);
  printFlag = 0;
}

That logic will fail if you get interrupted between the Serial.println of your copy and the printFlag =0; The iSR will properly increase the count and set the printFlag to 1 which then you would discard just afterwards and thus never print that event --> So the printFlag is also part of the critical section.

@Coding Badly and J-M-L, Thank you! My initial intention was to keep the interval with noInterrupts() as short as possible and since accessing printFlag is atomic, I left it outside the critical section. I definitely see the potential problem now (thanks J-M-L) and that debugging requires special attention to the changes an ISR can make.

dlloyd: ...since accessing printFlag is atomic...

Sort of atomic. You can consider byte-sized data atomic when only one side reads and only one side writes. In your case both sides write.

So for volatile byte variables,

[color=teal]ISR     loop()   atomic   Use cli() or noInterrupts()[/color] 
read    read     yes      not required
read    write    yes      not required
write   read     yes      not required
write   write    no       required

Exactly.

The Arduino core and libraries are built on top of AVR-Libc. This set of functions contains a rather ingenious way of having atomic sections of code. If you #include <util/atomic.h> you have access to the ATOMIC_BLOCK() macro that lets you have interrupts disabled within a curly braced block, and have them automatically re-enabled when the code exits the block through any path; no need to explicitly turn interrupts back on.

#include <util/atomic.h>

if (printFlag) {
  word inputCountCopy;
  ATOMIC_BLOCK(ATOMIC_FORCEON) // ATOMIC_RESTORESTATE is also an option
  {
    inputCountCopy = inputCount;
    printFlag = 0;
  }
  Serial.println(inputCountCopy);
}