Reading a multibyte volatile variable that is updated in an ISR

Question

Consider the Arduino code at the end of this post for full reference that makes LED13 blink in a 0.5Hz rhythm driven by timers. It is just a proof of concept, I know it can be improved for various aspects, but lets focus on the following issue. Although in the PoC below I only use bit0, the goal of my PoC is to find a robust way to access the full 32-bit integer outside the ISR. The source sniplet discussed is this:

void loop() {
  digitalWrite( led , secondCounter & 1 );
}

The ISR increases the secondsCounter every second and the led follows bit0 of that counter, making it blink.

The resulting disassembly listing for the loop looks like this:

000002ae <loop>:
 2ae:   60 91 00 02     lds     r22, 0x0200     ; Read secondCounter from memory
 2b2:   70 91 01 02     lds     r23, 0x0201     ; 4 bytes, 32-vit integer
 2b6:   80 91 02 02     lds     r24, 0x0202
 2ba:   90 91 03 02     lds     r25, 0x0203
 2be:   61 70           andi    r22, 0x01       ; 1
 2c0:   77 27           eor     r23, r23
 2c2:   88 27           eor     r24, r24
 2c4:   99 27           eor     r25, r25
 2c6:   8d e0           ldi     r24, 0x0D       ; 13
 2c8:   0c 94 0a 02     jmp     0x414           ; 0x414 <digitalWrite>

Notice that in the background Timer event interrupts are fired all the time and there is a possibility that an interrupt is serviced while r22-r25 are being read from memory. This may result in a corrupted integer value being read from memory, but it can easily be fixed by disabling interrupts while the variable is read from memory:

void loop() {
  noInterrupts();                               // Prevent secondCounter from being updated in ISR while being read from memory in main loop.
  digitalWrite( led , secondCounter & 1 );
  interrupts();                                 // Enable interrupts 
}

Which results in:

000002ae <loop>:
 2ae:   f8 94           cli                     ; Disable interrupts
 2b0:   60 91 00 02     lds     r22, 0x0200
 2b4:   70 91 01 02     lds     r23, 0x0201
 2b8:   80 91 02 02     lds     r24, 0x0202
 2bc:   90 91 03 02     lds     r25, 0x0203
 2c0:   61 70           andi    r22, 0x01       ; 1
 2c2:   77 27           eor     r23, r23
 2c4:   88 27           eor     r24, r24
 2c6:   99 27           eor     r25, r25
 2c8:   8d e0           ldi     r24, 0x0D       ; 13
 2ca:   0e 94 0d 02     call    0x41a           ; 0x41a <digitalWrite>
 2ce:   78 94           sei                     ; Enable interrupts
 2d0:   08 95           ret

But now the entire digitalWrite routine (which takes a relatively long time) is executed while interrupts are disabled and pending interrupts have to wait a long time before being serviced.

A solution seems to be the use of a dummy variable:

void loop() {
  noInterrupts();                                     // Prevent secondCounter from being updated in ISR while being read from memory in main loop.
  uint32_t seconds = secondCounter;
  interrupts();                                       // Enable interrupts 
  digitalWrite( led , seconds & 1 );
}

Which results in nice and clean assembly:

000002ae <loop>:
 2ae:   f8 94           cli                     ; Disable interrupts
 2b0:   60 91 00 02     lds     r22, 0x0200     ; Read variable from memory
 2b4:   70 91 01 02     lds     r23, 0x0201
 2b8:   80 91 02 02     lds     r24, 0x0202
 2bc:   90 91 03 02     lds     r25, 0x0203
 2c0:   78 94           sei                     ; Enable interrupts
 2c2:   61 70           andi    r22, 0x01       ; Do other stuff
 2c4:   77 27           eor     r23, r23
 2c6:   88 27           eor     r24, r24
 2c8:   99 27           eor     r25, r25
 2ca:   8d e0           ldi     r24, 0x0D       ; 13
 2cc:   0c 94 0c 02     jmp     0x418   ; 0x418 <digitalWrite>

