0

Running on atmega168p, I've got an interesting behavior I can't explain. A global volatile var that's called pos over here is not updated as it should. Once I add some code below the assignment, it starts working.

#define ENCODER_DO_NOT_USE_INTERRUPTS
#include <Encoder.h>

Encoder myEnc(A2, A3);

void setup() {
  Serial.begin(115200);
  while (!Serial); Serial.println("Started.");
  PCMSK1 |= bit(PCINT10) | bit(PCINT11) | bit(PCINT12);
  PCIFR  |= bit (PCIF1);   // clear any outstanding interrupts
  PCICR |= bit(PCIE1);
}

volatile int pos  = 0;
int oldpos = 0;

void loop() {
  if (oldpos != pos) {
    oldpos = pos;
    Serial.println(pos);
  }
  delay(100);
}

// PCINT10, PCINT11
ISR(PCINT1_vect) {
  pos = myEnc.read();
  //  adding digitalRead(A0)
  // or even Serial.println(pos) updates the var correctly
}

Switching from int to byte doesn't change anything. Encoder library is https://github.com/PaulStoffregen/Encoder. Any clues?

4
  • It looks like ISR is being fired only once. bummer.
    – kotique
    Jul 28, 2019 at 22:03
  • I think it would be better to use a library that support PCINT interrupts instead of hacking it onto a library that doesn't. For example, github.com/kr4fty/EncoderPCI looks like it supports PCINT.
    – Gerben
    Jul 29, 2019 at 8:57
  • Related: 3 ways to ensure atomic access on Arduino/AVR microcontrollers: stackoverflow.com/questions/36381932/… Aug 17, 2020 at 0:09
  • @kotique, you have unmanaged race conditions in your code. I've addressed them and given a thorough explanation here: arduino.stackexchange.com/a/77512/7727. I can't say those are the only problems, but you definitely have race conditions. Making the volatile variable an int8_t won't fix them, they are still there. Give it a shot with my corrections I describe in my action and see what happens now. Aug 17, 2020 at 2:30

3 Answers 3

1

You are using pos outside of the interrupt without turning the interrupt off. pos is two bytes, so it takes the Arduino more than one cycle to read in that variable no matter how it is handled. If an interrupt occurs during that time, you get a corrupted value.

You need to have a cli() and then make a copy of pos to another variable and then a sei() and then use that copy in your calculations in loop.

2
  • Or use a int8_t if that’s possible.
    – RubberDuck
    Jul 30, 2019 at 10:30
  • You should've read till the end of the question, which states "Switching from int to byte doesn't change anything."
    – kotique
    Aug 16, 2020 at 12:15
0

Well, the long awaited answer to this question is simple - the library that I've tried to use in my code was over-optimizing things by going full ASM and interfering with my stuff. Using C version straightened things up.

For anyone getting strange behaviour I recommend taking a quick glance at the lib's code you're using, maybe it's not your fault after all.

1
  • Sounds like a dodgy library, if it interferes with C++ code. It perhaps is meddling with registers without telling the compiler.
    – Nick Gammon
    Aug 17, 2020 at 6:13
0

How to recognize and fix race conditions in Arduino by using atomic access guards:

Switching from int to byte doesn't change anything. Encoder library is https://github.com/PaulStoffregen/Encoder. Any clues?

Your code here:

void loop() {
  if (oldpos != pos) {
    oldpos = pos;
    Serial.println(pos);
  }
  delay(100);
}

has an unaddressed race condition, which leads to undefined behavior and data corruption. Making oldpos into a uint8_t or int8_t, each of which is 1 byte, instead of int, which is 2 bytes, won't fix it, because you need the whole section from if (oldpos != pos) { through Serial.println(pos); to be atomic so that the value of pos you use to enter the if block does not get changed by the interrupt while you are in the if block. I'll explain more later. This is reminds me of a problem I had a few years back, here: C++ decrementing an element of a single-byte (volatile) array is not atomic! WHY? (Also: how do I force atomicity in Atmel AVR mcus/Arduino).

