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

Author: matthijskooijman

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

@@ -51,70 +51,89 @@ void attachInterrupt(uint8_t interruptNum, void (*userFunc)(void), int mode) {
 	// even present on the 32U4 this is the only way to distinguish between them.
     case 0:
       EICRA = (EICRA & ~((1<<ISC00) | (1<<ISC01))) | (mode << ISC00);
+      EIFR |= (1 << INTF0);
       break;
     case 1:
       EICRA = (EICRA & ~((1<<ISC10) | (1<<ISC11))) | (mode << ISC10);
+      EIFR |= (1 << INTF1);
       break;
     case 2:
       EICRA = (EICRA & ~((1<<ISC20) | (1<<ISC21))) | (mode << ISC20);
+      EIFR |= (1 << INTF2);
       break;
     case 3:
       EICRA = (EICRA & ~((1<<ISC30) | (1<<ISC31))) | (mode << ISC30);
+      EIFR |= (1 << INTF3);
       break;
     case 4:
       EICRB = (EICRB & ~((1<<ISC60) | (1<<ISC61))) | (mode << ISC60);
+      EIFR |= (1 << INTF6);
       break;
 #elif defined(EICRA) && defined(EICRB)
     case 2:
       EICRA = (EICRA & ~((1 << ISC00) | (1 << ISC01))) | (mode << ISC00);
+      EIFR |= (1 << INTF0);
       break;
     case 3:
       EICRA = (EICRA & ~((1 << ISC10) | (1 << ISC11))) | (mode << ISC10);
+      EIFR |= (1 << INTF1);
       break;
     case 4:
       EICRA = (EICRA & ~((1 << ISC20) | (1 << ISC21))) | (mode << ISC20);
+      EIFR |= (1 << INTF2);
       break;
     case 5:
       EICRA = (EICRA & ~((1 << ISC30) | (1 << ISC31))) | (mode << ISC30);
+      EIFR |= (1 << INTF4);
       break;
     case 0:
       EICRB = (EICRB & ~((1 << ISC40) | (1 << ISC41))) | (mode << ISC40);
+      EIFR |= (1 << INTF4);
       break;
     case 1:
       EICRB = (EICRB & ~((1 << ISC50) | (1 << ISC51))) | (mode << ISC50);
+      EIFR |= (1 << INTF5);
       break;
     case 6:
       EICRB = (EICRB & ~((1 << ISC60) | (1 << ISC61))) | (mode << ISC60);
+      EIFR |= (1 << INTF6);
       break;
     case 7:
       EICRB = (EICRB & ~((1 << ISC70) | (1 << ISC71))) | (mode << ISC70);
+      EIFR |= (1 << INTF7);
       break;
 #else
     case 0:
-    #if defined(EICRA) && defined(ISC00)
+    #if defined(EICRA) && defined(ISC00) && defined(EIFR)
       EICRA = (EICRA & ~((1 << ISC00) | (1 << ISC01))) | (mode << ISC00);
-    #elif defined(MCUCR) && defined(ISC00)
+      EIFR |= (1 << INTF0);
+    #elif defined(MCUCR) && defined(ISC00) && defined(GIFR)
       MCUCR = (MCUCR & ~((1 << ISC00) | (1 << ISC01))) | (mode << ISC00);
+      GIFR |= (1 << INTF0);
     #else
       #error attachInterrupt not finished for this CPU (case 0)
     #endif
       break;
 
     case 1:
-    #if defined(EICRA) && defined(ISC10) && defined(ISC11)
+    #if defined(EICRA) && defined(ISC10) && defined(ISC11) && defined(EIFR)
       EICRA = (EICRA & ~((1 << ISC10) | (1 << ISC11))) | (mode << ISC10);
-    #elif defined(MCUCR) && defined(ISC10) && defined(ISC11)
+      EIFR |= (1 << INTF1);
+    #elif defined(MCUCR) && defined(ISC10) && defined(ISC11) && defined(GIFR)
       MCUCR = (MCUCR & ~((1 << ISC10) | (1 << ISC11))) | (mode << ISC10);
+      GIFR |= (1 << INTF1);
     #else
       #warning attachInterrupt may need some more work for this cpu (case 1)
     #endif
       break;
 
     case 2:
-    #if defined(EICRA) && defined(ISC20) && defined(ISC21)
+    #if defined(EICRA) && defined(ISC20) && defined(ISC21) && defined(EIFR)
       EICRA = (EICRA & ~((1 << ISC20) | (1 << ISC21))) | (mode << ISC20);
-    #elif defined(MCUCR) && defined(ISC20) && defined(ISC21)
+      EIFR |= (1 << INTF2);
+    #elif defined(MCUCR) && defined(ISC20) && defined(ISC21) && defined(GIFR)
       MCUCR = (MCUCR & ~((1 << ISC20) | (1 << ISC21))) | (mode << ISC20);
+      GIFR |= (1 << INTF2);
     #endif
       break;
 #endif

hardware/arduino/sam/cores/arduino/WInterrupts.c

@@ -78,16 +78,6 @@ void attachInterrupt(uint32_t pin, void (*callback)(void), uint32_t mode)
 	for (t = mask; t>1; t>>=1, pos++)
 		;
 
-	// Set callback function
-	if (pio == PIOA)
-		callbacksPioA[pos] = callback;
-	if (pio == PIOB)
-		callbacksPioB[pos] = callback;
-	if (pio == PIOC)
-		callbacksPioC[pos] = callback;
-	if (pio == PIOD)
-		callbacksPioD[pos] = callback;
-
 	// Configure the interrupt mode
 	if (mode == CHANGE) {
 		// Disable additional interrupt mode (detects both rising and falling edges)
@@ -115,6 +105,44 @@ void attachInterrupt(uint32_t pin, void (*callback)(void), uint32_t mode)
 		}
 	}
 
+	// Set callback function and call handler to clear existing
+	// flags. 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, we call the handler here which reads the
+	// status register and calls handlers for any pending
+	// interrupts (the interrupt we are attaching here isn't enabled
+	// yet and should be skipped). 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.
+	if (pio == PIOA) {
+		callbacksPioA[pos] = callback;
+		PIOA_Handler();
+	}
+	if (pio == PIOB) {
+		callbacksPioB[pos] = callback;
+		PIOD_Handler();
+	}
+	if (pio == PIOC) {
+		callbacksPioC[pos] = callback;
+		PIOD_Handler();
+	}
+	if (pio == PIOD) {
+		callbacksPioD[pos] = callback;
+		PIOD_Handler();
+	}
+
 	enableInterrupt(pin);
 }