bugAVR C Runtime Library - Bugs: bug #30363, _delay_xx() functions in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #30363: _delay_xx() functions in <util/delay.h> are broken

Submitter:  Bill Perry <bperrybap>
Submitted:  Mon 05 Jul 2010 10:34:10 PM UTC
   
 
Category:  Header Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Header files
Status:  Fixed Assigned to:  aboyapati
Percent Complete:  100% Open/Closed:  Closed
Release:  1.7.* Fixed Release:  None

Jump to the original submission

Wed 13 Oct 2010 09:38:06 AM UTC, comment #15: 

Final patch, version 3 attached.

Previous patch has the issue that if no optimization is used when building the application with the compiler that has builtin_avr_delay_cycles() support, no code is produced. This patch fixes the issue. Patch tested.

(file #21669)

Anitha Boyapati <aboyapati>
Mon 11 Oct 2010 02:26:18 PM UTC, comment #14: 

Updated patch attached. Please review.

(file #21657)

Anitha Boyapati <aboyapati>
Thu 07 Oct 2010 10:15:49 PM UTC, comment #13: 

My guess is that many people either don't use these
routines or use the delay routines
for fairly long >1ms gross/rough delays as
many of the responses to my comments in the AVR freaks forum
threads when I brought up issues about cycle accuracy particularly
at the low end suggested just using "a few NOPs" for short hardware setup time delays rather than use these routines.

For long delays, I'm assuming that usually the rounding style really doesn't matter because at several milliseconds or more,
being off by a clock cycle (too long or too short) may not matter.

And given the granularity of the current delay routines
that used the basic loops, the majority of people could
not have been caring too much about the delay accuracy -
especially down to the individual cycle.

My focus has always been to make the functions work more predictably and for hardware setup times, where there is no alternative to CPU busy-wait delays, such as NOPs, loops or etc because things like a timer are way too long.

Hardware setup times need a delay "at least as long as"
a certain period of time. The delay can be longer but never
shorter.
With "round up" a delay may be longer than requested, but for hardware setup timing that is desirable over getting a delay that is too short that can cause the code to violate the hardware timing.

That is what drove me to abandon
the current <util/delay.h> routines.

It is also why I lean twards "round up" being the default.
But I'm also assuming (which may be incorrect)
that if somebody wants a long delay say >1ms they probably don't care if they get an extra clock cycle of delay; however, at the short end for hardware type delays you don't want 0, and you need it to be "at least as long as" as you ask for.

That said, it is just my preference, and it can
probabaly be argued either way, there may be cases
where people want delays "up to but no longer than" a
certain value. (truncation of cycle count)
So I could live with "round up" not being the default as long it can be configured to round up

Implication wise, I think "round up" with the better granularity
of the new delay cycles code is better and more predictable behavior than what is there now.

I think no matter which method is the default going forward, it becomes primarily an issue of documentation.

With respect to picking a default (round, vs truncate vs round up)
It seems easier and clearer to describe by being able to say, the
delay functions by default always provide the shortest possible delay that satisfies the requested delay without being shorter than the requested delay.

vs if you round or round down/truncate the delay is a little less
predictable. Depending on the requested delay and F_CPU,
it might be longer (in the case of rounding calculating to round up)
or shorter or even eliminated in the case of rounding down
and allowing the cycle count to drop to 0.

==

It also seems like there is agreement that without some define
to insert the old code, there is no way to provide complete
backward compatibility and that people may want/need
different styles of cycle rounding.

=========================================================

So I'll toss out a suggestion:
What about if by default delay routines provide a delay of
"at least as long as" a delay as was requested. (round up)

And that there are additional defines to manipulate how
the cycle calculation is done along with a temporary
define (that will eventually be deprecated) that reverts
back to the existing code for all those that want/need
something different?

So
Round up by default
1 define to round to nearest value (up or down)
1 define to round down/truncate

1 define (that will eventually be deprecated) that
  generates the old/current 3/4 loop count rounding
  using the basic loop functions.

While my preference is for "round up" being the default,
any of the 3 methods could be the default.
(although allowing a delay to be reduced to 0 by default still hurts my head)

That way everybody can get what they want and this
can be put to bed.

The only other thing that would be nice would be either
issue a warning or even crash the compile if the argument
passed to _delay_xx() was not a constant.
(But this kind of relates to the enhancement to
support variable delays and not this current bug fix)

--- bill


Bill Perry <bperrybap>
Thu 07 Oct 2010 01:34:32 PM UTC, comment #12: 

Bill Perry:

>> Backward compatibility:

...

>> For the short end, there are a few options:
>> 2) Force it to be 1 cycle if it falls to 0


Forcing it to 1 might introduce special handling which I think is unnecessary.


>> 3) have another define that sets the MIN cycles so user
>> can create some backward combility by defining it to 3 (or 1)
>> if necessary. So if this define is not set, it defaults
>> to allowing 0 (no delay).