So, let's walk through a case where making a 2-byte variable into a 1-byte variable WILL solve the problem, first, to ensure we understand why one might think making it a 1-byte variable is the solution.

Let's look at this example. It has a race condition and undefined behavior. Can you spot it?

volatile uint16_t isr_counter = 0;

void setup()
{
    // enable the ISR here
}

// Any ISR
ISR()
{
    my_var++;
}

void loop()
{
    Serial.println(isr_counter); // <== race condition!
}

The answer is that isr_counter is a 2-byte variable, and AVR microcontrollers have 8-bit (1-byte) CPUs, so, it is possible that as isr_counter is being read in loop() to be printed, 1 of the 2 bytes is read out correctly to be printed, then the ISR fires, changes the variable, and gives control back to the main loop. The 2nd byte, now not what it was supposed to be, is read out, leading to a corrupted variable! Ex: let's assume isr_counter was 0xFFFF, or 65535, at the moment it is supposed to be printed, and lets say the low byte is read out correctly as 0xFF, then the ISR fires, the variable is incremented to 0x0000 since it overflows, then the control goes back to the main loop, and the 2nd byte is read out, it is now corrupted. What will be printed would be the contents of the variable containing 0x00FF, or 255, when what should have been printed out was 65535, and the next value after that to be printed out would have been 0. And what if the high byte was read out correctly before the low byte? (I'm not sure which order AVR mcus read the bytes), then your corrupted value would be 0xFF00, or 65280, when what should have been printed out was 65535 (0xFFFF), and the next value after that to be printed out would have been 0 (0x0000).

So, simply making the variable a uint8_t magically solves this race condition for this one particular case because 8-bit variables on 8-bit CPUs are read or written atomically, which means "as an atomic, or whole, single, unit, not broken up or corrupted by interrupts or other threads". All operations which take 1 single CPU instruction cycle are automatically atomic, because they cannot be interrupted. For any multi-instruction-cycle code block which we need to be atomic, we must force it to be atomic by using what I call "atomic access guards". Reading a 1-byte variable on an 8-bit CPU, or writing a 1-byte variable on an 8-bit CPU, is 1 instruction, so it is naturally atomic. Incrementing or decrementing a 1-byte variable, however, is NOT an atomic operation, since the variable must be read, modified, and written. Reading a 2-or-more-byte varible on an 8-bit CPU, or writing a 2-or-more-byte varible on an 8-bit CPU, is also NOT atomic, since it takes one instruction per byte. This means just reading a 2-byte variable or writing a 2-byte variable can be interrupted by an ISR in the middle of the operation, which leads to data corruption if you do not use atomic access guards!

Now, your case: would making pos into a 1-byte variable as volatile int8_t pos = 0; solve the race condition? No, not at all. Let's look at your code again, this time imagining that pos is an int8_t instead of an int (which is int16_t on AVR Arduino):

void loop() {
  if (oldpos != pos) {
    oldpos = pos;
    Serial.println(pos);
  }
  delay(100);
}

Let's walk through a potential scenario: pos is 7. oldpos is 6. So, oldpos != pos is true, and you enter the if statement. Now, right as you enter, the ISR fires, pos is changed by the ISR to 6 (or to any other number--it's still a race condition and undefined behavior), oldpos is now set to 6, which is what it already was, and you have data corruption because it should have been set to 7! Note that when I say this is undefined behavior in this case, I simply mean we cannot predict what will happen. This is buggy code. Code should always be predictable. Undefined behavior is a bug! Note also that when talking about "undefined behavior" according to C or C++ standards, we mean "the standard doe not define what will happen", which means your code may act one way on one architecture or with one compiler, and differently on another architecture or with a different compiler. This is not what I am talking about here. Here, when I say "undefined behavior" I don't mean "according to the C or C++ standard", I mean, "according to the rules of logic."

