Waypoint mobility model for ns-3: Bugfixes for 892, 893

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/f78de8c92bf43e49/bc0383b38c48eba0

[Tom Henderson, 2009-10-31 00:57:21]

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, 2009-10-31 21:14:07]

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, 2009-11-01 04:25:45]

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, 2009-11-04 07:24:53]

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, 2009-11-09 17:27:21]

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.

[Phillip Sitbon, 2010-08-05 21:29:50]

Relevant code changes to address the following:

http://www.nsnam.org/bugzilla/show_bug.cgi?id=892
http://www.nsnam.org/bugzilla/show_bug.cgi?id=893

[Phillip Sitbon, 2010-08-09 23:59:43]

Unit tests and final bug-addressing features added.

[Mathieu Lacage, 2010-08-10 06:36:59]

I did not intend to review this, really, but, here it is.

http://codereview.appspot.com/144064/diff/12001/13001
File src/mobility/waypoint-mobility-model.cc (left):

http://codereview.appspot.com/144064/diff/12001/13001#oldcode27
src/mobility/waypoint-mobility-model.cc:27: NS_LOG_COMPONENT_DEFINE
("WaypointMobilityModel");
would be nice to keep it around and actually use NS_LOG_FUNCTION (this << args)
in a couple of relevant methods below.

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

http://codereview.appspot.com/144064/diff/12001/13001#newcode54
src/mobility/waypoint-mobility-model.cc:54: .AddAttribute ("LazyNotify", "Only
call NotifyCourseChange when position is calculated.",
I have not followed the relevant bug reports but I wanted to point out that
NotifyCourseChange was designed to be called whenever the mobility model decides
to change the 'course' of a node. i.e., whenever the speed vector changes. So, I
don't know what LazyNotify does but the default behavior should be to make sure
that NotifyCourseChange is called whenever the speed vector changes (either its
direction or norm) at the time it changes.

http://codereview.appspot.com/144064/diff/12001/13001#newcode68
src/mobility/waypoint-mobility-model.cc:68: : m_first (true),
m_lazyNotify(true), m_initialPositionIsWaypoint(false)
I don't know what the coding style script will do but I would have expected it
to insert newlines between ','

http://codereview.appspot.com/144064/diff/12001/13001#newcode312
src/mobility/waypoint-mobility-model.cc:312: struct
WaypointMobilityModelTestSuite : public TestSuite
I know that a lot of tests don't do this but it's better to prefix this with
'static' so that the g_waypointXXX variable does not get exported from the final
binary.

[Tom Henderson, 2010-08-12 20:36:06]

> http://codereview.appspot.com/144064/diff/12001/13001#newcode54
> src/mobility/waypoint-mobility-model.cc:54: .AddAttribute ("LazyNotify", "Only
> call NotifyCourseChange when position is calculated.",
> I have not followed the relevant bug reports but I wanted to point out that
> NotifyCourseChange was designed to be called whenever the mobility model
decides
> to change the 'course' of a node. i.e., whenever the speed vector changes. So,
I
> don't know what LazyNotify does but the default behavior should be to make
sure
> that NotifyCourseChange is called whenever the speed vector changes (either
its
> direction or norm) at the time it changes.

To me, this is the main issue blocking merge (the others can be easily handled).
 A brief history:

in existing code, NotifyCourseChange is only triggered when Set/Get(Position) is
called.  If, for instance, a waypoint for a node is set at time 10, 20, 30, 40
seconds, but no client actually calls GetPosition() until time 40+, the
intermediate waypoints are skipped over; only one course change will be
notified.  

I call this behavior "LazyNotify" and suggested that it instead be made to
always schedule an update for each Waypoint time.  Phillip mentioned that he
preferred it to be lazy, for performance reasons.  I suggested an attribute to
toggle this behavior.  Phillip recommended to make default to "Lazy" (preserve
current behavior) and I agreed to it.

If it were up to me, the default should be to not be lazy, and allow the user to
invoke Lazy only if he/she wants the performance gains and knows what he/she is
doing.  It should be documented that setting LazyNotify to true will change the
semantics of the course change callback for this model.

So, I think we mainly need to agree on what NotifyCourseChange means and agree
on this default for this model.  I would prefer to make the lazy notification to
be default off because I think it will be less surprising (more intuitive) to
users.  In particular, the reason I got involved in this is because a user told
me that he thought there was a bug in the course change notifications, that he
was not getting notified for all of the waypoints that he registered.

[Phillip Sitbon, 2010-08-12 22:26:59]

I think at this point, it makes sense to make the default non-lazy. I'll try to
get the final patch in tonight and try to amend the docs reflecting the new
parameter/behavior.

[Phillip Sitbon, 2010-08-13 03:30:45]

Hello all, these changes address the most recent comments, and despite my
performance concerns I agree that predictable behavior is important in a
situation like this so the lazy (previous) behavior is now off by default. I
don't have time to polish the documentation but hopefully the behavior is
clear/consistent this time around.

Regards,

Phillip