Problem with delay loop

[Royce Pereira]

Hi all,

In the latest WinAVR (avr-gcc (GCC) 4.1.2 (WinAVR 20070525) I found this.

Check this out:
//======================
void delay(unsigned del_cnt)
    {
       while(del_cnt--);

       return;
    }
//=======================

Compiles as (from the .lss file):

//=======================
void delay(word cnt)
    {
  2aa: 08 95       ret

       while(cnt--);

       return;
    }
//=======================

The loop is not executed !!

Where as in an older version (WinAVR-20060421)

It compiles correctly as:
//==========================================
void delay(word cnt)
    {
       while(cnt--);
  286: 01 97       sbiw r24, 0x01 ; 1
  288: 2f ef       ldi r18, 0xFF ; 255
  28a: 8f 3f       cpi r24, 0xFF ; 255
  28c: 92 07       cpc r25, r18
  28e: d9 f7       brne .-10     ; 0x286 <delay>
  290: 08 95       ret


       return;
    }
//=======================================

--
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/
_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Wouter van Gulik]

Royce Pereira schreef:

> Hi all,
>
> In the latest WinAVR (avr-gcc (GCC) 4.1.2 (WinAVR 20070525) I found this.
>
> Check this out:
> //======================
> void delay(unsigned del_cnt)
>     {
>        while(del_cnt--);
>
>        return;
>     }
> //=======================
>

Well writing your own delay_loops is not recommended, because the
optimiser might optimise your loop away. Use <util/delay.h> instead.

Please note that delay.h might not work if compiling without optimiser
(but then again, your loop will not be gone)

HTH,

Wouter


_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Klaus Rudolph]

The code has been optimized. Well done!
If you need the variable access use 'volatile'

> Hi all,
>
> In the latest WinAVR (avr-gcc (GCC) 4.1.2 (WinAVR 20070525) I found this.
>
> Check this out:
> //======================
> void delay(unsigned del_cnt)
>     {
>        while(del_cnt--);
>
>        return;
>     }
> //=======================
>
> Compiles as (from the .lss file):
>
> //=======================
> void delay(word cnt)
>     {
>   2aa: 08 95       ret
>
>        while(cnt--);
>
>        return;
>     }
> //=======================
>
> The loop is not executed !!
>
> Where as in an older version (WinAVR-20060421)
>
> It compiles correctly as:
> //==========================================
> void delay(word cnt)
>     {
>        while(cnt--);
>   286: 01 97       sbiw r24, 0x01 ; 1
>   288: 2f ef       ldi r18, 0xFF ; 255
>   28a: 8f 3f       cpi r24, 0xFF ; 255
>   28c: 92 07       cpc r25, r18
>   28e: d9 f7       brne .-10     ; 0x286 <delay>
>   290: 08 95       ret
>
>
>        return;
>     }
> //=======================================
>
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> AVR-GCC-list mailing list
> AVR-GCC-list@...
> http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Royce Pereira]

Hi,

On Fri, 28 Sep 2007 13:42:18 +0530, Klaus Rudolph <lts-rudolph@...> wrote:

> The code has been optimized. Well done!
> If you need the variable access use 'volatile'
>
Why does it get optimised?
I understand the meaning of 'volatile', but why is it required here ?
It is clear that the variable is changing in the code itself (not outside it).

Again- it worked in the older avr-gcc. Was that a bug(the fact that it worked)?

even rewriting it as:
//--------------------
void delay(unsigned cnt)
    {
       while(cnt)
           cnt--;
    }
//--------------------
does not work. (Why not ?)

Thanks,

--Royce.

