Ns-3 Net-anim Interface

[Tom Henderson, 2009-09-13 01:59:05]

I would like to see these points considered before merging:

1) align topology helpers with what we have previously done
2) consider to put animation helper in src/contrib (and the node location helper
merged with animation helper), until a broader plan for animation helper is
developed (i.e. is this just a NetAnim helper or more broadly an animation
helper)
3) consider whether the new trace source added  should be in the channel
instead.

http://codereview.appspot.com/117051/diff/1/7
File src/devices/point-to-point/point-to-point-net-device.h (right):

http://codereview.appspot.com/117051/diff/1/7#newcode399
Line 399: 
I may have asked this before, but why is this callback in the NetDevice instead
of in the Channel?  It seems to just indirect through the NetDevice.

http://codereview.appspot.com/117051/diff/1/9
File src/helper/animation-interface.h (right):

http://codereview.appspot.com/117051/diff/1/9#newcode42
Line 42: class AnimationInterface
Is this intended to be a helper class for all future animators or just NetAnim
in particular?

http://codereview.appspot.com/117051/diff/1/9#newcode85
Line 85: bool SetInternalAnimation ();
Since this is not implemented, do we need this method now?

http://codereview.appspot.com/117051/diff/1/13
File src/helper/grid-helper.h (right):

http://codereview.appspot.com/117051/diff/1/13#newcode31
Line 31: class GridHelper
Defining a separate topology-specific helper API goes against the grain of the
current topology helpers which can be found in the device helper APIs.  For
instance, making a method PointToPointHelper::InstallGrid () would be more in
line with the API convention.

http://codereview.appspot.com/117051/diff/1/19
File src/helper/node-location.h (right):

http://codereview.appspot.com/117051/diff/1/19#newcode36
Line 36: class NodeLocation : public Object
This class doesn't seem to have anything to do with NodeLocation generically,
but is specific to the animator-- can it be added to the animation helper
instead?

[Josh Pelkey, 2009-09-16 18:33:01]

On 2009/09/13 01:59:05, Tom H. wrote:
> I would like to see these points considered before merging:
> 
> 1) align topology helpers with what we have previously done
> 2) consider to put animation helper in src/contrib (and the node location
helper
> merged with animation helper), until a broader plan for animation helper is
> developed (i.e. is this just a NetAnim helper or more broadly an animation
> helper)
> 3) consider whether the new trace source added  should be in the channel
> instead.
> 
> http://codereview.appspot.com/117051/diff/1/7
> File src/devices/point-to-point/point-to-point-net-device.h (right):
> 
> http://codereview.appspot.com/117051/diff/1/7#newcode399
> Line 399: 
> I may have asked this before, but why is this callback in the NetDevice
instead
> of in the Channel?  It seems to just indirect through the NetDevice.
> 
> http://codereview.appspot.com/117051/diff/1/9
> File src/helper/animation-interface.h (right):
> 
> http://codereview.appspot.com/117051/diff/1/9#newcode42
> Line 42: class AnimationInterface
> Is this intended to be a helper class for all future animators or just NetAnim
> in particular?
> 
> http://codereview.appspot.com/117051/diff/1/9#newcode85
> Line 85: bool SetInternalAnimation ();
> Since this is not implemented, do we need this method now?
> 
> http://codereview.appspot.com/117051/diff/1/13
> File src/helper/grid-helper.h (right):
> 
> http://codereview.appspot.com/117051/diff/1/13#newcode31
> Line 31: class GridHelper
> Defining a separate topology-specific helper API goes against the grain of the
> current topology helpers which can be found in the device helper APIs.  For
> instance, making a method PointToPointHelper::InstallGrid () would be more in
> line with the API convention.
> 
> http://codereview.appspot.com/117051/diff/1/19
> File src/helper/node-location.h (right):
> 
> http://codereview.appspot.com/117051/diff/1/19#newcode36
> Line 36: class NodeLocation : public Object
> This class doesn't seem to have anything to do with NodeLocation generically,
> but is specific to the animator-- can it be added to the animation helper
> instead?

A second patch set has been posted.  This includes name changes to the helper
classes as well as some code modification so that IP addresses and the stack can
be assigned outside of the helper class.  The helper classes are still based on
the newly proposed methodology.

[Tom Henderson, 2009-09-17 05:14:01]

http://codereview.appspot.com/117051/diff/51/1028
File src/helper/animation-interface.h (right):

