Are function pointer assignments atomic in Arduino?

Question

The following snippets are from TimerOne library source code:

// TimerOne.h:
void (*isrCallback)();
// TimerOne.cpp:
ISR(TIMER1_OVF_vect) // interrupt service routine that wraps a user defined function supplied by attachInterrupt
{
 Timer1.isrCallback();
}
// TimerOne.cpp:
void TimerOne::attachInterrupt(void (*isr)(), long microseconds)
{
 if(microseconds > 0) setPeriod(microseconds);
 isrCallback = isr; // register the user's callback with the real ISR
 TIMSK1 = _BV(TOIE1); // sets the timer overflow interrupt enable bit
 resume(); 
}

The question: if the timer is already running, and the main program calls attachInterrupt(), could the timer interrupt occur there during the function pointer assignment isrCallback = isr;? Then, with lucky timing, Timer1.isrCallback(); function pointer would consist partly of the old and partly of the new address, causing the ISR jump to a bogus location?

I suppose this might be the case, since function pointers are certainly wider than 1 byte, and accessing> 1 byte data isn't atomic. Possible workarounds could be:

• Always call detachInterrupt() to make sure the timer isn't running, before calling attachInterrupt(), i.e. clarify Timer1 docs.
• Or, modify Timer1, disabling timer overflow interrupts temporarily just before isrCallback = isr;

Does this make sense, or is there something in Timer1 sources or function pointer assignments that I've missed?

Answer 1

Have a look at the code for attachInterrupt() and detachInterrupt() in /Applications/Arduino.app/Contents/Resources/Java/hardware/arduino/cores/arduino/WInterrupts.c (well, that's where they are on a Mac, anyway. Arduino file structure on other OSes' probably looks similar in the lower levels of the path).

It appears that attachInterrupt() assumes that the interrupt in question is not yet enabled because it writes the function pointer without taking any precautions. Note that detachInterrupts() disables the target interrupt before writing a NULL-pointer to its vector. So I'd at least use a detachInterrupt() / attachInterrupt() pair

I'd want to run any such code in a critical section, myself. It appears your first way (detach, then attach) would work, though I can't be sure it couldn't miss an unfortunately timed interrupt. The datasheet for your MCU might have more to say on that. But neither am I sure at this point, that a global cli()/sei() wouldn't miss it either. The ATMega2560 datasheet, section 6.8, does say "When using the SEI instruction to enable interrupts, the instruction following SEI will be executed before any pending interrupts, as shown in this example", seeming to imply that it can buffer an interrupt while the interrupts are off.

• Joonas Pulakka: It's indeed useful to dive to the sources:) TimerOne's attach / detach interrupt mechanism seems to be done similarly as the standard (WInterrupt's) and so has the same "features".

Answer 2

It looks like you have a point. The logical thing to do would be to disable interrupts in such a way that you don't re-enable them if they were disabled in the first place. For example:

 uint8_t oldSREG = SREG; // remember if interrupts are on
 cli(); // make the next line interruptible
 isrCallback = isr; // register the user's callback with the real ISR
 SREG = oldSREG; // turn interrupts back on, if they were on before

---

"When using the SEI instruction to enable interrupts, the instruction following SEI will be executed before any pending interrupts, as shown in this example"

The intention of this is to let you write code like this:

 sei (); // enable interrupts
 sleep_cpu (); // sleep

Without that provision you might get an interrupt between those two lines, and thus sleep indefinitely (because the interrupt which was going to wake you has occurred before you slept). The provision in the processor that the next instruction, after interrupts become enabled if they were not enabled before, is always executed, guards against this.

• Edgar Bonet: Wouldn't it be more efficient to TIMSK1=0; TIFR1=_BV(TOV1); isrCallback=isr; TIMSK1=_BV(TOIE1);? It spares one CPU register and doesn't contribute interrupt latency.
• Nick Gammon: What about the other bits, like ICIE1, OCIE1B, OCIE1A? I understand about the latency, but interrupts should be able to cope with a couple of clock cycles of not being available. There may be a race condition. The interrupt might be triggered already (ie. the flag in the CPU is set) and turning TIMSK1 off might not stop it being handled. You might also need to reset TOV1 (by writing 1 to it in TIFR1) to ensure that doesn't happen. The uncertainty there leads me to think that turning interrupts off globally is the safer course.