Bug 893 - Lazy CourseChange notification for WaypointMobilityModel
Lazy CourseChange notification for WaypointMobilityModel
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: mobility models
pre-release
All All
: P5 normal
Assigned To: ns-bugs
:
Depends on: 892
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-21 17:17 EDT by Tom Henderson
Modified: 2010-10-05 04:38 EDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Henderson 2010-04-21 17:17:07 EDT
The nature of the WaypointMobilityModel is that it only updates its position upon calls to GetPosition() or GetVelocity().  If for instance, a user specifies a waypoint at time t=10 (10,10,10), and another at time t=20 (20,20,20), but the GetPosition() is called at time t=12, the CourseChangeNotification will fire at time t=12 and display the position at time t=12, even though the model behaves as if the CourseChange really occurred at time t=10.  

I propose that CourseChange notifications should map to the actual waypoint times, and this can be accomplished by scheduling an Update() at time t whenever AddMobility() is called for a new waypoint at time t.  Note that some of the logic of Update() will need to slightly change to accommodate this, since Updates are only processed if the Simulator now time is strictly greater than the time of the next waypoint; i.e. simply adding Simulator::Schedule (waypoint.time, &WaypointMobilityModel::Update, this) to the AddWaypoint method will not have any effect.
Comment 1 Tom Henderson 2010-04-21 21:56:41 EDT
marking depends on 892 because it would probably be easier to deal with both at the same time.  I can produce a patch once 892 is decided.
Comment 2 Phillip Sitbon 2010-08-05 17:12:46 EDT
Unless I'm missing something, the suggestion of adding Simulator::Schedule
(waypoint.time, &WaypointMobilityModel::Update, this) to the AddWaypoint method actually will work.

As long as we call Update() at the time of each waypoint (see course of events when now == m_next.time), the newWaypoint will be true and NotifyCourseChange will be called at the proper time. At this point, m_current is set to m_next, and the new m_next triggers a NotifyCourseChange again _only_ when now == m_next.time again.

Tom, your suggestion of making this configurable has placated my performance concerns, as long as the new (non-lazy) behavior is disabled by default :)
Comment 3 Tom Henderson 2010-08-06 00:16:13 EDT
(In reply to comment #2)
> Unless I'm missing something, the suggestion of adding Simulator::Schedule
> (waypoint.time, &WaypointMobilityModel::Update, this) to the AddWaypoint method
> actually will work.
> 
> As long as we call Update() at the time of each waypoint (see course of events
> when now == m_next.time), the newWaypoint will be true and NotifyCourseChange
> will be called at the proper time. At this point, m_current is set to m_next,
> and the new m_next triggers a NotifyCourseChange again _only_ when now ==
> m_next.time again.

It has been a while since I drew that conclusion but I think my concern was due to the statement preceding it:

  if ( now <= m_current.time )
    {
      return;
    }

if m_current is set to m_next, and the next time Update() is called (at m_next.time), now will be equal to m_current.time (which was set to m_next.time previously), and the method will return without the notification.

But, I think a test case will settle the issue, so if you think it will work OK and have a test for it, I'm fine with your suggestion.

> 
> Tom, your suggestion of making this configurable has placated my performance
> concerns, as long as the new (non-lazy) behavior is disabled by default :)