>> Hi all,
>>
>> In the latest WinAVR (avr-gcc (GCC) 4.1.2 (WinAVR 20070525) I found this.
>>
>> Check this out:
>> //======================
>> void delay(unsigned del_cnt)
>>     {
>>        while(del_cnt--);
>>
>>        return;
>>     }
>> //=======================
>>
>> Compiles as (from the .lss file):
>>
>> //=======================
>> void delay(word cnt)
>>     {
>>   2aa: 08 95       ret
>>
>>        while(cnt--);
>>
>>        return;
>>     }
>> //=======================
>>
>> The loop is not executed !!
>>
>> Where as in an older version (WinAVR-20060421)
>>
>> It compiles correctly as:
>> //==========================================
>> void delay(word cnt)
>>     {
>>        while(cnt--);
>>   286: 01 97       sbiw r24, 0x01 ; 1
>>   288: 2f ef       ldi r18, 0xFF ; 255
>>   28a: 8f 3f       cpi r24, 0xFF ; 255
>>   28c: 92 07       cpc r25, r18
>>   28e: d9 f7       brne .-10     ; 0x286 <delay>
>>   290: 08 95       ret
>>
>>
>>        return;
>>     }
>> //=======================================
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>> _______________________________________________
>> AVR-GCC-list mailing list
>> AVR-GCC-list@...
>> http://lists.nongnu.org/mailman/listinfo/avr-gcc-list
>
>

--
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/


_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[David Brown-6]

This is probably in the FAQ somewhere - if not, it should be!

The compiler is smart enough to figure out that your delay function does
no useful work - thus the optimiser does not generate any code.  This is
correct compilation - it's your code that is wrong.  The difference is
that the newer version of the compiler is smarter than the older version
(or newer makefiles have higher optimisation enabled by default).

The correct way to write such a loop is:

void delay(unsigned int del_cnt) {
        volatile unsigned int n = del_cnt;
        while (n--);
}

(Incidentally, *never* use "unsigned" as a shorthand - write the proper
type name "unsigned int".  The same applies to "long", "short",
"signed", etc.)

If you don't know how to use "volatile", there should be plenty of
references on the web (including the archives for this mailing list),
which will give you a better explanation.

mvh.,

David


Royce Pereira wrote:

> Hi all,
>
> In the latest WinAVR (avr-gcc (GCC) 4.1.2 (WinAVR 20070525) I found this.
>
> Check this out:
> //======================
> void delay(unsigned del_cnt)
>     {
>        while(del_cnt--);
>
>        return;
>     }
> //=======================
>
> Compiles as (from the .lss file):
>
> //=======================
> void delay(word cnt)
>     {
>   2aa: 08 95       ret
>
>        while(cnt--);
>
>        return;
>     }
> //=======================
>
> The loop is not executed !!
>
> Where as in an older version (WinAVR-20060421)
>
> It compiles correctly as:
> //==========================================
> void delay(word cnt)
>     {
>        while(cnt--);
>   286: 01 97       sbiw r24, 0x01 ; 1
>   288: 2f ef       ldi r18, 0xFF ; 255
>   28a: 8f 3f       cpi r24, 0xFF ; 255
>   28c: 92 07       cpc r25, r18
>   28e: d9 f7       brne .-10     ; 0x286 <delay>
>   290: 08 95       ret
>
>
>        return;
>     }
> //=======================================
>
> As a result none of my delays used in my older programs work when recompiled...
>
> Thanks,
>
> --Royce
>

_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Royce Pereira]

Hi,
On Fri, 28 Sep 2007 14:47:26 +0530, David Brown <david_brown@...> wrote:

>
> This is probably in the FAQ somewhere - if not, it should be!
>
> The compiler is smart enough to figure out that your delay function does
> no useful work - thus the optimiser does not generate any code.  This is
> correct compilation - it's your code that is wrong.  The difference is
> that the newer version of the compiler is smarter than the older version
> (or newer makefiles have higher optimisation enabled by default).
>
> The correct way to write such a loop is:
>
> void delay(unsigned int del_cnt) {
> volatile unsigned int n = del_cnt;
> while (n--);
> }

So I have to write more 'C' code :) to get the same stuff done, in the 'new & smarter' compiler! Interesting.....

Doesn't seem right, some how.

Regards,
--Royce.

> mvh.,
>
> David
>
>
> Royce Pereira wrote:
>> Hi all,
>>
>> In the latest WinAVR (avr-gcc (GCC) 4.1.2 (WinAVR 20070525) I found this.
>>
>> Check this out:
>> //======================
>> void delay(unsigned del_cnt)
>>     {
>>        while(del_cnt--);
>>
>>        return;
>>     }
>> //=======================
>>
>> Compiles as (from the .lss file):
>>
>> //=======================
>> void delay(word cnt)
>>     {
>>   2aa: 08 95       ret
>>
>>        while(cnt--);
>>
>>        return;
>>     }
>> //=======================
>>
>> The loop is not executed !!
>>
>> Where as in an older version (WinAVR-20060421)
>>
>> It compiles correctly as:
>> //==========================================
>> void delay(word cnt)
>>     {
>>        while(cnt--);
>>   286: 01 97       sbiw r24, 0x01 ; 1
>>   288: 2f ef       ldi r18, 0xFF ; 255
>>   28a: 8f 3f       cpi r24, 0xFF ; 255
>>   28c: 92 07       cpc r25, r18
>>   28e: d9 f7       brne .-10     ; 0x286 <delay>
>>   290: 08 95       ret
>>
>>
>>        return;
>>     }
>> //=======================================
>>
>> As a result none of my delays used in my older programs work when recompiled...
>>
>> Thanks,
>>
>> --Royce
>>
>
>
>
> _______________________________________________
> AVR-GCC-list mailing list
> AVR-GCC-list@...
> http://lists.nongnu.org/mailman/listinfo/avr-gcc-list
>

--
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/


_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Royce Pereira]

