0

Here is my c code

char global_variable = 0;
ISR(){
    PORTA = global_variable;
    toggle_led;//to make sure that the interrupt is triggered
}
int main(){
    while(1){
        _delay_ms(500);
        gobal_variable++;
        PORTB = global_variable;
    }
    return 0;
}

The bottom line is that I have a global variable modified by the main function and read by both the main and ISR -interrupt handler.
When the global variable is read by main I get the expected values, but in ISR I get the value that was first assigned to the global variable.
I know this is an optimization issue but I don't understand what makes the compiler see the right value in the main and the initial value in the ISR

Note: when I modified the variable in the ISR, I read it right in the ISR but in the main I got the initial value.

Abdo Saied Anwar
  • 111
  • 2
  • 10
  • 1
    doesn't volatile help? – 4pie0 Aug 29 '15 at 12:44
  • Which CPU are you working on? – too honest for this site Aug 29 '15 at 12:54
  • From the embedded tag usage guidance: "Apart from the embedded tag, also tag your question with a specific development platform, tool chain, and/or target platform. Relevant tags are the specific compiler, specific processor, specific evaluation boards etc. that are used. Avoid using manufacturer tags such as for example microchip as they generally add nothing of value. Instead, specify the microcontroller being used, for example pic18." – Lundin Sep 03 '15 at 06:55
  • but I am not allowed to use more than 5 tags and I see that these tags are more relevant than the micro-controller as this is dependent on the compiler not the micro controller I am using – Abdo Saied Anwar Sep 04 '15 at 09:13

2 Answers2

2

In this case you should insert a memory barrier to assert all writes will be completed before you read. In user space you should declare this variable as volatile.

volatile char global_variable = 0;
4pie0
  • 29,204
  • 9
  • 82
  • 118
  • A barrier is not required here, `volatile` is sufficient. – too honest for this site Aug 29 '15 at 12:56
  • @Olaf in the kernel you would insert memory barrier. volatile is not necessary and often is inapropriate, because even spin_lock can assert memory fetch and stores are made properly before section is entered – 4pie0 Aug 29 '15 at 13:02
  • `volatile` is very well necessary. Otherwise the compiler can use constant `0` in `ISR` and need not write anything to the var in `main`. But please explain why the barrier would be required. – too honest for this site Aug 29 '15 at 13:04
  • No, not if memory barrier is inserted. Call to barrier() tells the compiler to insert a memory barrier but has no effect on the hardware. Compiled code stores to memory all values that are currently modified and resident in CPU registers, and rereads them later when they are needed. A call to barrier prevents compiler optimizations across the barrier but leaves the hardware free to do its own reordering. – 4pie0 Aug 29 '15 at 13:05
  • The barrier only tells the CPU to finish all memory accesses (and possibly the compiler not to optimize across it), but not the compiler the side-effect of the global variable. – too honest for this site Aug 29 '15 at 13:06
  • Any reads & writes (optionally) must complete when barrier is inserted. Memory barriers also ensure that all pending reads/writes are executed when the barrier is reached, so it effectively gives us everything we need by itself, making volatile unnecessary. We can just remove the volatile qualifier entirely. – 4pie0 Aug 29 '15 at 13:14
  • Yes, you already said that (and that's what a barrier is for - note, however, there is no `barrier()` function in the C standard) but where does the barrier imply the compiler must not optimize away acceses it can prove irrelevant for control-flow? And here the barrier is irrelevant, as the accesses to `volatile` have to be ordered by the compiler, too). – too honest for this site Aug 29 '15 at 13:22
  • "The problem ... multithreaded context ... can't rely on volatile **alone**." Didn't you say "volatile is **not** necessary" (both emphasis by me). Note that this only applies to multi-CPU systems with non-snooped memory. As OP did not tag with an OS, this is very likely bare-metal, and single-CPU. So no need to update another bus-master. – too honest for this site Aug 29 '15 at 13:28
  • Ok, let's say you set barriers: where **exactly** would you insert them? And why at this place? – too honest for this site Aug 29 '15 at 13:29
  • There is no async read/write, as there is only one write, which has to happen after the read, because the value is used for the increment. – too honest for this site Aug 29 '15 at 13:31
  • Oh, btw. This looks like PIC-MCU. Sorry, no barriers available. – too honest for this site Aug 29 '15 at 13:34
  • Yes, and put a full blown Linux on an MCU with possibly just 1Ki Flash and 64 bytes RAM. LOL. You never worked bare-metal, right? I added some code to my answer. Perhaps with that you notice that there is no need for a barrier. You forgot there is only one read/writer and only one reader which is async anyway. – too honest for this site Aug 29 '15 at 13:37
  • We don't bode here but refer to what has been asked. – 4pie0 Aug 29 '15 at 13:40
  • I did and seem to be fairly correct, according to OP's comments. Ok, I assumed PIC wrongly (it's actually a small AVR), but what I said stands. – too honest for this site Aug 29 '15 at 13:45
  • Questions tagged embedded are most often bare metal, single-core MCU systems. Always assume this unless the question says otherwise. – Lundin Sep 03 '15 at 06:54
0

ISR has no correct declaration. You really should get used to use prototype-style declarations. Compile with C99 or C11 and you will get a warning. Similar for main:

void ISR(void)

int main(void)

Whereas the signature of main depends on your environment, assuming you are on a bare-metal embedded syetem, i.e. a freestanding environment.

