New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

attachInterrupt(...) should ignore pending interrupts (clear interrupt flag) [imported] #510

Open
cmaglie opened this Issue Nov 15, 2012 · 4 comments

Comments

8 participants
@cmaglie
Copy link
Member

cmaglie commented Nov 15, 2012

This is Issue 510 moved from a Google Code project.
Added by 2011-03-23T05:33:55.000Z by kylehard...@gmail.com.
Please review that bug for more context and additional comments, but update this bug.

Original labels: Type-Defect, Priority-Medium, Component-Core

Original description

What steps will reproduce the problem?
Use this code:

/*

Attach your interrupt signaling button to DPIN2
Attach you attachInterrupt button to DPIN13

Fire up the serial monitor:

The first number shows if the interrupt is currently attached.
The second number shows if the interrupt is in a signaled state, i.e. if it is prepared to fire off the ISR.
  Note that in normal operation, this interrupt ready flag is unset immediately upon call of the ISR.
The third number shows a count of the calls to isrSignal, within which it is incremented by 1.

Follow these steps:
Trigger your interrupt signaling button once. 
You will see that the interrupt is detached by (EIMSK & 0x1) being 0.
(EIFR & 1) will be 0, as the trigger flag is not seen at this point in the code: The interrupt triggers and the flag is cleared.
The count will be 1.

Trigger the interrupt signal button again. 
EIMSK & 1 will not change: The interrupt was already not attached.
EIFR & 1 will now be 1, because the interrupt flag signals regardless of whether an interrupt is attached.
The count will still be 1, as the ISR was not called due to the interrupt not being attached.

Now press your attachInterrupt button to cause the interrupt to be reattached.
EIMSK & 1 will be 1. The ISR is reattached.
EIFR & 1 will be 0: The flag has been cleared.
The count will be 2. This is wrong, it should be 1 as the ISR should only have been called 
  once: The first time the interrupt button was pressed. The second time the button was pressed,
  the interrupt was not attached, so the user does not expect intervening triggers of the interrupt
  to cause an immediate call to the ISR upon reattachment.

*/
# define INTERRUPT_SIGNAL_PIN 2
# define ATTACH_INTERRUPT_PIN 13

volatile boolean signaled = false;
volatile long signalCounter = 0;

void setup () {
  Serial.begin(115200);
  pinMode(INTERRUPT_SIGNAL_PIN,INPUT);
  pinMode(ATTACH_INTERRUPT_PIN,INPUT);
  attachInterrupt(0,isrSignal,RISING);
}

void loop () {
  if (digitalRead(ATTACH_INTERRUPT_PIN) == HIGH) {
    attachInterrupt(0,isrSignal,RISING);
  }
  Serial.print( EIMSK, BIN );
  Serial.print(' ');
  Serial.print( EIFR, BIN );
  Serial.print(' ');
  Serial.println( signalCounter );
}

void isrSignal () {
  detachInterrupt(0);
  signalCounter++;
  signaled = true;
}

What is the expected output? What do you see instead?
Explained in the comment in the code.

What version of the Arduino software are you using? On what operating
system? Which Arduino board are you using?
This issue is seen in version 0022 of the IDE on Windows XP using a Duemilanove. However, this issue has nothing to do with any of this: See below.

Please provide any additional information below.

The solution to this issue is to read the datasheet for certain Atmegas, for example the 1284P, which on page 69 says:

When changing the ISCn bit, an interrupt can occur. Therefore, it is recommended to first disable INTn by clearing its Interrupt Enable bit in the EIMSK Register. Then, the ISCn bit can be changed. Finally, the INTn interrupt flag should be cleared by writing a logical one to its Interrupt Flag bit (INTFn) in the EIFR Register before the interrupt is re-enabled.

This is in reference to attaching an interrupt and is in the EICRA subsection of the External Interrupts section.

The short version: The interrupt flag should be unset when an interrupt is attached, otherwise the ISR can be triggered immediately upon attachment.

The fix involves a modification to WInterrupts.c. Currently, there is a switch() full of #ifs, each condition containing a variation of the following two lines of code:

EICRA = (EICRA & ~((1 << ISC00) | (1 << ISC01))) | (mode << ISC00);
EIMSK |= (1 << INT0);

The first line sets EICRA, which determines under which interrupt triggers conditions an ISR will be triggered (RISING, FALLING, etc.). The second line activates the correct bit in EIMSK high, which enables the external interrupt pin.

The fix is the addition of two more lines of code:

EIMSK &= ~(1 << INT0); //Added
EICRA = (EICRA & ~((1 << ISC00) | (1 << ISC01))) | (mode << ISC00);
EIFR |= (1 << INTF0); //Added
EIMSK |= (1 << INT0);

The first added line makes sure that the external interrupt pin is disabled while changing the value of EICRA, as recommended in the datasheet. The second added line clears the interrupt triggered flag. It looks like the flag is being set by use of |=, but this is the correct method. See page 69: "Alternatively, the flag can be cleared by writing a logical one to it."

