Bug 2390

Summary: WaypointMobilityModel::AddWaypoint lazy notify schedules an event using absolute time (should be relative time)
Product: ns-3 Reporter: Hossam Khader <hossamkhader>
Component: mobility modelsAssignee: ns-bugs <ns-bugs>
Status: RESOLVED FIXED    
Severity: normal CC: tomh, tommaso.pecorella
Priority: P5    
Version: unspecified   
Hardware: All   
OS: All   
Attachments: suggested patch
Hossam's patch + tests

Description Hossam Khader 2016-04-26 18:28:40 UTC
Created attachment 2403 [details]
suggested patch

WaypointMobilityModel::AddWaypoint lazy notify schedules an event using absolute time (i.e. waypoint.time)

Works fine when the waypoint is added at time = 0, but if the waypoint is added at time > 0 will not work (should be waypoint.time - Simulator::Now ())

Simulator::Schedule (waypoint.time, &WaypointMobilityModel::Update, this);

should be:

Simulator::Schedule (waypoint.time - Simulator::Now (), &WaypointMobilityModel::Update, this);
Comment 1 Tom Henderson 2016-09-01 18:01:43 UTC
I agree with the fix; just would like to understand why our test did not pick this up (perhaps need to tweak the test).
Comment 2 Hossam Khader 2016-09-01 18:09:51 UTC
If all the waypoints are added at t=0, then the bug has no impact. I think that is why the bug was never picked up.

In my test, I added waypoints at t>0
Comment 3 Tommaso Pecorella 2016-09-08 20:37:51 UTC
(In reply to Tom Henderson from comment #1)
> I agree with the fix; just would like to understand why our test did not
> pick this up (perhaps need to tweak the test).

"tweak" is an euphemism.
The test is totally not working - at all.

The reason is that the testa are carried out in a "CourseChange" function, that SHOULD be called by the appropriate callback, that SHOULD be tied by this call:

  Config::Connect ("/NodeList/*/$ns3::WaypointMobilityModel/CourseChange", ...

The problem is... in the test there's no node. DOH.

I'll update the test.
Comment 4 Tommaso Pecorella 2016-09-08 21:04:37 UTC
Created attachment 2568 [details]
Hossam's patch + tests

The patch is correct. The tests are (fortunately) working as intended.

Fortunately because before they wasn't checking anything and after fixing them they do pass.

I added a new test showing Hossam's bug and patch.
Comment 5 Tom Henderson 2016-09-12 16:16:21 UTC
Ok with me to merge; suggest to split into two patches: Hossam's patch to fix, and separately Tommaso's fixed test case.
Comment 6 Tommaso Pecorella 2016-09-12 17:05:14 UTC
changeset 12319:748ee83bdcec