If statement behaves erratically

Question

I connected the output from an encoder motor to int0. this is used to increment a counter at every pulse. in the loop(), I check this counter and stops the motor if it is above certain value. But this "if statement" works erratically. The while loop should break at 380, but sometimes it breaks at 256.

Serial.print(cnt);
cnt=0;
Serial.print("  ");
Serial.print(cnt);
motor1_up(255);
motor2_up(255);
while(1)
{
    if(cnt>380)
    {
        Serial.print(" break ");
        Serial.print(cnt);
        break;
    }
}
motor1_down(255);
motor2_down(255);
Serial.print("  ");
delay(40);
Serial.println(cnt);
motor1_down(0);
motor2_down(0);
delay(2000);

Here is the serial output

 1  0 break 381  389
 396  0 break 256  264
 271  0 break 381  389
 396  0 break 381  389
 396  0 break 256  264
 270  0 break 381  389
 397  0 break 381  390
 396  0 break 381  389
 397  0 break 381  389
 396  0 break 256  264
 271  0 break 381  389

Answer 1

Not only must cnt variable be declared volatile, but accessing it from the main loop must also be protected because:

• it can be modified by an ISR
• it is longer than 1 byte

In AVR architecture (8-bits), read/write operations on bytes are guaranteed to be atomic, i.e. no function can access a byte while it is read/written by an ISR.

However, operations on types larger than bytes are not atomic!

This means that reading e.g. an int variable from your main loop might be interrupted in the middle of the operation (only 1 byte read) by an ISR that could modify the value (both bytes this time); then when the ISR is finished the main loop would resume reading the second byte -modified by the ISR- of the variable, while keeping the first original byte -not modified by the ISR.

I would advise you to write a pair of functions to read and write cnt in a protected way, i.e. with interrupts disabled:

volatile int cnt;

void set_cnt(int val) {
    uint8_t status = SREG;
    cli();
    cnt = val;
    SREG = status;
}

int get_cnt() {
    uint8_t status = SREG;
    cli();
    int val = cnt;
    SREG = status;
    return val;
}

What those functions do are:

1. get the current status of exceptions (from SREG register)
2. disable exceptions with cli()
3. access cnt without any risk of it being accessed concurrently
4. restore initial status of exceptions (to SREG register)

Everywhere in your code (except in code called from an ISR where it is not necessary), you must then call these functions to access cnt variable.

Answer 2

It looks like it could be a caching issue. If you're accessing a variable from the main loop and from an interrupt handler, then you need to ensure it's declared as volatile. For example:

volatile int cnt = 0;

That tells the compiler that the contents of the variable could change unexpectedly, i.e. when an Interrupt Service Routine is triggered. The result is that it always fetches the actual value from RAM, instead of relying on a cached copy which could be out-of-date.