http://codereview.appspot.com/117051/diff/51/1028#newcode85
Line 85: bool SetInternalAnimation ();
I still suggest to delete this non-implemented method.  I didn't realize that
ns-3 will need to be built with qt.

http://codereview.appspot.com/117051/diff/51/1034
File src/helper/node-location.h (right):

http://codereview.appspot.com/117051/diff/51/1034#newcode36
Line 36: class NodeLocation : public Object
Am still not clear that this is the right name for this object (as opposed to
"CanvasLocation" or "NodeLayoutLocation" or something distinct from conveying
that this is a position that affects propagation delay) or whether we want to
handle this separate from the MobilityModel positioning (which could be exported
to the animator also).

http://codereview.appspot.com/117051/diff/51/1037
File src/helper/point-to-point-grid-helper.cc (right):

http://codereview.appspot.com/117051/diff/51/1037#newcode81
Line 81: #ifdef NS3_MPI
should this be part of MPI patch?

http://codereview.appspot.com/117051/diff/51/1037#newcode148
Line 148: // Assign addresses to all row devices in the grid.
maybe a bounds check here that rowIp and colIp do not result in overlapping
addresses?

http://codereview.appspot.com/117051/diff/51/1037#newcode187
Line 187: double xDist = lrx - ulx;
there is no sanity checking of these inputs

http://codereview.appspot.com/117051/diff/51/1037#newcode223
Line 223: // Right now this just gets one of the addresses of the
any bounds checking needed on input parameters?

http://codereview.appspot.com/117051/diff/51/1038
File src/helper/point-to-point-grid-helper.h (right):

http://codereview.appspot.com/117051/diff/51/1038#newcode39
Line 39: Ipv4Address GetAddress (uint32_t row, uint32_t col);
how will IPv6 be handled?  Overload GetAddress() or change to GetIpv4Address()?

[faker.moatamri, 2009-11-09 14:22:13]

http://codereview.appspot.com/117051/diff/8027/8028
File examples/animation/test-dumbbell-animation.cc (right):

