ns-3 antenna model by the LENA project

[Tom Henderson, 2012-04-09 05:38:36]

Just some minor comments to please consider before merging.   I like your
documentation of the test cases (which would be nice to make a standard
practice).

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.cc
File src/antenna/model/angles.cc (right):

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.cc#newc...
src/antenna/model/angles.cc:23: #include <math.h>
we seem to be going with cmath.h now

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.h
File src/antenna/model/angles.h (right):

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.h#newco...
src/antenna/model/angles.h:65: *
suggest to clarify the ascii art that standard spherical coordinates are used,
with phi measured counterclockwise in the X/Y plane off the x-axis, and theta
measured off the z-axis.

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.h#newco...
src/antenna/model/angles.h:67: struct Angles
would 'Direction' be a more appropriate name for this class?

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.h#newco...
src/antenna/model/angles.h:70: * default constructor, will inizialize phi and
theta to zero
initialize

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/antenna-model.h
File src/antenna/model/antenna-model.h (right):

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/antenna-model....
src/antenna/model/antenna-model.h:37: * spherical coordinates, in particolar of
the azimuth and inclination
s/in particolar of/in particular,/

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/antenna-model....
src/antenna/model/antenna-model.h:62: * \return the gain in dB of the antenna
radiation pattern at the specified angles
Suggest a bit more detail in the documentation.  The gain being returned is the
gain over isotropic (dBi).  As for whether it is directive gain or power gain, I
believe that this API is returning a power gain (and if efficiency is less than
one, it is just built into the model), but perhaps you can clarify.

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/cosine-antenna...
File src/antenna/model/cosine-antenna-model.cc (right):

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/cosine-antenna...
src/antenna/model/cosine-antenna-model.cc:24: #include <math.h>
cmath.h

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/cosine-antenna...
src/antenna/model/cosine-antenna-model.cc:115: // the array factor, the actual
beawidth would change, and in
beamwidth

[Tom Henderson, 2012-04-09 06:53:09]

Also noting that operator>> is needed for Angles class.

[Nicola Baldo, 2012-04-11 09:10:22]

Hi Tom, 

thanks for your review. I've addressed most of your comments in this revision:
http://code.nsnam.org/nbaldo/ns-3-antenna/rev/1cc2ff632220

Regards,

Nicola

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.cc
File src/antenna/model/angles.cc (right):

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.cc#newc...
src/antenna/model/angles.cc:23: #include <math.h>
On 2012/04/09 05:38:36, Tom Henderson wrote:
> we seem to be going with cmath.h now

Done.

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.h
File src/antenna/model/angles.h (right):

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.h#newco...
src/antenna/model/angles.h:65: *
On 2012/04/09 05:38:36, Tom Henderson wrote:
> suggest to clarify the ascii art that standard spherical coordinates are used,
> with phi measured counterclockwise in the X/Y plane off the x-axis, and theta
> measured off the z-axis.

Done.

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.h#newco...
src/antenna/model/angles.h:67: struct Angles
On 2012/04/09 05:38:36, Tom Henderson wrote:
> would 'Direction' be a more appropriate name for this class?

Could be. Shall we trigger a poll on ns-developers? I'll leave it as-is for now.

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/angles.h#newco...
src/antenna/model/angles.h:70: * default constructor, will inizialize phi and
theta to zero
On 2012/04/09 05:38:36, Tom Henderson wrote:
> initialize

Done.

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/antenna-model.h
File src/antenna/model/antenna-model.h (right):

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/antenna-model....
src/antenna/model/antenna-model.h:37: * spherical coordinates, in particolar of
the azimuth and inclination
On 2012/04/09 05:38:36, Tom Henderson wrote:
> s/in particolar of/in particular,/

Done.

http://codereview.appspot.com/5726045/diff/1/src/antenna/model/antenna-model....
src/antenna/model/antenna-model.h:62: * \return the gain in dB of the antenna
radiation pattern at the specified angles
On 2012/04/09 05:38:36, Tom Henderson wrote:
> Suggest a bit more detail in the documentation.  The gain being returned is
the
> gain over isotropic (dBi).  As for whether it is directive gain or power gain,
I
> believe that this API is returning a power gain (and if efficiency is less
than
> one, it is just built into the model), but perhaps you can clarify.

I actually missed this distinction, but based on your comment plus a quick
investigation I agree that the model is meant to return a power gain. I've
updated the doxygen to clarify this.

[tomh_tomh.org, 2012-04-11 14:23:03]

> On 2012/04/09 05:38:36, Tom Henderson wrote:
>> would 'Direction' be a more appropriate name for this class?
>
> Could be. Shall we trigger a poll on ns-developers? I'll leave it as-is
> for now.
>

I think it is up to you and doesn't require a poll.  I wasn't sure about 
what Angles were when I first read the API, and then I wondered whether 
another name would help, and Direction seemed the best I could think of 
(there didn't seem to be a standard name for this in the literature). 
I'm also OK if you ignore this comment and leave as is.

- Tom