Bug 1761

Summary: Rounding with olsr::EmfToSeconds
Product: ns-3 Reporter: Brian Swenson <bswenson3>
Component: olsrAssignee: Gustavo J. A. M. Carneiro <gjcarneiro>
Status: RESOLVED FIXED    
Severity: normal CC: ns-bugs, pdbarnes, suresh.lalith, tommaso.pecorella
Priority: P5    
Version: ns-3-dev   
Hardware: PC   
OS: All   
See Also: https://www.nsnam.org/bugzilla/show_bug.cgi?id=1856
https://www.nsnam.org/bugzilla/show_bug.cgi?id=1904
Attachments: Simple sample program demonstrating the issue
bug fix

Description Brian Swenson 2013-09-11 11:34:29 UTC
Created attachment 1669 [details]
Simple sample program demonstrating the issue

Hello

The following came up when I was comparing pcaps obtained running the regression tests for OLSR using the --int64x64-as-double configuration option in ns-3.

The pcaps with this option differ from the pcaps without this option.  One of the differences is with the validity time of messages.  These are converted using the functions EmfToSeconds and SecondsToEmf.  EmfToSeconds is rounding up even on small decimal values.

std::cout << EmfToSeconds(SecondsToEmf(15.0)) << std::endl;
std::cout << EmfToSeconds(SecondsToEmf(15.000000000000002)) << std::endl;
std::cout << EmfToSeconds(SecondsToEmf(15.1)) << std::endl;

returns the following:

15
15.5
15.5

EmfToSeconds and SecondsToEmf are located in src/olsr/model/olsr-header.cc/.h

When using --int64x64-as-double there ends up being a small decimal value on the original time which appears to be automatically rounded up.  Thus the pcaps are different and the regression tests fail.  Since it appears that SecondsToEmf is only accurate to .5 second perhaps the function should first round the value to be converted?
Comment 1 Peter Barnes 2014-03-02 04:14:46 UTC
Brian, could you please check this again with

Bug 1856 Patch r10637 67601c471c22
http://code.nsnam.org/ns-3-dev/rev/67601c471c22
Comment 2 Tommaso Pecorella 2015-10-31 17:15:12 UTC
Created attachment 2170 [details]
bug fix

Dunno if the problem with int128 is still there, but the problem isn't the int128.

The issue is that the function is rounding numbers to the highest fraction, and not to the nearest. The standard is quite explicit on this:
     -    compute the expression 16*(T/(C*(2^b))-1), which may not be a
          integer, and round it up.  This results in the value for 'a'

Notice: "round it up". In the code we do a ceil, and that's not rounding.
Comment 3 Tommaso Pecorella 2015-10-31 17:38:14 UTC
Pushed in changeset:   11746:5daa429b3df7
Comment 4 Gustavo J. A. M. Carneiro 2015-11-01 05:56:14 UTC
(In reply to Tommaso Pecorella from comment #2)
> Created attachment 2170 [details]
> bug fix
> 
> Dunno if the problem with int128 is still there, but the problem isn't the
> int128.
> 
> The issue is that the function is rounding numbers to the highest fraction,
> and not to the nearest. The standard is quite explicit on this:
>      -    compute the expression 16*(T/(C*(2^b))-1), which may not be a
>           integer, and round it up.  This results in the value for 'a'
> 
> Notice: "round it up". In the code we do a ceil, and that's not rounding.

Not sure I agree.  Doesn't ceil() precisely mean "rounding up"?  In [1], ceil(x) is described as "Rounds x upward".

[1] http://www.cplusplus.com/reference/cmath/ceil/