Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(356)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 3 months ago by Phillip Sitbon
Modified:
10 years, 5 months ago
Reviewers:
Tom Henderson, Mathieu Lacage, ns-3-reviews
CC:
pavel boyko , josh pelkey
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 12

Patch Set 2 : Addressing some of the reviewer comments #

Total comments: 2

Patch Set 3 : Style update #

Total comments: 1

Patch Set 4 : Addressing bugs 892, 893 #

Patch Set 5 : Final behavior changes for fixes to 892,893. Added unit tests. #

Total comments: 4

Patch Set 6 : Defaults change & addressing comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+160 lines, -9 lines) Patch
M src/mobility/waypoint-mobility-model.h View 1 2 3 4 2 chunks +4 lines, -0 lines 0 comments Download
M src/mobility/waypoint-mobility-model.cc View 1 2 3 4 5 8 chunks +156 lines, -9 lines 0 comments Download

Messages

Total messages: 11
Tom Henderson
Suggest to add a unit test that inserts some waypoints, calls SetPosition (), etc.. Otherwise, ...
11 years, 2 months ago (2009-10-31 00:57:21 UTC) #1
faker.moatamri
Hi Phillip, You will find my comments for your code in the appspot link that ...
11 years, 2 months ago (2009-10-31 21:14:07 UTC) #2
Phillip Sitbon
Thanks for your reviews, Tom & Faker. I attempted to address all of the comments ...
11 years, 2 months ago (2009-11-01 04:25:45 UTC) #3
Mathieu Lacage
the code looks great (modulo the small coding style nits I commented upon). Bonus points ...
11 years, 2 months ago (2009-11-04 07:24:53 UTC) #4
Phillip Sitbon
This looks like the last edit needed besides the addition of tests & examples. http://codereview.appspot.com/144064/diff/3001/3003 ...
11 years, 2 months ago (2009-11-09 17:27:21 UTC) #5
Phillip Sitbon
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
10 years, 5 months ago (2010-08-05 21:29:50 UTC) #6
Phillip Sitbon
Unit tests and final bug-addressing features added.
10 years, 5 months ago (2010-08-09 23:59:43 UTC) #7
Mathieu Lacage
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 ...
10 years, 5 months ago (2010-08-10 06:36:59 UTC) #8
Tom Henderson
> 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.", > ...
10 years, 5 months ago (2010-08-12 20:36:06 UTC) #9
Phillip Sitbon
I think at this point, it makes sense to make the default non-lazy. I'll try ...
10 years, 5 months ago (2010-08-12 22:26:59 UTC) #10
Phillip Sitbon
10 years, 5 months ago (2010-08-13 03:30:45 UTC) #11
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b