Using cli() and sei() in a library

Hi,
I have some library code in which I need to disable interrupts for a short moment. When I try to use cli() / sei(), I get:

/Users/schwager/Documents/Arduino/libraries/AdaEncoder/AdaEncoder.cpp:290: error: 'cli' was not declared in this scope
/Users/schwager/Documents/Arduino/libraries/AdaEncoder/AdaEncoder.cpp:295: error: 'sei' was not declared in this scope

I find that I cannot include interrupt.h in my library .cpp file, either (this would be the interrupt.h from AVR library, which on Mac is in /Applications/Arduino.app/Contents/Resources/Java/hardware/tools/avr/etc/options/gcc-version/include/avr/interrupt.h).

Can anyone explain why cli() / sei() are not able to be used in my library? Is there a way I can include an AVR .h file, or anything I can do that would be more elegant that, say, copying the macros from the interrupt.h file?

Thanks.

There are arduino supplied functions you can use:

http://arduino.cc/en/Reference/NoInterrupts

http://arduino.cc/en/Reference/Interrupts

The trick is knowing when and why to use such functions.

Lefty

I find that I cannot include interrupt.h in my library .cpp file, either

You can, but, you also need to include it in the sketch.

Thanks for the replies. Can you explain how I might include it? ...Some modification to the #include directive, perhaps? As mentioned above, just doing a #include fails.

Thanks.
-GreyGnome

While it is nice that the Arduino environment provides the noInterrupts() and interrupts()
functions in attempt to provide some level of atomicity, it is not guaranteed to work.
Those two functions map directly to cli() and sei() and there as much discussion
about using simple interrupt masking for atomic operations that need protection
from interrupts over on the AVR Freaks site.

The bottom line is that method only works most of the time, it is possible that the compiler
can optimize your code and re-order things and you end up with code
that masks interrupts, unmasks interrtupts, then does your code that was supposed to be inbetween
the mask/unmask, which is not at all what you wanted.
Whether the re-ordering happens or not depends on your code so it is somewhat unpredictable.

Luckily AVR libC provides a construct to block interrupts and avoid the compiler from re-ordering things.
You will need to

#include <util/atomic.h>

Then for your actual code:

    ATOMIC_BLOCK(ATOMIC_FORCEON)
     {
        // your atomic/uninterrupted code here
     }

For more on this see the avr libc man page:
http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html

--- bill

Isn't (ATOMIC_RESTORESTATE) rather then (ATOMIC_FORCEON)
the safer type to use?

#define ATOMIC_RESTORESTATE

This is a possible parameter for ATOMIC_BLOCK. When used, it will cause the ATOMIC_BLOCK to restore the previous state of the SREG register, saved before the Global Interrupt Status flag bit was disabled. The net effect of this is to make the ATOMIC_BLOCK's contents guaranteed atomic, without changing the state of the Global Interrupt Status flag when execution of the block completes.

Vs

#define ATOMIC_FORCEON

This is a possible parameter for ATOMIC_BLOCK. When used, it will cause the ATOMIC_BLOCK to force the state of the SREG register on exit, enabling the Global Interrupt Status flag bit. This saves on flash space as the previous value of the SREG register does not need to be saved at the start of the block.

Care should be taken that ATOMIC_FORCEON is only used when it is known that interrupts are enabled before the block's execution or when the side effects of enabling global interrupts at the block's completion are known and understood.

retrolefty:
Isn't (ATOMIC_RESTORESTATE) rather then (ATOMIC_FORCEON)
the safer type to use?

