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

[Bernard Fouché]

Henrik, if ms is directly multiplied by 'Ticks in Hertz', then the 
maximum value of argument 'ms' has to be ((2^32)-1)/'Ticks in Hertz'.  
If there are 1000 ticks per seconds, that's about 1 hour and 11 minutes, 
very far from 49 days so already in that function the overflow problem 
is to be handled.

I think that the 'arm arch' function is good, however I wonder why the 
person who wrote the code split the choice between  the two calculus 
with 0x3E8000. If overflow is to be avoided because of a multiplication 
by 1000, then it should be ((2^32)-1)/1000 (4294967 in decimal, or hex 
0x418937. Using 1024 ticks per second would give 0x400000).

Now the rounding problem comes from having a number of ticks per second 
slightly lower than the divisor (1000). I don't think that adding 500 
fixes the problem, since you then have rounding errors  if you have a 
number of ticks slightly lower than 500 :)

What about (for the second calculus, the first being kept 'as is' even 
if 0x3E8000 is still 'magical'):

((ms * NutGetTickClock()) + (NutGetTickClock ()/2)) / 1000UL;


  Bernard

ennut at thezachariasgroup.com wrote:
>    In the ARM arch the following code works well:
>
> /*!
>   * \brief Calculate system ticks for a given number of milliseconds.
>   */
> u_long NutTimerMillisToTicks(u_long ms) {
>      if (ms >= 0x3E8000) {
>        return (ms / 1000) * NutGetTickClock();
>      }
>      else {
>        return (ms * NutGetTickClock()) / 1000;
>      }
> }
>
>
> At 07:11 AM 7/15/2008, you wrote:
>   
>> 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
>>>       
>> _______________________________________________
>> http://lists.egnite.de/mailman/listinfo/en-nut-discussion
>>     
>
> _______________________________________________
> http://lists.egnite.de/mailman/listinfo/en-nut-discussion
>
>