The solution to fix your race condition is to enforce atomic access using what I like to call "atomic access guards". On multi-threaded systems, this means to use locks, mutexes, and semaphores. On single-threaded systems, such as this, it means to disable any interrupt which could interfere and cause the race condition (you can think about interrupts interacting with the main loop as a parallel to one thread interacting with another thread, so it is as though you had a multi-threaded system just by having a single-threaded system + interrupts). For AVR microcontrollers, disabling interrupts to act as atomic access guards is usually done by disabling global (or all) interrupts. BUT, disabling interrupts introduces jitter by delaying the processing of the ISR, so it should be done for as short of a time period as possible! That means you should just disable interrupts, make copies of volatile variables, and re-enable interrupts. This is an important point though: sometimes, in the case of using a 1-byte volatile variable, using a copy of your variable, which can NOT be changed at any moment by an ISR, also, in and of itself, acts as a form of "atomic access guards" and solves the problem all by itself. I'll post an example of this below.

But: DANGER DANGER! It's a "best practice" to always back up the interrupt state, disable interrupts, then restore the interrupt state, rather than just turning interrupts off then back on all the time. Why? Because what if you write a function which sometimes gets called from within an ISR itself? If you enable interrupts inside an ISR, you just enabled nested interrupts, which leads to more race conditions. If you know exactly what you are doing and you want nested interrupts, this is fine! But, chances are one is just making a mistake here and accidentally enabling nested interrupts without even realizing it, leading to really-hard-to-track down data corruption due to nested interrupts. Safety-critical systems can crash and hurt people when this happens, so we can't ever allow unmanaged race conditions.

So, do it like this:

  1. Force atomicity in loop() by backing up the interrupt state.
  2. Make copies of the volatile variables.
  3. Then restore the global interrupt state.
  4. Use only the copy of the varible in the rest of your code. This (using the copy) is also a form of "atomic access guards" all by itself for later code blocks using the variable because using the copy instead of the volatile variable means that the variable can NOT be changed at any moment by an ISR. This enforces atomicity as well!

Making a copy of the volatile variable and nothing else, while interrupts are disabled, minimizes time where interrupts are disabled, making your code the best it can possibly be! It should now look like this:

1 of 3: THE BEST WAY!:

volatile int pos  = 0; // assume this variable is still an `int`, which is 2-bytes

void loop() {
    uint8_t SREG_bak = SREG; // back up the global interrupt state
    noInterrupts(); // disable interrupts
    // make copies of your volatile variables here
    int pos_copy = pos;
    // restore interrupt state, turning them back ON if they were ON before, 
    // or leaving them OFF if they were OFF before.
    SREG = SREG_bak; 

    // Now do what you want with the copy, using pos_copy ONLY,
    // to avoid race conditions! (ie: using this copy here enforces
    // this entire block to be naturally atomic now, and by itself
    // is a form of "atomic access guards", as explained above.)
    if (oldpos != pos_copy) {
        oldpos = pos_copy;
        Serial.println(pos_copy);
    }
    delay(100);
}

Solved!

Let's look at 2 other examples, however, for completeness.

Here, we force the whole if statement block to be atomic. This looks correct at first, but has two problems: 1) [a true bug] Serial.println will block forever if the outgoing serial buffer is full, as it relies on interrupts to empty it, and if interrupts are off, that will ever happen, and 2) [an undesirable effect leading to performance issues, but not a bug] it keeps interrupts off for a longer time period than is necessary. The code above was best: it kept interrupts off the absolute minimum time possible by just making copies of the volatile variables, and it ensured this whole if statement block was atomic by simply using the copy there, which automatically acts as an "atomic access guard" by itself and enforces atomicity in the if statement block.

2 of 3: this still has 2 problems: one is a bug, and one is a performance issue but not a bug:

volatile int pos  = 0; // assume this variable is still an `int`, which is 2-bytes

