Print node ID and time when printing routing tables

Print node ID and time when printing routing tables

[Tommaso Pecorella, 2015-11-03 22:48:39]

Better pretty-printing of routing tables

[n.p, 2015-11-04 15:00:40]

For me looks good. Only one (optional) comment.

https://codereview.appspot.com/275100043/diff/1/src/olsr/model/olsr-routing-p...
File src/olsr/model/olsr-routing-protocol.cc (right):

https://codereview.appspot.com/275100043/diff/1/src/olsr/model/olsr-routing-p...
src/olsr/model/olsr-routing-protocol.cc:289: *os << std::endl;
Why not adding the info that GetNRoutes returned 0 (No routes found) ?

[Peter Barnes, 2015-11-04 18:39:15]

https://codereview.appspot.com/275100043/diff/1/src/aodv/model/aodv-routing-p...
File src/aodv/model/aodv-routing-protocol.cc (right):

https://codereview.appspot.com/275100043/diff/1/src/aodv/model/aodv-routing-p...
src/aodv/model/aodv-routing-protocol.cc:302: << " Time: " << Simulator::Now
().GetSeconds () << "s "
I would prefer

  << " Time: " << Now().As (Time::S) 
  << "AODV..."

Ditto other files.

[Tommaso Pecorella, 2015-11-04 22:01:01]

On 2015/11/04 21:59:20, Tommaso Pecorella wrote:

Forgot Ipv4StaticRouting, but it will be in the final patch.

[Tom Henderson, 2015-11-05 21:55:55]

On 2015/11/04 18:39:15, Peter Barnes wrote:
>
https://codereview.appspot.com/275100043/diff/1/src/aodv/model/aodv-routing-p...
> File src/aodv/model/aodv-routing-protocol.cc (right):
> 
>
https://codereview.appspot.com/275100043/diff/1/src/aodv/model/aodv-routing-p...
> src/aodv/model/aodv-routing-protocol.cc:302: << " Time: " << Simulator::Now
> ().GetSeconds () << "s "
> I would prefer
> 
>   << " Time: " << Now().As (Time::S) 
>   << "AODV..."
> 
> Ditto other files.

I didn't know about this As() method but I agree that this is nicer than having
to add the "s" later.

Slightly off topic, but:
- do you know why there is a prepending "+" sign with this format?  as in:
``+3140.0ms`.  Is the value ever negative? Or is this following a convention?
- do you think we should promote this as a best current practice and change some
other files, such as the tutorial and e.g.
src/core/examples/sample-simulator.cc?

[Tom Henderson, 2015-11-05 22:02:57]

On 2015/11/05 21:55:55, Tom Henderson wrote:
> On 2015/11/04 18:39:15, Peter Barnes wrote:
> >
>
https://codereview.appspot.com/275100043/diff/1/src/aodv/model/aodv-routing-p...
> > File src/aodv/model/aodv-routing-protocol.cc (right):
> > 
> >
>
https://codereview.appspot.com/275100043/diff/1/src/aodv/model/aodv-routing-p...
> > src/aodv/model/aodv-routing-protocol.cc:302: << " Time: " << Simulator::Now
> > ().GetSeconds () << "s "
> > I would prefer
> > 
> >   << " Time: " << Now().As (Time::S) 
> >   << "AODV..."
> > 
> > Ditto other files.
> 
> I didn't know about this As() method but I agree that this is nicer than
having
> to add the "s" later.
> 
> Slightly off topic, but:
> - do you know why there is a prepending "+" sign with this format?  as in:
> ``+3140.0ms`.  Is the value ever negative? Or is this following a convention?
> - do you think we should promote this as a best current practice and change
some
> other files, such as the tutorial and e.g.
> src/core/examples/sample-simulator.cc?

third related question:  
- should Now() instead be Node::GetLocalTime() going forward?

[Peter Barnes, 2015-11-05 22:59:46]