This fix is tested and fully working.

The first added line of code, which disables the external interrupt pin, is not required for this specific fix. However, it is recommended in the datasheet and seems to be able to help in certain fringe conditions.

@isriram

This comment has been minimized.

Copy link

isriram commented Oct 16, 2013

Yep. This one bit me as well. I had to clear the interrupt using the EIFR |= 0x01 immediately after I attach the interrupt or else there were false triggers. I was using FALLING condition.

@ffissore ffissore added the New label Feb 27, 2014

matthijskooijman added a commit to Pinoccio/library-pinoccio that referenced this issue Jul 1, 2014

Support waking up on pin changes
Using the power.wakeup.pin("pin_name", [enable]) command, you can now
enable wakeup from sleep on changes to specific pins. The pin must be
configured to input (or input_pullup) before sleeping to make this work.

SleepHandler enables the corresponding interrupt handler for waking up
the scout, but nothing actually happens in the interrupt. After wakeup,
if the pin change holds on long enough, the normal event handlers are
triggered and can be used to react to events.

Not all pins are available, only the pins that have external interrupt
or pin change interrupts (which aren't a lot on 256rfr2 unfortunately).
Supported pins are: D2, D4, D5, D7, BATT_ALERT, SS, MOSI, MISO, SCK,
TX1, RX1, SDA, SCL.

Of these, pins D4, D5, D7 and BATT_ALERT only support waking up on a low
level, not on a change. This means that wakeup is enabled for one of
these pins and it is low when starting the sleep, the scout will
immediately wake up again, effectively preventing it from sleeping.

The pins shown above are the ones supported by SleepHandler, but the
power.wakeup.pins currently only allows passing D2, D4, D5 and D7, the
others are refused. This will probably change in the future.

Furthermore, due to a bug [1] in the Arduino core, for the SCL, SDA, RX1
and TX1 pins when a pin change happens while not sleeping, the next
sleep will be immediately interrupted because the interrupt flag remains
set and immediately triggers the interrupt.

[1]: arduino/Arduino#510

Note that not a lot of effort is done to prevent race conditions: For
example, if the pin change occurs just before sleeping, then the
corresponding pin change handler might not be run, or only after
sleeping. If these race conditions are problematic, short response times
are needed or the pulses are very short, a custom sketch might still be
needed.

matthijskooijman added a commit to Pinoccio/library-pinoccio that referenced this issue Jul 31, 2014

Support waking up on pin changes
Using the power.wakeup.pin("pin_name", [enable]) command, you can now
enable wakeup from sleep on changes to specific pins. The pin must be
configured to input (or input_pullup) before sleeping to make this work.

SleepHandler enables the corresponding interrupt handler for waking up
the scout, but nothing actually happens in the interrupt. After wakeup,
if the pin change holds on long enough, the normal event handlers are
triggered and can be used to react to events.

Not all pins are available, only the pins that have external interrupt
or pin change interrupts (which aren't a lot on 256rfr2 unfortunately).
Supported pins are: D2, D4, D5, D7, BATT_ALERT, SS, MOSI, MISO, SCK,
TX1, RX1, SDA, SCL.

Of these, pins D4, D5, D7 and BATT_ALERT only support waking up on a low
level, not on a change. This means that wakeup is enabled for one of
these pins and it is low when starting the sleep, the scout will
immediately wake up again, effectively preventing it from sleeping.

The pins shown above are the ones supported by SleepHandler, but the
power.wakeup.pins currently only allows passing D2, D4, D5 and D7, the
others are refused. This will probably change in the future.

Furthermore, due to a bug [1] in the Arduino core, for the SCL, SDA, RX1
and TX1 pins when a pin change happens while not sleeping, the next
sleep will be immediately interrupted because the interrupt flag remains
set and immediately triggers the interrupt.

[1]: arduino/Arduino#510

Note that not a lot of effort is done to prevent race conditions: For
example, if the pin change occurs just before sleeping, then the
corresponding pin change handler might not be run, or only after
sleeping. If these race conditions are problematic, short response times
are needed or the pulses are very short, a custom sketch might still be
needed.

@matthijskooijman matthijskooijman removed the New label Sep 11, 2014

matthijskooijman added a commit to matthijskooijman/Arduino that referenced this issue Oct 27, 2014

Let attachInterrupt clear pending interrupts
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 arduino#510.
@ortegafernando

This comment has been minimized.

Copy link

ortegafernando commented Dec 11, 2016

hi,
Any plan to move this to master brand ?
regards

@PaulStoffregen

This comment has been minimized.

Copy link
Collaborator

PaulStoffregen commented Dec 11, 2016

Adafruit CC3300 and perhaps other libraries depend on this behavior. Changing it will cause programs to break.

@morefigs

This comment has been minimized.

Copy link

morefigs commented May 29, 2017

@cmaglie wouldn't EIFR |= (1 << INTF0); clear all interrupt flags? Or is that the intended consequence?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment