Clearing the interrupt flag on attachInterrupt

[Matthijs Kooijman]

Hey folks,

the current attachInterrupt code attaches the interrupt without clearing
the interrupt flag. Since the flag can be set even when the interrupt is
not enabled (not attached), this can lead to the interrupt handler
triggering right away, even when the condition specified in the
attachInterrupt call has not occured (certainly not after the
attachInterrupt call, perhaps never at all).

This was reported in https://github.com/arduino/Arduino/issues/510

I consider this a bug - you would expect that the handler passed to
attachInterrupt only triggers when the specified condition
(low/rising/falling/any edge) occurs _after_ the attachInterrupt call.

I want to submit a pullrequest to fix this. Since this changes behaviour
- can anyone think of a case where the old behaviour makes sense and
and/or a reason to either not change this, or make clearing the flag
optional?

Gr.

Matthijs

[Rob Tillaart]

good proposal imho. 

[Paul Stoffregen]

The CC3000 library depends on this behavior. Other libraries might too.

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.

When/if (or ideally before) this bug is fixed, the API should really
have at least a way to temporarily disable and reenable an attached
interrupt.

[Matthijs Kooijman]

Hey Paul,

> 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.

Thanks, that sounds like a reasonable usecase that is currently made
possible by API abuse :-)

> When/if (or ideally before) this bug is fixed, the API should really
> have at least a way to temporarily disable and reenable an attached
> interrupt.

Something like a disableInterrupt / enableInterrupt should work for
this, right? I think that enableInterrupt should have a clear_pending
flag to allow both behaviours (unlike attachInterrupt which IMHO should
always clear the pending interrupts - it doesn't make sense to get
interrupts from before you set up the interrupt handler and settings).

I did a quick implementation of this on AVR:

https://github.com/arduino/Arduino/pull/2159

Still completely untested. I have a CC3000 breakout lying here that I
was still planning to play with, so I'll try to see if I can convert
their code to use the new API.

Gr.

Matthijs

[Paul Stoffregen]

On 07/02/2014 01:30 PM, Matthijs Kooijman wrote:
> Something like a disableInterrupt / enableInterrupt should work for
> this, right? I think that enableInterrupt should have a clear_pending
> flag to allow both behaviours (unlike attachInterrupt which IMHO
> should always clear the pending interrupts - it doesn't make sense to
> get interrupts from before you set up the interrupt handler and settings).

I would prefer to see enableInterrupt() take only a single parameter and
never clear pending interrupt status. I see you used enableInterrupt as
the final action in attachInterrupt, which was probably the motivation
for having the 2nd parameter?

If the 2 arg version is used as a public API, can the clearPending
default to false? The expected usage is temporarily disabling an
interrupt without detaching it, while working with shared data
structures that must not be accessed by the interrupt function, and then
enable the interrupt again when you're done. You'd usually (perhaps
always?) want a pending interrupt that occurred during the disable
period to be serviced when you reenable.

Might also be good to think about what happens if someone uses
enableInterrupt without first calling attachInterrupt...

> I did a quick implementation of this on AVR:
>
> https://github.com/arduino/Arduino/pull/2159
>
> Still completely untested. I have a CC3000 breakout lying here that I
> was still planning to play with, so I'll try to see if I can convert
> their code to use the new API.

I'm pretty sure CC3000 could easily win a prize for the most horrible
code of all Arduino libraries.

Luckily, CC3000 does seem to have been designed for basically this
4-function API. I'm pretty sure SpiPauseSpi() and SpiResumeSpi() are
the 2 that would use disableInterrupt and enableInterrupt.

[Matthijs Kooijman]

Hey Paul,

> I would prefer to see enableInterrupt() take only a single parameter
> and never clear pending interrupt status. I see you used
> enableInterrupt as the final action in attachInterrupt, which was
> probably the motivation for having the 2nd parameter?

Sortof, but I can just as easily move the clearing of the flag into
attachInterrupt (since it needs to happen _before_ enabling the
interrupt).

> If the 2 arg version is used as a public API, can the clearPending
> default to false?

Of course, not sure why I wrote true there. If we keep this version,
false as the default makes a lot more sense.

> The expected usage is temporarily disabling an
> interrupt without detaching it, while working with shared data
> structures that must not be accessed by the interrupt function, and
> then enable the interrupt again when you're done. You'd usually
> (perhaps always?) want a pending interrupt that occurred during the
> disable period to be serviced when you reenable.

That makes sense. Also, if you really want to clear pending interrupts,
you can always detach and reattach the interupt instead of
disabling/enabling it, I guess?

> Might also be good to think about what happens if someone uses
> enableInterrupt without first calling attachInterrupt...

Right now, it just enables the interrupt, but since the handler is still
NULL, it'll just be a no-op.

Gr.

Matthijs

[Matthijs Kooijman]

Hey folks,

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

https://github.com/arduino/Arduino/pull/2159

Gr.

Matthijs

[Matthijs Kooijman]

Hey folks,

