From ef95809f0c754669efa7c5cd56209cc5a17e8930 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 2 Jul 2014 22:12:06 +0200 Subject: [PATCH 1/5] Fix indentation and trailing whitespace in WInterrupts.c --- .../arduino/avr/cores/arduino/WInterrupts.c | 68 +++++++++---------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/WInterrupts.c b/hardware/arduino/avr/cores/arduino/WInterrupts.c index d3fbf100e3e..09fb241c363 100644 --- a/hardware/arduino/avr/cores/arduino/WInterrupts.c +++ b/hardware/arduino/avr/cores/arduino/WInterrupts.c @@ -19,7 +19,7 @@ Public License along with this library; if not, write to the Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - + Modified 24 November 2006 by David A. Mellis Modified 1 August 2010 by Mark Sproul */ @@ -38,39 +38,39 @@ static volatile voidFuncPtr intFunc[EXTERNAL_NUM_INTERRUPTS]; void attachInterrupt(uint8_t interruptNum, void (*userFunc)(void), int mode) { if(interruptNum < EXTERNAL_NUM_INTERRUPTS) { intFunc[interruptNum] = userFunc; - + // Configure the interrupt mode (trigger on low input, any change, rising // edge, or falling edge). The mode constants were chosen to correspond // to the configuration bits in the hardware register, so we simply shift // the mode into place. - + // Enable the interrupt. - + switch (interruptNum) { #if defined(__AVR_ATmega32U4__) // I hate doing this, but the register assignment differs between the 1280/2560 - // and the 32U4. Since avrlib defines registers PCMSK1 and PCMSK2 that aren't + // and the 32U4. Since avrlib defines registers PCMSK1 and PCMSK2 that aren't // even present on the 32U4 this is the only way to distinguish between them. case 0: - EICRA = (EICRA & ~((1< Date: Mon, 27 Oct 2014 15:26:45 +0100 Subject: [PATCH 2/5] [sam] Do not run disabled interrupt handlers On SAM: - all pins in a port share a single interrupt pin - the interrupt status flag for a pin is set even when it is disabled - The actual ISR does not look at the interrupt enable flags, it just runs the callback for every pin whos interrupt status flag is set. This meant that if: - an interrupt was attached and later detached, - then the interrupt condition occured, setting the flag and - then another (still attached) interrupt on a different pin in the same port occured, then the detached interrupt callback would also be called. By masking off the Interrupts Status Register using the Interrupt Mask Register, only actually enabled interrupt handlers are called. The incorrect behaviour has been verified using the following test sketch: /* To test: - Upload this sketch to the Arduino Due - Open the serial monitor at 115200 - Connect a jumper wire to ground and touch the other end to pins 25 and 26, alternating between them */ void int25() { Serial.println("INT25"); } void int26() { Serial.println("INT26"); } void setup() { Serial.begin(115200); pinMode(25, INPUT_PULLUP); pinMode(26, INPUT_PULLUP); attachInterrupt(25, int25, CHANGE); attachInterrupt(26, int26, CHANGE); detachInterrupt(26); } void loop() { } --- hardware/arduino/sam/cores/arduino/WInterrupts.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hardware/arduino/sam/cores/arduino/WInterrupts.c b/hardware/arduino/sam/cores/arduino/WInterrupts.c index 87b83e4572b..b718680199f 100644 --- a/hardware/arduino/sam/cores/arduino/WInterrupts.c +++ b/hardware/arduino/sam/cores/arduino/WInterrupts.c @@ -134,7 +134,7 @@ extern "C" { #endif void PIOA_Handler(void) { - uint32_t isr = PIOA->PIO_ISR; + uint32_t isr = PIOA->PIO_ISR & PIOA->PIO_IMR; uint32_t i; for (i=0; i<32; i++, isr>>=1) { if ((isr & 0x1) == 0) @@ -145,7 +145,7 @@ void PIOA_Handler(void) { } void PIOB_Handler(void) { - uint32_t isr = PIOB->PIO_ISR; + uint32_t isr = PIOB->PIO_ISR & PIOB->PIO_IMR; uint32_t i; for (i=0; i<32; i++, isr>>=1) { if ((isr & 0x1) == 0) @@ -156,7 +156,7 @@ void PIOB_Handler(void) { } void PIOC_Handler(void) { - uint32_t isr = PIOC->PIO_ISR; + uint32_t isr = PIOC->PIO_ISR & PIOC->PIO_IMR; uint32_t i; for (i=0; i<32; i++, isr>>=1) { if ((isr & 0x1) == 0) @@ -167,7 +167,7 @@ void PIOC_Handler(void) { } void PIOD_Handler(void) { - uint32_t isr = PIOD->PIO_ISR; + uint32_t isr = PIOD->PIO_ISR & PIOD->PIO_IMR; uint32_t i; for (i=0; i<32; i++, isr>>=1) { if ((isr & 0x1) == 0) From 938a2667dbc6adfdbbc406bb85de9248988c3039 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 2 Jul 2014 22:17:09 +0200 Subject: [PATCH 3/5] Add enableInterrupt and disableInterrupt These allow temporarily enabling and disabling a single interrupt. This is currently pretty much the same as detaching and re-attaching the interrupt, but this will change when attachInterrupt is changed to clear any pending interrupts next. This commit introduces duplicate code, between attach/detach and enable/disable, which will be removed next (doing this in one commit makes the diff harder to review). --- hardware/arduino/avr/cores/arduino/Arduino.h | 2 + .../arduino/avr/cores/arduino/WInterrupts.c | 164 ++++++++++++++++++ .../arduino/sam/cores/arduino/WInterrupts.c | 19 ++ .../arduino/sam/cores/arduino/WInterrupts.h | 3 + 4 files changed, 188 insertions(+) diff --git a/hardware/arduino/avr/cores/arduino/Arduino.h b/hardware/arduino/avr/cores/arduino/Arduino.h index ac775f13c5c..776b84e3161 100644 --- a/hardware/arduino/avr/cores/arduino/Arduino.h +++ b/hardware/arduino/avr/cores/arduino/Arduino.h @@ -140,6 +140,8 @@ uint8_t shiftIn(uint8_t dataPin, uint8_t clockPin, uint8_t bitOrder); void attachInterrupt(uint8_t, void (*)(void), int mode); void detachInterrupt(uint8_t); +void enableInterrupt(uint8_t interruptNum); +void disableInterrupt(uint8_t interruptNum); void setup(void); void loop(void); diff --git a/hardware/arduino/avr/cores/arduino/WInterrupts.c b/hardware/arduino/avr/cores/arduino/WInterrupts.c index 09fb241c363..9b4672a7aaa 100644 --- a/hardware/arduino/avr/cores/arduino/WInterrupts.c +++ b/hardware/arduino/avr/cores/arduino/WInterrupts.c @@ -152,6 +152,94 @@ void attachInterrupt(uint8_t interruptNum, void (*userFunc)(void), int mode) { } } +void enableInterrupt(uint8_t interruptNum) { + if(interruptNum < EXTERNAL_NUM_INTERRUPTS) { + + // Enable the interrupt. + switch (interruptNum) { +#if defined(__AVR_ATmega32U4__) + // I hate doing this, but the register assignment differs between the 1280/2560 + // and the 32U4. Since avrlib defines registers PCMSK1 and PCMSK2 that aren't + // even present on the 32U4 this is the only way to distinguish between them. + case 0: + EIMSK |= (1<PIO_IER = mask; } +void enableInterrupt(uint32_t pin) +{ + Pio *pio = g_APinDescription[pin].pPort; + uint32_t mask = g_APinDescription[pin].ulPin; + + // Enable interrupt + pio->PIO_IER = mask; +} + void detachInterrupt(uint32_t pin) { // Retrieve pin information @@ -129,6 +138,16 @@ void detachInterrupt(uint32_t pin) pio->PIO_IDR = mask; } +void disableInterrupt(uint32_t pin) +{ + // Retrieve pin information + Pio *pio = g_APinDescription[pin].pPort; + uint32_t mask = g_APinDescription[pin].ulPin; + + // Disable interrupt + pio->PIO_IDR = mask; +} + #ifdef __cplusplus extern "C" { #endif diff --git a/hardware/arduino/sam/cores/arduino/WInterrupts.h b/hardware/arduino/sam/cores/arduino/WInterrupts.h index bb698cdf384..1ea1679ccc1 100644 --- a/hardware/arduino/sam/cores/arduino/WInterrupts.h +++ b/hardware/arduino/sam/cores/arduino/WInterrupts.h @@ -29,6 +29,9 @@ void attachInterrupt(uint32_t pin, void (*callback)(void), uint32_t mode); void detachInterrupt(uint32_t pin); +void enableInterrupt(uint32_t pin); +void disableInterrupt(uint32_t pin); + #ifdef __cplusplus } #endif From 09b4bfa2ed0f7626a20439cdd49384da4d71f723 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Mon, 27 Oct 2014 16:50:18 +0100 Subject: [PATCH 4/5] Let attachInterrupt and detachInterrupt call enableInterrupt and disableInterrupt This removes duplicate code and shrinks the resulting binary slightly. --- .../arduino/avr/cores/arduino/WInterrupts.c | 120 ++---------------- .../arduino/sam/cores/arduino/WInterrupts.c | 10 +- 2 files changed, 13 insertions(+), 117 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/WInterrupts.c b/hardware/arduino/avr/cores/arduino/WInterrupts.c index 9b4672a7aaa..fa2a71aecc3 100644 --- a/hardware/arduino/avr/cores/arduino/WInterrupts.c +++ b/hardware/arduino/avr/cores/arduino/WInterrupts.c @@ -44,8 +44,6 @@ void attachInterrupt(uint8_t interruptNum, void (*userFunc)(void), int mode) { // to the configuration bits in the hardware register, so we simply shift // the mode into place. - // Enable the interrupt. - switch (interruptNum) { #if defined(__AVR_ATmega32U4__) // I hate doing this, but the register assignment differs between the 1280/2560 @@ -53,102 +51,77 @@ 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<PIO_IER = mask; + enableInterrupt(pin); } void enableInterrupt(uint32_t pin) @@ -130,12 +129,7 @@ void enableInterrupt(uint32_t pin) void detachInterrupt(uint32_t pin) { - // Retrieve pin information - Pio *pio = g_APinDescription[pin].pPort; - uint32_t mask = g_APinDescription[pin].ulPin; - - // Disable interrupt - pio->PIO_IDR = mask; + disableInterrupt(pin); } void disableInterrupt(uint32_t pin) From c82e9aadd358127e4f272dc99bc2a6a4d70963c6 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 2 Jul 2014 22:23:33 +0200 Subject: [PATCH 5/5] 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. --- .../arduino/avr/cores/arduino/WInterrupts.c | 31 +++++++++--- .../arduino/sam/cores/arduino/WInterrupts.c | 48 +++++++++++++++---- 2 files changed, 63 insertions(+), 16 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/WInterrupts.c b/hardware/arduino/avr/cores/arduino/WInterrupts.c index fa2a71aecc3..7af25eefdc9 100644 --- a/hardware/arduino/avr/cores/arduino/WInterrupts.c +++ b/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<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); }