Allow enabling/disabling external interrupts and clear pending interrupts on attach #2159

matthijskooijman · 2014-07-02T20:30:29Z

This is a fix for ~~#510~~arduino/ArduinoCore-avr#244, plus the addition of a new enableInterrupt and disableInterrupt API to cater for libraries that relied on the broken behaviour of attachInterrupt.

See also this discussion: https://groups.google.com/a/arduino.cc/d/topic/developers/I9iJYIcbPLs/discussion

This code is still unfinished:

Only quickly compile tested
Not implemented on SAM yet

matthijskooijman · 2014-07-07T10:28:55Z

I updated the code based on feedback from @PaulStoffregen: enableInterrupt now now never clears pending interrupts, that code is moved into attachInterrupt (which always clears).

On SAM: - all pins in a port share a single interrupt pin - the interrupt status flag for a pin is set even when it is disabled - The actual ISR does not look at the interrupt enable flags, it just runs the callback for every pin whos interrupt status flag is set. This meant that if: - an interrupt was attached and later detached, - then the interrupt condition occured, setting the flag and - then another (still attached) interrupt on a different pin in the same port occured, then the detached interrupt callback would also be called. By masking off the Interrupts Status Register using the Interrupt Mask Register, only actually enabled interrupt handlers are called. The incorrect behaviour has been verified using the following test sketch: /* To test: - Upload this sketch to the Arduino Due - Open the serial monitor at 115200 - Connect a jumper wire to ground and touch the other end to pins 25 and 26, alternating between them */ void int25() { Serial.println("INT25"); } void int26() { Serial.println("INT26"); } void setup() { Serial.begin(115200); pinMode(25, INPUT_PULLUP); pinMode(26, INPUT_PULLUP); attachInterrupt(25, int25, CHANGE); attachInterrupt(26, int26, CHANGE); detachInterrupt(26); } void loop() { }

These allow temporarily enabling and disabling a single interrupt. This is currently pretty much the same as detaching and re-attaching the interrupt, but this will change when attachInterrupt is changed to clear any pending interrupts next. This commit introduces duplicate code, between attach/detach and enable/disable, which will be removed next (doing this in one commit makes the diff harder to review).

…bleInterrupt This removes duplicate code and shrinks the resulting binary slightly.

Prevously, when attachInterrupt was called but the (previously configured or default) interrupt condition has already occured while the interrupt was (still) detached, the interrupt triggers immediately once after attaching, even though the condition didn't occur then. This fixes this problem by clearing any pending interrupts after configuring the interrupt level, but before enabling it. Note that some libraries actually depend on this broken behaviour. Paul Stoffregen wrote: > As a matter of principle, I agree, this is a bug. In practice, this > behavior really is needed by libraries that use attachInterrupt(), > but need to temporarily prevent the interrupt from occurring while > they do lengthy operations which shouldn't globally disable > interrupts. When they reenable the interrupt, the desired behavior > of course it to immediately respond to any pending interrupt that > was detected during that disabled time. However, now that the enableInterupt and disabledInterrupt functions are available, these libraries can use those instead of relying on this bug in attachInterrupt. On the SAM core, the only way to clear pending interrupt flags is to read the Interrupt Status Register. However, this always clears _all_ flags at once. To not lose any interrupts, after reading the status register to clear it, interrupt handlers for any pending interrupts are called. This does mean that if attachInterrupt is called with interrupts globally disabled, interrupt handlers are called nonetheless. This seems to be the lesser of three evils: - Not clearing the status register can cause a bogus interrupt shortly after calling attachInterrupt (this happened in earlier Arduino versions). - Reading the status register but not running interrupts can cause interrupts to be missed when attachInterrupts is called with interrupts disabled (and depending on timing probably also with them enabled). - Running the handlers (as now) causes interrupt handlers to run while calling attachInterrupt with interrupts disabled. This fixes #510.

matthijskooijman · 2014-10-27T17:33:32Z

I just updated this PR to also support SAM and split a commit in two. For clearing an interrupt on SAM, there is a caveat I also pointed out on the mailling list, see the last commit for details.

I haven't runtime-tested the code on Due due to lack of a Due (I'll probably get one soon, though). Any testing on Due is welcomed.

I haven't runtime-tested the code on AVR after the PR update. The code for AVR shouldn't have changed, but it should probably be tested again nonetheless. I started on this, but then found the bootloader on my Uno was broken en somehow my ICSP programmer was also broken and gave up for today...

matthijskooijman · 2014-10-29T16:41:02Z

I just tested this on my Uno, works as expected: Interrupts occuring while detached are ignored, interrupts while disabled are remembered and fired when enabling. For reference, here is the test sketch I used (25 and 26 are legacy from my Due test sketch):