http://codereview.appspot.com/117051/diff/8027/8028#newcode1
examples/animation/test-dumbbell-animation.cc:1: /* -*- Mode:C++;
c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
If it is a test it should be included in the test-case/test-examples framework

http://codereview.appspot.com/117051/diff/8027/8028#newcode86
examples/animation/test-dumbbell-animation.cc:86:
clientHelper.SetAttribute("Remote", remoteAddress);
coding style in all your files

http://codereview.appspot.com/117051/diff/8027/8029
File examples/animation/test-grid-animation.cc (right):

http://codereview.appspot.com/117051/diff/8027/8029#newcode49
examples/animation/test-grid-animation.cc:49: if (xSize < 2 && ySize < 2)
you can gather those 3 ifs into one with a multiple conditions

http://codereview.appspot.com/117051/diff/8027/8045
File src/helper/animation-interface.cc (right):

http://codereview.appspot.com/117051/diff/8027/8045#newcode94
src/helper/animation-interface.cc:94: return false; // Not implemented yet
any projects?

http://codereview.appspot.com/117051/diff/8027/8046
File src/helper/animation-interface.h (right):

http://codereview.appspot.com/117051/diff/8027/8046#newcode21
src/helper/animation-interface.h:21: #ifndef __ANIMATION_INTERFACE__H__
Usually in NS3 it is written as ANIMATION_INTERFACE_H, please change it in all
files

http://codereview.appspot.com/117051/diff/8027/8046#newcode49
src/helper/animation-interface.h:49: /**
please add a destructor, good practices

http://codereview.appspot.com/117051/diff/8027/8049
File src/helper/point-to-point-dumbbell-helper.cc (right):

http://codereview.appspot.com/117051/diff/8027/8049#newcode135
src/helper/point-to-point-dumbbell-helper.cc:135: // Assign to right size
you mean side?

http://codereview.appspot.com/117051/diff/8027/8050
File src/helper/point-to-point-dumbbell-helper.h (right):

http://codereview.appspot.com/117051/diff/8027/8050#newcode40
src/helper/point-to-point-dumbbell-helper.h:40: PointToPointHelper&
bottleneckHelper);
destructor, good practices

http://codereview.appspot.com/117051/diff/8027/8051
File src/helper/point-to-point-grid-helper.cc (right):

http://codereview.appspot.com/117051/diff/8027/8051#newcode47
src/helper/point-to-point-grid-helper.cc:47: rowNodes.Create(1);
space before parentheses

http://codereview.appspot.com/117051/diff/8027/8051#newcode145
src/helper/point-to-point-grid-helper.cc:145: double xAdder = xDist / m_xSize;
m_xSize cannot be 0?

[Josh Pelkey, 2009-11-13 16:59:45]

Updates after Faker's comments.

http://codereview.appspot.com/117051/diff/8027/8028
File examples/animation/test-dumbbell-animation.cc (right):

http://codereview.appspot.com/117051/diff/8027/8028#newcode1
examples/animation/test-dumbbell-animation.cc:1: /* -*- Mode:C++;
c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
On 2009/11/09 14:22:13, faker.moatamri wrote:
> If it is a test it should be included in the test-case/test-examples framework

This is really just a simple example of how to create a dumbbell topology and
set up the animation.  Would you like me to change the file name?

http://codereview.appspot.com/117051/diff/8027/8028#newcode86
examples/animation/test-dumbbell-animation.cc:86:
clientHelper.SetAttribute("Remote", remoteAddress);
On 2009/11/09 14:22:13, faker.moatamri wrote:
> coding style in all your files

Done.

http://codereview.appspot.com/117051/diff/8027/8029
File examples/animation/test-grid-animation.cc (right):

http://codereview.appspot.com/117051/diff/8027/8029#newcode49
examples/animation/test-grid-animation.cc:49: if (xSize < 2 && ySize < 2)
On 2009/11/09 14:22:13, faker.moatamri wrote:
> you can gather those 3 ifs into one with a multiple conditions

Done.

http://codereview.appspot.com/117051/diff/8027/8045
File src/helper/animation-interface.cc (right):

http://codereview.appspot.com/117051/diff/8027/8045#newcode94
src/helper/animation-interface.cc:94: return false; // Not implemented yet
On 2009/11/09 14:22:13, faker.moatamri wrote:
> any projects?

took this out for now

http://codereview.appspot.com/117051/diff/8027/8046
File src/helper/animation-interface.h (right):

http://codereview.appspot.com/117051/diff/8027/8046#newcode21
src/helper/animation-interface.h:21: #ifndef __ANIMATION_INTERFACE__H__
On 2009/11/09 14:22:13, faker.moatamri wrote:
> Usually in NS3 it is written as ANIMATION_INTERFACE_H, please change it in all
> files

Done.

http://codereview.appspot.com/117051/diff/8027/8046#newcode49
src/helper/animation-interface.h:49: /**
On 2009/11/09 14:22:13, faker.moatamri wrote:
> please add a destructor, good practices

Done.

http://codereview.appspot.com/117051/diff/8027/8049
File src/helper/point-to-point-dumbbell-helper.cc (right):

http://codereview.appspot.com/117051/diff/8027/8049#newcode135
src/helper/point-to-point-dumbbell-helper.cc:135: // Assign to right size
On 2009/11/09 14:22:13, faker.moatamri wrote:
> you mean side?

Done.

http://codereview.appspot.com/117051/diff/8027/8050
File src/helper/point-to-point-dumbbell-helper.h (right):

http://codereview.appspot.com/117051/diff/8027/8050#newcode40
src/helper/point-to-point-dumbbell-helper.h:40: PointToPointHelper&
bottleneckHelper);
On 2009/11/09 14:22:13, faker.moatamri wrote:
> destructor, good practices

Done.

http://codereview.appspot.com/117051/diff/8027/8051
File src/helper/point-to-point-grid-helper.cc (right):

http://codereview.appspot.com/117051/diff/8027/8051#newcode47
src/helper/point-to-point-grid-helper.cc:47: rowNodes.Create(1);
On 2009/11/09 14:22:13, faker.moatamri wrote:
> space before parentheses

My space bar is really hard to press :)

http://codereview.appspot.com/117051/diff/8027/8051#newcode145
src/helper/point-to-point-grid-helper.cc:145: double xAdder = xDist / m_xSize;
On 2009/11/09 14:22:13, faker.moatamri wrote:
> m_xSize cannot be 0?

fixed by adding a check to the constructor.

[Tom Henderson, 2009-11-16 05:31:43]

I support moving this out of contrib for ns-3.7.  However, I would like to see
the following changes before merge.
1) resolve my concerns about NodeLocation class name listed below
2) this patchset changes the convention for topology objects in ns-3.  If we
agree on PointToPointGridHelper class, then we should change
PointToPointHelper::InstallStar() to PointToPointStarHelper class (likewise for
CsmaHelper::InstallStar) or, we should make
PointToPointGridHelper/PointToPointDumbbellHelper
specializations of PointToPointHelper, overriding Install or other methods as
needed.
3) RELEASE NOTES and CHANGES.html

I would like to see the following done before ns-3.7 release.
- transfer wiki content to the manual chapter doc/manual/animation.texi
(including screenshots)
 -- clarify the scope of applicability (PointToPoint, interaction with MPI
distributed, etc.)
 -- comment on the prospects for extending this to broadcast channels  Is such
an extension in work?  If not, would you accept patches to extend it for Csma,
Wifi, and Wimax?  Or do you see this animator as being fundamentally
incompatible with such devices, and limited to PointToPoint-ish devices?
- Doxygen on new public API

http://codereview.appspot.com/117051/diff/12002/11020
File src/helper/node-location.cc (right):

http://codereview.appspot.com/117051/diff/12002/11020#newcode30
src/helper/node-location.cc:30: .AddAttribute ("Position", "The current position
of the mobility model.",
this attribute has nothing to do with Position or mobility model.  See my other
comment about possible user confusion with this class's name/attribute.

http://codereview.appspot.com/117051/diff/12002/11021
File src/helper/node-location.h (right):

http://codereview.appspot.com/117051/diff/12002/11021#newcode40
src/helper/node-location.h:40: class NodeLocation : public Object
class name:  why is this limited to Node?  Also, I am very concerned that this
NodeLocation will be confused with Position by users.  I commented on this
before. 

Suggested name changes:
 - Coordinate
 - VectorCoordinate
 - CanvasLocation

http://codereview.appspot.com/117051/diff/12002/11023
File src/helper/point-to-point-dumbbell-helper.h (right):

http://codereview.appspot.com/117051/diff/12002/11023#newcode48
src/helper/point-to-point-dumbbell-helper.h:48: Ipv4Address GetRightIpv4Address
(uint32_t) const; // Get right leaf address
We now support IPv4 and IPv6 in ns-3 so I think we should start to try to fully
support IPv6 in our other classes.

http://codereview.appspot.com/117051/diff/12002/11025
File src/helper/point-to-point-grid-helper.h (right):

http://codereview.appspot.com/117051/diff/12002/11025#newcode32
src/helper/point-to-point-grid-helper.h:32: class PointToPointGridHelper
See my main comment above about the topology helper alignment (regarding what to
do about InstallStar())

[Josh Pelkey, 2009-11-23 16:01:54]

Tom,

I hope that I have addressed all of your comments which required action before
merging.  I have aligned the CsmaStarHelper and PointToPointStarHelper (along
with the examples that used these methods) with what we have done.  I also added
a star-animation example.  Next, I changed "NodeLocation" to "CanvasLocation". 
Finally, I updated RELEASE_NOTES and CHANGES.html.  I did not get to look at
Ipv6 like you recommended.  Please let me know if this should be done before we
can merge.

NOTE: Aligning CsmaStarHelper has caused a change in the regression.  After
studying the pcaps, it looks like some minor differences exist.  Mostly the MAC
addresses are simply different.  However, total number of events and general
trace remains the same.

Things left before release:

-- doxygenize helpers
-- clarify the scope of applicability
-- comments on extending

http://codereview.appspot.com/117051/diff/12002/11020
File src/helper/node-location.cc (right):

http://codereview.appspot.com/117051/diff/12002/11020#newcode30
src/helper/node-location.cc:30: .AddAttribute ("Position", "The current position
of the mobility model.",
On 2009/11/16 05:31:43, Tom Henderson wrote:
> this attribute has nothing to do with Position or mobility model.  See my
other
> comment about possible user confusion with this class's name/attribute.

Done.

http://codereview.appspot.com/117051/diff/12002/11021
File src/helper/node-location.h (right):

http://codereview.appspot.com/117051/diff/12002/11021#newcode40
src/helper/node-location.h:40: class NodeLocation : public Object
On 2009/11/16 05:31:43, Tom Henderson wrote:
> class name:  why is this limited to Node?  Also, I am very concerned that this
> NodeLocation will be confused with Position by users.  I commented on this
> before. 
> 
> Suggested name changes:
>  - Coordinate
>  - VectorCoordinate
>  - CanvasLocation
> 

Changed to CanvasLocation

http://codereview.appspot.com/117051/diff/12002/11023
File src/helper/point-to-point-dumbbell-helper.h (right):

http://codereview.appspot.com/117051/diff/12002/11023#newcode48
src/helper/point-to-point-dumbbell-helper.h:48: Ipv4Address GetRightIpv4Address
(uint32_t) const; // Get right leaf address
On 2009/11/16 05:31:43, Tom Henderson wrote:
> We now support IPv4 and IPv6 in ns-3 so I think we should start to try to
fully
> support IPv6 in our other classes.  

I haven't been able to look at this yet.  I'd like to table this for now, if its
ok.

http://codereview.appspot.com/117051/diff/12002/11025
File src/helper/point-to-point-grid-helper.h (right):

http://codereview.appspot.com/117051/diff/12002/11025#newcode32
src/helper/point-to-point-grid-helper.h:32: class PointToPointGridHelper
On 2009/11/16 05:31:43, Tom Henderson wrote:
> See my main comment above about the topology helper alignment (regarding what
to
> do about InstallStar())

I aligned the old topology helpers.  PointToPointStarHelper and CsmaStarHelper
now exist.

[Tom Henderson, 2009-11-24 05:59:34]

On 2009/11/23 16:01:54, jpelkey wrote:
> Tom,
> 
> I hope that I have addressed all of your comments which required action before
> merging.  I have aligned the CsmaStarHelper and PointToPointStarHelper (along
> with the examples that used these methods) with what we have done.  I also
added
> a star-animation example.  Next, I changed "NodeLocation" to "CanvasLocation".

> Finally, I updated RELEASE_NOTES and CHANGES.html.  I did not get to look at
> Ipv6 like you recommended.  Please let me know if this should be done before
we
> can merge.

Josh, thanks for the update.  I am OK with tabling IPv6 for now.

> 
> NOTE: Aligning CsmaStarHelper has caused a change in the regression.  After
> studying the pcaps, it looks like some minor differences exist.  Mostly the
MAC
> addresses are simply different.  However, total number of events and general
> trace remains the same.
> 
> Things left before release:
> 
> -- doxygenize helpers
> -- clarify the scope of applicability
> -- comments on extending

Yes, I support merging it now and dealing with the above issues prior to the
release.

[Josh Pelkey, 2009-11-24 16:02:41]

On 2009/11/24 05:59:34, Tom Henderson wrote:
> On 2009/11/23 16:01:54, jpelkey wrote:
> > Tom,
> > 
> > I hope that I have addressed all of your comments which required action
before
> > merging.  I have aligned the CsmaStarHelper and PointToPointStarHelper
(along
> > with the examples that used these methods) with what we have done.  I also
> added
> > a star-animation example.  Next, I changed "NodeLocation" to
"CanvasLocation".
> 
> > Finally, I updated RELEASE_NOTES and CHANGES.html.  I did not get to look at
> > Ipv6 like you recommended.  Please let me know if this should be done before
> we
> > can merge.
> 
> Josh, thanks for the update.  I am OK with tabling IPv6 for now.
> 
> > 
> > NOTE: Aligning CsmaStarHelper has caused a change in the regression.  After
> > studying the pcaps, it looks like some minor differences exist.  Mostly the
> MAC
> > addresses are simply different.  However, total number of events and general
> > trace remains the same.
> > 
> > Things left before release:
> > 
> > -- doxygenize helpers
> > -- clarify the scope of applicability
> > -- comments on extending
> 
> Yes, I support merging it now and dealing with the above issues prior to the
> release.

Thank you Faker and Tom for your comments and help.  I will be working on the
remaining open issues before release.

changeset aae948449722

[Josh Pelkey, 2009-12-14 16:31:06]

I have extended the animation section in the manual as well as doxygenized the
helpers.  I started a new codereview for the documentation here:
http://codereview.appspot.com/176077/show

One thing I wasn't sure about was creating a section on NetAnim, since it isn't
a part of ns-3.  I added it anyway, since it was on the wiki.

I am happy to merge this myself.  I just thought I'd put it up here first, since
it is the first time I have contributed to the manual.

--
Josh Pelkey