Do I need to do atomic reads INSIDE AN ISR?

Hi all,

I know that if I have a 16 or 32 bit volatile variable that is changed within an ISR, I need to do an atomic read of that var outside the ISR, for example to test a 16 bit variable for zero outside the ISR (named "count"):

uint16_t tmp;
cli(); // interrupts off
tmp = count; // atomic read
sei(); // interrupts back on
if (tmp == 0).......

Now, inside the ISR, some code like this:

ISR (TIMER0_COMPA_vect)
{
    while (count--) {
        // do something
    }
}

The "while" statement checks if "count" is true, but is that check atomic, or do I need to do something like this:

ISR (TIMER0_COMPA_vect)
{
    cli();
    while (count--) {
        // do something
    }
    sei();
}

Thanks.

No, since interrupts are always disabled within the ISR.

Regards,
Ray L.

RayLivingston:
No, since interrupts are always disabled within the ISR.

Regards,
Ray L.

I didn't know that interrupts are disabled inside an ISR!

I guess that answers my question, and thank you (karma++ also!)

while (count--) {
        // do something
    }

I assume this was just an example you were searching for.
Normally you don't want to spend much time in an ISR as you will effect other code that might be waiting for their interrupt execution.
Such as: Serial, millis, micros etc.

Interrupts aren't always off in an ISR. In your third code example they are enabled after the sei() statement. But they are disabled when the ISR is initially invoked and most of the time it makes sense to leave them that way.

If you haven't read this, Nick covers things here.

.

A follow up...

jboyton:
...and most of the time it makes sense to leave them that way.

Enabling interrupts in an interrupt service routine is a very bad choice. Don't do it. If you must, still don't do it. Find another solution.

I think you're being a little too harsh. It has it's place. It can be useful. It can be safe.

It has it's place. It can be useful. It can be safe.

It is not necessary as you can work on things back in loop()

LarryD:
It is not necessary as you can work on things back in loop()

Not always.

LarryD:
It is not necessary as you can work on things back in loop()

Not conveniently...

Consider a system with one high priority high speed interrupt, a second less urgent
interrupt that needs to talk SPI or some such, and a main body handling some serial
I/O and other stuff.

The main loop() might block at random outputing serial, so wouldn't be very
prompt for handling the SPI task (lets say its an RF transceiver doing some mesh
network stuff). However using an interrupt for the RF transceiver means it will always
run in a nice bounded speedy way whatever loop() is up to.
Make the transceiver ISR re-enable interrupts allows the really high priority ISR to run
whenever it wants (lets say it handles quadrature encoder at upto 50kHz).

So there are realistic situations where re-enabling interrupts in one ISR is good for another
ISR's performance.

If you re-enable interrupts inside an ISR, then code that runs there should do atomic reads of variables that might be modified by other interrupts. Note that atomic access is "moderately expensive"; you don't normally do cli/sei, you have to read and save the state of the interrupt enable flag, disable it, and restore it to what it was...
It also means:

  1. probably you should have accessed that variable before you re-enabled interrupts.
  2. having multiple ISRs accessing the same global variable(s) seems like a really bad idea, even compared to re-enabling interrupts in the first place.
  3. re-enabling interrupts (on AVR) means re-enabling ALL interrupts, not just the high-priority interrupts that you're trying to accommodate. The whole "high/low priority" interrupt idea is best handled on an architecture that supports multiple priorities in hardware.

MarkT:
The main loop() might block at random outputing serial, so wouldn't be very
prompt for handling the SPI task (lets say its an RF transceiver doing some mesh
network stuff). However using an interrupt for the RF transceiver means it will always
run in a nice bounded speedy way whatever loop() is up to.

Haven't you got that backwards? Short of rewriting the code that handles timer 0 interrupts, and serial interrupts, you still have the issue that your RF transceiver may have to wait for them. And enabling interrupts in your high-priority task now potentially makes it run slower.

And if you do enable interrupts inside an ISR, now you have to protect access to external variables (make such access atomic) which then slows down the interrupt further.