Hi,

On Fri, 28 Sep 2007 14:47:26 +0530, David Brown <david_brown@...> wrote:

>
> This is probably in the FAQ somewhere - if not, it should be!
>
> The compiler is smart enough to figure out that your delay function does
> no useful work - thus the optimiser does not generate any code.  This is
> correct compilation - it's your code that is wrong.  The difference is
> that the newer version of the compiler is smarter than the older version
> (or newer makefiles have higher optimisation enabled by default).
>
> The correct way to write such a loop is:
>
> void delay(unsigned int del_cnt) {
> volatile unsigned int n = del_cnt;
> while (n--);
> }

I tried that and -Yikes !! Here's what I get :( :
Compare it to the short & sweet loop of the earlier avr-gcc version (below)...
//-------------------------------------------
void delay(unsigned int del_cnt)
    {
  2aa: cf 93       push r28
  2ac: df 93       push r29
  2ae: cd b7       in r28, 0x3d ; 61
  2b0: de b7       in r29, 0x3e ; 62
  2b2: 22 97       sbiw r28, 0x02 ; 2
  2b4: 0f b6       in r0, 0x3f ; 63
  2b6: f8 94       cli
  2b8: de bf       out 0x3e, r29 ; 62
  2ba: 0f be       out 0x3f, r0 ; 63
  2bc: cd bf       out 0x3d, r28 ; 61
       volatile unsigned int n = del_cnt;
  2be: 9a 83       std Y+2, r25 ; 0x02
  2c0: 89 83       std Y+1, r24 ; 0x01
       while(n--);
  2c2: 89 81       ldd r24, Y+1 ; 0x01
  2c4: 9a 81       ldd r25, Y+2 ; 0x02
  2c6: 01 97       sbiw r24, 0x01 ; 1
  2c8: 9a 83       std Y+2, r25 ; 0x02
  2ca: 89 83       std Y+1, r24 ; 0x01
  2cc: 89 81       ldd r24, Y+1 ; 0x01
  2ce: 9a 81       ldd r25, Y+2 ; 0x02
  2d0: 8f 5f       subi r24, 0xFF ; 255
  2d2: 9f 4f       sbci r25, 0xFF ; 255
  2d4: b1 f7       brne .-20     ; 0x2c2 <delay+0x18>
  2d6: 22 96       adiw r28, 0x02 ; 2
  2d8: 0f b6       in r0, 0x3f ; 63
  2da: f8 94       cli
  2dc: de bf       out 0x3e, r29 ; 62
  2de: 0f be       out 0x3f, r0 ; 63
  2e0: cd bf       out 0x3d, r28 ; 61
  2e2: df 91       pop r29
  2e4: cf 91       pop r28
  2e6: 08 95       ret

000002e8 <wr_lcd_hi>:

       return;
    }
//=======================

Output of the older -WinAVR-20060421-version (and without the 'volatile' thingy too!)

>> //==========================================
>> void delay(word cnt)
>>     {
>>        while(cnt--);
>>   286: 01 97       sbiw r24, 0x01 ; 1
>>   288: 2f ef       ldi r18, 0xFF ; 255
>>   28a: 8f 3f       cpi r24, 0xFF ; 255
>>   28c: 92 07       cpc r25, r18
>>   28e: d9 f7       brne .-10     ; 0x286 <delay>
>>   290: 08 95       ret
>>
>>
>>        return;
>>     }
>> //=======================================
>>

Thanks,

--Royce.
--
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/


_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Klaus Rudolph]

Please use optimizer! Something like -O2 -O3 -Os ... as you need!