> On Nov 5, 2015, at 2:02 PM, tomh.org@gmail.com wrote:
> 
> On 2015/11/05 21:55:55, Tom Henderson wrote:
>
https://codereview.appspot.com/275100043/diff/1/src/aodv/model/aodv-routing-p...
>> > src/aodv/model/aodv-routing-protocol.cc:302: << " Time: " <<
Simulator::Now
>> > ().GetSeconds () << "s “
>> 
>> > I would prefer
>> >
>> >   << " Time: " << Now().As (Time::S)
>> >   << "AODV..."
>> >
>> > Ditto other files.
> 
> I didn't know about this As() method but I agree that this is nicer
> than having to add the "s" later.
> 
> Slightly off topic, but:
> - do you know why there is a prepending "+" sign with this format?  as in: 
``+3140.0ms`.  Is the value ever negative? Or is this following a convention?

operator<< (Time) has acted like std::showpos() since almost forever
(History: r6821 had os << (uint64_t) , so no showpos
  r9134 had os << (int64x64_t), and int64x64_t acts like showpos
  Not clear when the change was made.  It’s been like that ever since.
\history)

Absolute (virtual) Times can’t be negative, but time differences (T1 - T2) can.

> - do you think we should promote this as a best current practice and 
> change some other files, such as the tutorial and e.g.
> src/core/examples/sample-simulator.cc?

Sure.  When I implemented Time::As that was the direction I was going, but I
didn’t have the time or inclination then to push that system wide.


> third related question:
> - should Now() instead be Node::GetLocalTime() going forward?

Yes, in principle.  (I missed that Matt’s local clock patch had been pushed. 
Good work, Matt.)   

IIRC we need two (or three) bits of work first:

1.  Go through existing code base and decide which Now ()’s should become
GetNode ()->GetLocalTime ().  Not all of them should be changed.  

IIRC Channels need to convert from local time to (true) virtual time when
sending packets between nodes.  SpectrumPhy and friends are probably in the same
boat.  What about other physical layer components?Anything which gets data from
more than one Node should be looked at carefully.  There are other places,
mostly in core, which should only use virtual time, basically anywhere not
modeling dependence on a local hardware clock.

2.  Better documentation explaining the rationale behind #1.  I think there is
almost nothing at the moment, just doxygen at Node;:GetLocalTime, nothing even
in the description of the Node class.  At minimum this should be explained in
- doxygen for Node
- a sentence about local clocks in the tutorial Conceptual Overview#Node
- a section in Models > Node and NetDevices Overview
- a section (mostly a placeholder at this point) about noisy clocks

3.  An implementation of a noisy local clock.  The docs will make more sense
with an implementation and example.

My big concern is that the distinction between Now() and GetLocalTime() will be
confusing, so we better be prepared when we make widespread use of local
clocks.

Peter
____________
Peter Barnes
pdbj@mac.com

[Peter Barnes, 2015-11-05 23:03:54]

On Nov 5, 2015, at 2:59 PM, Peter Barnes <pdbj@mac.com> wrote:
> 3.  An implementation of a noisy local clock.  The docs will make more sense
with an implementation and example.

A good starting point for implementing noisy clocks:
https://faculty.washington.edu/dbp/PDFFILES/tech-report-02-03.pdf

P
____________
Peter Barnes
pdbj@mac.com

[Tommaso Pecorella, 2015-11-06 00:45:02]

On 2015/11/05 23:03:54, Peter Barnes wrote:
> On Nov 5, 2015, at 2:59 PM, Peter Barnes <mailto:pdbj@mac.com> wrote:
> > 3.  An implementation of a noisy local clock.  The docs will make more sense
> with an implementation and example.
> 
> A good starting point for implementing noisy clocks:
> https://faculty.washington.edu/dbp/PDFFILES/tech-report-02-03.pdf

I agree on all, but I wonder if it's not better to have an output like:
Node ##, Time is ####, Local Clock is ####

The rationale is: the output is to help the user understand what's going on with
the routing tables. The printing function is called by an helper which is
scheduled with an absolute (simulation-wide, exact) time.
Just imagine the storm of questions arising from this case "I scheduled a rating
table writing every 3 seconds and the nodes are all printing stuff at different
times that are more or less the one I said but not the time I said, there's a
bug".
Also, we need an "official" way to all the different times, in order to make
sure that everyone is referring to the right thing in the right place.

[Peter Barnes, 2015-11-06 01:02:54]

[Reply about noisy clock issues dropped this cc, since that discussion isn't
relevant for this review.]