handling SREG to control interrupts: any gotcha's?

Go To Last Post
18 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have a program that does serial communications. I use generic circular buffers for incoming and outgoing data. The buffers are accessed by the same code which is called by both the main program and from inside the serial interrupt handlers

I was getting glitches that I traced down to non-atomic access of the buffers by the main program.

It would be easy to stick a cli() and an sei() at the beginning and end of the buffer access routines. But that would enable interrupts in the middle of the serial interrupt handler. Not good.

So I save the interrupt status by saving (uint8_t int_reg = SREG) the entire SREG, just before executing the cli(); When I want to "restore" interrupts, I write the stored value back to SREG.

So, I believe this will work, and as far as I can tell, none of the other bits in SREG are going to be changed between the time I save and restore the SREG value. But are there any gotcha's?

-Tony

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This is a fairly standard technique so you shouldn't have any problems with it.

Cliff

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Or, at the cost of one extra clock (depending on your compiler), you can be immune:

if (saved_sreg & (1 << SREG_I)) /* SBRC Rx,7 */
    sei();                      /* SEI       */

(Yes, I'm Old and Superstitious (:-)).)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

mckenney wrote:
Or, at the cost of one extra clock (depending on your compiler), you can be immune:

if (saved_sreg & (1 << SREG_I)) /* SBRC Rx,7 */
    sei();                      /* SEI       */

(Yes, I'm Old and Superstitious (:-)).)

This seems like a good technique just to be sure that the other bits are preserved in case they are important.

Thanks.

-Tony

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

An alternative would be to use the new atomic block macros that I made for AVRLibC:

#ifndef ATOMIC_H
#define ATOMIC_H

	#include 
	#include 

	static __inline__ uint8_t __iSeiRetVal(void)               { sei(); return 1; }   
	static __inline__ uint8_t __iCliRetVal(void)               { cli(); return 1; }   
	static __inline__ void    __iSeiParam(const uint8_t *__s)  { sei(); __asm__ volatile ("" ::: "memory"); (void)__s; }
	static __inline__ void    __iCliParam(const uint8_t *__s)  { cli(); __asm__ volatile ("" ::: "memory"); (void)__s; }
	static __inline__ void    __iRestore(const  uint8_t *__s)  { SREG = *__s; __asm__ volatile ("" ::: "memory"); }

	#define ATOMIC_BLOCK(exitmode)     for ( exitmode, __ToDo = __iCliRetVal(); __ToDo ; __ToDo = 0 )

	#define ATOMIC_RESTORESTATE        uint8_t sreg_save __attribute__((__cleanup__(__iRestore)))  = SREG
	#define ATOMIC_FORCEON             uint8_t sreg_save __attribute__((__cleanup__(__iSeiParam))) = 0
	
	#define NON_ATOMIC_BLOCK(type)     for ( type, __ToDo = __iSeiRetVal(); __ToDo ; __ToDo = 0 )
	
	#define NONATOMIC_RESTORESTATE     uint8_t sreg_save __attribute__((__cleanup__(__iRestore)))  = SREG
	#define NONATOMIC_FORCEOFF         uint8_t sreg_save __attribute__((__cleanup__(__iCliParam))) = 0

#endif

Those are completely foolproof; no matter how the block exits, the SREG will be restored. Use like:

ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
{
  // Atomic code here
}

That will save SREG at the start of the block, and restore it after any exit return path. Requires compilation in GNU99 standards mode, and at optimization levels other than -O0 it will compile down to the optimal assembly.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I guess I'm just a geezer bit jockey, too ancient to learn new-fangled tricks. Since I know my AVR apps keep interrupts enabled after startup, I just do

#asm("CLI");
		rx_msg0--;
#asm("SEI");

when I need to fetch/modify sensitive items.

I can see the utility in generic or shared code that might be hidden in a library or invoked from ISRs. So good work, Dean & collaborators.

Lee

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Lee,

Yes, that works just fine for simple atomic operations. However, as discussed in the other recent atomic operations thread, complex blocks cause problems:

cli();
DoSomething();
SomeVar++;
if (SomeOtherVar == 0x12)
  return;
AnotherVar--;
sei();

That would cause some unintended opeations; namely if "SomeOtherVar" is 0x12, "AnotherVar" is not decremented - but interrupts are also not re-enabled afterwards. Using a macro such as mine guards against all this, as it provides a foolproof mechanism (for GCC, at least) to guard against these sorts of subtle bugs.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

...complex blocks cause problems: ...

Yes, they certainly do.

"Doctor, doctor, my arm hurts when I do this!"
"Well, then don't do that anymore."

What does that other regular poster have for his tag line: "Keep it simple and it won't bite as hard." Something like that. Yes, sometimes I'd like to do a conditional thingie with a piece of data that needs to be protected. But I just bite the bullet and fetch it to a temporary (which is usually a register variable anyway so it leats to quick repeated accesses).

I just have never run into it in practice.

Lee

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Certainly it's (almost?) always possible to program around such limitations, and it's probably going to end up being very nearly identical in optimality.

But it's also nice to have a magic pill yu can use to protect yourself from having to program around it in the first place.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
An alternative would be to use the new atomic block macros that I made for AVRLibC:

This is great. Thank you. I _do_ have a complex routine and I had to be careful with enabling interrupts in various exit mechanisms.

I have to carefully inspect your code to understand what it is doing. Your explanation of its net effects and how to use it.

-Tony

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

No problem - eventually that should be included in AVRLibC.

For an explanation of how it works, see my posts in this thread. One of my posts in the same thread also explains the different parameters for the macro.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:
No problem - eventually that should be included in AVRLibC.

For an explanation of how it works, see my posts in this thread. One of my posts in the same thread also explains the different parameters for the macro.

- Dean :twisted:

Dean, thanks for the link. That discussion has helped me begin to understand how it works. I never know of the "cleanup" capability. I assume other compilers do not have it?

I think I will try to use your macros if I have other critical sections. Otherwise I will stay with that I have written manually.

My understanding of your work is at that low-middle level. I mostly understand what you have done, but I can not reproduce it. Mostly I need to learn more about macros!

(This and more #ifdef syntax to include OR capability.)

-Tony

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If the receive interrupt is the only routine putting chars into the receive buffer which are retrieved by the main line code and vice versa for the transmit buffer, you can code it without disabling interrupts, you just have to make sure the compiler doesn't optimize the code incorrectly.

Jeff

Jeff Dombach, JLD Systems
"We do the stuff behind the buttons!"
Your source for embedded solutions with a 100% Guarantee.
http://www.jldsystems.com
Phone 717.892.1100

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The "cleanup" attribute is, AFAIK, a GNU99 extension. It associates a special function with any local automatic variable, so that function will always run whenever that variable goes out of scope. I guess it's somewhat akin to an object destructor in C++.

AFAIK, it's available on any target of the GCC compiler, but I wouldn't expect to see it under any other brand of compiler.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I should have mentioned that - my macros are GCC-only, and require compilation under the GNU99 standards mode. Aside from the cleanup attribute, the "memory" ASM statement (suggested by Joerg to prevent GCC from re-ordering the contents) is also GCC only.

Having said that, I think they're damn useful and I'm going to fight hard to get both it and my better GCC Interrupt macro into AVRLibC.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi Dean, great work!

 

I see you got your atomic.h included into AVR-Libc. Nice work!

-How'd you go about doing that?

 

Ok for my real question, on your 8 year old thread:

 

I've traced your macros down to see how they work, but what about the for loop?

 

for ( exitmode, __ToDo = __iCliRetVal(); __ToDo ; __ToDo = 0 )

What type does __ToDo have? 

 

For example, it seems to me that

 

ATOMIC_BLOCK(ATOMIC_RESTORESTATE)

breaks down to 

 

for (uint8_t sreg_save = SREG, __ToDo = 1; __ToDo ; __ToDo = 0)
  //for loop scope ends
SREG = *pointer_to_sreg_save;
__asm__ volatile ("" ::: "memory");

Yet I can't quite wrap my head around the __ToDo.... Is it a macro? A variable? Am I missing something about how for loops can be initialized?

 

==================================================================

 

Explanation of breakdown:

 

Place ATOMIC_RESTORESTATE into ATOMIC_BLOCK, like this:

ATOMIC_BLOCK(ATOMIC_RESTORESTATE)

 

and you get: 

ATOMIC_BLOCK(uint8_t sreg_save __attribute__((__cleanup__(__iRestore))) = SREG)

 

Now, expand the ATOMIC_BLOCK macro and you get:

for (uint8_t sreg_save __attribute__((__cleanup__(__iRestore))) = SREG, __ToDo = __iCliRetVal(); __ToDo ; __ToDo = 0)

 

Apply the cleanup __attribute__ and you get a call to the __iRestore cleanup function when sreg_save goes out of scope, as follows:

for (uint8_t sreg_save = SREG, __ToDo = __iCliRetVal(); __ToDo ; __ToDo = 0)
  //for loop scope ends
SREG = *pointer_to_sreg_save;
__asm__ volatile ("" ::: "memory");

 

__iCliRetVal() disables interrupts (via cli()) and returns 1, so you get:

for (uint8_t sreg_save = SREG, __ToDo = 1; __ToDo ; __ToDo = 0)
  //for loop scope ends
SREG = *pointer_to_sreg_save;
__asm__ volatile ("" ::: "memory");

 

UPDATE:

I got caught up on the simplest piece of the whole thing perhaps (probably because I'm not used to ever seeing more than 1 variable declared in a for loop initializer). There's a comma there, meaning __ToDo is a variable of type uint8_t. Solved, right?

Last Edited: Fri. Sep 25, 2015 - 05:07 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

What type does __ToDo have? 

uint8_t. I just built:

$ cat avr.c
#include <avr/io.h>
#include <util/atomic.h>

int n;

int main(void) {
	volatile int x;
	while(1) {
		ATOMIC_BLOCK(ATOMIC_FORCEON) {
			x = n;
		}
	}
}
$ avr-gcc -std=gnu99 -mmcu=atmega168 -save-temps -Os avr.c -o avr.elf
$ cat avr.i | tail -n 10
int n;

int main(void) {
 volatile int x;
 while(1) {
  for ( uint8_t sreg_save __attribute__((__cleanup__(__iSeiParam))) = 0, __ToDo = __iCliRetVal(); __ToDo ; __ToDo = 0 ) {
   x = n;
  }
 }
}

It's not really any different to:

uint8_t foo, bar;

both of those are uint8_t too.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

What type does __ToDo have? 

uint8_t. 

 

It's not really any different to:

uint8_t foo, bar;

both of those are uint8_t too.

 

Thanks! Got it. It just looked weird to me in the for loop I suppose. I wasn't aware you could declare or initialize multiple variables in the for loop declaration itself.