void loop() {
    uint8_t SREG_bak = SREG; // back up the global interrupt state
    noInterrupts(); // disable interrupts

    if (oldpos != pos) {
        oldpos = pos;
        Serial.println(pos);
    }

    // restore interrupt state, turning them back ON if they were ON before, 
    // or leaving them OFF if they were OFF before.
    SREG = SREG_bak; 

    delay(100);
}

And lastly, what if the volatile variable was a 1-byte variable? Well, then you could simplify to this, since reading a 1-byte variable is naturally an atomic operation!

3 of 3: this works too (without bugs and without performance issues), but ONLY if your volatile variable here is 1-byte!:

// this variable is now a 1-byte variable, which has naturally atomic read and
// write operations! (but not increment or decrement)
volatile int8_t pos  = 0; 

void loop() {
    // Make copies of your volatile variables here.
    // Being a 1-byte variable on an 8-bit (1-byte) CPU architecture, this operation
    // is already naturally atomic, so no need for "atomic access interrupt guards". 
    int8_t pos_copy = pos; 

    // Now do what you want with the copy, using pos_copy ONLY,
    // to avoid race conditions! (ie: using this copy here enforces
    // this entire block to be naturally atomic now, and by itself
    // is a form of "atomic access guards", as explained above.)
    if (oldpos != pos_copy) {
        oldpos = pos_copy;
        Serial.println(pos_copy);
    }
    delay(100);
}

For 3 other ways to create atomic access guards in 8-bit AVR-based Arduinos, see my answer from 2016 here: https://stackoverflow.com/questions/36381932/c-decrementing-an-element-of-a-single-byte-volatile-array-is-not-atomic-why/39693278#39693278.

Last comment: Paul Stoffregen is one of the most-expert in Arduino in our day. I'm sure he makes mistakes, but my default is to trust anything he's written. He's a most-excellent software developer and engineer: one of the absolute best in the entire world. Look for bugs in what he writes, and let him know if you found any, but you have unaddressed race conditions here, so that's what I'd look at first.

References:

  1. My own race condition problem: https://stackoverflow.com/questions/36381932/c-decrementing-an-element-of-a-single-byte-volatile-array-is-not-atomic-why
  2. My own race condition solution, demonstrating 3 (or 4, depending on how you look at it) ways to enforce "atomic access guards" on AVR-based 8-bit Arduinos: https://stackoverflow.com/questions/36381932/c-decrementing-an-element-of-a-single-byte-volatile-array-is-not-atomic-why/39693278#39693278
9
  • I agree about making the copy (as in your example using pos_copy) however for a one-byte variable you wouldn't need to disable interrupts, as copying from one byte to the "copy" would be atomic. However it could be a subtle bug-in-waiting if someone one day decides to make pos two bytes without realizing why you had not put the atomic access guard there. Perhaps a comment would help in that case.
    – Nick Gammon
    Aug 17, 2020 at 6:08
  • 1
    In your example, the delay(100) wouldn't work if interrupts were off so perhaps saving and restoring SREG is overkill. You could just turn interrupts back on. If they somehow get turned off elsewhere then the delay will fail, and you would need to address that.
    – Nick Gammon
    Aug 17, 2020 at 6:11
  • You are right on the 2nd comment, but I think it's just a best practice sort of thing, since frequently I wrap this stuff up into functions which I intend to be called in or out of ISRs. Aug 17, 2020 at 6:15
  • Regarding your 1st comment: however for a one-byte variable you wouldn't need to disable interrupts, as copying from one byte to the "copy" would be atomic., I agree with you on this point: as copying from one byte to the "copy" would be atomic., but disagree with you on this: for a one-byte variable you wouldn't need to disable interrupts, because what needs to be atomic is from the if statement down through the serial print, inclusive. That whole block needs to be atomic, or else you evaluate the if statement on one variable state but do something with it (and print it) on another. Aug 17, 2020 at 6:18
  • In other words, the atomic access guards for the 1-byte variable are still required to ensure that what you are using inside the if block is also what you used to decide to enter the if block in the first place, no? Aug 17, 2020 at 6:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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