Question: The use of an extra temporary variable seems cumbersome, is there a smarter and tidier solution to this?

Failed attempt: I tried creating a function like:

uint32_t atomic_int32( uint32 var ) {
  noInterrupts();
  uint32_t tmp = var;
  interrupts();
  return tmp;
}

Combined with:

  digitalWrite( led , atomic_int32( secondCounter ) & 1 );

But the compiler 'optimizes' the reading of the variable from memory outside the cli and sei instructions. Actually there is nothing in between those instructions at all:

000002ae <loop>:
 2ae:   80 91 00 02     lds     r24, 0x0200     ; Read variable from memory
 2b2:   90 91 01 02     lds     r25, 0x0201
 2b6:   a0 91 02 02     lds     r26, 0x0202
 2ba:   b0 91 03 02     lds     r27, 0x0203
 2be:   f8 94           cli                     ; Disable interrupts
 2c0:   78 94           sei                     ; Enable interrupts
 2c2:   60 e0           ldi     r22, 0x00       ; 0
 2c4:   8d e0           ldi     r24, 0x0D       ; 13
 2c6:   0c 94 09 02     jmp     0x412           ; 0x412 <digitalWrite>

---

Full code

/*
 * (c) J.P. Hendrix
 *
 * http://blog.linformatronics.nl/213/electronics/timed-1-millisecond-interrupt-routine-for-arduino
 * 
 * Timed interrupt using Timer2
 * ISR is called every 1ms
 *
 * The LED on pin13 will blink in a 0.5Hz rhythm
 */

#define _BC(bit) ( 0 << ( bit ) )
#define _BS(bit) ( 1 << ( bit ) )

// Pin 13 has an LED connected on most Arduino boards.
const uint8_t led = 13;
volatile uint16_t millisecondCounter = 0;
volatile uint32_t secondCounter = 0;

void setup() {                
  // initialize the digital pin as an output.
  pinMode( led , OUTPUT);

  // Set up Timer2 for 1 ms interrupts
  // - Arduino is clocked at 16MHz
  // - The timer is clocked through a /128 prescaler, thus 125kHz (TCCR2)
  // - The interrupt is generated when the counter hits 125 (OCR2A)
  //    at which moment the Timer is reset to 0, resulting in 1kHz intervals
  TCCR2A = _BC( COM2A1 ) | _BC( COM2A0 ) |              // Normal port operation, OC2A disconnected
           _BC( COM2B1 ) | _BC( COM2B0 ) |              // Normal port operation, OC2B disconnected 
           _BS( WGM21 )  | _BC( WGM20 );                // Clear timer on compare match

  TCCR2B = _BC( FOC2A )  | _BC( FOC2B )  |
           _BC( WGM22 )  |                              // Clear timer on compare match
           _BS( CS22 )   | _BC( CS21  )  | _BS( CS20 ); // prescaler f = clk2 / 128

  OCR2A  = 125 - 1;                                         // 16MHz / 128 = 125kHz => 125kHz/125 = 1kHz
  TCNT2  = 0; 
  TIMSK2 = _BC( OCIE2B ) | _BS( OCIE2A ) | _BC( TOIE2 );// Enable compare match A interrupts
  sei();                                                // Enable global interrupts
}

// Attach interrupt routine to the Timer Compare Interrupt
ISR( TIMER2_COMPA_vect ) {
  millisecondCounter += 1;
  if ( millisecondCounter == 1000 ) {                   // 1000 milliseconds equals 1 second
    secondCounter += 1;
    millisecondCounter = 0;
  }
};

void loop() {
    digitalWrite( led , secondCounter & 1 );
}

Answer 1

First of all, you're worrying about a non-issue. Since the operation only depends on the value of the LSB of the first byte of the 32-bit variable, whether or not the other bytes change while that particular stretch of code is executing has no effect whatsoever.