Depending on the target, you have to att compiler-specific attributes to tag the function as interrupt handler.

Said that, you are missing to declare global_variable volatile. You should know, as you already added the tag.

The compiler cannot know the ISR is called anyway and that the variable is modified outside its control-flow. So it can presume it to have the default value, which is 0. Using the volatile qualifier tells it exactly that it cannot and the variable is in fact modified outside.

Note that due to all this, the compiler must not optimize acceses to volatile objects. Thus, you should limit accesses to such objects to the minimum. In main, it is better to use a helper variable to count and just write the updated value once to the volatile object and the counter otherwise. This way you avoid one read and one write:

volatile unsigned char global_variable;

...

int main(void)
{
    unsigned char counter;
    while ( 1 ) {
        _delay_ms(500);
        gobal_variable = counter++;
        PORTB = counter;
    }
    return 0;
}

Notice that I changed the type to unsigned char this is vital, as char can be signed or unsigned and signed integer overflow invokes undefined behaviour in C. Unsigned overflow is defined to simply wrap (i.e.: MAX+1 == 0). A better datatype to use would be uint8_t since C99 (strongly recommended) to explicitly state you are using an 8 bit variable (char does not guarantee this).

Note: According to your comment below, you use an AVR MCU. This is single-core and does not even support memory barriers. So there is absolutely no need for such. Also, as you have a single writer and the writes are atomic (i.e. all-or-nothing of the variable is updated), there is also no need for more complex synchronisation.

However, if you increase the size of the counter, you have to take provisions in ISR or main to ensure reading the value consistently. This because the AVR is an 8 bit machine, thus the update and reads are not atomic.

Note: Due to popular demand, you should check your target if it actually performs the writes atomic. This is also true for 8 bit values. If you are not sure, check the generated assembler code. However, for the AVR, PIC, MSP430, ARM-Cortex-M (iff the busses and registers support byte-writes), the code above is safe unless you use DMA on one of the variables.

too honest for this site
  • 12,050
  • 4
  • 30
  • 52
  • yes, I know that volatile does solve the problem. but I was trying to understand how and why the compiler read two different values for the same variable. but according to your answer, the compiler deals with the variable in ISR as a constant with the initial value of the variable. – Abdo Saied Anwar Aug 29 '15 at 13:24
  • As for the last note in your answer, do you meen to do something like this `char c;` `c++;` `global_variable = c;` If that is what you mean, I don't see the advantage – Abdo Saied Anwar Aug 29 '15 at 13:28
  • @AbdoSaiedAnwar: I updated by answer. That way, perpahs the supportes of barriers evrywhere might see why they are not needed for your code. (I assume that is a PIC; am I correct?) – too honest for this site Aug 29 '15 at 13:36
  • No, I am using AVR - Atmega16 – Abdo Saied Anwar Aug 29 '15 at 13:38
  • Crapp, I always confuse them- Sorry, but all I wrote still applies to the AVR, too. – too honest for this site Aug 29 '15 at 13:41
  • still I can't see the advantage of making the local variable counter. I also need to assign the value of the variable to PORTC in ISR – Abdo Saied Anwar Aug 29 '15 at 15:16
  • `volatile` inhibits optimizations. Use them sparsely and keep accesses at a minimum. In general, load them once to a local var and use that whereever possible. Note that `volatile` vars are normally not held/cached in CPU registers. – too honest for this site Aug 29 '15 at 15:19
  • Keep in mind that there's no guarantee that the writes to the variable are actually atomic. It seems likely that they are, but the code could as well get translated to read-modify-write. To ensure that the writes are atomic, you need to disassemble the program and check. – Lundin Sep 03 '15 at 06:45
  • @Lundin: That's why I added the last paragraph. 8 bit accesses are atomic on any platform with `CHAR_BIT ==8`. The incrementation of the counter need not be atomic (just the write). – too honest for this site Sep 03 '15 at 10:57
  • The size of the data bus guarantees nothing. Any access to a C variable could get interpreted as 3 assembler instructions, read-modify-write: "put stack variable in register A" then "modify register A" and then "put register A back on the stack". If an interrupt hit between any of those instructions, any modification of the variable done by the ISR will get discarded. In this specific case, the ISR just reads the variable, so it shouldn't be an issue here. – Lundin Sep 03 '15 at 11:29
  • @Lundin: Thinking of the first Alphas, you are basically right. (But, do you know any architecture with an 8 bit bus which uses RMW to write an 8 bit variable?) Nevertheless, the AVR definitively writes 8 bit fields atomically and RMW **instructions**, too - unless Atmel developed multicore AVRs. – too honest for this site Sep 03 '15 at 11:39
  • You'll get issues with this on any microcontroller when you use C code such as `PORTA |= MASK` rather than `PORTA = MASK`. (Unrelated, this is also a very common bug when it comes to clearing on interrupt flags). Which is why disassembling is important. – Lundin Sep 03 '15 at 11:48
  • I'm well aware of this and take approriate measures (I'm not new to this business). You might notice I added another note. mspgcc for instance had a corresponding note using the expected instructions. Note that I did not mention RMW anyway, but writes. A single answer cannot cover all aspects of embedded programming. And while I also disassemble questionable code, this is no guarantee the same pattern will be used everywhere. So better you know your platform and tools, too. – too honest for this site Sep 03 '15 at 12:00