Has the volatile for ISR/main variables thingie been fixed?

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

Today, I tried something new. I've got this interrupt:

in usart.c:

unsigned char led;

ISR(USART_RXC_vect)//interrupt vector for the PC usart
{
	unsigned char a;
	a=usart_getchar();
	led = a;
	UDR = ++a;
	return;
}


in usart.h:
extern unsigned char led;

and this main() code:

int main(void)
{
	io_init();
	turn_LED_off();
	while(1)
	{
		if(led == 'a') turn_LED_off();
		if(led == 'b') turn_LED_on();
		wait(5000);
	}
}

And it works properly.

No volatile.

Although according to cliffs signature:

It MUST be defined volatile. Till this day, i took it without questioning, but, why? Are there any risks in this in this approach? Seems to work properly.

There are pointy haired bald people.
Time flies when you have a bad prescaler selected.

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

It works fine in that instance, but without the volatile modifier the compiler is free to optimize out the loading of the variable contents for each loop pass, and instead load value into a register once and keep using the cached value.

In short, the "volatile" problem can't be fixed, because it's not broken. You should still declare your shared variables volatile, otherwise the compiler might restructure things and you'll be wondering why your previously working code is now broken.

- 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:
In short, the "volatile" problem can't be fixed, because it's not broken. You should still declare your shared variables volatile, otherwise the compiler might restructure things and you'll be wondering why your previously working code is now broken.
Dean, I was employeed as a professional embedded software engineer for 35-years. I worked mostly with the C-language the last 20-years of my career. I lost count of the number of times I tried to get this point across to experienced coworkers who just didn't get it.

I'm impressed with your summary of "the problem". Keep up the good work.

Don

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

Cheers Don!

