[avr-gcc-list] Re: memcpy() : problem when passing destination pointer

[David Brown]

Bernard Fouché wrote:
> Hi Dave.
> > In both of these cases, "volatile" is insufficient because it does not 
> > imply "atomic".  In the first case, you need an atomic 
> > read-modify-write, and in the second you need an atomic read of a wide 
> > value.  In both cases you can achieve that by enclosing the the 
> > variable access in a cli-sei pair.  For example, the latter case can 
> > be fixed by changing the routine to something like
> >  
> > main()  // or "int main(void)", yada yada
> > {
> >    uint32_t ms;   // does not need to be volatile
> >  
> >    do{ cli(); ms = Milliseconds; sei(); } while (ms < SOME_LIMIT);
> > }
> >  
> My point was to show that 'volatile' is not sufficient in many cases to 
> avoid messing the data. (It's incredible to still see discussions about 
> 'volatile' after so many years!)
>
> For the first example, since ISRs are said to be nested, you may not be 
> able to use cli/sei or blocking interrupts, but may need to use two 
> different counters.
>
> For the second example, there is a much more efficient way, because your 
> main loop will lock interrupts most of the time.

While such non-locking atomic reads are normally the best choice, they 
are not ideal in all cases.  In particular, they may be bigger or slower 
than locked code (especially if you have to check the consistency of 
more than a byte or two of data), and they may have unbounded execution 
time - if interrupts regularly take more than 1 ms run time, you could 
"hang" indefinitely in getMs().  Disabling interrupts has the 
disadvantage of disabling interrupts (obviously), but it is easier to 
guarantee its run time.

> For instance:
>
> uint32_t getMs(void)
> {
>   uint32_t Ms;
>    volatile uint8_t *p=(uint8_t *)&MilliSeconds;
>    uint8_t a;
>    uint8_t b;
>
>    do{
>       a=*p;
>       Ms=MilliSeconds;
>       b=*p;
>     }while(a!=b);
>
>   return Ms;
> }
>
> Interrupts are never locked, the function just checks that the lower 
> byte of the 32 bits integer does not change before and after having read 
> the counter, hence it insures that the 32 bits were read correctly. 
> Works because AVR is little endian, otherwise 'p' has to be set 
> differently.
>
> However it may not be perfect, if ever the system has other ISR routines 
> that allow the MilliSeconds counter to progress but forbid getMs() to 
> run between reading 'a' and 'b' for more than 255 ms!
>
>  Bernard