Disabling Interrupts vs Volatile declarations

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

Hi,
I'm writing code for the 2560 at present.

A common pattern the the code base I am currently using is:

uint16 get_value(void) {
   DISABLE_INTERRUPTS
   uint16 val = staticVal; //staticVal modified in IRPT
   RESTORE_INTERRUPTS
   return val;
}

where the macros are:

#define DISABLE_INTERRUPTS uint8 __oldSREG = SREG; cli();
#define RESTORE_INTERRUPTS SREG = __oldSREG;

it appears that unless staticVal is declared volatile the compiler will optimise the restoration of SREG placing it immediately after the call to cli.

comparing the resulant asm we get:

000000cc :
  cc:	8f b7       	in	r24, 0x3f	; 63
  ce:	f8 94       	cli
  d0:	8f bf       	out	0x3f, r24	; 63
  d2:	80 91 00 00 	lds	r24, 0x0000
			d4: R_AVR_16	.bss+0xa
  d6:	90 91 00 00 	lds	r25, 0x0000
			d8: R_AVR_16	.bss+0xb
  da:	08 95       	ret

versus

000000cc :
  cc:	2f b7       	in	r18, 0x3f	; 63
  ce:	f8 94       	cli
  d0:	80 91 00 00 	lds	r24, 0x0000
			d2: R_AVR_16	.bss+0xa
  d4:	90 91 00 00 	lds	r25, 0x0000
			d6: R_AVR_16	.bss+0xb
  d8:	a0 91 00 00 	lds	r26, 0x0000
			da: R_AVR_16	.bss+0xc
  dc:	b0 91 00 00 	lds	r27, 0x0000
			de: R_AVR_16	.bss+0xd
  e0:	2f bf       	out	0x3f, r18	; 63
  e2:	08 95       	ret

The outcome is that we have a non atomic read; fair enough this is what volatile is for.

Does this mean every variable accessed this way be declared volatile in addition to disabling/enabling interrupts?

Is it possible to declare the macros for DISABLE and RESTORE interrupts such that the compile will correctly bookend the code block? (there could be other impact aside from just reading shared memory if these are moved unexpectedly)

Thanks in advance for any help you can provide.

sd

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

Which compiler are you using? Most have pre-defined macros for atomic access (I know that avr-gcc does). These might work better than yours.

Regards,
Steve A.

The Board helps those that help themselves.

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

If you made those macros subroutines I expect the compiler would not be smart enough to screw it up.

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

unfortunately yes, you will need to have the variable as volatile, or otherwise force a sequence point. This is because the compiler sees the reading of the variable having no external effect, so it is free to move the access to wherever it sees fit. [even making the macros sub-routines, as suggested above, will not fix this]

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

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

volatile and atomic access are two different and separate items! Volatile forces the compiler to always read/write the variable thus bypassing any sort of optimisation the compiler might like to apply. Atomic access is something you must do in order for access to shared variables to be done as an un-interruptable operation.

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

atomic access to a multibyte variable (e.g, an int), in 8 bit micros, seems to always require disabling interrupts, if that variable is accessed by the ISR. Same for a single byte variable if its altered by the ISR.

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

any operation that is required to happen un-interrupted needs to be atomic. If the variable is shared, it must also be volatile, regardless of whether you make the access to it atomic or not.

Disabling global interrupts is the easiest way to make access to a variable atomic. However it is not the only way. One could also disable only the pertinent interrupt sources, such that the value will not get altered, but other interrupts are still allowed to execute.

Also note that atomic access is not just for multi-byte variables, it also applies to 8 bit variables where a read-modify-write operation is being performed, or any other sequence of operations that must be performed un-interrupted, like timed sequences.

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

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

If you are using AVR-GCC, then use the atomic block macros in - they are bullet proof and take care of restoring SREG no matter how you exit the block.

- Dean :twisted:

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

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

Thanks for all the pointers.

I now have plenty to try.

I tried making my macros subroutines but this failed to improve matters.

We're currently stuck with an old version of avr-gcc pre (for reasons that I won't go into at the moment). I think fixing this would probably be the best way forward.
Just copying the file in has not worked for me as it appears to need c99 mode enabled and enabling this conflicts with some unnamed unions in our code.

Thanks again,
sd

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

As I said, making the macros into sub-routines is not going to fix the problem. You NEED to declare the variable as volatile, otherwise the compiler optimizes the access away until the last possible moment.

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

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

servodude wrote:
Just copying the file in has not worked for me as it appears to need c99 mode enabled and enabling this conflicts with some unnamed unions in our code.
Try gnu99 instead of c99.

Stefan Ernst

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
#define MEM_BARRIER asm volatile ("" ::: "memory")

uint16_t myvar;


uint16_t test( void )
{
  uint16_t val;
  cli();
  val = myvar;
  MEM_BARRIER;
  sei();
  return val;
}

Peter

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

Peter, you don't want to use sei() in this instance, you want to return the value of the global interrupt flag to what it was to start with since it may not have been enabled in the first place.

Regards,
Steve A.

The Board helps those that help themselves.

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

servodude wrote:
We're currently stuck with an old version of avr-gcc pre (for reasons that I won't go into at the moment).
The macros defined in should be backward compatible as they do not require any special compiler or library support. Install the latest WinAVR and copy the atomic.h file to your local directory, then compile with the old compiler.

Stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

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

stu_san wrote:
The macros defined in should be backward compatible as they do not require any special compiler or library support.
I disagree. The macros ensure a proper restoring of the sreg-int-flag no matter what exit path is taken (you can even use 'return' within the atomic block). This is realized by using the cleanup attribute. So the macros require special compiler support after all.

Stefan Ernst

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

Koshchi wrote:
Peter, you don't want to use sei() in this instance

Why not?

It works on all my sources perfectly. :)

The same can be achieved on the new AVR-GCC with:

ATOMIC_BLOCK(ATOMIC_FORCEON){
// ..
}

Typically I want to handle interrupts as fast as possible and thus interrupts should always be enabled, immediately after the atomic statement was finished.

Nevertheless this was not the point.
The point was not reordering the code and this was, what I wanted to demonstrate. :wink:

Peter

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

Quote:
Typically I want to handle interrupts as fast as possible and thus interrupts should always be enabled, immediately after the atomic statement was finished.

But only if they were enabled before the atomic statement.

Regards,
Steve A.

The Board helps those that help themselves.

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

Koshchi wrote:
But only if they were enabled before the atomic statement.

If not enabled, then the atomic statement was unneeded.

To restore the interrupt enable state was only needed in such rarely cases, if a function was called from both: interrupt and main. :!:

Since I avoid calling subroutines from an interrupt, the interrupt enable state (enabled) was always known in the whole program.
No need to waste code to save a known state. :wink:

Peter

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

Quote:

If not enabled, then the atomic statement was unneeded.


Not necessarily true, as a utility routine could be used when interrupts are enabled >>or<< disabled.

Quote:

Since I avoid calling subroutines from an interrupt, the interrupt enable state (enabled) was always known in the whole program.

True enough, and then you don't run into it. A sneaky example might be EEPROM access where you don't really think about "calling a subroutine" or an expanded macro of code. [LOL--I also avoid doing EEPROM reads in ISRs 'cause I don't want to be stuck there for several milliseconds if a write is in progress.]

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

danni wrote:

#define MEM_BARRIER asm volatile ("" ::: "memory")

uint16_t myvar;


uint16_t test( void )
{
  uint16_t val;
  cli();
  val = myvar;
  MEM_BARRIER;
  sei();
  return val;
}

Peter

This is not safe without a memory barrier above the variable read.