Bug 893

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.

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 :)

(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.

(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).

(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.

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.

(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

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?

(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.

  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?

>   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).