External interrupts flags are not cleared before enabling the interrupt

[Sulymarco]

Hi everybody,

Bug description:

If we generate a falling edge on an interrupt source pin when the interrupt is disabled, when we enable the interrupt the interrupt service is triggered immediately.
(This has been tested with falling edge mode, but it should happen with every external interrupt mode)

Setup description to reproduce the bug:

Arduino Zero with two N.O. momentary switches connected on pins D2 and D3 to GND.

Sketch used to show up the bug:

//------ External interrupt test for Arduino Zero ------------------------------------------------------------

// Bug description: 
// if we generate a falling edge on an interrupt source pin when the interrupt is disabled,
// when we enable the interrupt the interrupt service is triggered immediately. 

// Bug analysis:
// the external interrupt flag is not cleared before enabling the interrupt 


#define INT_SOURCE  2                                    // interrupt source
#define INT_ENABLE  3                                    // interrupt enable
#define LED  13

void setup() 
{
  pinMode(INT_SOURCE, INPUT_PULLUP);                
  pinMode(INT_ENABLE, INPUT_PULLUP);
  pinMode(LED, OUTPUT);
                                                         // this is to allow the bug to show up
                                                         // enable the interrupt
                                                         // disable the interrupt     
  attachInterrupt(digitalPinToInterrupt(INT_SOURCE), IntService, FALLING);    
  detachInterrupt(digitalPinToInterrupt(INT_SOURCE));               
}                                       


void loop() 
{
  if(!digitalRead (INT_ENABLE))                          // if the enable switch is pressed
  {                                                      // enable the interrupt 
      attachInterrupt(digitalPinToInterrupt(INT_SOURCE), IntService, FALLING);
                                                         //
      while(1){}                                         // and wait for ever
  }
}


void IntService()                                        // when an interrupt is detected
{                                                        // light on the led
  digitalWrite (LED, HIGH);                              //
}                                                        //

Procedure description:

1. Reset the board
2. Pulse the the switch connected to D2 to genetate a falling edge on the interrupt pin (note that now the interrupt is disabled).
3. Pulse the switch connected to D3 to enable the interrupt.
4. The led lights on immediately indicating that the interrupt has been detected.

Bug analysis:

the external interrupt flag in register EIC->INTFLAG is not cleared before enabling the interrupt.

Any comment is welcome.

Marco

[matthijskooijman]

A related issue for the AVR core is here: arduino/ArduinoCore-avr#244 (with a very old PR to fix it: arduino/Arduino#2159). Note that a complication is that some code actually relies on this behaviour when temporarily disabling an interrupt (with no alternative because of the lack of explicit disable/enableInterrupt functions), so this is not entirely as trivial as it seems (see also the linked issue and PR for more discussion).

[Sulymarco]

First of all thanks for your comment.

In my opinion this is a bug and not a feature.
It is a standard procedure to clear the flag before enabling an interrupt.
Writing a software that rely on a bug, as some libraries do, is a bigger bug itself: bugs must be killed and not be allowed to spread.

It looks something like the wrong position of one of the connector on the Ardino boards: it was a mistake, of course, but when the Arduino team discovered it, the first batch of 100 boards had already been made and so they didn't have the guts to fix it.
The consequence of this are millions of people struggling to make a prototype shield with a standard 2.54 pitch breadboard.
That is the way it goes! :-)

Marco

[matthijskooijman]

In my opinion this is a bug and not a feature.

Yeah it is, it's just that fixing the bug will break some existing code, so this must be done carefully. Especially since those libraries that rely on this bug have no other way to implement this currently (they cannot be fixed right now), so maybe some 2-step process (introducing enable/disableInterrupt, then later fix attachInterrupt) would be appropriate.

[Sulymarco]

Yeah it is, it's just that fixing the bug will break some existing code, so this must be done carefully

Ok I understand your worries.

What about adding a default parameter to the attachinterrupt function?
I mean something like this:

void attachInterrupt(uint32_t pin, voidFuncPtr callback, uint32_t mode, bool clear = false);

This will mantain full compatibility with the current implementation if we call the function with just three parameters, but, if we call it setting the last parameter to true, the interrupt flag will be cleared before enabling the interrupt.

[matthijskooijman]

This will mantain full compatibility with the current implementation if we call the function with just three parameters, but, if we call it setting the last parameter to true, the interrupt flag will be cleared before enabling the interrupt.

Hm, I think this could maybe work. However:

• attachInterrupt might be a C function (as opposed to C++) now, which does not allow default parameters and parameter overloading.
• Adding extra optional parameters seems nice, but really scales badly (though in practice, C/C++ has nothing to really fix this, like named parameters or something like that).
• boolean parameters are not really readable, it's hard to see what they mean in a function call.

Maybe all of these could be solved by instead adding a attachAndClearInterrupt(). The docs should then probably recommend that people use attachAndClearInterrupt() instead of attachInterrupt() by default, since clearing any previous interrupts is almost always what you want.

A further extension (that would also work with your optional argument approach, btw) could be to add attachWithoutClearInterrupt() as an alias of the current attachInterrupt(). This allows users that rely on one behavior or the other, to call the explicit function. Users that don't care or don't know call attachInterrupt() and then after a few releases, we can switch the default attachInterrupt() to be an alias of attachAndClearInterrupt().

I haven't thought this true completely, but at first glance, this sounds like a reasonable approach.

Note that this change would need to be implemented across all cores to remain portable, so this might be something to be discussed in the ArduinoCore-API repository (which is slowly becoming the home of portable API changes). Also note that I think there have been some changes to attachInterrupt related to passing arguments tot the ISR there, which might again interfere with this (and/or result in needing not 2 but 4 different functions or something...).

[Sulymarco]

You are right, I missed that attachInterrupt is a C function :-(