Simplify your delay loop:
void delay(volatile word cnt) { ...

Have fun!


Royce Pereira schrieb:

> Hi,
>
> On Fri, 28 Sep 2007 14:47:26 +0530, David Brown <david_brown@...> wrote:
>
>> This is probably in the FAQ somewhere - if not, it should be!
>>
>> The compiler is smart enough to figure out that your delay function does
>> no useful work - thus the optimiser does not generate any code.  This is
>> correct compilation - it's your code that is wrong.  The difference is
>> that the newer version of the compiler is smarter than the older version
>> (or newer makefiles have higher optimisation enabled by default).
>>
>> The correct way to write such a loop is:
>>
>> void delay(unsigned int del_cnt) {
>> volatile unsigned int n = del_cnt;
>> while (n--);
>> }
>
> I tried that and -Yikes !! Here's what I get :( :
> Compare it to the short & sweet loop of the earlier avr-gcc version (below)...
> //-------------------------------------------
> void delay(unsigned int del_cnt)
>     {
>   2aa: cf 93       push r28
>   2ac: df 93       push r29
>   2ae: cd b7       in r28, 0x3d ; 61
>   2b0: de b7       in r29, 0x3e ; 62
>   2b2: 22 97       sbiw r28, 0x02 ; 2
>   2b4: 0f b6       in r0, 0x3f ; 63
>   2b6: f8 94       cli
>   2b8: de bf       out 0x3e, r29 ; 62
>   2ba: 0f be       out 0x3f, r0 ; 63
>   2bc: cd bf       out 0x3d, r28 ; 61
>        volatile unsigned int n = del_cnt;
>   2be: 9a 83       std Y+2, r25 ; 0x02
>   2c0: 89 83       std Y+1, r24 ; 0x01
>        while(n--);
>   2c2: 89 81       ldd r24, Y+1 ; 0x01
>   2c4: 9a 81       ldd r25, Y+2 ; 0x02
>   2c6: 01 97       sbiw r24, 0x01 ; 1
>   2c8: 9a 83       std Y+2, r25 ; 0x02
>   2ca: 89 83       std Y+1, r24 ; 0x01
>   2cc: 89 81       ldd r24, Y+1 ; 0x01
>   2ce: 9a 81       ldd r25, Y+2 ; 0x02
>   2d0: 8f 5f       subi r24, 0xFF ; 255
>   2d2: 9f 4f       sbci r25, 0xFF ; 255
>   2d4: b1 f7       brne .-20     ; 0x2c2 <delay+0x18>
>   2d6: 22 96       adiw r28, 0x02 ; 2
>   2d8: 0f b6       in r0, 0x3f ; 63
>   2da: f8 94       cli
>   2dc: de bf       out 0x3e, r29 ; 62
>   2de: 0f be       out 0x3f, r0 ; 63
>   2e0: cd bf       out 0x3d, r28 ; 61
>   2e2: df 91       pop r29
>   2e4: cf 91       pop r28
>   2e6: 08 95       ret
>
> 000002e8 <wr_lcd_hi>:
>
>        return;
>     }
> //=======================
>
> Output of the older -WinAVR-20060421-version (and without the 'volatile' thingy too!)
>>> //==========================================
>>> void delay(word cnt)
>>>     {
>>>        while(cnt--);
>>>   286: 01 97       sbiw r24, 0x01 ; 1
>>>   288: 2f ef       ldi r18, 0xFF ; 255
>>>   28a: 8f 3f       cpi r24, 0xFF ; 255
>>>   28c: 92 07       cpc r25, r18
>>>   28e: d9 f7       brne .-10     ; 0x286 <delay>
>>>   290: 08 95       ret
>>>
>>>
>>>        return;
>>>     }
>>> //=======================================
>>>
>
> Thanks,
>
> --Royce.

_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Royce Pereira]

Hi Klaus,
On Fri, 28 Sep 2007 14:57:14 +0530, Klaus Rudolph <lts-rudolph@...> wrote:

> Please use optimizer! Something like -O2 -O3 -Os ... as you need!

My makefile already has
OPT = s

>
> Simplify your delay loop:
> void delay(volatile word cnt) { ...
>
Already tried that. No change.

If it's worked for you with whatever you have suggested, could you paste a snippet of your output ? Thanks!

regards,

--Royce.




--
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/


_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Graham Davies]

Royce Pereira wrote:
> So I have to write more 'C' code :) to get the same
> stuff done, in the 'new & smarter' compiler!

Not more code, just correct code.

Have you tried returning the final value of your delay argument from the
function?  If the compiler optimizes only within the boundaries of
individual functions it won't be able to take advantage of the fact that the
result is not used.  Strictly, this isn't correct either as the compiler
could just assign zero and return.  But, it may not be smart enough to do
that and you may get closer to your original assembly language.  Another way
to go would be to write the function directly in assembly language.

Graham.




_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Klaus Rudolph]

I think we are discussing not the solution :-)

The compiler optimizes unused code away, that is OK!
If we use a volatile, the WRITE ACCESS could not longer be optimized and
also a new READ ACCESS before subtraction must! be done. That is what
the compiler do, that is also OK!
If there is a "local" volatile variable, the compiler build a new stack
frame which results in a big prologue and epilogue for the function,
which is OK!

The solution to have a time consumtion function is to use the library!
Sorry that the volatile hack is not what you need. But it burnes time as
expected :-) and nothing else is requested. If you need a handcafted
delay loop use inline assembler instead. The behaviour of the compiler
can change from version to version. And I hope it will change a lot to
get better code!

Bye
  Klaus

