Waypoint mobility model for ns-3

Adds a Waypoint object as a (time,value) pair and a WaypointMobilityModel to the
ns-3 mobility group.

Discussion leading up to this code:
http://groups.google.com/group/ns-3-users/browse_thread/thread/f78de8c92bf43e...

[Tom Henderson]

Suggest to add a unit test that inserts some waypoints, calls SetPosition (),
etc..

Otherwise, I would say +1 to merging (Mathieu Lacage is the maintainer)

http://codereview.appspot.com/144064/diff/1/3
File src/mobility/waypoint-mobility-model.cc (right):

http://codereview.appspot.com/144064/diff/1/3#newcode123
Line 123: const double t_span = (m_next.time - m_current.time).GetSeconds();
I just noticed a few missing spaces before parentheses-- here is one of them.

http://codereview.appspot.com/144064/diff/1/4
File src/mobility/waypoint-mobility-model.h (right):

http://codereview.appspot.com/144064/diff/1/4#newcode34
Line 34: * Each object determines its velocity at a given time from a set of
"velocity and position" ?

http://codereview.appspot.com/144064/diff/1/4#newcode39
Line 39: * The intial position of each object corresponds to the position of
initial

http://codereview.appspot.com/144064/diff/1/4#newcode42
Line 42: * velocity is zero.
Suggest to add something like (if you agree): "When a node is in between
waypoints (in time), it moves with a constant velocity between the position at
the previous waypoint and the position at the current waypoint.  To make a node
hold a certain position for a time interval, two waypoints with the same
position (but different times) should be inserted sequentially."

http://codereview.appspot.com/144064/diff/1/4#newcode45
Line 45: * of an object will set its velocity to zero until the next waypoint,
can you clarify, is it zero until the next waypoint time, at which time it jumps
to the position of the waypoint?  Or is it zero until the next query (which may
be before the next waypoint time), in which case it treats setting the position
as if it had been a waypoint?

http://codereview.appspot.com/144064/diff/1/6
File src/mobility/waypoint.h (right):

http://codereview.appspot.com/144064/diff/1/6#newcode43
Line 43: Waypoint (const Time &_time, const Vector &_position);
please use camelBack name convention (no leading underscore)

[faker.moatamri]

Hi Phillip,
You will find my comments for your code in the appspot link that you privded.
Best regards
Faker Moatamri

http://codereview.appspot.com/144064/diff/1/3
File src/mobility/waypoint-mobility-model.cc (right):

http://codereview.appspot.com/144064/diff/1/3#newcode56
Line 56: WaypointMobilityModel::WaypointMobilityModel ()
any destructor somewhere?

http://codereview.appspot.com/144064/diff/1/3#newcode124
Line 124: m_velocity.x = (m_next.position.x - m_current.position.x) / t_span;
I would recommand to check if t_span is not 0 or near 0, in that case, it would
fail at runtime.

http://codereview.appspot.com/144064/diff/1/3#newcode139
Line 139: return;
useless

http://codereview.appspot.com/144064/diff/1/4
File src/mobility/waypoint-mobility-model.h (right):

http://codereview.appspot.com/144064/diff/1/4#newcode58
Line 58: WaypointMobilityModel ();
destructor?

http://codereview.appspot.com/144064/diff/1/5
File src/mobility/waypoint.cc (right):

http://codereview.appspot.com/144064/diff/1/5#newcode29
Line 29: {}
destructor

http://codereview.appspot.com/144064/diff/1/6
File src/mobility/waypoint.h (right):

http://codereview.appspot.com/144064/diff/1/6#newcode50
Line 50: /* The waypoint time */
the member variables should be private, you must have an excellent reason to
make them public!

[Phillip Sitbon]

Thanks for your reviews, Tom & Faker. I attempted to address all of the comments
you left in the latest update. I suppose replying to the comments in-line would
have been useful, but I wasn't so sure after already putting up the new code (is
that the preferred method?).

http://codereview.appspot.com/144064/diff/1/4#newcode45
Tom, as far as setting the position of the object, the velocity is zero and it
does jump to the next waypoint at the waypoint time. It seemed there needs to be
a discontinuity somewhere when manually setting the position, so that's just the
behavior I chose (it doesn't have to remain that way). The addition to the
documentation hopefully clarifies this.

Both setting the position and the method of calling NotifyCourseChange seem a
little hackish to me, and I did my best to make it not-too-quirky while still
being compatible with other mobility models.

http://codereview.appspot.com/144064/diff/1/3#newcode124
Faker, t_span being > 0 is enforced in AddWaypoint, but I opted for a debug
assertion anyway - the only way that case will arise is if there's a more
serious problem afoot.

http://codereview.appspot.com/144064/diff/1/6#newcode43
http://codereview.appspot.com/144064/diff/1/6#newcode50
I used Vector and Rectangle as my templates for this type- it's meant to be a
struct, but I opted to make it a class to be consistent with the others (same
goes for the underscores, but I agree camelBack looks better). This is also the
reason for the public member variables.

I added a destructor to the mobility model, but I'm not sure if it's appropriate
for the Waypoint object.

I am working on examples/test cases, and I can spend more time with them now
that I haven't seen any objections to the overall structure of the new mobility
model.

Cheers,

Phillip

[Mathieu Lacage]

the code looks great (modulo the small coding style nits I commented upon).
Bonus points will go to tests but, I would be fine with this code being merged
as-is.

http://codereview.appspot.com/144064/diff/1006/1008
File src/mobility/waypoint-mobility-model.cc (right):

http://codereview.appspot.com/144064/diff/1006/1008#newcode101
Line 101: if ( now <= m_current.time )
coding style:
if ()
  {
    // single line statement
  }

http://codereview.appspot.com/144064/diff/1006/1008#newcode116
Line 116: m_current.time = now;
coding style requires parens, even for single line statements

[Phillip Sitbon]

This looks like the last edit needed besides the addition of tests & examples.

http://codereview.appspot.com/144064/diff/3001/3003
File src/mobility/waypoint-mobility-model.cc (right):

http://codereview.appspot.com/144064/diff/3001/3003#newcode101
src/mobility/waypoint-mobility-model.cc:101: if ( now <= m_current.time )
The coding style document online (http://www.nsnam.org/codingstyle.html)
suggests that single-statement conditional is accepted - changed it anyway, will
change anywhere else I noticed it as well.