[SOLVED] Variables do not change in an interrupt function

With timer 1 enabled,

void setup() {
  pinMode(LED_BUILTIN, OUTPUT);

  TCCR1A = 0; // CTC mode
  TCCR1B = bit(WGM12) | 5; // CTC, 15.6 kHz clock
  OCR1A = 0xC35; // 5 Hz
  OCR1B = 0x200;
  TIMSK1 = bit(OCIE1B) | bit(OCIE1A);
}

void loop() {}

the following interrupt code produces a blinking LED:

ISR(TIMER1_COMPA_vect) {
  digitalWrite(LED_BUILTIN, !digitalRead(LED_BUILTIN));
}

However, this code does not (the LED is always on):

ISR(TIMER1_COMPA_vect) {
  static uint8_t led_on = 1;
  digitalWrite(LED_BUILTIN, led_on);
  led_on = !led_on;
}

Neither does this version:

uint8_t led_on = 1;
ISR(TIMER1_COMPA_vect) {
  digitalWrite(LED_BUILTIN, led_on);
  led_on = !led_on;
}

What is going on?

Edit: I have found out that both versions work if I disable OCIE1B. This puzzles me even further.

Edit: Apparently, an unexpected interrupt (without handler) resets the board by default. And it looks like on my board LED output survives the reset.

Read up about Volatile variable declaration