Setting MIN cycles does not create complete backward compatibility. For instance, see the following representation:

If x represents _tmp with delay cycles builtin, then we get a delay of trunc(x) clock cycles as per new implementation, (trunc(x/4))*4  clock cycles as per old implementation which results in differences in ranges.

For e.g., one instance of granularity difference is:

In older implementation, for the range in milliseconds [0.004, 0.008), 4 clock cycles of delay is produced

In newer implementation, for the range in milliseconds [0.004, 0.005), 4 clock cycles of delay is produced.

Introducing a MIN cycles will not address this issue.

>> 4) have a define to return to old behavior.


My concern is that old behavior is patchy. Any code working by accident cannot be maintained in the long run. It is best advised to make changes earlier than later.

Old behavior can be brought back with a macro #ifdef _OLD_DELAY_CYCLE which will be deprecated in future version. Thoughts ?


Anitha Boyapati <aboyapati>
Thu 07 Oct 2010 09:45:06 AM UTC, comment #11: 


>> Other than loop/cycle count truncation was the previous way of
>> doing things, what is the reasoning for preferring truncation
>> to be the new default vs "round up" as the default going
>> forward?


It is mostly driven by the previous implementation.

>> "round up" as a default would require the user to have to
>> define something to ever get a delay that is less than what
>> is asked for


I think it can be argued either wise - "round up" or "round down". Both have trade-offs. When "round up" is used, the user might end up wondering why delay is a little more than what he bargained for.


>> I am worried about delays being reduced or even eliminated
>> due to cycle count truncation to 0 or rounding cycle count
>> down to 0 because this is creating a different behavior than
>> the previous code.


>> For example, if somebody called _delay_us(.500) and was
>> running on 1Mz clock. Previously they got a 3us delay, but if
>> truncation is the default and there is no bump to a non zero
>> number of cycles, they would get no delay, which might not
>> work.


Even if "round up" is used, a clock cycle of '1' is given out for cases where cycle count is 0.*. It is still different from the expected 3 cycle clock. This has to be corrected anyway. How would "round up" solve the situation? or rather how can introducing non-zero delay make any difference?

I am open to considering "round up" as default option. However, I am trying to understand the full implication of it, if there is any.


>> I'm assuming ceil() is a gcc builtin or
>> could be something like this?


ceil() is from avr-libc. I planned to re-use available functions for optimization considerations.

http://www.nongnu.org/avr-libc/user-manual/group__avr__math.html#ga61470611f23ceef5d3e9cf63d84cd8a7

I'll talk about backward compatibility in next comment.

Anitha Boyapati <aboyapati>
Thu 07 Oct 2010 12:14:26 AM UTC, comment #10: 

Other than loop/cycle count truncation was the previous way of doing things, what is the reasoning for preferring truncation to be the new default vs "round up" as the default going forward?

The reason I ask is that given the truncation boundary is different than the previous one and the behavior will be
slightly different anyway, does it still make sense to make truncation the default?

I can live with anything being the default as long as there is a way to tune it to "round up".
But my preference would be to make "round up" the default case, because it ensures the delay is always something and will be at least as long as you ask for without having to have any worries or special cases for 0 cycles.
"round up" as a default would require the user to have to define something to ever get a delay that is less than what is asked for.