It took a little while to wrap my head around it when I first saw it, however after seeing the examples of the compiler optimizing out the reloading (which it's perfectly entitled to do, and which gives great performance boosts) of the shared variable it all just "clicked".

- Dean :twisted:

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

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

also if you longjmp somewhere, if the variables aren't volatile they might be lost

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

unsigned char led = 0;

int main(void) 
{ 
   unsigned char led2;

   led2 = led;
   io_init(); 
   turn_LED_off(); 
   while(1) 
   { 
      if(led2 == 'a') turn_LED_off(); 
      if(led2 == 'b') turn_LED_on(); 
      wait(5000); 
   } 
} 

ISR(USART_RXC_vect)//interrupt vector for the PC usart 
{ 
   unsigned char a; 
   a=usart_getchar(); 
   led = a; 
   UDR = ++a; 
   return; 
} 


So this may not work as expected then (I havn't tried

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

That's unlikely to work as you expect at all, since you only read "led" once, before you enter the main loop.

Beyond that, even if you read "led" in the main loop, all bet's are off, since "led" is not declared as volatile. It may work with optimizations turned off, but once you let the compiler start optimizing, it's an near certainty that it will break.

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

Actually what you'l get (with -Os) is:

000000bc 
: bc: ff cf rjmp .-2 ; 0xbc

So what you've got yourself there is a completely "do nothing" program.

BTW that was stubbing all the routines you didn't bother to provide:

void io_init(void) {};
void turn_LED_off(void) {};
void turn_LED_on(void) {};
unsigned char usart_getchar(void) {};
void wait(int n) {};

But there's loads wrong with that program. You only ever load 'led2' from 'led' outside the while(1) so what's the point of testing it for 'a' or 'b' - it never gets a chance to be changed. In fact I don't see what the point of 'led2' is at all?

You also call uart_getchar() from within the ISR(). This is a "Very Bad Idea"(tm). As soon as you call a function in an ISR you lead to an almost complete state save/restore.

You also load the character to transmit into UDr without bothering to check whether it's ready to accept a character - not a great idea.

Oh and if, by any chance the body of 'wait()' looks anything like:

void wait(int n) {
  for (i=0; i

then it simply won't wait at all (because it'll be optimised away which is exactly why util/delay.h exists!)

But otherwise it seems fine ;)

Cliff

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

ooops

//#defines led_on...
//...
//...

unsigned char led = 0; 

int main(void) 
{ 
   unsigned char led2; 

   //initialize()....

   io_init(); 
   turn_LED_off(); 
   while(1) 
   { 
      led2 = led; 
      if(led2 == 'a') turn_LED_off(); 
      if(led2 == 'b') turn_LED_on(); 
      wait(5000); 
   } 
} 

ISR(USART_RXC_vect)//interrupt vector for the PC usart 
{ 
   unsigned char a; 
   a=usart_getchar(); 
   led = a; 
   UDR = ++a; 
   return; 
} 

I meant to have that within the main loop.

So the compiler would only load led2 with the led value once, so in fact, it would look as if it were outside the main loop. Correct?

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

what about this:

volatile unsigned int x = 0x00FF; 

int main(void) 
{ 
   unsigned int a; 

   //initialize().... 

   while(1)
   { 
      a=x; 
   } 
} 

ISR(timer)
{ 
   x++; 
} 

Does the compiler place cli and sei around the transfer of contents of x to a. Because this takes multiple instructions or does the programmer have to take care of this?

eg.
1. copy high byte of x to a (a = 0x0000)
2. interrupt occurs
3. x++; (x = 0x0100 now)
4. return from interrupt
5. copy low byte of x to a (a = 0x0000)

Now a does not reflect the true value of x.

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

robinsm wrote:

Does the compiler place cli and sei around the transfer of contents of x to a. Because this takes multiple instructions or does the programmer have to take care of this?

eg.
1. copy high byte of x to a (a = 0x0000)
2. interrupt occurs
3. x++; (x = 0x0100 now)
4. return from interrupt
5. copy low byte of x to a (a = 0x0000)

Now a does not reflect the true value of x.

you as the programmer need to protect the read... the compiler does not know if interrupts are enabled or not, and will not add code to make a 16bit read atomic.

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

robinsm wrote:

I meant to have that within the main loop.

So the compiler would only load led2 with the led value once, so in fact, it would look as if it were outside the main loop. Correct?

possibly, yes... the actual result will depend on optimization, and what other code was in the loop.

without volatile, the reads might get optimized away. adding volatile guarantees that reads won't get optimized away.

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

cliff: I now have a fairly big project (5k of 8k prog. memory used), the global variables are all over the place (ISR/main, ISR/ISR... etc. variables), of all types and forms and sizes, and I must say, that I've got nothing volatile, and the system works perfectly.

But to be on the safe side, I'll do a volatization of the thing... Are there any collected examples of when the compiler messes up?

BTW: What's wrong with calling an usart_getchar() from the within ISR?

unsigned char usart_getchar(void) 	//wait for the char to be received and then return it
{
	loop_until_bit_is_set(UCSRA,RXC);
	return UDR;
}

If I'm not supposed to call it then, when?

There are pointy haired bald people.
Time flies when you have a bad prescaler selected.

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

David,

loop_until_bit_is_set() is not a function call, it's a macro. That's fine within an ISR as it's no different to you typing a few lines of C.

If, however, you had a call to a real function such as uart_getchar() in your ISR (and the compiler didn't inline it) then you'd find that about 16 additional PUSH instructions were added to the prologue and 16 POPs to the epilogue - that's why you don't want to call a function.

BTW I'd highly recommend always looking at the generated code in either the .s files, the .lss file or the debugger to see when something dire like this has happened.

Cliff

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

daqq wrote:
cliff: I now have a fairly big project (5k of 8k prog. memory used), the global variables are all over the place (ISR/main, ISR/ISR... etc. variables), of all types and forms and sizes, and I must say, that I've got nothing volatile, and the system works perfectly.

Then you've gotten lucky so far, or perhaps you have not enabled any optimization.

daqq wrote:
But to be on the safe side, I'll do a volatization of the thing... Are there any collected examples of when the compiler messes up?

The compiler does not mess up. It's you the programmer that messes up.

The compielr understands code to be "synchronous", meaning that it knows each read & write to a variable. And can therefore eliminates reads and writes to the actual memory location to make things faster. Instead of reading the RAM location each and every time, it reads once into a local copy, manipulates that local copy, and then eventually writes it back out. This can be much faster than reading from memory, and writing to memory each time the variable is accessed. This type of optimization is perfectly fine with a normal program, since the code executes linerally.

The problem is that a program with interrupts executes some code ASYNCHRONOUS to the rest of the program. The compiler has no way of knowing when an interrupt executes, or when an interrupt routine may modify a value. As such you use the volatile keyword to tell the compiler not to optimize away any reads, to guarantee that you always have the most recent value of the variable.

daqq wrote:

BTW: What's wrong with calling an usart_getchar() from the within ISR?

unsigned char usart_getchar(void) 	//wait for the char to be received and then return it
{
	loop_until_bit_is_set(UCSRA,RXC);
	return UDR;
}

If I'm not supposed to call it then, when?

well the interrupt gets called when the flag is set, so the wait loop is redundant. Even worse would be a situation where you enter the ISR, and the flag gets cleared before your wait loop, so now you're stuck in the ISR (triggered by a flag) waiting for the flag to get set again, before allowing the rest of your program to resume execution.

Instead of calling usart_getchar() simply read UDR in your ISR directly.

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

Daqq's example will not, might, or will work, depending on what's not included. The following is not something to depend upon, merely to detail why it sometimes works, sometimes doesn't:

If, inside the loop, there are no function calls (the three functions are actually inlined, or macros), then even a basic optimizer will move the load of led outside the loop.

If there is a function call to a function in the same compilation, then a more advanced optimizer may choose to inline the function, or look into the called function and determine that led does not get modified. Then the load of led ends up outside the loop. To trigger this scenario with gcc, declare the functions static and use -O3.

If there is a call to a function that is outside of the compilation unit, and the led variable is an exported symbol (not static), then the compiler is forced to assume that the function reads and writes led, and it will work (a reload will happen sometime after the function call) - until you encounter a really smart 'linker'.

The lack of volatile does not only affect loading - also storing. Suppose, in daqq's interrupt, he's doing something to a shared resource. So he makes up a flag to temporarily disable that particular operations in the timer interrupt:

unsigned char flag; //warning - should be volatile.

IRQ(timer_42)
{
  if (!flag) {
    bla1;
  }
}

void configure_resource(void)
{
  flag=1;
  bla2;
  flag=0;
}

In this case the compiler is free to reorder the contents of configure_resource, and ends up with just flag=0. That is, bla2 will get executed, flag will never get set (flag=1, flag=0, hey, the first is superflous, poof), and sometimes bla1 will get executed while doing bla2.

And the question is: volatile access is defined as having side effects. This might mean that the compiler can't move modification of non-volatile variables bla2 does outside the flag assignments, or it does not. I don't know which it is, I suspect it can. It won't matter when hardware registers and async variables are also volatile though...

This has been sitting in the editor for a while and might be out of sequence :-)

/Kasper

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

glitch wrote:

daqq wrote: I now have a fairly big project [...] I've got nothing volatile, and the system works perfectly.

[...] perhaps you have not enabled any optimization.


Which might be the effect of setting up a avr-gcc project in AVR Studio and not change the -O0 compiler option that Studio defaults to...

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

He's alive! (we now have proof that a man with a laptop can run faster than a man with a bucket of cement).

Quote:
Which might be the effect of setting up a avr-gcc project in AVR Studio and not change the -O0 compiler option that Studio defaults to...
Now don't go screwing up his perfectly running, twice as large as needed application, by telling him about the secret codes!

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

Quote:
Which might be the effect of setting up a avr-gcc project in AVR Studio and not change the -O0 compiler option that Studio defaults to...
I've got -O2 enabled, so that's not the problem.

Quote:
Now don't go screwing up his perfectly running, twice as large as needed application, by telling him about the secret codes!
Not nice! ;-) O2 does a pretty good job.
Trying sizes:
O0 - over 8K
O1 - 4762B
O2 - 4752B
O3 - 4924B
Os - 4638B

There are pointy haired bald people.
Time flies when you have a bad prescaler selected.