But led_on is only used inside the interrupt function (it is even static in the second example), so declaring it volatile should not change anything (and it doesn't, I checked).

Please post a complete non-working example.

digitalWrite(…) is documented to take HIGH or LOW for the second parameter. Bad things can happen when you ignore the documentation.

digitalRead(…) is documented to return HIGH or LOW.

Note that **!**1 is not equal to zero. In fact, 1 and **!**1 both happen to have the same effect (but not necessarily the same value) as HIGH.

On the other hand, it happens that **!**HIGH is LOW and **!**LOW is HIGH.

Note that !1 is not equal to zero.

Um... I'm pretty sure it is... Were you thinking of "~1" ?
"!HIGH" is by no means guaranteed to be "LOW", but that's a different issue.

aarg:
Please post a complete non-working example.

Done. Timer initialization code is now in the beginning of this thread.

vaj4088:
digitalWrite(...) is documented to take HIGH or LOW for the second parameter. Bad things can happen when you ignore the documentation.

digitalRead(...) is documented to return HIGH or LOW.

Note that **!**1 is not equal to zero. In fact, 1 and **!**1 both happen to have the same effect (but not necessarily the same value) as HIGH.

On the other hand, it happens that **!**HIGH is LOW and **!**LOW is HIGH.

HIGH and LOW are defined in Arduino.h to be 0x1 and 0x0 respectively. I agree that using 0 and 1 blindly is a bad idea, but in this case nothing changes. For example, the following generic code also doesn't blink the LED:

ISR(TIMER1_COMPA_vect) {
  static bool led_on = true;
  digitalWrite(13, led_on ? HIGH : LOW);
  led_on = !led_on;
}

I wrote this code:

void setup() {
	Serial.begin(115200);
	Serial.print("LOW = ")  ; Serial.println(LOW, HEX);
	Serial.print("!LOW = ") ; Serial.println(!LOW, HEX);
	Serial.print("HIGH = ") ; Serial.println(HIGH, HEX);
	Serial.print("!HIGH = "); Serial.println(!HIGH, HEX);
	Serial.print("0 = ")    ; Serial.println(0, HEX);
	Serial.print("!0 = ")   ; Serial.println(!0, HEX);
	Serial.print("1 = ")    ; Serial.println(1, HEX);
	Serial.print("!1 = ")   ; Serial.println(!1, HEX);
}

void loop() {
}

and got this result using Arduino IDE v 1.8.1:

LOW = 0
!LOW = 1
HIGH = 1
!HIGH = 0
0 = 0
!0 = 1
1 = 1
!1 = 0

so westfw is correct and I was wrong.

the following interrupt code produces a blinking LED:

Code: [Select]
ISR(TIMER1_COMPA_vect) {
digitalWrite(13, !digitalRead(13));
}

Interesting. I do not see a blink with your code using Arduino 1.8.1 with Arduino UNO.

void setup() {
  pinMode(13, OUTPUT);

  TCCR1A = 0; // CTC mode
  TCCR1B = bit(WGM12) | 5; // CTC, 15.6 kHz clock
  OCR1A = 0xC35; // 5 Hz
  OCR1B = 0x200;
  TIMSK1 = bit(OCIE1B) | bit(OCIE1A);
}

void loop() {}

ISR(TIMER1_COMPA_vect) {
  digitalWrite(13, !digitalRead(13));
}

However, If I add a dummy TIMER_COMPB_vect all works as intended with all three versions. You cannot enable an interrupt without the matching ISR. I'm not sure why the compiler does not complain.

void setup() {
  pinMode(13, OUTPUT);

  TCCR1A = 0; // CTC mode
  TCCR1B = bit(WGM12) | 5; // CTC, 15.6 kHz clock
  OCR1A = 0xC35; // 5 Hz
  OCR1B = 0x200;
  TIMSK1 = bit(OCIE1B) | bit(OCIE1A);
}

void loop() {}

//ISR(TIMER1_COMPA_vect) {
//  digitalWrite(13, !digitalRead(13));
//}

//ISR(TIMER1_COMPA_vect) {
// static uint8_t led_on = 1;
// digitalWrite(13, led_on);
// led_on = !led_on;
//}

uint8_t led_on = 1;
ISR(TIMER1_COMPA_vect) {
  digitalWrite(13, led_on);
  led_on = !led_on;
}


ISR(TIMER1_COMPB_vect) {

}

I'm not sure why the compiler does not complain.

The interrupt table is just a list of pointers. OP declared that his/her code needed to jump to the function at some address in the table. That there is nothing but a NULL pointer at that address is not a syntax problem, so the compiler rightfully does NOT complain. The compiler is not designed to detect logic problems.

typograph:
But led_on is only used inside the interrupt function (it is even static in the second example), so declaring it volatile should not change anything (and it doesn't, I checked).

Just always declare variables volatile if you read or write them in an ISR. Otherwise the compiler can
optimize them away in various (hard to predict) circumstances. Dataflow optimizers need this information
to get things right.

MarkT:
Just always declare variables volatile if you read or write them in an ISR. Otherwise the compiler can
optimize them away in various (hard to predict) circumstances.

No.
The time for volatile is when one thread/context needs to access a variable that is being modified in another.

When using an ISR the two threads/contexts would be the ISR and the foreground.
If just an ISR is accessing & modifying a variable there is no need to use volatile, just like when only the foreground code is accessing a variable there is no need to use volatile.

--- bill

Nope, you don’t understand how compilers optimize… Declare all the variables that aren’t const
as volatile and it is guaranteed to work whatever the optimization level or version of the compiler.

MarkT:
Nope, you don't understand how compilers optimize... Declare all the variables that aren't const
as volatile and it is guaranteed to work whatever the optimization level or version of the compiler.

I assume your comment was aimed at me.
Sure your above statement about always using volatile if not const is correct. However, that is not what you said in your previous statement:

Just always declare variables volatile if you read or write them in an ISR. Otherwise the compiler can
optimize them away in various (hard to predict) circumstances.

Your previous statement is simply factually incorrect for the situation when only the ISR is using/accessing the global variable.

This new statement is pushing ALWAYS using volatile which an excessive and unneeded use of volatile that will disable certain types of optimizations for situations when it is not needed.
There is never an issue of accessing a stale version of a global variable if there is only a single context since there is no other context that is potentially modifying the variable out of view of the compiler for the localized code in the module being compiled.
That context can be foreground or ISR doesn't matter.
Also, any/all local optimizations related to variable caching into registers is guaranteed to be flushed back to the variable memory across function calls.
When volatile is not used, the compiler can never optimize away the memory updates of the global variable since the variable is global in scope and the compiler must ensure that all other code outside of this scope has access to the updated variable once the local scope completes/finishes/exits/calls a sub function.
And this is the reason a global variable can be modified and the modified/updated variable can be seen by a sub function that is called.

While there are some cases where volatile can and must be used to force immediate memory access (like to h/w registers to ensure certain sequencing/ordering) it is not necessary to use volatile to cause an ISR to update global variables that only the ISR uses.
The global variable is guaranteed to be updated back to memory before the ISR exits so the updated value will be seen when the ISR runs again.

There were some cases about 8-9 years ago when this wasn't the case, but this was actually a compiler issue with the pre/post pre-amble code being used by the compiler for ISR functions.
The ISR was incorrectly handling registers and pre-maturely returning, it affect globals as well as caused some register corruption.
Once that bug was fixed, the issue went away.

I actually smashed into this the 3rd day I starting using avr-gcc.

--- bill

cattledog:
Interesting. I do not see a blink with your code using Arduino 1.8.1 with Arduino UNO.

However, If I add a dummy TIMER_COMPB_vect all works as intended with all three versions. You cannot enable an interrupt without the matching ISR. I'm not sure why the compiler does not complain.

Thank you for this insight. I think I have now understood what was going on.

PaulS:
The interrupt table is just a list of pointers. OP declared that his/her code needed to jump to the function at some address in the table. That there is nothing but a NULL pointer at that address is not a syntax problem, so the compiler rightfully does NOT complain. The compiler is not designed to detect logic problems.

Actually, the pointer is not NULL. avg-gcc puts a pointer to __bad_interrupt() there. And __bad_interrupt calls the RESET handler.

As far as I can tell on my Duemilanove the state of the LED survives the reset. At least, if I set it to LOW in setup() the blinking stops.

Declaring the variable as volatile works for me.