==

(truncation and rounding do have a few potential backward compatibility issues when cycle count falls below 1)

I am worried about delays being reduced or even eliminated due to cycle count truncation to 0 or rounding cycle count down to 0 because this is creating a different behavior than the previous code.
I am concerned that this total elimination of delays will break some code that is accidentally depending on the delay from _delay_us() being rounded up to 3 cycles because this new code will completely eliminate some delays that previously were 3 cycles.

For example, if somebody called _delay_us(.500) and was running on 1Mz clock.
Previously they got a 3us delay, but if truncation is the
default and there is no bump to a non zero number of cycles,
they would get no delay, which might not work.

My assumption is that nobody calls _delay_xx() unless they
really wanted/needed some sort of delay, even if just 1 cycle. So I'm wondering if it is ever desirable to completely eliminate a delay?
i.e. should the minimum delay always be at least 1 cycle?

I'm not sure what the answer to this is to make the delay
code better and more accurate going forward without breaking some older code.
But my main concern is the effects of eliminating delays completely.

Even making the minimum delay 1 cycle vs 3 cycles could potentially break some code, but for some reason, to me, a 1 cycle minimum seems to feel a bit better than totally eliminating a delay.
But I'll admit I may not be the best source of input on this as I will always be using "round up".

===

I'm assuming ceil() is a gcc builtin or
could be something like this?

#define _delay_ceil(x) \
 (x == (uint32_t)x ? (uint32_t)x : (uint32_t)x + 1)

==========================================================
Long delays:
On the longer delays. I'm with you on this. I don't see
the need to super long delays either, but thought I'd ask.
My focus has been to ensure that the delay functions work on the very short end for hardware setup timing where there is no other alternative to using CPU spin loops.

==========================================================
Backward compatibility:
As mentioned above, my biggest concern is at the short end
where code may be "accidentally" working by getting additional
delay cycles. There are bound to be some issues, where
people hand tuned things or depend on the "odd" 3/4 cycle
rounding and I think will be no way to guarantee not breaking those (unless there is a backward compatibility mode).
For the short end, there are a few options:
1) let it fall to 0 (eliminate the delay) - this worries me.
2) Force it to be 1 cycle if it falls to 0
3) have another define that sets the MIN cycles so user
   can create some backward combility by defining it to 3 (or 1)
   if necessary. So if this define is not set, it defaults
   to allowing 0 (no delay).

4) have a define to return to old behavior.
   If it was really required or we were really worried about it,
   there could be a backward compatibility define that forced
   it back to the way it works now. i.e. truncation and odd
   rounding to 3/4 cycle/loop boundaries.


==========================================================
supporting delays using variables (non constants).

Yes this is unrelated to this bug.
And does deserve its own bug report.
This is the modification of the existing functions or
creation of new functions to allow users to create delays
that are specified by using a variable vs a constant.

i.e (ignore that optimizer might convert this to const in this simple case)

int x;
  x = 10;
  _delay_ms(x);

Maybe this is best handled by new library functions like
delay_ms(int x);
that is a wrapper around _delay_ms();

Anyway, you are right in that it is not related to this bug
and for as simple as it sounds there are actually quite
a few issues/challenges to make it fully work in the general case.

--- bill

Bill Perry <bperrybap>
Wed 06 Oct 2010 09:43:57 AM UTC, comment #9: 


>> 2) How to round cycle delay values (up, down, closest)
>> Maybe have a compile time ifdef to set this?



Compilation time #ifdef sounds logical with truncate possibly as a default value. A quick implementation would probably look like:

        #if defined(_DELAY_REQUIRES_ROUNDING_)
                FIXME: what if _tmp is -ve?
                __ticks_dc = (uint32_t)(__tmp+0.5);

        #elif defined(_DELAY_REQUIRES_CEILING)
                __ticks_dc = (uint32_t)(ceil(__tmp));

        #else
                truncate by default
                __ticks_dc = (uint32_t)__tmp;
        #endif

 So, ticks less than zero now will not have special treatment. They have to be either rounded up, down or closest as is chosen at compile time.