> Royce Pereira wrote:
>> So I have to write more 'C' code :) to get the same
>> stuff done, in the 'new & smarter' compiler!
>
> Not more code, just correct code.
>
> Have you tried returning the final value of your delay argument from the
> function?  If the compiler optimizes only within the boundaries of
> individual functions it won't be able to take advantage of the fact that
> the result is not used.  Strictly, this isn't correct either as the
> compiler could just assign zero and return.  But, it may not be smart
> enough to do that and you may get closer to your original assembly
> language.  Another way to go would be to write the function directly in
> assembly language.
>
> Graham.
>
>
>
>
> _______________________________________________
> AVR-GCC-list mailing list
> AVR-GCC-list@...
> http://lists.nongnu.org/mailman/listinfo/avr-gcc-list
>

_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Weddington, Eric-2]

> -----Original Message-----
> From:
> avr-gcc-list-bounces+eweddington=cso.atmel.com@...
> [mailto:avr-gcc-list-bounces+eweddington=cso.atmel.com@nongnu.
> org] On Behalf Of David Brown
> Sent: Friday, September 28, 2007 3:17 AM
> To: AVR-GCC
> Subject: Re: [avr-gcc-list] Problem with delay loop
>
>
> This is probably in the FAQ somewhere - if not, it should be!

I checked the avr-libc docs, and it's not in the FAQ strangely enough. It's
asked so often, it should be #2.

Would someone be willing to write up some text and submit it to the Patch
Tracker on the avr-libc project? We'll make sure that it gets in the docs
for the next release.

Thanks
Eric Weddington




_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[David Kelly]

On Fri, Sep 28, 2007 at 01:29:19PM +0530, Royce Pereira wrote:

> Hi all,
>
> In the latest WinAVR (avr-gcc (GCC) 4.1.2 (WinAVR 20070525) I found this.
>
> Check this out:
> //======================
> void delay(unsigned del_cnt)
>     {
>        while(del_cnt--);
>
>        return;
>     }
> //=======================

I agree with what others have said, no matter the old compiler did what
you intended with the above code, the code is less that fully correct.
Is legal for the compiler to burn cycles in the loop, is legal for the
compiler to recognize a do-nothing loop and remove it. You are not the
first to discover this that is why the volatile modifier exists.

Why are you not using hardware timers? First thing I do on a new project
is set up a hardware timer to tick away at 60 or 100 Hz or something
close. Then in the interrupt routine I commonly create "software timers"
something like this:

        if( timer_serial_cmd )
                timer_serial_cmd--;

Elsewhere in my code I might start receiving a serial command string
where no two characters should arrive more than 100 ms apart. At 60 Hz
that would be 6 ticks, 10 at 100 Hz. So I put 6 or 10 in
timer_serial_cmd and if ever I find timer_serial_cmd to be zero I know
too much time has passed.

Notice timer_serial_cmd needs to be volatile but you might get away
without. I use uint8_t more than anything else as my variable type. If
your timer_serial_cmd is 16 bits then if it is not zero interrupts need
to be locked out while it is initialized. Or a quick and dirty way is to
write the value twice knowing the 60 Hz interrupt isn't going to fire
twice in 2 lines of code:

        volatile uint16_t timer_serial_cmd;

        timer_serial_cmd = 1000;
        timer_serial_cmd = 1000;

or
        cli();
        timer_serial_cmd = 1000;
        sei();

For finer delays don't forget the OCR functions on the timers. Is pretty
easy to write and create very accurate delays with:

void delay( uint8_t delay );
{
        OCR2A = TCNT2 + delay;
        TIFR2 = (1<<OCF2A); //  clear possible pending

        //  wait until TCNT2 matches OCR2A
        while( ( TIFR2 && (1<<OCF2A) ) == 0 )
                ;
}

If you are not already using timers then you need to set a clock source
to start the timer running. See TCCR2B for the above.

With small variations all of the above works for timers 0, 1, and 2.

--
David Kelly N4HHE, dkelly@...
========================================================================
Whom computers would destroy, they must first drive mad.


_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[David Kelly]

On Fri, Sep 28, 2007 at 09:05:40AM -0500, David Kelly wrote:

>
> For finer delays don't forget the OCR functions on the timers. Is pretty
> easy to write and create very accurate delays with:
>
> void delay( uint8_t delay );
> {
> OCR2A = TCNT2 + delay;
> TIFR2 = (1<<OCF2A); //  clear possible pending
>
> //  wait until TCNT2 matches OCR2A
> while( ( TIFR2 && (1<<OCF2A) ) == 0 )

                       ^^ arrgh!

> ;
> }

Above should be while( ( TIFR2 & (1<<OCF2A) ) == 0 ), bitwise and, not
logical and.

--
David Kelly N4HHE, dkelly@...
========================================================================
Whom computers would destroy, they must first drive mad.


_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Dave Hansen-2]

