Updated patch attached. Please review.

(file #21657)

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 ?

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

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'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... ?

> 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.

> 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.

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