Added vanet-highway, an ability to simulate vehicular ad-hoc networks with ns-3 (issue176060)

49 views
Skip to first unread message

mathieu...@gmail.com

unread,
Dec 14, 2009, 10:03:39 AM12/14/09
to hadia...@gmail.com, ns-3-r...@googlegroups.com, re...@codereview.appspotmail.com
Hi,

I assume that you do intend to propose this code for merging in ns-3 so
I reviewed this code with this in mind. You will find below a couple of
low-level comments: these comments do not attempt to catch all instances
of specific issues. They merely point out a couple of instances of
recurring patterns which will need to be dealt with for merging.

Before you look at the details, a couple of high-level comments:
1) you need to follow the coding style:
http://www.nsnam.org/codingstyle.html
Typical issues I spotted:
- member variables must be prefixed with m_ to allow those who read
the code to distinguish them from local variables
- member variables must not be made public, except for structures
- method and variable names should not use shortcuts: full words
should be used.
- indentation is 2 spaces. The following does not match our coding
style:
if (...)
{ // no ident
}
the proper way (as per the ns-3 coding style) is:
if (...)
{// ident: 2 spaces
one_statement; // ident again by 2 spaces
}

2) you need to document the model you have implemented. I am
personally not familiar with what VANET people do so, you need to
provide at least some pointers to documentation which describes these
models

3) it would be nice if you could explain the difference between what
you have implemented and the ns-3 mobility models implemented in
src/mobility/*

4) for merging, you will need to make a clear distinction between
'core' models and 'helpers'. The latter all go in src/helper and are
used only to setup a simulation scenario while the former will go in
another source directory and will generate and consume events at
runtime.


http://codereview.appspot.com/176060/diff/1/3
File vanet-highway/Controller.h (right):

http://codereview.appspot.com/176060/diff/1/3#newcode43
vanet-highway/Controller.h:43: double T;
what is a 'T' ???

http://codereview.appspot.com/176060/diff/1/5
File vanet-highway/HadiHeader.h (right):

http://codereview.appspot.com/176060/diff/1/5#newcode74
vanet-highway/HadiHeader.h:74: void SetVID(int id);
SetVehicleId

http://codereview.appspot.com/176060/diff/1/5#newcode122
vanet-highway/HadiHeader.h:122: void SetAcc(double ac);
SetAcceleration

http://codereview.appspot.com/176060/diff/1/5#newcode130
vanet-highway/HadiHeader.h:130: void SetDir(int direction);
SetDirection

http://codereview.appspot.com/176060/diff/1/7
File vanet-highway/Highway.h (right):

http://codereview.appspot.com/176060/diff/1/7#newcode47
vanet-highway/Highway.h:47: list<Ptr<Vehicle> > vehicles[5]; //
list of vehicles in positive direction (+1) up to maximum 5 lanes.
m_vehicles

http://codereview.appspot.com/176060/diff/1/7#newcode84
vanet-highway/Highway.h:84: WifiHelper wifiHelper;
these are public member variables ? In ns-3, we never export member
variables as public members (except for structures). You need to
introduce methods to access them.

Furthermore, all member variables are prefixed with m_

http://codereview.appspot.com/176060/diff/1/7#newcode124
vanet-highway/Highway.h:124: bool GetTwoDirectional();
The documentation uses the proper term: BiDirectional instead of
TwoDirectional.

http://codereview.appspot.com/176060/diff/1/9
File vanet-highway/LaneChange.h (right):

http://codereview.appspot.com/176060/diff/1/9#newcode35
vanet-highway/LaneChange.h:35: * \brief IDM/MOBIL Lane-Change Model.
reference to documents describing these models ?

http://codereview.appspot.com/176060/diff/1/9#newcode59
vanet-highway/LaneChange.h:59: double GetP();
In ns-3, we try to avoid shortcuts: GetPoliteness

http://codereview.appspot.com/176060/diff/1/9#newcode67
vanet-highway/LaneChange.h:67: double GetDB();
changing threshold ? what does this threshold control ?

http://codereview.appspot.com/176060/diff/1/9#newcode83
vanet-highway/LaneChange.h:83: double GetBSave();
GetMaximumSafeDeceleration ?

http://codereview.appspot.com/176060/diff/1/15
File vanet-highway/Vehicle.h (right):

http://codereview.appspot.com/176060/diff/1/15#newcode38
vanet-highway/Vehicle.h:38: using namespace std;
In ns-3, you can do 'using namespace XXX' in examples but not in models.
Instead, you need to prefix everything with the proper namespace. i.e.,
'Node' becomes 'ns3::Node'

http://codereview.appspot.com/176060/diff/1/15#newcode41
vanet-highway/Vehicle.h:41: namespace Hadi
In ns-3, models all belong to the 'ns3' namespace.

http://codereview.appspot.com/176060/diff/1/15#newcode54
vanet-highway/Vehicle.h:54: class Vehicle : public Node
Node must not be subclassed. If you wish to associate extra
functionality to a Node, you need to:
- subclass Object instead of Node
- create a helper class which creates your 'Vehicle' class and calls
node->AggregateObject (vehicle). Later on, you can call
node->GetObject<Vehicle> () to get it back.

http://codereview.appspot.com/176060/diff/1/15#newcode75
vanet-highway/Vehicle.h:75: /// Deconstructor.
the proper term is "destructor"

http://codereview.appspot.com/176060
Reply all
Reply to author
Forward
0 new messages