Some parts of this message have been removed. Learn more about Nabble's security policy.

> Date: Fri, 28 Sep 2007 14:21:38 +0530
> To: lts-rudolph@...
> Subject: Re: [avr-gcc-list] Problem with delay loop
> From: roycejp@...
> CC: AVR-GCC-list@...
>
> Hi,
>
> On Fri, 28 Sep 2007 13:42:18 +0530, Klaus Rudolph <lts-rudolph@...> wrote:
>
> > The code has been optimized. Well done!
> > If you need the variable access use 'volatile'
> >
> Why does it get optimised?

 
Because it is allowed, and gcc is sophisticated enough to do so.

> I understand the meaning of 'volatile', but why is it required here ?
 
What is the meaning of volatile?  Hint: it has nothing to do with "sharing."

> It is clear that the variable is changing in the code itself (not outside it).
 
Precisely.  The optimizer recognized that neither the intermediate values nor even the final value of the variable had any effect on the outcome of the program, and therefore removed the calculation of those values from the code.

>
> Again- it worked in the older avr-gcc. Was that a bug(the fact that it worked)?
 
No.  It was a missed opportunity for optimization.  The program produced the same result, it just took longer to do so.
 
Again, you would do well to investigate the library's delay.h header and the facilities it provides.  These work very well.
 
HTH,
 
   -=Dave

 

---

Explore the seven wonders of the world Learn more!
_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Royce Pereira]

Hi all,

OK fine I agree.

we have to use 'volatile' and all.

But why must it generate such horrid code...
(I reproduce the comparison again below to get the *real* issue into focus)

The compiler output with the 'correct' code ('volatile' used):
//---------------------------------------------------
void delay(unsigned int del_cnt)
     {
   2aa: cf 93       push r28
   2ac: df 93       push r29
   2ae: cd b7       in r28, 0x3d ; 61
   2b0: de b7       in r29, 0x3e ; 62
   2b2: 22 97       sbiw r28, 0x02 ; 2
   2b4: 0f b6       in r0, 0x3f ; 63
   2b6: f8 94       cli ;<-----disabling interrupts? Why?
   2b8: de bf       out 0x3e, r29 ; 62
   2ba: 0f be       out 0x3f, r0 ; 63
   2bc: cd bf       out 0x3d, r28 ; 61
        volatile unsigned int n = del_cnt;
   2be: 9a 83       std Y+2, r25 ; 0x02
   2c0: 89 83       std Y+1, r24 ; 0x01
        while(n--);
   2c2: 89 81       ldd r24, Y+1 ; 0x01
   2c4: 9a 81       ldd r25, Y+2 ; 0x02
   2c6: 01 97       sbiw r24, 0x01 ; 1
   2c8: 9a 83       std Y+2, r25 ; 0x02
   2ca: 89 83       std Y+1, r24 ; 0x01
   2cc: 89 81       ldd r24, Y+1 ; 0x01
   2ce: 9a 81       ldd r25, Y+2 ; 0x02
   2d0: 8f 5f       subi r24, 0xFF ; 255
   2d2: 9f 4f       sbci r25, 0xFF ; 255
   2d4: b1 f7       brne .-20     ; 0x2c2 <delay+0x18>
   2d6: 22 96       adiw r28, 0x02 ; 2
   2d8: 0f b6       in r0, 0x3f ; 63
   2da: f8 94       cli
   2dc: de bf       out 0x3e, r29 ; 62
   2de: 0f be       out 0x3f, r0 ; 63
   2e0: cd bf       out 0x3d, r28 ; 61
   2e2: df 91       pop r29
   2e4: cf 91       pop r28
   2e6: 08 95       ret

        return;
     }
//=======================

Output of the older -WinAVR-20060421-version (with the alleged 'wrong' C code)

>> //==========================================
>> void delay(word cnt)
>>     {
>>        while(cnt--);
>>   286: 01 97       sbiw r24, 0x01 ; 1
>>   288: 2f ef       ldi r18, 0xFF ; 255
>>   28a: 8f 3f       cpi r24, 0xFF ; 255
>>   28c: 92 07       cpc r25, r18
>>   28e: d9 f7       brne .-10     ; 0x286 <delay>
>>   290: 08 95       ret
>>          return;
>>     }
>> //=======================================
>>

So which is the right output we want? Obviously the second.

Agreed not using 'volatile' optimises the code to a mere 'ret'. I'm ok with that.

But should'nt the 'correct' code produce the same output (the shorter version above)??

Thanks,

--Royce.



--
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/


_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[David Kelly]

On Fri, Sep 28, 2007 at 11:26:43PM +0530, Royce Pereira wrote:
> Hi all,
>
> OK fine I agree.
>
> we have to use 'volatile' and all.
>
> But why must it generate such horrid code...

