1

I have a very simple ISR:

const uint32_t PATTERN_A = 0xFF00FF00;
const uint32_t PATTERN_B = 0x00FF00FF;
volatile uint32_t count = 0;    
volatile uint32_t toggle = PATTERN_A;

// My ISR
void callback()
{
    count++;
    toggle = (count % 2) ? PATTERN_A : PATTERN_B;
}

The idea is that the toggle variable takes values 0xFF00FF00 or 0xFF00FF00 depending on the evenness of the count variable. This is the only place where toggle is changed, but it's used (read-only) in the sketch.

I don't know why, but toggle gets all sort of random values during execution.

What is happening here?

4

The problem is a combination of several causes.

Assignments are not atomic

An atomic operation is something that is completely done or not. The opposite is some operation that can be disrupted while executing, leaving thing in a inconsistent state.

This is how the count++ works at machine level in Arduino:

void callback()
{
    count++;
  fa:   80 91 0a 02     lds r24, 0x020A
  fe:   90 91 0b 02     lds r25, 0x020B
 102:   a0 91 0c 02     lds r26, 0x020C
 106:   b0 91 0d 02     lds r27, 0x020D
 10a:   01 96           adiw    r24, 0x01   ; 1
 10c:   a1 1d           adc r26, r1
 10e:   b1 1d           adc r27, r1
 110:   80 93 0a 02     sts 0x020A, r24
 114:   90 93 0b 02     sts 0x020B, r25
 118:   a0 93 0c 02     sts 0x020C, r26
 11c:   b0 93 0d 02     sts 0x020D, r27

The first four instruction loads the four count bytes into four separate register, the laboriously increment it by one and then store it back in RAM.

Now, toggle is a variable of interest and you want to use it inside loop, like this:

void loop()
{
    uint32_t x = toggle;
    ...
}

And at machine level you get

    uint32_t x = toggle;
 414:   60 91 04 01     lds r22, 0x0104
 418:   70 91 05 01     lds r23, 0x0105
 41c:   80 91 06 01     lds r24, 0x0106
 420:   90 91 07 01     lds r25, 0x0107

Here we are simply copying the toggle variable into our x variable. As you can see, the copy takes four machine instructions to get completed.

Now, what happens if an interrupt comes while you are copying (or using) toggle?

An interrupt suspend the assignment, go to the ISR (callback in this case and change toggle value. Upon return, copying continues, mixing bytes from the old and new value.

You must use volatile

Any global variable used inside an ISR must be declared volatile. This is an hint to the compiler that the variable could change unexpectedly. Therefore, the volatile variable must be always read afresh, discarding any cached value.

Critical section

The rule for using values changed by ISR is simple:

  1. Always copy the ISR's global variables to your own temporary variable.
  2. Before coping, disable interrupts and reenable it after the copy is done. This prevent the ISR from executing while your are copy the values.

Demo

This little demo shows you all of this in action.

The sketch collect all the values from toggle (not disabling interrupts) during one minute and after that print a table showing what it got.

/*
 * This sketch show how to deal with variables shared between ISR and main thread. 
 * 
 * By Look Alterno 2018
 */

#include "TimerOne.h"

volatile uint32_t count = 0;
const uint32_t PATTERN_A = 0xFF00FF00;
const uint32_t PATTERN_B = 0x00FF00FF;

volatile uint32_t toggle = PATTERN_A;

#define MAX_CASES 16
struct {
    unsigned long value;
    int count;
} found[MAX_CASES];
int freeFound = 0;

#define PRINT_INTERVAL 60000L

void setup() {
    Serial.begin(9600);

    while(!Serial);

    Serial.println("");
    Serial.println("Test assignment under interrupts");
    Serial.println("");

    Serial.print("Value A=");
    Serial.println(PATTERN_A, HEX);
    Serial.print("Value B=");
    Serial.println(PATTERN_B, HEX);

    Timer1.initialize(500);
    Timer1.pwm(9, 512);
    Timer1.attachInterrupt(callback);

    pinMode(A0, INPUT);
    randomSeed(analogRead(A0));
}

void callback()
{
    count++;
    toggle = (count % 2) ? PATTERN_A : PATTERN_B;
}

void loop()
{
  static unsigned long prev = 0;  

  //  A random delay to simulate some real, variable work being done.
    delayMicroseconds(random(250));

  // This is the critical section.
//  noInterrupts();
    uint32_t x = toggle;
//  interrupts();

  // Now, let's see what value we got.
    if(x != PATTERN_A && x != PATTERN_B) {
        insert(x);
    }

  // Print values found 
  unsigned long now = millis();
  if (now - prev > PRINT_INTERVAL) {
    printFound();    
    prev = now;
  }
}

/*
 * Add a value to a table for counting.
 */
void insert(unsigned long x)
{
    int index = 0;

    while(index < freeFound && found[index].value != x) {
        index++;
    }

    if(index < MAX_CASES) {
        if(index == freeFound) {
            found[index].value = x;
            found[index].count = 1;
            freeFound++;
      Serial.print(freeFound);
      Serial.print(". Add=");
      Serial.println(x, HEX);
        } else {
            found[index].count++;
        }
    } else {
        Serial.print("*** Table overflow. value=");
        Serial.print(x, HEX);
        Serial.println(" ***");
    }
}

/*
 * Print the table of found values so far.
 */
void printFound() {
  Serial.println("");
  Serial.println("Values found");
  Serial.println("");
  Serial.println("Value - Count");
  for (int i=0; i < freeFound; i++) {
    Serial.print(found[i].value, HEX); Serial.print(" - "); Serial.println(found[i].count); 
  }
}

If you uncomment noInterrupts and interrupts, every thing works just fine.

Other CPU

More powerful CPUs have atomic assignments, but following this practices makes your code portable across platforms, no matter what. Think of defensive programming

  • 1
    you can do it branch-less by defining an array with both states const uint32_t PATTERN[2] = { 0x00FF00FF, 0xFF00FF00 }; and evaluating it via PATTERN[count & 1];. – Kwasmich Feb 21 '18 at 15:17
  • @Kwasmich. Your proposal is more compact, but I'm trying to keep it simple and focused on the central issue. As a personal matter, I prefer the "?:" pattern. You can explore this issue comparing the standard "?:" vs your proposal, from performance, convenience and clarity. I'm waiting for your post. – user31481 Feb 21 '18 at 15:59
  • @Kwasmich: Note that branchless is not necessarily better. Especially on the AVR which, owing to its very shallow pipeline, has very cheap branching. – Edgar Bonet Feb 21 '18 at 17:13
  • 2
    Moderator's note: Due to the dispute about this answer we have reverted the community wiki flag, and rolled back the answer to the (almost) original one. Anyone who believes they can post a better answer is free to do so. – Nick Gammon Feb 22 '18 at 6:15
  • @User42 You are right. There is no point in making the code more compact, if it does not save time or flash space. – Kwasmich Feb 22 '18 at 14:08