[En-Nut-Discussion] NutGetTickClock rounding (again)

[Henrik Maier]

Bernhard,

> - If 'ms' is 'big', and if the compiler evaluates the multiplication
first,
> then an overflow can happen.

The problem with the current implementation is not an overflow issue but a
rounding issue. It is simply a matter of applying proper rounding to an
integer division. An overflow issue only occurs if "ms" is > 49.7 days (for
a typical NutGetTickClock of ~1000).

However it is probably a good suggestion to document the potential overflow
for NutTimerMillisToTicks and add a caveat to its documentation. 

However my concern with function NutTimerMillisToTicks is small values for
the "ms" parameter, values between 1 and 10 ms for example.

> - since NutGetTickClock() gives a result in 'ticks per Hertz' then it
seems
> more appropriate to reduce 'ms' to a value in seconds before the next
> calculation.

In theory this is correct, but as we don't have floating point math,
impracticable.

Your implementation would return 1 for all ms values < 1000 as anything
smaller than 1000 divided by 1000 equals to 0 in integer land.

Henrik

> -----Original Message-----
> From: en-nut-discussion-bounces at egnite.de [mailto:en-nut-discussion-
> bounces at egnite.de] On Behalf Of Bernard Fouché
> Sent: Tuesday, 15 July 2008 7:04 PM
> To: Ethernut User Chat (English)
> Subject: Re: [En-Nut-Discussion] NutGetTickClock rounding (again)
> 
> What about:
> 
> u_long NutTimerMillisToTicks(u_long ms)
> {
>     u_long x;
> 
>     x = ( ms/1000UL ) * NutGetTickClock();
>     if (x == 0) {
>         x = 1;
>     }
> 
>     return (x);
> }
> 
> This because:
> 
> - If 'ms' is 'big', and if the compiler evaluates the multiplication
first,
> then an overflow can happen.
> 
> - since NutGetTickClock() gives a result in 'ticks per Hertz' then it
seems
> more appropriate to reduce 'ms' to a value in seconds before the next
> calculation.
> 
>     Bernard
> 
> Henrik Maier wrote:
> > There has be some discussion about a better implementation and bug-fix
of
> > NutTimerMillisToTicks (refer to posts in 2006, subject "Request for
> Comment:
> > NutTimerMillisToTicks()").
> >
> > This has been applied in recent Nut/OS releases (>4.2.1 onwards).
> >
> > However the current implementation does only round for the special case
> when
> > the result would be 0 (see if clause below). However for other cases
with
> > small ms values it does not round at all and we end up with large errors
> > (50%) for small ms values in cases where NutGetTickClock() is just below
> > 1000, for example 997. For clock frequencies where NutGetTickClock() is
> > greater than 1000 it does not matter but for cases where it is less than
> > 1000 it results in unnecessary inaccuracies.
> >
> > I came across this issue when recently upgrading from Nut/OS 4.2.1 to
> 4.4.1
> > and some of my time-out calculations did not work anymore as they were
> too
> > short.
> >
> >
> > Current implementation:
> > -----------------------
> >
> > u_long NutTimerMillisToTicks(u_long ms)
> > {
> >     u_long x;
> >
> >     x = ms * NutGetTickClock() / 1000UL;
> >     if (x == 0) {
> >         x = 1;
> >     }
> >
> >     return (x);
> > }
> >
> > [..snip..]
> >
> > Suggested implementation:
> > -------------------------
> > u_long NutTimerMillisToTicks(u_long ms)
> > {
> >     return (ms * NutGetTickClock() + 500) / 1000UL;
> > }
> >
> _______________________________________________
> http://lists.egnite.de/mailman/listinfo/en-nut-discussion