There are those here who would tell you that any calibrated delay loop
is horrible code.

The stack has to be protected when space is being allocated, believe
this is normal for any auto variable allocation because the registers
being used to offset the stack are the same the runtime will use to
process an interrupt. Notice the SREG is restored in the middle of
updating the stack pointer? This is because the next instruction is
already in the pipe and can't be interrupted.

Make it "static volatile" for simpler code. The other was nasty because
it had to access stack-relative, and because it was volatile it *had* to
be fetched and stored each time.

void delay_loop(unsigned int delay)
{
  ba:   90 93 01 01     sts     0x0101, r25
  be:   80 93 00 01     sts     0x0100, r24
        static volatile unsigned int vdelay;

        vdelay = delay;
        while( vdelay-- )
  c2:   80 91 00 01     lds     r24, 0x0100
  c6:   90 91 01 01     lds     r25, 0x0101
  ca:   01 97           sbiw    r24, 0x01       ; 1
  cc:   90 93 01 01     sts     0x0101, r25
  d0:   80 93 00 01     sts     0x0100, r24
  d4:   80 91 00 01     lds     r24, 0x0100
  d8:   90 91 01 01     lds     r25, 0x0101
  dc:   8f 5f           subi    r24, 0xFF       ; 255
  de:   9f 4f           sbci    r25, 0xFF       ; 255
  e0:   81 f7           brne    .-32            ; 0xc2 <delay_loop+0x8>
  e2:   08 95           ret

--
David Kelly N4HHE, dkelly@...
========================================================================
Whom computers would destroy, they must first drive mad.


_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[Graham Davies]

David Kelly wrote:
> ... because it was volatile it *had* to
> be fetched and stored each time.

Oh boy.  That's a really interesting statement (to me anyway, but I have a
volatile fetish).  You're saying that having chosen to put the variable in
memory, the compiler is obliged to fetch it prior to and store it after each
increment.  I can't disagree with that.  But, does it explain the "horrid
code"?  Does the compiler *have* to put the variable in memory?  It is
declared as an automatic variable, right?  That means that the compiler has
complete freedom in deciding where to put it and a register is a perfectly
good choice.  The scope is local, so the variable can't be accessed outside
the function.  It isn't static, so its value isn't preserved when execution
leaves the function.  Is there a body of opinion that the volatile modifier
obliges the compiler to put a variable in memory?  I'm just interested.  If
this is too off-topic, feel free to ignore me.  Actually, feel free to
ignore me at any time.

Graham.




_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[David Kelly]

On Sep 28, 2007, at 8:58 PM, Graham Davies wrote:

> David Kelly wrote:
>> ... because it was volatile it *had* to
>> be fetched and stored each time.
>
> Oh boy.  That's a really interesting statement (to me anyway, but I  
> have a volatile fetish).  You're saying that having chosen to put  
> the variable in memory, the compiler is obliged to fetch it prior  
> to and store it after each increment.  I can't disagree with that.  
> But, does it explain the "horrid code"?  Does the compiler *have*  
> to put the variable in memory?

I guess it doesn't really have to if it can assure that the variable  
has not been passed by address reference to another routine such as  
an interrupt. Thats pretty much the definition of volatile in the  
first place, that one is telling the compiler that it can be changed  
somewhere somehow outside of the compiler's control.

>   It is declared as an automatic variable, right?  That means that  
> the compiler has complete freedom in deciding where to put it and a  
> register is a perfectly good choice.  The scope is local, so the  
> variable can't be accessed outside the function.

It could be passed to another function by address.

>   It isn't static, so its value isn't preserved when execution  
> leaves the function.  Is there a body of opinion that the volatile  
> modifier obliges the compiler to put a variable in memory?  I'm  
> just interested.  If this is too off-topic, feel free to ignore  
> me.  Actually, feel free to ignore me at any time.

I think there is room for other variations on "volatile" as much of  
what Graham says applies nicely in interrupt service routines. No  
need to treat most application volatile variables with kid gloves  
when interrupts are masked. Many variables mapped to a peripheral  
device needs volatile no matter what. Not for an ouput port, but an  
input port for sure, and hardware timers, etc. But not for my example  
software timer, at least not in the interrupt routine where it gets  
decremented:

        if( timer_serial_cmd )
                timer_serial_cmd--;

timer_serial_cmd above only needs to be volatile outside of the  
interrupt routine.

It is a stretch to say that any code intended to cause a deliberate  
delay is horrible. The code provided in <util/delay_basic.h> is about  
as painless as anything can be, no matter that it is assembly in a C  
function wrapper.

