ns-3 SOCIS 2016 midterm/final

ns-3 SOCIS 2016 midterm and final code review, from Michael Di Perna.

Midterm is patchset 1:

Generated documentation can be found at:  https://www.nsnam.org/reviews/2016/socis-midterm/fso.html

Final is patchset 2, which includes patchset 1:

Generated documentation can be found at:  https://www.nsnam.org/reviews/2016/socis-final/fso.html

[n.p, 2016-11-06 14:41:43]

I have reviewed the code, I have found some c++ minor things and some
documentation missing. On the manual, I would like to see also some use cases
for the work done.

Except these little things, the code looks ok, exept the part of the NetDevice,
which is not compatible with Traffic Control. This is a serious lack, because we
are struggling in porting old code to use tc layer, and it would be strange to
accept new code without the support to it.

So for me it's a conditional accept, where the conditional is based on the
traffic control support (by the way, it's very easy to add, don't be scared :D).

Anyway good job, if you (or the mentors) have questions, feel free to ask.

Natale

https://codereview.appspot.com/305270043/diff/1/src/fso/helper/fso-helper.h
File src/fso/helper/fso-helper.h (right):

https://codereview.appspot.com/305270043/diff/1/src/fso/helper/fso-helper.h#n...
src/fso/helper/fso-helper.h:11: #endif /* FSO_HELPER_H */
if the helper is not needed, please don't include it.

and (this holds for all the sources) please pass them through the check
formatter, in the utils/ directory.

https://codereview.appspot.com/305270043/diff/1/src/fso/model/fso-channel.h
File src/fso/model/fso-channel.h (right):

https://codereview.appspot.com/305270043/diff/1/src/fso/model/fso-channel.h#n...
src/fso/model/fso-channel.h:4: *
Is that a correct copyright statement? Please pay attention to this header, is
the most important.

https://codereview.appspot.com/305270043/diff/20001/src/fso/doc/fso.rst
File src/fso/doc/fso.rst (right):

https://codereview.appspot.com/305270043/diff/20001/src/fso/doc/fso.rst#newco...
src/fso/doc/fso.rst:176: 
Is this still true? I see some helpers in the the helpers/ directory.

https://codereview.appspot.com/305270043/diff/20001/src/fso/doc/fso.rst#newco...
src/fso/doc/fso.rst:228: "Preliminary Results of Terabit-per-second Long-Range
Free-Space Optical Transmission Experiment THRUST" and "Overview of the Laser
Communication System for the NICT Optical Ground Station and Laser Communication
Experiments on Ground-to-Satellite Links".
Can you provide links to the published material ?

https://codereview.appspot.com/305270043/diff/20001/src/fso/examples/fso-irra...
File src/fso/examples/fso-irradiance-curve.cc (right):

https://codereview.appspot.com/305270043/diff/20001/src/fso/examples/fso-irra...
src/fso/examples/fso-irradiance-curve.cc:67: Gnuplot2dDataset g_dataset;
This variable should be declared as static in order to not pollute the namespace
and to not be confused with other global variables

https://codereview.appspot.com/305270043/diff/20001/src/fso/examples/fso-irra...
src/fso/examples/fso-irradiance-curve.cc:69: void
this function should be declared as static in order to not pollute the namespace

https://codereview.appspot.com/305270043/diff/20001/src/fso/examples/fso-irra...
src/fso/examples/fso-irradiance-curve.cc:77: void
this function should be declared as static in order to not pollute the namespace

https://codereview.appspot.com/305270043/diff/20001/src/fso/model/fso-error-m...
File src/fso/model/fso-error-model.cc (right):

https://codereview.appspot.com/305270043/diff/20001/src/fso/model/fso-error-m...
src/fso/model/fso-error-model.cc:234: {
Make these static member of the class

https://codereview.appspot.com/305270043/diff/20001/src/fso/model/fso-error-m...
File src/fso/model/fso-error-model.h (right):

https://codereview.appspot.com/305270043/diff/20001/src/fso/model/fso-error-m...
src/fso/model/fso-error-model.h:48: double GWIntegralFunction (double x, void
*params);
These definitions will pollute the namespace. I would add these as static member
of the class.

https://codereview.appspot.com/305270043/diff/20001/src/fso/model/fso-mac.h
File src/fso/model/fso-mac.h (right):

https://codereview.appspot.com/305270043/diff/20001/src/fso/model/fso-mac.h#n...
src/fso/model/fso-mac.h:33: //#include "ns3/qos-utils.h"
Remove these comments if they are not needed

https://codereview.appspot.com/305270043/diff/20001/src/fso/model/fso-mac.h#n...
src/fso/model/fso-mac.h:39: class FsoMac  : public Object
The class misses the doxygen documentation.

https://codereview.appspot.com/305270043/diff/20001/src/fso/model/fso-net-dev...
File src/fso/model/fso-net-device.h (right):

https://codereview.appspot.com/305270043/diff/20001/src/fso/model/fso-net-dev...
src/fso/model/fso-net-device.h:47: class FsoNetDevice : public NetDevice
The class misses the support for the Traffic Control layer (and the FsoMac too).