void int25() {
  Serial.println("INT25");
}

void int26() {
  Serial.println("INT26");
}

void setup() {
  Serial.begin(115200);
  pinMode(2, INPUT_PULLUP);
  pinMode(3, INPUT_PULLUP);
  attachInterrupt(0, int25, CHANGE);
  attachInterrupt(1, int26, CHANGE);
  detachInterrupt(0);
  Serial.println("INIT");
  while(Serial.read() == -1);
  attachInterrupt(0, int25, CHANGE);
    Serial.println("ATTACH");
  while(Serial.read() == -1);
  disableInterrupt(0);
      Serial.println("DISABLE");
  while(Serial.read() == -1);
  enableInterrupt(0);
        Serial.println("ENABLE");
} 

void loop() {
}

NicoHood · 2015-09-02T09:49:49Z

Good to know that this exists. Can you build it please?

matthijskooijman · 2015-09-03T06:49:20Z

@ArduinoBot build this please

ArduinoBot · 2015-09-03T07:48:31Z

Build successful. Please test this code using one of the following:
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-2159-BUILD-390-linux32.tar.xz
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-2159-BUILD-390-linux64.tar.xz
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-2159-BUILD-390-windows.zip
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-2159-BUILD-390-macosx.zip

NicoHood · 2015-09-03T17:51:12Z

hardware/arduino/avr/cores/arduino/WInterrupts.c

@@ -154,26 +234,33 @@ void attachInterrupt(uint8_t interruptNum, void (*userFunc)(void), int mode) {

 void detachInterrupt(uint8_t interruptNum) {
  if(interruptNum < EXTERNAL_NUM_INTERRUPTS) {
+    disableInterrupt(interruptNum);
+    intFunc[interruptNum] = 0;


This should be nothing now
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/WInterrupts.c#L273

And the PR needs to be built against the master, otherwise its useless. May you update the PR or better create a new against the master?

CLAassistant · 2021-04-09T13:25:42Z

All committers have signed the CLA.

matthijskooijman added 5 commits October 27, 2014 16:39

Fix indentation and trailing whitespace in WInterrupts.c

ef95809

Let attachInterrupt and detachInterrupt call enableInterrupt and disa…

09b4bfa

…bleInterrupt This removes duplicate code and shrinks the resulting binary slightly.

matthijskooijman force-pushed the pending-interrupts branch from 1221070 to c82e9aa Compare October 27, 2014 15:52

matthijskooijman removed the Waiting for feedback More information must be provided before we can proceed label Oct 27, 2014

matthijskooijman referenced this pull request in xxxajk/Arduino-1 Nov 3, 2014

Faster Interrupts, no size change.

5d03b57

ffissore assigned cmaglie May 12, 2015

ffissore removed the Version: 1.5.x label Jun 5, 2015

matthijskooijman mentioned this pull request Sep 2, 2015

Faster isr #2408

Closed

NicoHood reviewed Sep 3, 2015
View reviewed changes

matthijskooijman mentioned this pull request Nov 28, 2019

attachInterrupt() should clear EIC INTFLAG or prior events will cause an interrupt arduino/ArduinoCore-samd#348

Open

matthijskooijman mentioned this pull request Jun 19, 2020

External interrupts flags are not cleared before enabling the interrupt arduino/ArduinoCore-samd#532

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow enabling/disabling external interrupts and clear pending interrupts on attach #2159

Allow enabling/disabling external interrupts and clear pending interrupts on attach #2159

matthijskooijman commented Jul 2, 2014 •

edited

Loading

matthijskooijman commented Jul 7, 2014

matthijskooijman commented Oct 27, 2014

matthijskooijman commented Oct 29, 2014

NicoHood commented Sep 2, 2015

matthijskooijman commented Sep 3, 2015

ArduinoBot commented Sep 3, 2015

NicoHood Sep 3, 2015

CLAassistant commented Apr 9, 2021 •

edited

Loading

Allow enabling/disabling external interrupts and clear pending interrupts on attach #2159

Are you sure you want to change the base?

Allow enabling/disabling external interrupts and clear pending interrupts on attach #2159

Conversation

matthijskooijman commented Jul 2, 2014 • edited Loading

matthijskooijman commented Jul 7, 2014

matthijskooijman commented Oct 27, 2014

matthijskooijman commented Oct 29, 2014

NicoHood commented Sep 2, 2015

matthijskooijman commented Sep 3, 2015

ArduinoBot commented Sep 3, 2015

NicoHood Sep 3, 2015

Choose a reason for hiding this comment

CLAassistant commented Apr 9, 2021 • edited Loading

matthijskooijman commented Jul 2, 2014 •

edited

Loading

CLAassistant commented Apr 9, 2021 •

edited

Loading