And then you even have the issue of re-entrancy. Say you enable interrupts in an ISR that outputs via SPI. And the interrupt that happens takes quite a long time. And your ISR is called again, re-entrantly. Now you have to code to allow for that as well.

And then you even have the issue of re-entrancy. Say you enable interrupts in an ISR that outputs via SPI. And the interrupt that happens takes quite a long time. And your ISR is called again, re-entrantly. Now you have to code to allow for that as well.

It is not as bad as it sounds.

The outline for a Re-entrant ISR is

Store the ISR return address
Store the content of any Reg that could be changed by the ISR
code of the ISR
Re-store the Reg's
Return from the ISR (re-enables interrupts)

With the AVR chips the return address is stored on the stack. Some needs to double check me but as I understand it the Reg's are allso saved to the stack so half the work is done for you. Of course you still need to be very careful about your own code but!

Mark

I didn't expect that the re-entrancy would be a problem, per se, but ISRs usually fiddle with hardware, or update global variables. This is the sort of thing you don't want to be doing while another copy, further up the stack, is half-way doing already.

This is the sort of thing you don't want to be doing while another copy, further up the stack, is half-way doing already.

Yes - that's the big problem!

Mark

LarryD:

while (count--) {

// do something
   }



I assume this was just an example you were searching for.
Normally you don't want to spend much time in an ISR as you will effect other code that might be waiting for their interrupt execution. 
Such as: Serial, millis, micros etc.

Here's the actual code that I was asking about:

ISR (TIMER0_COMPA_vect) // IR carrier generator
{
    if (count) {
        ((count--) % 2) ? led_ctrl (0) : led_ctrl (onoff); // pulse IR LED on and off
    } else {
        onoff = 0; // insure IR led cannot light now
        busy = 0; // flag ISR is done
    }
}

The ISR is called 80000 times per second (40 khz * 2) and to generate an IR remote control signal, I first set "busy" to 1, then put a count into "count".
The ISR turns the IR LED on and off 40000 times per second (hence the 80 khz rate... 1/2 for on and 1/2 for off). At the same time, the "count" variable is decremented and when it hits zero, the ISR clears "busy" so the "outside world" knows it's finished.
Then the next timing code is pushed into "count" until all the entries in a table are sent.
I just wanted to know if the first line of code (the "if (count)" part was being executed atomically, and since interrupts are disabled within an ISR, the answer is obviously "yes".

P.S. "count", "onoff" and "busy" are all declared as volatile.

@Krupski
I am interested in seeing the finished code if you would share it.

FYI
I have an IR NEC receive library in post #1.
It uses interrupts to decode RX bits.
here

.

LarryD:
@Krupski
I am interested in seeing the finished code if you would share it.

FYI
I have an IR NEC receive library in post #1.
It uses interrupts to decode RX bits.
here

.

Sure... it's attached. You can use this code to send any IR data you wish.

The data for the IR stream is formatted like this:

VALUE = (WIDTH / 25)
VALUE |= 0x8000 (ONLY if the IR LED should be on)
LAST VALUE = 0 (flags "end of table").

The value "25" comes from the IR carrier frequency (1/40000)*1e6 = 25 (microseconds)

The IR carrier frequency (ISR rate) is DESIRED_CARRIER_FREQ * 2

The reason that it's X2 is because each "on/off" pulse of the IR LED is 1/80000 on and 1/80000 off.

Looking at the code, you should have no problem understanding how it works, or how to make your own data tables to send other codes.

The Makefile is included for it (it's not an Arduino INO file, but a CPP file that you compile with AVR-GCC). Use these commands with the makefile:

make - just build/rebuild the code
make clean - erase all intermediate files and rebuild all
make all - build code, then use AVRDUDE to send it to the CPU

At the top of the Makefile are a few defines you will need to change if you want to use a different processor. All of the fuses (the 3, plus fock/unlock) are also set when programming from the Makefile.

The code, as you will see, is meant to be continuously powered and triggered by a pushbutton. After sending out the IR code, it goes back to sleep.

Any questions, please feel free.

ir.zip (5.91 KB)

Thank you!