Skip to content

Commit

Permalink
Let attachInterrupt clear pending interrupts
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@matthijskooijman
matthijskooijman committed Oct 27, 2014
1 parent 09b4bfa commit c82e9aa
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 16 deletions.
31 changes: 25 additions & 6 deletions hardware/arduino/avr/cores/arduino/WInterrupts.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
48 changes: 38 additions & 10 deletions hardware/arduino/sam/cores/arduino/WInterrupts.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}

Expand Down

0 comments on commit c82e9aa

Please sign in to comment.