I wrote earlier, if an Output Compare is not being used elsewhere,  
they make excellent delay timers without tying up the CPU in a busy  
loop. My example tied the CPU up, but thats not the way I generally  
use them. Software timers implemented in my tick interrupt for longer  
events, OCR's for precise short periods, and more complex use of the  
OCR hardware for repetitive events such as PWM motor control.

--
David Kelly N4HHE, dkelly@...
========================================================================
Whom computers would destroy, they must first drive mad.





_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list

[David Brown-6]

There are good reasons for much of the stuff produced by the use of
"volatile" - since the local variable is volatile, it has to be put on
the stack.  Generating a stack frame on the AVR needs a bit of messing
around in the prologue and epilogue, including turning off interrupts
while modifying the stack pointer (blame Atmel for that whoopsie).

What would be really nice is if gcc accepted the concept of "register
volatile" variables.  I've used them on other compilers in the past to
get exactly the effect we are looking for here.  But that was in the
days when "register" was a useful hint to an otherwise fairly poor
optimiser - gcc, I believe, ignores "register" for local variables.

Slightly better code can be generated with:

void delay2(unsigned int n) {
        static volatile unsigned int x;
        x = n;
        while (x--) ;
}

But even better is:

void delay3(volatile unsigned int n) {
        while (n--) {
                asm volatile(" " : : );
        };
}


mvh.,

David





Royce Pereira wrote:

> Hi all,
>
> OK fine I agree.
>
> we have to use 'volatile' and all.
>
> But why must it generate such horrid code...
> (I reproduce the comparison again below to get the *real* issue into focus)
>
> The compiler output with the 'correct' code ('volatile' used):
> //---------------------------------------------------
> void delay(unsigned int del_cnt)
>      {
>    2aa: cf 93       push r28
>    2ac: df 93       push r29
>    2ae: cd b7       in r28, 0x3d ; 61
>    2b0: de b7       in r29, 0x3e ; 62
>    2b2: 22 97       sbiw r28, 0x02 ; 2
>    2b4: 0f b6       in r0, 0x3f ; 63
>    2b6: f8 94       cli ;<-----disabling interrupts? Why?
>    2b8: de bf       out 0x3e, r29 ; 62
>    2ba: 0f be       out 0x3f, r0 ; 63
>    2bc: cd bf       out 0x3d, r28 ; 61
>         volatile unsigned int n = del_cnt;
>    2be: 9a 83       std Y+2, r25 ; 0x02
>    2c0: 89 83       std Y+1, r24 ; 0x01
>         while(n--);
>    2c2: 89 81       ldd r24, Y+1 ; 0x01
>    2c4: 9a 81       ldd r25, Y+2 ; 0x02
>    2c6: 01 97       sbiw r24, 0x01 ; 1
>    2c8: 9a 83       std Y+2, r25 ; 0x02
>    2ca: 89 83       std Y+1, r24 ; 0x01
>    2cc: 89 81       ldd r24, Y+1 ; 0x01
>    2ce: 9a 81       ldd r25, Y+2 ; 0x02
>    2d0: 8f 5f       subi r24, 0xFF ; 255
>    2d2: 9f 4f       sbci r25, 0xFF ; 255
>    2d4: b1 f7       brne .-20     ; 0x2c2 <delay+0x18>
>    2d6: 22 96       adiw r28, 0x02 ; 2
>    2d8: 0f b6       in r0, 0x3f ; 63
>    2da: f8 94       cli
>    2dc: de bf       out 0x3e, r29 ; 62
>    2de: 0f be       out 0x3f, r0 ; 63
>    2e0: cd bf       out 0x3d, r28 ; 61
>    2e2: df 91       pop r29
>    2e4: cf 91       pop r28
>    2e6: 08 95       ret
>
>         return;
>      }
> //=======================
>
> Output of the older -WinAVR-20060421-version (with the alleged 'wrong' C code)
>>> //==========================================
>>> void delay(word cnt)
>>>     {
>>>        while(cnt--);
>>>   286: 01 97       sbiw r24, 0x01 ; 1
>>>   288: 2f ef       ldi r18, 0xFF ; 255
>>>   28a: 8f 3f       cpi r24, 0xFF ; 255
>>>   28c: 92 07       cpc r25, r18
>>>   28e: d9 f7       brne .-10     ; 0x286 <delay>
>>>   290: 08 95       ret
>>>          return;
>>>     }
>>> //=======================================
>>>
>
> So which is the right output we want? Obviously the second.
>
> Agreed not using 'volatile' optimises the code to a mere 'ret'. I'm ok with that.
>
> But should'nt the 'correct' code produce the same output (the shorter version above)??
>
> Thanks,
>
> --Royce.
>
>
>

_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@...
http://lists.nongnu.org/mailman/listinfo/avr-gcc-list