I just tried to implement #2159 for SAM as well, but ran into some
problems. (https://github.com/arduino/Arduino/pull/2159)

On SAM, clearing interrupt flags happens by reading the status register,
which always clears _all_ interrupts for the same port. There is no way
to clear individual flags.

A possible approach would be to, when attaching an interrupt and its
flag must be cleared, simply pretend that an interrupt has fired - this
will read the status register, clearing it, and then handle any pending
interrupts. This works fine when interrupts are enabled, but when
attachInterrupt is called with interrupts disabled, it would be weird to
call the interrupt callbacks (a sketch might not expect this).

OTOH, it seems that calling attachInterrupt with interrupts disabled is
pretty much a corner case where it is ok to be a bit weird (The only
alternative I can see is to not clear the flag, which is even more
unexpected). Perhaps someone with more experience with the sam core
knows of an alternative solution?

Gr.

Matthijs

[Paul Stoffregen]

On 10/24/2014 10:20 AM, Matthijs Kooijman wrote:
> Hey folks,
>
> I just tried to implement #2159 for SAM as well, but ran into some
> problems. (https://github.com/arduino/Arduino/pull/2159)
>
> On SAM, clearing interrupt flags happens by reading the status register,
> which always clears _all_ interrupts for the same port. There is no way
> to clear individual flags.

Can this be accomplished by using the ICER# and ISER# registers within
the NVIC inside the ARM core, rather than the registers in Atmel's
peripheral?

Doing it inside the NVIC also avoids thorny bus bridge timing issues.

[Matthijs Kooijman]

Hey Paul,

> Can this be accomplished by using the ICER# and ISER# registers
> within the NVIC inside the ARM core, rather than the registers in
> Atmel's peripheral?

Well, AFAICS this only clears the interrupt flag for the entire port,
while we need to clear the interrupt flag just for the pin that is being
attachInterrupt'd. I think that there is a status reg (PIO_ISR) that
keeps a bit for the interrupt status for each pin in a port. This is
AND'd with the pin interrupt enable mask (PIO_IMR). All these pins are
then or'ed together and offered to the NVIC as a single interrupt.

What is not entirely clear to me is wether these ISR flags get set even
when the interrupt for a given pin is disabled in IMR. If I read the
diagram in section 32.5 (Functional description) of the SAM3X/A
datasheet right, the mask is applied to the value of the status
register, which suggests the bits in the status register are set even
when interrupts (for the given pin) are disabled in the mask register,
but I can't find any details about this in the datasheet.

If the status bits do _not_ get set while the pin is masked out in the
IMR register, then the problem I described does not exist, but also code
that relies on saving remembering interrupts while the IRQ is disabled
will not work either. Anyone with a due that can check wether the ISR
bits get set for pins that don't have any handlers registered?


A related observation I noticed in the code is that detachInterrupt on
sam currently disables the pin in the IMR, but leaves the callback
registered in the callbacksPioX arrays. Combined with the fact that the
actual interrupt routine does not look at IMR, just at ISR and the
callbacks arrays, I suspect that this might cause interrupts to be
triggered after they are detached, as a sid effect of another interrupt
being triggered on the same port.

To verify this, I created a small sketch:
https://gist.github.com/matthijskooijman/478727b188d41130ea73
This was compiletested only, I don't have a due. To test this, trigger
interrups on pin 25 and 26 by shorting it to ground. If my suspicion is
correct, touching pin 25 shouldn't show anything, but touching pin 26
afterwards will show both INT25 and INT26, even though 26 is already
detached.

If this is indeed the case, the proper and easy fix is to just reset the
callback to NULL in detachInterrupt.

Gr.

Matthijs

[Matthijs Kooijman]

Hey folks,

> A related observation I noticed in the code is that detachInterrupt on
> sam currently disables the pin in the IMR, but leaves the callback
> registered in the callbacksPioX arrays. Combined with the fact that the
> actual interrupt routine does not look at IMR, just at ISR and the
> callbacks arrays, I suspect that this might cause interrupts to be
> triggered after they are detached, as a sid effect of another interrupt
> being triggered on the same port.
>
> To verify this, I created a small sketch:
> https://gist.github.com/matthijskooijman/478727b188d41130ea73
> This was compiletested only, I don't have a due. To test this, trigger
> interrups on pin 25 and 26 by shorting it to ground. If my suspicion is
> correct, touching pin 25 shouldn't show anything, but touching pin 26
> afterwards will show both INT25 and INT26, even though 26 is already
> detached.

I found someone with a Due (third-party clone, though) to confirm this.
Should probably be retested on an official Due, but I'm pretty confident
my analysis was right. I added a fix for this problem as part of my PR:

https://github.com/arduino/Arduino/pull/2159


That PR was also updated to allow enable/disable for SAM and to let
attach clear the interrupt flags (with the caveat I mentioned before -
calling attachInterrupt with interrupts globally disabled can cause
unrelated pin interrupts to be triggered as if interrupts were globally
enabled). This is probably the best way to fix this.

Gr.

Matthijs