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.

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.