ns-3 LTE module candidate for the merge

[Tom Henderson, 2010-12-04 22:24:24]

Giuseppe, here are a few more comments.

http://codereview.appspot.com/3390042/diff/1/doc/manual/lte.texi
File doc/manual/lte.texi (right):

http://codereview.appspot.com/3390042/diff/1/doc/manual/lte.texi#newcode320
doc/manual/lte.texi:320: This model has been tested as follows...
Can you complete this section?

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/helper/lte-helper.h
File src/devices/lte/helper/lte-helper.h (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:75: void AddMobility (Ptr<LtePhy> phy,
Ptr<MobilityModel>m);
I did not realize (perhaps I missed this earlier) that spectrum Phys contain
their own mobility model, instead of nodes.  I guess that this is a question for
Nicola.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:88: * \param enb
Incomplete doxygen

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:103: void AddDownlinkChannelRealization
(Ptr<MobilityModel> enbMobility,
why not naming this "AddDownlinkChannel"?

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/model/ue-mac-ent...
File src/devices/lte/model/ue-mac-entity.cc (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/model/ue-mac-ent...
src/devices/lte/model/ue-mac-entity.cc:44: .SetParent<Object> ();
Please check all of the "SetParent() calls in your GetTypeIds() so that the
metadata tree preserves the class relationships.  Parent of UeMacEntity is
MacEntity.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/model/ue-net-dev...
File src/devices/lte/model/ue-net-device.h (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/model/ue-net-dev...
src/devices/lte/model/ue-net-device.h:110: bool SendPacket (Ptr<PacketBurst> p);
Missing doxygen.

Note, I suggest that you try to eliminate all new doxygen warnings as follows: 
run ./waf --doxygen on your new repository, and pipe the warning messages to a
file. Then, search for all warnings that pertain to your device directory.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/model/ue-net-dev...
src/devices/lte/model/ue-net-device.h:111: 
Missing doxygen.

Note, one way to pick out all of your missing doxygen is to run ./waf --doxygen
on your repo, pipe the warning messages to a file, and then search the file for
your directory name.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-bearer-...
File src/devices/lte/test/lte-bearer-test.cc (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-bearer-...
src/devices/lte/test/lte-bearer-test.cc:64: : TestCase ("Test the LTE Bearer.")
This test only tests that the Bearer can enqueue and dequeue one packet, so
suggest to have a more precise description of what you are testing here (e.g.
"Test that LTE Bearer can enqueue and dequeue one packet")

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-bearer-...
src/devices/lte/test/lte-bearer-test.cc:95: return true; // the queue should
have 1 packet
For future reference, please note that there are macros designed for testing
values in test DoRun() methods.  See src/test/csma-system-test.cc for examples).

e.g. NS_TEST_ASSERT_MSG_EQ ()

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-device-...
File src/devices/lte/test/lte-device-test.cc (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-device-...
src/devices/lte/test/lte-device-test.cc:49: */
this comment is copy pasted (should read that it tests the device)

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-device-...
src/devices/lte/test/lte-device-test.cc:62: : TestCase ("Test the LTE Device.")
could be more specific to say "Test that the LTE device can send a packet"

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-phy-tes...
File src/devices/lte/test/lte-phy-test.cc (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-phy-tes...
src/devices/lte/test/lte-phy-test.cc:57: : TestCase ("Test the LTE physical
layer.")
Test string may be more descriptive to state "Test that ENB and UE Phys can
transmit a packet"

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-phy-tes...
src/devices/lte/test/lte-phy-test.cc:95: Ipv4InterfaceContainer ENBinterface =
address.Assign (enbDevs);
There is a lot of stuff in this program definition that is  not used in your
actual test (e.g. internet addresses) so I suggest to strip out all unnecessary
dependencies on unused things in your tests.

[GiuseppePiro, 2010-12-06 11:59:20]

Hi Tom, 

thanks for your review. I improved the code according your suggestions. 

Regarding the definition of the mobility model into the SpectrumPhy class, I
would ask to Nicola his opinion. 


Regards,

Giuseppe

http://codereview.appspot.com/3390042/diff/1/doc/manual/lte.texi
File doc/manual/lte.texi (right):

http://codereview.appspot.com/3390042/diff/1/doc/manual/lte.texi#newcode320
doc/manual/lte.texi:320: This model has been tested as follows...
On 2010/12/04 22:24:24, Tom Henderson wrote:
> Can you complete this section?

Done.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/helper/lte-helper.h
File src/devices/lte/helper/lte-helper.h (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:75: void AddMobility (Ptr<LtePhy> phy,
Ptr<MobilityModel>m);
On 2010/12/04 22:24:24, Tom Henderson wrote:
> I did not realize (perhaps I missed this earlier) that spectrum Phys contain
> their own mobility model, instead of nodes.  I guess that this is a question
for
> Nicola.

Nicola, what do you think about ?

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:88: * \param enb
On 2010/12/04 22:24:24, Tom Henderson wrote:
> Incomplete doxygen

Done.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:103: void AddDownlinkChannelRealization
(Ptr<MobilityModel> enbMobility,
On 2010/12/04 22:24:24, Tom Henderson wrote:
> why not naming this "AddDownlinkChannel"?


I think that name "AddDownlinkChannelRealization" describes well what the method
do (create a new realization of the channel model between the two nodes).
Maybe  the name  "CreateDownlinkChannelRealization" is better! Are you agree ?

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/model/ue-mac-ent...
File src/devices/lte/model/ue-mac-entity.cc (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/model/ue-mac-ent...
src/devices/lte/model/ue-mac-entity.cc:44: .SetParent<Object> ();
On 2010/12/04 22:24:24, Tom Henderson wrote:
> Please check all of the "SetParent() calls in your GetTypeIds() so that the
> metadata tree preserves the class relationships.  Parent of UeMacEntity is
> MacEntity.

Done.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/model/ue-net-dev...
File src/devices/lte/model/ue-net-device.h (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/model/ue-net-dev...
src/devices/lte/model/ue-net-device.h:110: bool SendPacket (Ptr<PacketBurst> p);
On 2010/12/04 22:24:24, Tom Henderson wrote:
> Missing doxygen.
> 
> Note, I suggest that you try to eliminate all new doxygen warnings as follows:

> run ./waf --doxygen on your new repository, and pipe the warning messages to a
> file. Then, search for all warnings that pertain to your device directory.

This method has been defined into the LteNetDevice where the doxygen has been
defined. 
Do you think that it is necessary to add doxigen also here ?

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/model/ue-net-dev...
src/devices/lte/model/ue-net-device.h:111: 
On 2010/12/04 22:24:24, Tom Henderson wrote:
> Missing doxygen.
> 
> Note, one way to pick out all of your missing doxygen is to run ./waf
--doxygen
> on your repo, pipe the warning messages to a file, and then search the file
for
> your directory name.

Done.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-bearer-...
File src/devices/lte/test/lte-bearer-test.cc (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-bearer-...
src/devices/lte/test/lte-bearer-test.cc:64: : TestCase ("Test the LTE Bearer.")
On 2010/12/04 22:24:24, Tom Henderson wrote:
> This test only tests that the Bearer can enqueue and dequeue one packet, so
> suggest to have a more precise description of what you are testing here (e.g.
> "Test that LTE Bearer can enqueue and dequeue one packet")

Done.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-bearer-...
src/devices/lte/test/lte-bearer-test.cc:95: return true; // the queue should
have 1 packet
On 2010/12/04 22:24:24, Tom Henderson wrote:
> For future reference, please note that there are macros designed for testing
> values in test DoRun() methods.  See src/test/csma-system-test.cc for
examples).
> 
> e.g. NS_TEST_ASSERT_MSG_EQ ()
> 

Tom, thanks for the suggestion. I will consider this approach in the future.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-device-...
File src/devices/lte/test/lte-device-test.cc (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-device-...
src/devices/lte/test/lte-device-test.cc:49: */
On 2010/12/04 22:24:24, Tom Henderson wrote:
> this comment is copy pasted (should read that it tests the device)

Fixed.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-device-...
src/devices/lte/test/lte-device-test.cc:62: : TestCase ("Test the LTE Device.")
On 2010/12/04 22:24:24, Tom Henderson wrote:
> could be more specific to say "Test that the LTE device can send a packet"

Done.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-phy-tes...
File src/devices/lte/test/lte-phy-test.cc (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-phy-tes...
src/devices/lte/test/lte-phy-test.cc:57: : TestCase ("Test the LTE physical
layer.")
On 2010/12/04 22:24:24, Tom Henderson wrote:
> Test string may be more descriptive to state "Test that ENB and UE Phys can
> transmit a packet"

Done.

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/test/lte-phy-tes...
src/devices/lte/test/lte-phy-test.cc:95: Ipv4InterfaceContainer ENBinterface =
address.Assign (enbDevs);
On 2010/12/04 22:24:24, Tom Henderson wrote:
> There is a lot of stuff in this program definition that is  not used in your
> actual test (e.g. internet addresses) so I suggest to strip out all
unnecessary
> dependencies on unused things in your tests.

Done.

[Tom Henderson, 2010-12-07 05:57:20]

> src/devices/lte/helper/lte-helper.h:103: void AddDownlinkChannelRealization
> (Ptr<MobilityModel> enbMobility,
> On 2010/12/04 22:24:24, Tom Henderson wrote:
> > why not naming this "AddDownlinkChannel"?
> 
> 
> I think that name "AddDownlinkChannelRealization" describes well what the
method
> do (create a new realization of the channel model between the two nodes).
> Maybe  the name  "CreateDownlinkChannelRealization" is better! Are you agree ?

I am fine to leave it as you originally have it.

> 
> This method has been defined into the LteNetDevice where the doxygen has been
> defined. 
> Do you think that it is necessary to add doxigen also here ?

OK, I did not see the virtual keyword so thought it was new to this class.  If
Doxygen does not complain, then OK to leave it as it is (or make it virtual).

[GiuseppePiro, 2010-12-08 12:01:29]

On 7 December 2010 06:57, <tomh.org@gmail.com> wrote:

>
>  src/devices/lte/helper/lte-helper.h:103: void
>>
> AddDownlinkChannelRealization
>
>> (Ptr<MobilityModel> enbMobility,
>> On 2010/12/04 22:24:24, Tom Henderson wrote:
>> > why not naming this "AddDownlinkChannel"?
>>
>
>
>  I think that name "AddDownlinkChannelRealization" describes well what
>>
> the method
>
>> do (create a new realization of the channel model between the two
>>
> nodes).
>
>> Maybe  the name  "CreateDownlinkChannelRealization" is better! Are you
>>
> agree ?
>
> I am fine to leave it as you originally have it.
>
>
ok, done!


>
>
>  This method has been defined into the LteNetDevice where the doxygen
>>
> has been
>
>> defined.
>> Do you think that it is necessary to add doxigen also here ?
>>
>
> OK, I did not see the virtual keyword so thought it was new to this
> class.  If Doxygen does not complain, then OK to leave it as it is (or
> make it virtual).
>
>
>
>
>
yes, doxygen compiles without any warnings for the lte module.


>
> http://codereview.appspot.com/3390042/
>


Giuseppe

[Nicola Baldo, 2010-12-12 17:58:01]

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/helper/lte-helper.h
File src/devices/lte/helper/lte-helper.h (right):

http://codereview.appspot.com/3390042/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:75: void AddMobility (Ptr<LtePhy> phy,
Ptr<MobilityModel>m);
On 2010/12/04 22:24:24, Tom Henderson wrote:
> I did not realize (perhaps I missed this earlier) that spectrum Phys contain
> their own mobility model, instead of nodes.  I guess that this is a question
for
> Nicola.

Short answer: this is similar to what wifi does,see YansWifiPhy::SetMobility
(Ptr<Object> mobility)

Long answer: The interface with SpectrumChannel requires that you pass a
Ptr<MobilityModel> whenever you start a transmission. This is of course expected
to be a pointer to the MobilityModel of the sending node. The way the sender PHY
retrieves this pointer is left to the implementer. You could retrieve that using
GetNode()->GetObject<MobilityModel>() instead of using a m_mobilityModel member
variable. I though the latter approach was preferred for performance reasons.