But in the general case, your function atomic_int32() fails because it is the caller that reads the variable associated with the formal argument var before the function is called, while it is building the argument list on the stack. To fix this, and actually read the variable inside the function while the interrupts are off, you need to pass the address of the variable to the function, like this:

uint32_t atomic_int32 (volatile uint32_t *var)
{
  noInterrupts();
  uint32_t tmp = *var;
  interrupts();
  return tmp;
}

Call it like this:

digitalWrite (led, atomic_int32 (&secondCounter) & 1);

Answer 2

If you don't want to shut off interrupts, consider:

uint32_t atomic_int32 (uint32_t *var)
{
  uint32_t ret;

  do {
     ret = *(volatile uint32_t *)var;
  } while (ret != *(volatile uint32_t *)var);

  return ret;
}

The resulting assembly listing looks pretty straightforward:

  5e:   80 91 60 00     lds     r24, 0x0060     ; ret = *(volatile uint32_t *)var;
  62:   90 91 61 00     lds     r25, 0x0061
  66:   a0 91 62 00     lds     r26, 0x0062
  6a:   b0 91 63 00     lds     r27, 0x0063
  6e:   40 91 60 00     lds     r20, 0x0060     ; *(volatile uint32_t *)var
  72:   50 91 61 00     lds     r21, 0x0061
  76:   60 91 62 00     lds     r22, 0x0062
  7a:   70 91 63 00     lds     r23, 0x0063
  7e:   84 17           cp      r24, r20        ; ret != *(..)
  80:   95 07           cpc     r25, r21
  82:   a6 07           cpc     r26, r22
  84:   b7 07           cpc     r27, r23
  86:   59 f7           brne    .-42            ; while do ... 0x5e <main+0x1e>

Answer 3

Using a byte pointer to the long incurred no overhead (the compiler knew enough not to create the pointer variable) Indeed, it even saved some by only loading the byte of interest and no suspension of interrupts is required:

#include <WProgram.h>
volatile uint32_t secondCounter = 0;
const uint8_t led = 13;

void setup(){
   ;
}

void loop() {
   byte *p = (byte*)&secondCounter;
   digitalWrite( led , *p & 1 );
}

Compiled to:

0000042a <loop>:

void loop() {
   byte *p = (byte*)&secondCounter;
   digitalWrite( led , *p & 1 );
 42a:   60 91 09 01     lds r22, 0x0109
 42e:   61 70           andi    r22, 0x01   ; 1
 430:   8d e0           ldi r24, 0x0D   ; 13
 432:   0e 94 af 01     call    0x35e   ; 0x35e <digitalWrite>
}
 436:   08 95           ret

--- Update - Taking the calculation on the volatile out of the `digitalWrite()' call:

void loop() {
   uint8_t tmp;
   noInterrupts();                                     // Prevent secondCounter from being updated in ISR while being read from memory in main loop.
   tmp = ((uint8_t)(secondCounter & 0x00400000L) != 0);
   interrupts();                                       // Enable interrupts

   digitalWrite( led, tmp );
}

gives:

void loop() {
   uint8_t tmp;
   noInterrupts();                                     // Prevent secondCounter from being updated in ISR while being read from memory in main loop.
 42a:   f8 94           cli
   tmp = ((uint8_t)(secondCounter & 0x00400000L) != 0);
 42c:   80 91 09 01     lds r24, 0x0109
 430:   90 91 0a 01     lds r25, 0x010A
 434:   a0 91 0b 01     lds r26, 0x010B
 438:   b0 91 0c 01     lds r27, 0x010C
   interrupts();                                       // Enable interrupts
 43c:   78 94           sei

   digitalWrite( led, tmp );
 43e:   8d e0           ldi r24, 0x0D   ; 13
 440:   60 e0           ldi r22, 0x00   ; 0
 442:   0e 94 af 01     call    0x35e   ; 0x35e <digitalWrite>
}