>> 3) How to deal with very long delays that go beyond
>> the longest delay possible with the underlying delay
>> mechanism.


I think it may not be prudent to support longer delays in milli seconds and in micro seconds. The maximum delay that was supported in the older code (prior to fix) is 6.5535seconds with decreased resolution. The current fix makes it 4294967.295 ms or appx 4294.9s. Any delay beyond this, in my opinion, is definitely possible but probably as a new function _delay_seconds(). Comments ...?


>> 4) backward compatibility


So far I haven't got any clue on how to make it backward compatible. HAS_DELAY_CYCLES is used during avr-libc compilation itself. As of now, once the file delay.h is generated from delay.h.in, it is not possible to use the old code.

Given that current implementation has much better granularity, I think old code should probably be deprecated. I really would like to know others opinion on this.


>> 5) supporting delays using variables (non constants).


Can you give one use case for this that would be help me understand better? If this is something new and not related to current bug, maybe it is good to file a new bug and resolve it.


Thanks for suggestions. They are really helpful. Also, I just realized that the patch I had submitted earlier contained couple of issues.

If we agree on issue #2 and #4, then the patch is ready :-)



Anitha Boyapati <aboyapati>
Tue 05 Oct 2010 06:57:01 PM UTC, comment #8: 

While the fix is "ok". it doesn't solve some of the
other issues that plague these function calls.

The code I proposed a few months back was a quick and dirty solution to correct the grossly miscalculated delays when the gcc compiler toolset had built in delay support by the compiler.
It was not meant to be the long term fix.
I was assuming that this would be quickly tossed in and
the C library immediately re-released, while a better longer
term solution that delt with some of the other delay code issues was created.
Since that didn't happen, I think we have the opportunity to create a better longer term solution.

I also think the rounding issue needs to be better addressed
rather than simply always truncate down but round up when 0.
(My proposal was an attempt to always round up not just at 0)
Rounding down, does not work for some uses.

While the old delay code truncated but rounded 0 up,
the rounding in the old code was so "broken" and the granularity is now so much better, that I don't think the new code needs to necessarily use this truncation methodology.

None of this delay code  is difficult to write or fix.
I'll even write the code if we can agree on what it should do.

As I see it, there are 5 things that need to be addressed:
(some might be longer term than others)

1) fix the delay calculation to pass proper values to
  __builtin_avr_delay_cycles() based on users request.

2) How to round cycle delay values (up, down, closest)
    Maybe have a compile time ifdef to set this?

3) How to deal with very long delays that go beyond
   the longest delay possible with the underlying delay
   mechanism.

4) backward compatibility

I think that non constant delay functions should also be
addressed for the long term since it is a issue/ problem that crops up over and over again in the forums.

5) supporting delays using variables (non constants).


In my view this latest patch/solution, definitely solves #1, which was the biggest issue.
But it does not address #2, #3, or #5.

With respect to #2,
I would still argue that in the absence of being able to specify the rounding style (up,down, closest) in the API, that any cycle rounding should always round up rather than down.
This ensures that you always get a delay at least as long as you request, which is what is needed when using these delays for hardware setup timing delays.
Without a gurarantee of getting at least as long of delay as
requested, there is no way to use these delay functions
for hardware type delays and be guaranteed that you won't violate the hardware timing.
So for me personally, better delay resolution but always rounding down would not be enough for me to leave my existing _delay_us()/_delay_ns() solutions
and return back to using <util/delay.h> functions.
By always rounding down, they simply cannot solve my needs for
creating minimum hardware setup timing independent of F_CPU
clock and I bet my needs are not unique.

Maybe there could be a #define that the user could set
to set the rounding style?

With respect to #3,
The code could easily be altered to do additional loops
to support longer delays.

With respect #5
I would highly recommend that we either add
a wrapper function to the library
delay_ms(ms) that calls _delay_ms(1) in a loop
or update the existing macros to detect non constant
arguments (which can be done) and then create a loop
so that delays using variables can be supported.