For the general case yes. But if this is to protect code that is always in the foreground
with interrupts enabled, it is faster and less code to use ATOMIC_FORCEON.
(don't have to save and restore the interrupt mask which will always end with interrupts enabled anyway)
The reason I picked that one, is that the original question seemed about using
cli()/sei() which would imply masking and then unmasking interrupts around a portion of code,
so using ATOMIC_FORCEON would generate essentially the same code with the only difference
being that the it would never be re-ordered by the compiler.

But if the code needing atomicity is sometimes running with interrupts disabled and sometimes enabled,
then for sure ATOMIC_RESTORESTATE should be used to ensure that the previous state of the interrupt mask is restored.

--- bill

bperrybap:
The bottom line is that method only works most of the time, it is possible that the compiler
can optimize your code and re-order things and you end up with code
that masks interrupts, unmasks interrtupts, then does your code that was supposed to be inbetween
the mask/unmask, which is not at all what you wanted.

Wow. So you are saying that if I write this:

noInterrupts ();
a++;
Interrupts ();

The compiler might change this to:

noInterrupts ();
Interrupts ();
a++;

I would be pretty annoyed with the compiler if it re-ordered my code like that.

I can understand that the optimizer might change:

a++;
b++;

to:

b++;
a++;

Because the net effect is the same. But to change the order functions are called? Where is it documented it does this?

And if it is unsafe to just disable interrupts with cli (), then does that mean that this code in wiring.c is unsafe? And all the code like it?

unsigned long millis()
{
	unsigned long m;
	uint8_t oldSREG = SREG;

	// disable interrupts while we read timer0_millis or we might get an
	// inconsistent value (e.g. in the middle of a write to timer0_millis)
	cli();
	m = timer0_millis;
	SREG = oldSREG;

	return m;
}

interrupts is defined as:

#define interrupts() sei()

And sei is defined as:

# define sei()  __asm__ __volatile__ ("sei" ::)

Similar for noInterrupts.

That should be safe, surely? The compiler is told that the instruction is volatile. Presumably it doesn't optimize that away or change its order.

But it isn't safe.
Volatile does not prevent re-ordering of statements. It merely means don't optimize them away.
[ cli() and sei() are not functions, they are inline asm statements. ]
This topic is an ongoing discussion that occurs quite regularly on the AVR freaks site.
For a quick succinct explanation, see this recent thread.
It has all the explanation as to why the above is not safe in a fairly short thread:
http://www.avrfreaks.net/index.php?name=PNphpBB2&file=viewtopic&t=111367

Now, since so many people make the assumption that cli()/sei() will not be re-ordered,
their definition has been recently altered (after much argument over it) in more recent avr libC packages.
See the thread above for the new definition (it is near the end of the thread) and why/how
it works.
But basically it is defined as follows:

#define cli()  __asm__ __volatile__ ("cli" ::: "memory")

I don't know exactly when this modification was made, but having just looked I can say for sure
that it is not in the interrupt.h header file that ships with Arduino 0022 because the Arduino
packages are not always using the lates WinAVR.

For the linux crowd, if you are using Bingos .deb binary package, it does have the updated definition
as it contains all the latest patches.

So be very very careful when using cli()/sei() as like I said above, it is not guaranteed to work.
(I would avoid using it)
The safest thing, is to use to the atomic structure which will work on any release of the tools.

I've seen the cli()/sei() stuff in a few places in the Arduino code. But I've not really pushed
much on it because to me (and don't take this the wrong way), the Arduino environment is kind of
"Windows" like in that there are quite a few places that don't really work or only work
by accident.

--- bill

UPDATE:
The cli()/sei() changes to <avr/interrupt.h> were made in svn version 2141 on
Tue Jun 8 19:46:15 2010 UTC
which went out in WinAVR-2010-0110

The dates on all the headers shiped with Arduino 0022 are from December 2008 ... :astonished:

But I've not really pushed
much on it because to me (and don't take this the wrong way), the Arduino environment is kind of
"Windows" like in that there are quite a few places that don't really work or only work
by accident.

And how should we take it? You just slumin' here. :wink:

Now, since so many people make the assumption that cli()/sei() will not be re-ordered ...

Hmmm.

That doesn't explain whether, or not, millis () and similar functions are safe, since they don't use ATOMIC_BLOCK.


I'm still puzzled about the asssertion that the compiler "will move things around". Whilst I agree that moving things like additions around would be obviously safe, how about something like this:

push	r29
push	r28

Surely it can't reorder those without changing the effect when you later pop them?

Or:

out	0x3f, r0
nop
nop

Assuming you put the nop's in for delay purposes, the compiler couldn't usefully put them before the "out" instruction.

And in the same category, I would suggest:

cli
out 0x3f, r0
sei

Surely moving the out to outside the cli/sei pair comes into the same category? And you might ask, what optimization has been usefully performed if it did?


The change to "cli" ::: "memory" apparently tells the optimizer to not move "memory access" past this boundary. But this instruction doesn't access memory:

cli
out 0x3f, r0
sei

Hence that wouldn't make any difference.


I would have thought that the compiler would have a list of instructions that can't be reordered. Some obvious ones would be rts (you don't want to return from a subroutine before it has done everything). push/pop would be another two - since the order is usually critical. And I would suggest cli/sei would not be candidates for moving around willy nilly.

Those are so small/simple that they probably do not experience re-ordering.
But without looking at the generated code it is impossible to know for sure.
I saw some very simple examples over on avrfreaks where things broke.

There are other places in the core code and libraries that use that
use that construct as well.

I'm still puzzled about the asssertion that the compiler "will move things around". Whilst I agree that moving things like additions around would be obviously safe, how about something like this:

It isn't so much that the compiler "will move things around" but more that the compiler is allowed
to move things around because it wasn't explicitly told not to.
It is kind of like what happens spinning on a variable that was not declared volatile.
Since the compiler was not told that he was not allowed to optimize that variable
he can optimize away your intentions.

The change to "cli" ::: "memory" apparently tells the optimizer to not move "memory access" past this boundary. But this instruction doesn't access memory:


But it isn't really the cli/sei that gets moved but rather other memory accessing instructions that get
moved around that cause the cli/sei to get "moved".

cli

out 0x3f, r0
sei




Hence that wouldn't make any difference.



---



I would have thought that the compiler would have a list of instructions that can't be reordered. Some obvious ones would be rts (you don't want to return from a subroutine before it has done everything). push/pop would be another two - since the order is usually critical. And I would suggest cli/sei would not be candidates for moving around willy nilly.

This discussion crops up quite a bit over on AVR Freaks. You can do some searches for threads over
there and you will see some very lengthy thread discussions with all the gory details
and some real-world examples of code that breaks because of the reordering of instructions.
i.e. memory accesses that should be inside the cli/sei end up outside the instruction pair.

The discussions got quite spirited at times and at one point the main maintainer of AVR libC threatened
to completely remove cli() from the library. Claiming users should never use it.

In line assembler is always a bit tricky as you are essentially tossing in random instructions
into the instruction stream. It is very hard to account for everything and still allow inline assembler.

My view on all this is that the definition of cli() and sei() have been wrong all along
and should have always done the volatile, memory boundary block, or whatever it
takes to ensure no re-ordering ever happens on those macros/functions since
that is the way people expect it to work.

The positive thing is that the cli()/sei() macros have been updated so they now work
as everyone expects, so as soon as Arduino updates
their WinAVR package to the newer winAVR package, the issue becomes moot.
Or they could go in and do a tiny patch to <avr/interrupt.h> for cli()/sei() to avoid
having to update their entire toolset.

--- bill

Very interesting, thank you.

I note from this page: Extended Asm - Using the GNU Compiler Collection (GCC)

If your assembler instructions access memory in an unpredictable fashion, add `memory' to the list of clobbered registers. This will cause GCC to not keep memory values cached in registers across the assembler instruction and not optimize stores or loads to that memory.

That is not specifically saying that adding the "memory" clobbered register, that the compiler does not re-order instructions. It seems to be saying that it dumps its assumptions that the contents of registers correspond to what is in memory. To me that seems to be a different thing.

This seems relevant:

Similarly, you can't expect a sequence of volatile asm instructions to remain perfectly consecutive. If you want consecutive output, use a single asm.

So I can't see that the ATOMIC_BLOCK really does that. If I read that correctly the only really reliable way is to make up a single asm statement that does exactly what you want, without leaving asm.

The other point, made in another thread, is that dumping the assumptions that registers contain certain memory locations, hurts things that we don't want to hurt. Say that 15 registers all have nicely preloaded variables in them, and we do:

__asm__ __volatile__ ("cli" ::: "memory")

So the compiler reloads all 15 registers after the cli, which we didn't want it to do - after all clearing the interrupt mask does not change memory.

So, say we make that change. And programs call millis () - which a lot do. And millis () does cli () - which it does. Now a whole lot of optimized registers are dumped.

And even then the compiler manual does not say that using "memory" "prevents the compiler from re-ordering instructions".

I wonder if this is relevant?

The default under the Arduino IDE is compile with -Os optimization. Now the g++ manual says:

The following options are set for -O2, but are disabled under -Os: -falign-functions -falign-jumps -falign-loops -falign-labels -freorder-blocks -freorder-blocks-and-partition -fprefetch-loop-arrays -ftree-vect-loop-version

If "reorder-blocks" is the thing that might swap instruction order around (and I don't know for sure if it is) then the problem might not exist under the standard Arduino IDE compiles.

Just got back to this conversation. Fascinating. Thanks for the info, bperrybap. And for the erudite questions, Nick.