AVR 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 | Percent Complete: | 100% |
Assigned to: | aboyapati | 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: |
Anitha Boyapati <aboyapati>![]() |
Mon 11 Oct 2010 02:26:18 PM UTC, comment #14: Updated patch attached. Please review. |
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
For long delays, I'm assuming that usually the rounding style really doesn't matter because at several milliseconds or more,
And given the granularity of the current delay routines
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"
That is what drove me to abandon
It is also why I lean twards "round up" being the default.
That said, it is just my preference, and it can
Implication wise, I think "round up" with the better granularity
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)
vs if you round or round down/truncate the delay is a little less
==
It also seems like there is agreement that without some define
=========================================================
So I'll toss out a suggestion:
And that there are additional defines to manipulate how
So
1 define (that will eventually be deprecated) that
While my preference is for "round up" being the default,
That way everybody can get what they want and this
The only other thing that would be nice would be either
--- bill
|
Bill Perry <bperrybap> |
Thu 07 Oct 2010 01:34:32 PM UTC, comment #12: Bill Perry:
...
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.
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:
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.
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
I can live with anything being the default as long as there is a way to tune it to "round up".
==
(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.
For example, if somebody called _delay_us(.500) and was running on 1Mz clock.
My assumption is that nobody calls _delay_xx() unless they
I'm not sure what the answer to this is to make the delay
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.
===
I'm assuming ceil() is a gcc builtin or
#define _delay_ceil(x) \
==========================================================
==========================================================
4) have a define to return to old behavior.
==========================================================
Yes this is unrelated to this bug.
i.e (ignore that optimizer might convert this to const in this simple case)
int x;
Maybe this is best handled by new library functions like
Anyway, you are right in that it is not related to this bug
--- bill |
Bill Perry <bperrybap> |
Wed 06 Oct 2010 09:43:57 AM UTC, comment #9:
Compilation time #ifdef sounds logical with truncate possibly as a default value. A quick implementation would probably look like:
#if defined(_DELAY_REQUIRES_ROUNDING_)
#elif defined(_DELAY_REQUIRES_CEILING)
#else
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.
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.
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
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.
I also think the rounding issue needs to be better addressed
While the old delay code truncated but rounded 0 up,
None of this delay code is difficult to write or fix.
As I see it, there are 5 things that need to be addressed:
1) fix the delay calculation to pass proper values to
2) How to round cycle delay values (up, down, closest)
3) How to deal with very long delays that go beyond
4) backward compatibility
I think that non constant delay functions should also be
5) supporting delays using variables (non constants).
In my view this latest patch/solution, definitely solves #1, which was the biggest issue.
With respect to #2,
Maybe there could be a #define that the user could set
With respect to #3,
With respect #5
I think the easiest and cleanest
--------------------------------
I'll be happy to participate in anyway including writing
--- bill
|
Bill Perry <bperrybap> |
Tue 05 Oct 2010 10:53:36 AM UTC, comment #7: Patch attached. Build verified. Please review. |
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
(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,
The part about delaying the release process was actually kind of funny. :lol:
On a serious note,
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.
[b]But I'll offer to help in any way.
--- bill |
Bill Perry <bperrybap> |
Mon 19 Jul 2010 05:27:35 AM UTC, comment #4:
Anyway, if you've got the energy to push such a policy change through
|
Joerg Wunsch <joerg_wunsch>![]() |
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 can't <util/delay.h> include <avr/builtins.h> directly?
The ideal fix for something like this is to fix GCC itself to create defines for all built in functions.
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.
--- bill
|
Bill Perry <bperrybap> |
Sun 18 Jul 2010 09:28:04 PM UTC, comment #2:
The drawback is that a compiled version of the library is
|
Joerg Wunsch <joerg_wunsch>![]() |
Fri 16 Jul 2010 07:07:18 PM UTC, comment #1: As a followup comment
The define define value for __HAS_DELAY_CYCLES
While it may be ok to set it value to 1 by default in
For backward compability with the older compilers,
To be fully backward compatible with the older compilers
#ifndef __HAS_DELAY_CYCLES
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 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.
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.
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.
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.
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".
--- bill
|
Bill Perry <bperrybap> |
Depends on the following items: None found
Items that depend on this one: None found
Follow 15 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2011-02-15 | aboyapati | Discussion Lock | Locked | ![]() |
Unlocked |
2011-02-15 | aboyapati | Status | In Progress | ![]() |
Fixed |
Discussion Lock | Unlocked | ![]() |
Locked | ||
2010-10-13 | aboyapati | Percent Complete | 90% | ![]() |
100% |
Open/Closed | Open | ![]() |
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 | Status | Fixed | ![]() |
In Progress |
2010-10-05 | aboyapati | Attached File | - | ![]() |
Added bug30363.patch, #21625 |
Status | Confirmed | ![]() |
Fixed | ||
Percent Complete | 0% | ![]() |
90% | ||
2010-09-16 | aboyapati | Status | None | ![]() |
Confirmed |
Assigned to | None | ![]() |
aboyapati | ||
2010-07-18 | joerg_wunsch | Item Group | libc code | ![]() |
Header files |
2010-07-05 | bperrybap | Attached File | - | ![]() |
Added delay.h.in, #20904 |
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)