I think the easiest and cleanest
would be simply create the new wrapper function
delay_ms() and require that the argument be an integer.

--------------------------------

I'll be happy to participate in anyway including writing
any code and corresponding documentation, but before any solution goes out,
I'd like to get some clarity and position on #2, #3, and #5.


--- bill


Bill Perry <bperrybap>
Tue 05 Oct 2010 10:53:36 AM UTC, comment #7: 

Patch attached. Build verified. Please review.

(file #21625)

Anitha Boyapati <aboyapati>
Mon 04 Oct 2010 07:13:58 AM UTC, comment #6: 

Bill Perry's patch doesn't define the following cases:

1. The maximum delay that can be provided in _ms.

With the new implementation, the values change. If delay_ms() is considered, maximum delay that can be provided is:

_ms = (2^32-1)*(10^-3) /F_CPU in MHz
    = 4294967.295

(Note that the maximum value that can be given to __builtin_avr_delay_cycles() is 0xFFFF FFFF )

If the requested delay is greater than the above value in milli seconds, can the approach be similar to earlier one where the resolution is decreased to 1/10 ms?

2. Similarly maximum delay that can be provided in _us. (Resolution is not decreased in this case, but delay_ms will be called for values greater than (2^32-1)/F_CPU)

Comments... ?

Anitha Boyapati <aboyapati>
Wed 21 Jul 2010 06:25:31 PM UTC, comment #5: 

Eric,
Totally agree with your comments over on avrfreaks
thread:
http://www.avrfreaks.net/index.php?name=PNphpBB2&file=viewtopic&p=725947#725947

The part about delaying the release process was actually kind of funny. :lol:

On a serious note,
issues with <util/delay.h> and proposed fixes have actually been going on for several years now. The original posting about Han's better delay routines were made a couple of years ago.

My overall feeling is that library supplied delay routines need to "just work" and work for hardware type setup timing type of delays as there is no alternative to cycle delays for those types of very short delays.

[b]What can I do to help this along?[/b]

BTW, in testing between Han's delay code sequences and the __builtin_avr_delay_cycles() code, Han's code actually generates smaller code in some cases. It looks like there are some instructions that Han's used that are not being used by GCC. At some point this also might be something to look at.

In the big picture I think the <util/delay.h> routines need to deal with the cycle rounding better.
My opinion is that f you can't pick your rounding style, "always up" seems like the way to go as it ensures that the delays can b e used for hardware setup time delays.

[b]But I'll offer to help in any way.
From creating patches, to updating the doxygen doucmenation.
Just let me know.[/b]
Also, is the "mailing list" you refered to different from posting comments to the savannah bug report?


--- bill

Bill Perry <bperrybap>
Mon 19 Jul 2010 05:27:35 AM UTC, comment #4: 


> But should <util/delah.h> be using autonconfig information directly
> to determine if __built_avr_delay_cycles() is available?


That's the way it is designed right now.

> Why not require it to be told this information at compile time (by
> <avr/builtins.h> ) and in the absence of being told whether these
> functions are built in, assume that they are not?


User won't use it then, ever.  Who do you think would ever define that
macro then?  The user in his Makefile?  Nope, users assume things are
working out of the box.  It's already hard enough to teach them they
have to configure -mmcu=XXX and -DF_CPU=XXX in their Makefiles
(because this information can only be supplied by the user), I don't
trust them they would start investigate whether their compiler
implements a certain feature, and then set a macro based on that.

> Why can't <util/delay.h> include <avr/builtins.h> directly?


What would be gained by this, except of adding the overhead for
opening one more file?  Both simply use the same auto-configured
macro.

> The ideal fix for something like this is to fix GCC itself to create
> defines for all built in functions.


I whished that, too, although I also think it's going to be a
pollution of the macro name space.  Anyway, moot point, these macros
aren't there, and we've already got a hard time convincing the AVR-GCC
maintainers that we really want __builtin_avr_delay_cycles().  If we
extend that to first discussing that each builtin also ships with an
accompanying macro (which would then extend beyond the AVR-specific
feature set, thus require a generic GCC policy decision), we meet
again here in 3...5 years.  I didn't want to wait that long, sorry.

Anyway, if you've got the energy to push such a policy change through
the GCC folks, I'll promise you avr-libc will be the first to use it
then.

Joerg Wunsch <joerg_wunsch>
Group administrator
Sun 18 Jul 2010 11:19:27 PM UTC, comment #3: 

But should <util/delah.h> be using autonconfig information directly to determine if __built_avr_delay_cycles() is available?
Why not require it to be told this information at compile time (by <avr/builtins.h> ) and in the absence of being told whether these functions are built in, assume that they are not?


Why can't <util/delay.h> include <avr/builtins.h> directly?
rather than attempt to figure it out on its own?
And if it can't always include that header, have auto configure determine if it can include this file or not and then <util/delay.h> could disable the built in delay cycles function
instead of including the header if the user didn't explicitly do an override.

The ideal fix for something like this is to fix GCC itself to create defines for all built in functions.
gcc could define __BUILTIN_XXX or __HAS_XXX whenever the internal function XXX is defined. The MIPS gcc compiler creates these defines for every single builtin function manually on its own. The correct thing to do is to fix gcc upstream by adding the single function call to the function that creates internal defines each time the function to create an internal/builtin gcc function is called. It is a single call and would then work across ALL compilers.
That way every time an internal/builtin function is created, the compiler would create a corresponding define.

It is literally a single function call that needs to be added to the compiler to make this happen.

This would allow the preprecessor to have access to a well known define name for every built in gcc function regardless of CPU architecture.
This is so simple and would help solve this type of issue for everyone forever going forward.


--- bill


Bill Perry <bperrybap>
Sun 18 Jul 2010 09:28:04 PM UTC, comment #2: 


> The define define value for __HAS_DELAY_CYCLES
> in the new <util/delah.h> seems backwards.


This define is generated by the configure script, as it is
probing the compiler features.  This is the only way I could
see how to find out whether __builtin_avr_delay_cycles() is
available or not.

The drawback is that a compiled version of the library is
no longer portable into an environment using a compiler that
does not offer __builtin_avr_delay_cycles() when the compiler
used to compile the binary version did offer it.

Joerg Wunsch <joerg_wunsch>
Group administrator
Fri 16 Jul 2010 07:07:18 PM UTC, comment #1: 

As a followup comment

The define define value for __HAS_DELAY_CYCLES
in the new <util/delah.h> seems backwards.

While it may be ok to set it value to 1 by default in
builtins.h this does not seem appropriate in <util/delay.h>

For backward compability with the older compilers,
<util/delay.h> should not assume the presence of the built in
delay cycles functions.

To be fully backward compatible with the older compilers
the <util/delay.h> needs to define it like this:

#ifndef __HAS_DELAY_CYCLES
#define __HAS_DELAY_CYCLES 0
#endif

The definition in builtins.h should be different by setting __HAS_DELAY_CYCLES to 1 as that is the file that defines the compilers builtin functions, in the absence of the compiler doing itself (like the MIPS version of GCC does)

--- bill

Bill Perry <bperrybap>
Mon 05 Jul 2010 10:34:10 PM UTC, original submission:  

WIth the conversion to using __builtin_delay_cycles() the _delay_us() and _delay_ms() functions in <util/delay.h> were broken.

The problem is quite obvious, but the solution is not.
The problem is that the code is using a temporary variable that was calculating the number of "ticks" not cycles. The ticks value was 3 clocks in _delay_us() and 4 clocks in _delay_ms(). The new code is passing this temporary "ticks" value to the built in function __delay_builtin_cycles() rather than a cycle count which results in delaying the incorrect number of cycles.

The reason I say the solution is not obvious is due to both rounding concerns and backward compatibility.

The old routines rounded to a "ticks" value which was more than a single AVR cycle. The original code also always rounded down because of integer truncation. However, when the tick count was zero, a tick count of 1 was used.
This is potentially a backward compatibility issue when converting to the new builtin delay function and use very short microsecond delays.

The reason it can break old code is that old code might only be working "by accident". In other words the user might have been asking for an impossible delay (shorter than a delay "tick") but the code would delay a full tick, which for _delay_us() is 3 cycles. So while a user might have asked for 1 us he would get 3 us. The new code would potentially give a 1 us delay, which might actually break previously working code, which was working "by accident" because the actual delay was longer than what was asked for.

The other issue that comes into play is rounding itself.
For example, there are 3 ways to round cycles.
- up (delay is always at least as long as requested)
- down (delay is closest as possible without going over but can be worst case nearly 1 cycle shorter than requested)
- closest (picks closest cycle which could round up or down)

The are pluses and minuses to all 3 methods.

The real answer is have some sort of reconfigurability in the API itself but that potentially breaks backward compatibility.
While it would be nice to have a rounding option in the API functiuon itself, it could be handled in a limited way with ifdefs that could
be set by the makefile/compiler or by the user setting a #define before including the <util/delay.h> file.


My concern and focus in the past has been on ensuring that the delay functions can be used for low level hardware setup times, which simply was not possible with the previous routines as the truncation would often generate delays that were too short and when the delay were very small the delays might be 1 or 2 cycles longer than what would have been possible.
This can be a very big deal when writing an open source library for something like an i/o intensive GLCD that has many hardware setup times and will be running on a variety of AVRs at different clock rates.
Delays that are too short, and it doesn't function, delays longer than necessary and the performance goes way down.

I have also assumed that when people are asking for delays in the millisecond and longer range that they are not too concerned about the individual cycle rounding.

So until there is a new API for delays that supports a rounding option, I would propose that the code always round up to ensure that a delay is always at least as long as requested.

Yes that means that somebody that asks for a 0.200us (200ns) delay on a 1Mhz AVR gets a 1 us (1 AVR clock) delay, but that is the shortest delay possible in that environment.

I have attached a modified header file to show what I'm talking about (Note: the file attached is untested and is there for example)

If you guys are ok with this method, I will flesh it out, which includes not only full testing but also updating the doxygen documentation in the header file as well to reflect this new delay method its rounding details.

If you think there should be a rounding define/ifdef to allow uses to configure how the rounding works, I can even add that in as well.

A few things to consider when considering something other than "round up".
What happens if the number of cycles needed is less than 1?
Do you delay zero cycles or do you delay 1?
Arguments can be made either way, but for hardware delays, often no delay won't work, so you have to delay at least 1 cycle which on slow F_CPU rates, can be WAY longer than what is desired, but is often still necessary to make things work.
This rounding dilemma is avoided when always rounding up.

--- bill


Bill Perry <bperrybap>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #21669:  bug30363.v3.patch added by aboyapati (5KiB - text/x-patch)
file #21657:  bug30363.v2.patch added by aboyapati (4KiB - text/x-patch)
file #21625:  bug30363.patch added by aboyapati (2KiB - text/x-patch)
file #20904:  delay.h.in added by bperrybap (6KiB - application/octet-stream - Example (untested) <util/delay.h> header file.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by aboyapati (Updated the item)
  • -email is unavailable- added by joerg_wunsch (Posted a comment)
  • -email is unavailable- added by bperrybap (Submitted the item)
  •  

    Follow 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-02-15 aboyapati Discussion LockLocked None
    2011-02-15 aboyapati StatusIn Progress Fixed
        Discussion LockNone Locked
    2010-10-13 aboyapati Percent Complete90% 100%
        Open/ClosedOpen Closed
    2010-10-13 aboyapati Attached File- Added bug30363.v3.patch, #21669
    2010-10-11 aboyapati Attached File- Added bug30363.v2.patch, #21657
    2010-10-06 aboyapati StatusFixed In Progress
    2010-10-05 aboyapati Attached File- Added bug30363.patch, #21625
        StatusConfirmed Fixed
        Percent Complete0% 90%
    2010-09-16 aboyapati StatusNone Confirmed
        Assigned toNone aboyapati
    2010-07-18 joerg_wunsch Item Grouplibc code Header files
    2010-07-05 bperrybap Attached File- Added delay.h.in, #20904

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code