Yes, I did not post that suggestion in the tracker, but I suggested to you to make this an attribute to satisfy performance concerns, and I would be OK with making the default be the current behavior.
Comment 4 Phillip Sitbon 2010-08-06 18:36:55 EDT
(In reply to comment #3)

> if m_current is set to m_next, and the next time Update() is called (at
> m_next.time), now will be equal to m_current.time (which was set to m_next.time
> previously), and the method will return without the notification.
> 

Let me see if I can clarify what it's doing-

If m_current gets set to m_next, there's two possibilities for the value of 'now' depending on when Update() was last called. If Update hasn't been called in a while, there's a possibility that the loop will iterate again and advance to an even newer waypoint, until it reaches the point where now >= m_next.time. In the case of non-lazy updates, we don't have to worry about this loop ever executing more than once. At this point, 'newWaypoint' is true, so NotifyCourseChange is called at the end of Update(). By scheduling Update() to be called at exactly the time of each waypoint, we force m_current to roll over to m_next and call NotifyCourseChange right away.

Your concern does apply to the first waypoint, but I'm not certain of what the notification behavior should be there. If, at t = 0, we create the first waypoint at t = 10, which is when the node starts moving, then shouldn't the first notification be when the course changes at the second waypoint? If not, it'd be easy enough to schedule a notification at the time of the first waypoint as a special case.

> But, I think a test case will settle the issue, so if you think it will work OK
> and have a test for it, I'm fine with your suggestion.

I can write up a test case but I probably won't be able to present any verification before the 3.9 deadline (assuming end of day today).
Comment 5 Tom Henderson 2010-08-06 19:01:28 EDT
(In reply to comment #4)
> (In reply to comment #3)
> 
> > if m_current is set to m_next, and the next time Update() is called (at
> > m_next.time), now will be equal to m_current.time (which was set to m_next.time
> > previously), and the method will return without the notification.
> > 
> 
> Let me see if I can clarify what it's doing-
> 
> If m_current gets set to m_next, there's two possibilities for the value of
> 'now' depending on when Update() was last called. If Update hasn't been called
> in a while, there's a possibility that the loop will iterate again and advance
> to an even newer waypoint, until it reaches the point where now >= m_next.time.
> In the case of non-lazy updates, we don't have to worry about this loop ever
> executing more than once. At this point, 'newWaypoint' is true, so
> NotifyCourseChange is called at the end of Update(). By scheduling Update() to
> be called at exactly the time of each waypoint, we force m_current to roll over
> to m_next and call NotifyCourseChange right away.

OK
> 
> Your concern does apply to the first waypoint, but I'm not certain of what the
> notification behavior should be there. If, at t = 0, we create the first
> waypoint at t = 10, which is when the node starts moving, then shouldn't the
> first notification be when the course changes at the second waypoint? If not,
> it'd be easy enough to schedule a notification at the time of the first
> waypoint as a special case.

I would be OK with the first notification being at the second notification, as you stated above.

> 
> > But, I think a test case will settle the issue, so if you think it will work OK
> > and have a test for it, I'm fine with your suggestion.
> 
> I can write up a test case but I probably won't be able to present any
> verification before the 3.9 deadline (assuming end of day today).

I can give you relief on that deadline; this seems close to getting done.
Comment 6 Phillip Sitbon 2010-08-09 15:21:06 EDT
Just an update here:

For backwards compatibility with how this used to work, I settled with adding another attribute "InitialPositionIsWaypoint", which defaults to false.

This will allow the WaypointMobilityModel to behave as before where nodes are essentially waypoint-less when only SetPosition is used. I did this because I appreciate being able to use MobilityHelper and still have SetPosition do nothing if I add waypoints to each node after calling Install. The new behavior, in which using MobilityHelper and PositionAllocator sets up the first (t=0) waypoint for every node, can be achieved by setting InitialPositionIsWaypoint to true in the MobilityHelper initialization.
Comment 7 Phillip Sitbon 2010-08-09 20:00:44 EDT
(Please note that the previous comment has more context with #892, apologies for confusing the two bug entries)

I've made my final functionality changes at this point.

After writing the test cases and looking more closely at other mobility models, I've decided that the very first waypoints should also generate notifications.

The unit tests helped me sort out some corner cases that caused improper notification behavior.

Also, the attribute "InitialPositionIsWaypoint" defaults to false, allowing SetPosition to be ignored and MobilityHelper to act the same as it does now.


Patch set 5:
http://codereview.appspot.com/144064
Comment 8 Tom Henderson 2010-08-22 17:55:22 EDT
Mathieu suggested, at the tail end of the release review of this, to consider doing away with the Attribute and instead track whether there are listeners to the course change callback (and have lazy notify be a performance optimization for when there are no listeners).

Do people want to go with that solution?  It would be fine with me but we have no existing patch for it.

Mathieu, is there a way to learn whether there are listeners registered, using the existing TracedCallback, or do we need to add an explicit counter in the class?
Comment 9 Mathieu Lacage 2010-09-16 02:31:56 EDT
(In reply to comment #8)

> Mathieu, is there a way to learn whether there are listeners registered, using
> the existing TracedCallback, or do we need to add an explicit counter in the
> class?

Need to add a TracedCallback::IsEmpty method that forwards to the internal std::list and a MobilityModel::HasCourseChangeListeners that forwards to TracedCallback::IsEmpty.
Comment 10 Pavel Boyko 2010-09-24 06:30:14 EDT
  Well, 

  finally I have an opinion on this bug ;) 

  I suggest to keep enable/disable course change notifications attribute and make this a recommended practice for complex mobility models. I see two main reasons for this:

1. Course change notification itself can be meaningless for non-linear mobility models. For example some time ago we have extensively used circular mobility model which can continuously notify about course changes. Now we are close to implementing the airplane and m.b. the satellite mobility models which also do not have linear segments at all. To monitor nodes movements we use periodic position polling instead of listening to CourseChange in these cases. 

2. I am happy to note that currently traced callbacks allow me to _completely_ decouple event producers and consumers in the ns-3 -- in the sense that trace source knows strictly nothing about its listeners. I find this an important architecture achievement and want to keep it. Letting trace source know the number of listeners as proposed is definitely a step in the wrong direction. Anyway, runtime optimizations is too weak reason to break nice architecture invariants :) 

  Tom, please close this bug.

(In reply to comment #8)
> Mathieu suggested, at the tail end of the release review of this, to consider
> doing away with the Attribute and instead track whether there are listeners to
> the course change callback (and have lazy notify be a performance optimization
> for when there are no listeners).
> 
> Do people want to go with that solution?  It would be fine with me but we have
> no existing patch for it.
> 
> Mathieu, is there a way to learn whether there are listeners registered, using
> the existing TracedCallback, or do we need to add an explicit counter in the
> class?
Comment 11 Tom Henderson 2010-10-05 04:38:09 EDT
>   Tom, please close this bug.
> 

changeset:  c5a7581f1e9

I did not make any functional changes to Phillip's last patch, but I took the liberty of updating the doxygen, and also added a new mobility test suite  (in a subsequent changeset) with some more tests to convince myself that it was working as expected.  The new test suite (src/test/mobility-test-suite.cc) could be expanded to other mobility models, or merged with existing test code in waypoint-mobility-model.cc (I don't care strongly).