_delay_ms Problem in AVR studio

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

I've been using an old version of WinAVR for ages... so I figured I'd finally step up to AVR studio..

But now I have an issue..

I use a LOT...in fact the bulk of my code uses delays (yes I know it's bad code that uses a lot of cycles...cut me some slack I'm a mechanical engineer)

anyway...

if I use it in AVR Studio it won't let me use a variable to set the length of my delay.

for instance something like this

void beep(int beep_freq, int beep_duration)
{

	for(x=0; x < beep_duration; x++)
	{
			PORTB |= _BV(PB7);  //on
			PORTD &= ~_BV(PD5); //off
		_delay_us(beep_freq);
			PORTD |= _BV(PD5);  //on
			PORTB &= ~_BV(PB7); //off
		_delay_us(beep_freq);
	}		
}

gives me an error
"Error 1 __builtin_avr_delay_cycles expects an integer constant. "

I never ran into this while using winAVR

If I can only ever put a constant number in _delay_ms(); then 90% of my code becomes useless. :(

Any suggestions?

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

I don't know why the latest toolchain complains about it, but you can continue to use WinAVR with AVR Studio 5 (see http://avrstudio5.wordpress.com/2011/03/02/how-to-change-the-gcc-toolchain-used/

Regards

Senthil

 

blog | website

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

But using a variable breaks the rules for using _delay_us() given in the user manual (which is why it's showing an error now that the internal implementation has changed in AVR-Libc 1.7.1). If you want variable delays the idea is that you implement them as:

void delay_ms(uint16_t count) {
  while(count--) {
    _delay_ms(1);

  }
}

void delay_us(uint16_t count) {
  while(count--) {
    _delay_us(1);

  }
}

then use delay_ms() and delay_us() where you need variable delays.

Make the functions "static inline" if you don't want a CALL/RET overhead.

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

At the risk of getting flamed...

Yeah I know it breaks the rules... but it's a rule I never understood...nor cared about.

saadhu's idea of changing the toolchain worked.
now my old code compiles fine.

I'll try to de-sloppy my code in future..but for now....kludgey stuff is fine..

Thanks
AM

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

Quote:
Yeah I know it breaks the rules... but it's a rule I never understood...nor cared about.

And I suppose that you then also are prepared to face the consequences - i.e. that _delay_ms() will not work "as expected".

Quote:

If I can only ever put a constant number in _delay_ms(); then 90% of my code becomes useless.

It must have been more or less useless earlier too. Please note that there is no change to _delay_ms() from it earlier working with a variable parameter to now only working with a constant parameter. _delay_ms() and _delay_us() has always demanded constant parameters. The change you have been hit by is that there now is a specific error message when you break the rule.

Quote:
Any suggestions?

Yes: Write your own delay_ms() function, building on _delay_ms, and use that. Sketchy, not tested:

void delay_ms( int ms )
{
   for (int i = 0; i < ms; i++)
   {
      _delay_ms(1);
   }
}

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]

Last Edited: Wed. Apr 20, 2011 - 01:02 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Now both Johan and I have typed virtually identical code do you get the message to solve your 90% problem? You write your own delay_ms(var) and then global search/replace all your calls to "_delay_ms" with "delay_ms". End of problem.

(and this is the way you were supposed to do it in the first place)

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

I got bit by this as well when testing (albeit for only a short while) in AS5. Because it's the right thing to do, I fixed my code as described above. However, it seems odd to me that compiling the code in AS4 (with variables used in _delay_ms() calls) throws absolutely no errors or warnings. Then again, it also seems weird that it doesn't throw any kind of warning when you use a number (constant or otherwise) larger than the maximum allowable sleep period.

Science is not consensus. Science is numbers.

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

Quote:

However, it seems odd to me that compiling the code in AS4 (with variables used in _delay_ms() calls) throws absolutely no errors or warnings.

The breaking of the rules are most often revealed by the enormous increase in flash consumption, since the FP routines hve to e pulled in by the linker to the binary. Or by just seeing that the delays are not of correct length.

To me it seems weird that people use functions like _delay_ms() and are not finding during testing and verification that the delays are off with a substantial amount.

Oh, you don't test and verify..?

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

Just to note that the behaviour (whether it throws an error or not) has absolutely NOTHING to do with a difference between AS4 or AS5.

If you used WinAVR20100110 with AS5 or the "AVR Toolchain" in AS5 with AS4 you could switch the behaviour. The thing that changed is AVR-LibC. It was a 1.6.7 version in WinAVR20100110 and 1.7.1 in the AS5 Toolchain.

Now go to http://svn.savannah.nongnu.org/v... and look at what changed at 2103 and 2189. This modified behaviour relies on __builtin_avr_delay_cycles() which only became available in later issues of the compiler.

It's use is now enabled when __HAS_DELAY_CYCLES is true and it changes the behaviour of util/delay.h, previous conformant apps that adhered to the documentation will not have noticed this (except that the delays may be a shade more accurate), any that violated the rules given in the documentation may (as seen here) be broken by it.

This is why you should always read and adhere to documentation as the developers/testers will likely only be doing regression testing on the documented usage - not undocumented usage too.

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

No need to test when the code that flows from my brain, through my fingers, and into the computer is an image of perfection. Ha (in case the sarcasm isn't evident in the previous statement).

I suspect that many people (not all) use the _delay_ms() function more as a way of killing time than doing an absolutely precise delay. If I want to blink an LED at a 2Hz rate, I will verify that it is blinking twice a second by looking at the second hand on my watch (incidentally, a really good way for beginners to verify that all their clock fuses are set correctly when they are having uart problems). To this degree, I can personally state that _delay_ms() is fairly accurate with a variable argument.

Edit: of course, Clawson is correct about reading all of the documentation...

Science is not consensus. Science is numbers.

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

You do realise that your programs are probably 3K bigger than they need to be when you do this? If you program 128K AVRs you probably don't care. If you program 4K AVRs you should. When you pass a variable rather than a constant known at compile time to _delay_ms() or _delay_us() then the compiler cannot do the floating point calculation and so the whole FP lib will be attached to your program. If you are lucky and use a Makefile that specifies -lm then the "cost" is less than 1K but if you don't use -lm (and infuriatingly Atmel has never succeeded in making this the default in a GCC Makefile) then the FP comes from libgcc.a and is unthinkably inaccurate and bloaty and "costs" about 3K.

This, more than anything might be the reason to read the user manual ;-)

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

Most of my work has been done with MEGA1281 (128kB). However, that is still no reason for writing inefficient code. As I said above, I am using the functions correctly now -- to knowingly use a function in an unintended manner is asking for trouble (doing it unknowingly is still trouble, you're just not asking for it). That said, because I was already using the FP library, I only saw a decrease in program size of 0.1% (of total memory space) when I switched over to the correct function usage.

Science is not consensus. Science is numbers.

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

If you are using FP I hope you are using -lm though I guess it's true that you will probably already have hit some mysterious FP lib errors that would have brought you to that point anyway?

Cliff

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

Now that we have totally hijacked the thread...

From my makefile:

## Libraries
LIBS = -lc -lm -lprintf_flt 

I believe that this is correct. Is it?

Science is not consensus. Science is numbers.

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

Not sure why you'd want -lc as libc.a is linked automatically but otherwise that looks fine.

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

hobbss wrote:

I suspect that many people (not all) use the _delay_ms() function more as a way of killing time than doing an absolutely precise delay.

That's an assumption, and no, it's not always true.

There are two classes of users of these delay functions.

Those who absolutely insist on doing some type of do-nothing loop just to throw in a quick-and-dirty delay in a non-critical use, for example, for LED blinking.

And then there are those who do have a need for a somewhat critically-accurate do-nothing delay. For example, initialization on certain kinds of LCD displays require accurate minimum delays between line toggles.

I'm sure that there are many other examples that fit either category. The point is, is that we, as developers of the toolchain and libraries, need to be able to support both kinds of use cases the best that we can. Certainly the sloppy delay is easier to deal with as it's "close enough". Accurate delays have been harder to do, but we now have that with the built-in function that will accurately delay to the cycle. The trade-off is that this built-in function requires a constant type as its parameter for the number of cycles because the built-in function (as the name suggests) is evaluated *at compile time* and code is automatically generated and inserted by the compiler. Therefore, variables can't be used as those are only evaluated at run-time.

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

An excellent point. As one of the "general" delay users, I have no problem adding five lines of code (as in Cliff's post above) to ensure that the _delay_xs() function gets a constant argument. Earlier, I was just trying to explain to Johan why many people may not catch the problem when using a compiler that does not throw a warning or error when a variable is used for an argument in those functions, even though they may test their program rather rigorously with regard to their critical functionality.

Science is not consensus. Science is numbers.

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

I'm running into the same error "__builtin_avr_delay_cycles expects an integer constant."

But in my calls of _delay_ms(), I use a constant number.

_delay_ms(1);

And I've tracked down this error to be affected by how F_CPU is defined. Right now I am running an xmega128A1 at 32 Mhz.

If I define F_CPU directly, there are no issues.

#define F_CPU 32000000UL

However, if I define F_CPU using the software framework, I receive the above error message.

#define F_CPU sysclk_get_cpu_hz()

Does anyone have insight into why I am getting those errors, or what I could do to fix the issue? The second way of defining is based of the XPLAIN XMEGA DEMO, which compiles and runs fine.

Thank you for your help

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

Well fixed it, was missing the sysclk.h file from the AVR software framework when using sysclk_get_cpu_hz().

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

Quote:

Does anyone have insight into why I am getting those errors,

Remember that the preprocesor is nothing more than a string substitution system so when the code in delay.h does something like (this is an old version BTW):

void
_delay_ms(double __ms)
{
	uint16_t __ticks;
	double __tmp = ((F_CPU) / 4e3) * __ms;
	if (__tmp < 1.0)
		__ticks = 1;
	else if (__tmp > 65535)
	{
		//	__ticks = requested delay in 1/10 ms
		__ticks = (uint16_t) (__ms * 10.0);
		while(__ticks)
		{
			// wait 1/10 ms
			_delay_loop_2(((F_CPU) / 4e3) / 10);
			__ticks --;
		}
		return;
	}
	else
		__ticks = (uint16_t)__tmp;
	_delay_loop_2(__ticks);
}

If you have defined F_CPU as a literal constant this becomes:

void
_delay_ms(double __ms)
{
	uint16_t __ticks;
	double __tmp = ((32000000UL) / 4e3) * __ms;
	if (__tmp < 1.0)
		__ticks = 1;
	else if (__tmp > 65535)
	{
		//	__ticks = requested delay in 1/10 ms
		__ticks = (uint16_t) (__ms * 10.0);
		while(__ticks)
		{
			// wait 1/10 ms
			_delay_loop_2(((32000000UL) / 4e3) / 10);
			__ticks --;
		}
		return;
	}
	else
		__ticks = (uint16_t)__tmp;
	_delay_loop_2(__ticks);
}

As those constants are all known at compile time the compiler can thrown this entire thing away as it can be calculated during calculation.

If, however you define F_CPU as a call to a function its result cannot possibly known at compile time so the preprocesor generates:

void
_delay_ms(double __ms)
{
	uint16_t __ticks;
	double __tmp = ((sysclk_get_cpu_hz()) / 4e3) * __ms;
	if (__tmp < 1.0)
		__ticks = 1;
	else if (__tmp > 65535)
	{
		//	__ticks = requested delay in 1/10 ms
		__ticks = (uint16_t) (__ms * 10.0);
		while(__ticks)
		{
			// wait 1/10 ms
			_delay_loop_2(((sysclk_get_cpu_hz()) / 4e3) / 10);
			__ticks --;
		}
		return;
	}
	else
		__ticks = (uint16_t)__tmp;
	_delay_loop_2(__ticks);
}

As this code now includes a function call the compiler has no choice but to generate code for this entire thing to be used at run-time including the huge over-head of fp calculation in there.

What you are doing is a Very Bad Idea(tm).

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

I see, thanks for the info Clawson. I suppose the XPLAIN DEMO used that function to make it easier to quickly change the CPU frequency without having to change another define.

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

Actually it may not be as bad as you say with the AVR software framework. sysclk_get_cpu_hz() is an inline function, that works out to call other inline functions. I believe the code will work out to a constant in the end as well. I've tried compiling with both methods of defining F_CPU and have the same program size.

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

Quote:

I believe the code will work out to a constant in the end as well

The key thing is whether that is a constant known at compile time or run time. If the code is:

static inline uint32_t sysclk_get_cpu_hz(void) {
  return 32000000UL;
}

then, yes, this would resolve at compile time.

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

[rant]
I hate to drag an old horse out in the street and beat it but having to use a constant for _delay_xx is totally unacceptable.
Having to use;

for(int i = 0; i < some_value; i++)
   _delay_us(1);

is totally useless the overhead added by the for loop makes it worthless.
[/rant]

Ok I'm done, but that's 30 minutes I wasted finding out about this bone head feature that I'll never get back.

Happy Trails,

Mike

JaxCoder.com

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

While thoroughly flogged, the horse still manages to raise it's head somewhat and speak to you:

I can promise you that you will be greeted with multiple cheers from different directions if you contribute a set of delay functions that accept variable delay times as parameter, and that are accurate down to the last clock cycle.

The problem is Really Hard, and in some cases of-course not solvable. Think about it..

Consider an AVR running at 1 MHz, and where you do e.g.

_delay_us(t);

and where at some time t happens to be e.g. 2. There is no way that you can have an algorithm executing at run-time that can determine what to execute for that delay - and that will execute anywhere near 2 us.

(If the delay time is known at compilation, then the compiler can spend all the time and clock cycles it needs to determine that two NOPs will do just fine in this case.)

If you want accurate and down-to-the-last-clock-cycle timing then timed delay loops are not for you. Consider a hardware timer/counter.

Quote:
this bone head feature

Is it your perception that the avr-gcc/avrlibc developers has designed the delay features the way they are because of sloppiness, lazyness or pure evil?

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]

Last Edited: Mon. Jan 14, 2013 - 09:31 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yeah I've been jacking with it for a while and I came up with a half-ass solution that's accurate to about +/-3%;

static inline void delay(uint16_t) __attribute__((always_inline));

static void delay(uint16_t count)
{
	for(uint16_t i = 0; i < count; i++)
		asm volatile("nop");
}

void InitSPI(double __us)
{
	SPI_BB_DDR |= _BV(SPI_BB_SCLK) | _BV(SPI_BB_MOSI) | _BV(SPI_BB_SS);
		
	SPI_BB_PORT &= ~_BV(SPI_BB_SCLK);
		
	cycles_per_us = .000001 / (1.0 / F_CPU);
	time_count = (uint16_t)(((cycles_per_us * __us) - 9.0) / 7.0) ;
}

where 9.0 is the number of cycles to setup loop and 7.0 is the number of cycles in the loop.
usage;

    delay(time_count);

I admit I haven't gained a lot but it works. This may not be the final version I'm thinking it would be better if it was totally in assembler but this is a proof of concept so to speak.

Happy Trails,

Mike

JaxCoder.com

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

Quote:
I admit I haven't gained a lot but it works.
You've gained nothing, in fact you've lost. Your solution does nothing different than _delay_us does except that you calculate the integer constant at run time instead of at compile time. Not only that, your loop is less accurate than _delay_us (3 clocks per loop compared to your 7, plus likely has less setup time).

Regards,
Steve A.

The Board helps those that help themselves.

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

Koshchi wrote:
Quote:
I admit I haven't gained a lot but it works.
You've gained nothing, in fact you've lost. Your solution does nothing different than _delay_us does except that you calculate the integer constant at run time instead of at compile time. Not only that, your loop is less accurate than _delay_us (3 clocks per loop compared to your 7, plus likely has less setup time).

Dang and I was all proud thinkin I did something special. :)

Happy Trails,

Mike

JaxCoder.com

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

If only the authors had written a user manual that explained the (easily worakroundable) limitations of the functions eh?

Oh wait a minute, they did...

http://www.nongnu.org/avr-libc/u...

and for those who don't want those limitations and are willing to do the cycle count calculation manually up front there are:

http://www.nongnu.org/avr-libc/u...

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

clawson,
I saw that when I started digging around after I'd already written mine.
Guess I'll have to do more research before ranting eh? Or "Be sure to put brain in gear before putting mouth in motion". :)

Happy Trails,

Mike

JaxCoder.com

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

Well reading the manual before using something is often the best strategy. Otherwise I'm always reminded of Gert Frobe's character in Those Magnificent Men in their Flying Machines reading the pilots manual as he's flying along which is fine until the seagull takes it out of his hands before he'd read the chapter on landing.

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

I've got ADH, oo shiny so reading the manual on anything is only done if there is no way to figure it out.
That's why I'm single, didn't read the manual? :)

Happy Trails,

Mike

JaxCoder.com