Take the 2-minute tour ×
Electrical Engineering Stack Exchange is a question and answer site for electronics and electrical engineering professionals, students, and enthusiasts. It's 100% free, no registration required.

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 );
}
share|improve this question
3  
I think the intermediate variable is the best way to avoid the problem (unless you can get away with an 8bit variable or do the check inside the interrupt), that is probably why this way is shown as an example in the atomic library –  alexan_e Mar 12 at 20:34
    
tmp needs to be volatile to prevent it being optimised out. –  Martin Mar 12 at 20:38

3 Answers 3

up vote 2 down vote accepted

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);
share|improve this answer
    
The volatile fixed it, I figured it out before I noticed your edit. I threw in an extra inline and it now works as I imagined, the resulting assembler is nice and clean. I'll clean up the comments. –  jippie Mar 13 at 7:03

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>
share|improve this answer
    
+1 for thinking out of the box. –  alexan_e Mar 12 at 21:02
    
I added the disassembly for information purposes. If you don't like the edit, feel free to roll back. –  jippie Mar 13 at 6:53

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>
}
share|improve this answer
    
The LED flashing and subsequent use of only bit 0 is the Proof of Concept part of my code. I am trying to come up with robust code that can actually access the whole 32 bits. I could have used an 8 bit counter otherwise. I attempted to clarify that in my question. –  jippie Mar 12 at 21:34
    
You'll need to either hold off interrupts during the access - which you did; you just didn't like the duration they were off - or to keep reading the volatile until successive reads match as markrages suggests. You can reduce the interrupts-off time by taking the volatile access out of the function call. The optimizer is then free to protect only the access and protects only the 4 byte-reads. But consider when starting down this road, you may be writing to the particular optimizer as much as to the language and you may no longer be portable, even to the next rev of the compiler. –  JRobert Mar 13 at 2:18
    
Not sure if I understand 'But consider when starting do... ' why would it get optimizer specific? I don't see the potential problem. –  jippie Mar 13 at 7:02
    
I was, and I assume you were, guided by the compiler output, meaning at that point I was writing "gcc", not just C. More than once I've seen optimizers screw up code that the same compiler, barefoot, had generated correctly. I think/believe/hope(!) optimizers have come a long way since then but stuff happens - as in your "Failed Attempt" example. I believe I was completely rigoruos in lifting an expression out of a call into a separate statement (but don't I always! :) b/c I did some of the parsing. It's just a reminder to be aware that within optimizers "there be dragons". –  JRobert Mar 13 at 11:59

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.