LTE module for ns-3

[Nicola Baldo, 2010-10-21 17:29:36]

Hi Giuseppe,

here are some comments on the latest code that you posted for review. Sorry for
the delay... the fact is that you really wrote so much code that it takes ages
to go through all of it!

First, a few generic issues:

1) There is one file (.lte-propagation-loss-model.cc.swp) which should not be
included in the patch

2) your examples do not pass valgrind tests. I think this is a prerequisite for
merging into ns-3-dev. Looking at your code, I see several places in which you
allocate objects like this:
  Ptr<Something> = new Something; 
you should change it to
  Ptr<Something> = CreateObject<Something> ();
maybe it won't fix all of your valgrind errors, but definitely several of them.

Please find the detailed comments below.

http://codereview.appspot.com/2349043/diff/1/examples/spectrum/waf
File examples/spectrum/waf (right):

http://codereview.appspot.com/2349043/diff/1/examples/spectrum/waf#newcode1
examples/spectrum/waf:1: exec "`dirname "$0"`"/../../waf "$@"
I see no change in this file, so it should not be included in the patch

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/AUTHORS
File src/devices/lte/AUTHORS (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/AUTHORS#newcode6
src/devices/lte/AUTHORS:6: G.Piro@poliba.it
I suggest to mention the Google Summer of Code for the funding, and also the
mentors for their supervision.

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

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:50: class LteHelper //:public
PcapHelperForDevice, public AsciiTraceHelperForDevice
if you don't use it, delete it rather than commenting it

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:75: Ptr<LtePhy> CreatePhy
(Ptr<SpectrumChannel> dlChannel, Ptr<SpectrumChannel> ulChannel, NetDeviceType
t);
several public methods like this are actually only used inside the LteHelper
class, and are not expected to be called by the user directly. You should make
these methods private.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:136: Ptr<SingleModelSpectrumChannel>
GetDownlinkChannel ();
This method is not used and should be deleted.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:141: Ptr<SingleModelSpectrumChannel>
GetUplinkChannel ();
Also this method is not used and should be deleted.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module.cc
File src/devices/lte/model/amc-module.cc (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.cc:35: int CQIIndex[15] = {
coding style: replace CQI with Cqi throughout this file

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.cc:48: int MCSIndex[32] = {
coding style: replace MCS with Mcs throughout this file

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.cc:115:
AmcModule::GetCqiFromFromSpectralEfficiency (double s)
FromFrom ?

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module.h
File src/devices/lte/model/amc-module.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.h:57: int GetCqiFromFromSpectralEfficiency
(double s);
I see you call this only within the AmcModule. Should you make this private
instead?

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.h:65: int GetMCSFromCQI (int cqi);
coding style: should be GetMcsFromCqi

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.h:72: int GetTBSizeFromMCS (int mcs);
coding style: should be GetTbSizeFromMcs

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.h:80: double GetSpectralEfficiencyFromCQI (int
cqi);
coding style: CQI --> Cqi

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
File src/devices/lte/model/bearer-qos-parameters.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
src/devices/lte/model/bearer-qos-parameters.h:36: class BearerQoSParameters :
public Object
coding style: should be BearerQosParameters (lower-case "s")

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
src/devices/lte/model/bearer-qos-parameters.h:72: enum BearerQoSType
coding style: BearerQoSType --> BearerQosType

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
src/devices/lte/model/bearer-qos-parameters.h:93: void SetQCI (int qci);
coding style: QCI --> Qci

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
src/devices/lte/model/bearer-qos-parameters.h:120: void SetGBR (double gbr);
coding style: GBR --> Gbr

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
src/devices/lte/model/bearer-qos-parameters.h:129: void SetMBR (double mbr);
coding style: MBR --> Mbr

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/channel-re...
File src/devices/lte/model/channel-realization.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/channel-re...
src/devices/lte/model/channel-realization.h:21: #ifndef CHANNEL_REALIZZATION_H
only one "Z"

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/ideal-cont...
File src/devices/lte/model/ideal-control-messages.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/ideal-cont...
src/devices/lte/model/ideal-control-messages.h:37: class IdealControlMessage :
public Object
Does IdealControlMessage really need Object functionality? 
If all you need is reference counting, you should rather inherit from
SimpleRefCount.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/loss-model.h
File src/devices/lte/model/loss-model.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/loss-model...
src/devices/lte/model/loss-model.h:36: class LossModel : public Object
You should find a more meaningful name: in fact, in ns-3 there is already a
PropagationLossModel class, so if you call your class LossModel it generates
confusion. 
Since this class does not actually provide any loss modeling functionality, why
don't you just call it DiscreteTimeModel or something like that?

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-mac-qu...
File src/devices/lte/model/lte-mac-queue.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-mac-qu...
src/devices/lte/model/lte-mac-queue.h:104: * \brief Get the length of the queue
with MAC/RLC/CRC overhead
what do you mean by CRC?

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

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-net-de...
src/devices/lte/model/lte-net-device.h:98: //PIRO: RLC entity has been moved to
each bearer!
ok then you can remove this code

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-net-de...
src/devices/lte/model/lte-net-device.h:135: * Starts the run of this device
"Start the run" is not very clear

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-phy.h
File src/devices/lte/model/lte-phy.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-phy.h#...
src/devices/lte/model/lte-phy.h:125: void SetDownlinkSubChannels
(std::vector<int> mask );
you should explain in general what you mean by sub channel (a resource block?),
and what does each integer value represent in the simulator

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-phy.h#...
src/devices/lte/model/lte-phy.h:135: void SetUplinkSubChannels (std::vector<int>
mask);
same comment as for SetDownlinkSubChannels

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
File src/devices/lte/model/lte-spectrum-error-model.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
src/devices/lte/model/lte-spectrum-error-model.h:46: class LteSpectrumErrorModel
: public SpectrumErrorModel
The LteSpectrumErrorModel class is not used in any other file. Furthermore, you
clearly say that the model is not implemented. Therefore I would not include the
files lte-spectrum-error-model.* in the patch to be merged to ns-3-dev.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
File src/devices/lte/model/lte-spectrum-phy.cc (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
src/devices/lte/model/lte-spectrum-phy.cc:332: // m_interference->AddSignal
(rxPsd, duration);
You are not doing any interference calculation. To get interference calculation
right (e.g., for inter-cell interference), it would be required to also
implement cell synchronization properly. I guess that's too much work for now,
so I suggest that you just do the following:

- do not use SpectrumInterference any more in LteSpectrumPhy

- just have a m_noisePowerSpectralDensity variable as an attribute of the
SpectrumPhy. After all, right now you use SpectrumInterference only to store the
noise PSD.

This approach has the additional merit that the modifications to
src/devices/spectrum-interference.* are not needed any more :-)

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
File src/devices/lte/model/lte-spectrum-phy.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
src/devices/lte/model/lte-spectrum-phy.h:3: * Copyright (c) 2010 TELEMATICS LAB,
DEE - Politecnico di Bari
This file is clearly a modified version of
src/devices/spectrum/half-duplex-ideal-phy.h 

The GPL of course allows you to copy & modify existing code, but only if you do
the following:

1) preserve the Copyright statement of the original file (you cannot delete it
or change the name of the copyright holder);

2) write a comment saying explicitly that you modified the file 

As for the "Authors:" statement, the GPL does not say anything about it.  Still,
it would be nice if you could append your name to the pre-existing author names,
without deleting them. 

The same comments applies to lte-spectrum-phy.cc. If you know other files that
are affected by this issue, please do the necessary corrections.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/multipath-...
File src/devices/lte/model/multipath-loss-model.cc (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/multipath-...
src/devices/lte/model/multipath-loss-model.cc:136: speed = 30;
can you put some comment explaining what you are doing in the above?

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/multipath-...
File src/devices/lte/model/multipath-loss-model.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/multipath-...
src/devices/lte/model/multipath-loss-model.h:37: class MultipathLossModel :
public LossModel
if what you do in practice is to implement Jakes, why don't you name it
JakesFadingModel or something similar?

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/penetratio...
File src/devices/lte/model/penetration-loss-model.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/penetratio...
src/devices/lte/model/penetration-loss-model.h:31: * \brief This class models
the propagation loss model due to the path loss
the comment does probably not refer to this class

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/radio-bear...
File src/devices/lte/model/radio-bearer-instance.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/radio-bear...
src/devices/lte/model/radio-bearer-instance.h:122: void SetQoSParameters
(Ptr<BearerQoSParameters> qosParameters);
coding style: should be SetQosParameters

[GiuseppePiro, 2010-12-01 16:52:45]

Hi Nicola, Hi All,

sorry for the delay! I hope to have fixed all things that Nicola suggested
during the latest review.  

As Nicola described last week on ns-developers list, we can now create a
repository with a clean history and then Test the code with vandrind.

Regards,

Giuseppe

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/AUTHORS
File src/devices/lte/AUTHORS (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/AUTHORS#newcode6
src/devices/lte/AUTHORS:6: G.Piro@poliba.it
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> I suggest to mention the Google Summer of Code for the funding, and also the
> mentors for their supervision.

Done.

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

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:50: class LteHelper //:public
PcapHelperForDevice, public AsciiTraceHelperForDevice
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> if you don't use it, delete it rather than commenting it

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:75: Ptr<LtePhy> CreatePhy
(Ptr<SpectrumChannel> dlChannel, Ptr<SpectrumChannel> ulChannel, NetDeviceType
t);
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> several public methods like this are actually only used inside the LteHelper
> class, and are not expected to be called by the user directly. You should make
> these methods private. 

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:136: Ptr<SingleModelSpectrumChannel>
GetDownlinkChannel ();
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> This method is not used and should be deleted.

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/helper/lte-helpe...
src/devices/lte/helper/lte-helper.h:141: Ptr<SingleModelSpectrumChannel>
GetUplinkChannel ();
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> Also this method is not used and should be deleted.

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module.cc
File src/devices/lte/model/amc-module.cc (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.cc:35: int CQIIndex[15] = {
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: replace CQI with Cqi throughout this file

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.cc:48: int MCSIndex[32] = {
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: replace MCS with Mcs throughout this file

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.cc:115:
AmcModule::GetCqiFromFromSpectralEfficiency (double s)
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> FromFrom ?

Fixed

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module.h
File src/devices/lte/model/amc-module.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.h:57: int GetCqiFromFromSpectralEfficiency
(double s);
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> I see you call this only within the AmcModule. Should you make this private
> instead?

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.h:65: int GetMCSFromCQI (int cqi);
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: should be GetMcsFromCqi

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.h:72: int GetTBSizeFromMCS (int mcs);
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: should be GetTbSizeFromMcs

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/amc-module...
src/devices/lte/model/amc-module.h:80: double GetSpectralEfficiencyFromCQI (int
cqi);
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: CQI --> Cqi

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
File src/devices/lte/model/bearer-qos-parameters.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
src/devices/lte/model/bearer-qos-parameters.h:36: class BearerQoSParameters :
public Object
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: should be BearerQosParameters (lower-case "s")

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
src/devices/lte/model/bearer-qos-parameters.h:72: enum BearerQoSType
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: BearerQoSType --> BearerQosType

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
src/devices/lte/model/bearer-qos-parameters.h:93: void SetQCI (int qci);
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: QCI --> Qci

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
src/devices/lte/model/bearer-qos-parameters.h:120: void SetGBR (double gbr);
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: GBR --> Gbr

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/bearer-qos...
src/devices/lte/model/bearer-qos-parameters.h:129: void SetMBR (double mbr);
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: MBR --> Mbr

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/channel-re...
File src/devices/lte/model/channel-realization.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/channel-re...
src/devices/lte/model/channel-realization.h:21: #ifndef CHANNEL_REALIZZATION_H
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> only one "Z"

oooops..! Fixed

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/ideal-cont...
File src/devices/lte/model/ideal-control-messages.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/ideal-cont...
src/devices/lte/model/ideal-control-messages.h:37: class IdealControlMessage :
public Object
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> Does IdealControlMessage really need Object functionality? 
> If all you need is reference counting, you should rather inherit from
> SimpleRefCount.

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/loss-model.h
File src/devices/lte/model/loss-model.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/loss-model...
src/devices/lte/model/loss-model.h:36: class LossModel : public Object
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> You should find a more meaningful name: in fact, in ns-3 there is already a
> PropagationLossModel class, so if you call your class LossModel it generates
> confusion. 
> Since this class does not actually provide any loss modeling functionality,
why
> don't you just call it DiscreteTimeModel or something like that?

Ok, I'm agree with you! I changed the class name to DiscreteTimeLossModel

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-mac-qu...
File src/devices/lte/model/lte-mac-queue.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-mac-qu...
src/devices/lte/model/lte-mac-queue.h:104: * \brief Get the length of the queue
with MAC/RLC/CRC overhead
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> what do you mean by CRC?

CRC is the checksum added at the end of the packet wich is used to check its
integrity.

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

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-net-de...
src/devices/lte/model/lte-net-device.h:98: //PIRO: RLC entity has been moved to
each bearer!
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> ok then you can remove this code

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-net-de...
src/devices/lte/model/lte-net-device.h:135: * Starts the run of this device
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> "Start the run" is not very clear

Improved the method description

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-phy.h
File src/devices/lte/model/lte-phy.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-phy.h#...
src/devices/lte/model/lte-phy.h:125: void SetDownlinkSubChannels
(std::vector<int> mask );

Improved description

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-phy.h#...
src/devices/lte/model/lte-phy.h:135: void SetUplinkSubChannels (std::vector<int>
mask);
Improved description

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
File src/devices/lte/model/lte-spectrum-error-model.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
src/devices/lte/model/lte-spectrum-error-model.h:46: class LteSpectrumErrorModel
: public SpectrumErrorModel
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> The LteSpectrumErrorModel class is not used in any other file. Furthermore,
you
> clearly say that the model is not implemented. Therefore I would not include
the
> files lte-spectrum-error-model.* in the patch to be merged to ns-3-dev.


Removed!

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
File src/devices/lte/model/lte-spectrum-phy.cc (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
src/devices/lte/model/lte-spectrum-phy.cc:332: // m_interference->AddSignal
(rxPsd, duration);
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> You are not doing any interference calculation. To get interference
calculation
> right (e.g., for inter-cell interference), it would be required to also
> implement cell synchronization properly. I guess that's too much work for now,
> so I suggest that you just do the following:
> 
> - do not use SpectrumInterference any more in LteSpectrumPhy
> 
> - just have a m_noisePowerSpectralDensity variable as an attribute of the
> SpectrumPhy. After all, right now you use SpectrumInterference only to store
the
> noise PSD.
> 
> This approach has the additional merit that the modifications to
> src/devices/spectrum-interference.* are not needed any more :-)

Done.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
File src/devices/lte/model/lte-spectrum-phy.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/lte-spectr...
src/devices/lte/model/lte-spectrum-phy.h:3: * Copyright (c) 2010 TELEMATICS LAB,
DEE - Politecnico di Bari
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> This file is clearly a modified version of
> src/devices/spectrum/half-duplex-ideal-phy.h 
> 
> The GPL of course allows you to copy & modify existing code, but only if you
do
> the following:
> 
> 1) preserve the Copyright statement of the original file (you cannot delete it
> or change the name of the copyright holder);
> 
> 2) write a comment saying explicitly that you modified the file 
> 
> As for the "Authors:" statement, the GPL does not say anything about it. 
Still,
> it would be nice if you could append your name to the pre-existing author
names,
> without deleting them. 
> 
> The same comments applies to lte-spectrum-phy.cc. If you know other files that
> are affected by this issue, please do the necessary corrections.


Fixed!

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/multipath-...
File src/devices/lte/model/multipath-loss-model.cc (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/multipath-...
src/devices/lte/model/multipath-loss-model.cc:136: speed = 30;
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> can you put some comment explaining what you are doing in the above?

Ok, comments have been added.

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/multipath-...
File src/devices/lte/model/multipath-loss-model.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/multipath-...
src/devices/lte/model/multipath-loss-model.h:37: class MultipathLossModel :
public LossModel
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> if what you do in practice is to implement Jakes, why don't you name it
> JakesFadingModel or something similar?

Ok, The class name is now JakesFadingLossModel

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/penetratio...
File src/devices/lte/model/penetration-loss-model.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/penetratio...
src/devices/lte/model/penetration-loss-model.h:31: * \brief This class models
the propagation loss model due to the path loss
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> the comment does probably not refer to this class

Fixed!

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/radio-bear...
File src/devices/lte/model/radio-bearer-instance.h (right):

http://codereview.appspot.com/2349043/diff/1/src/devices/lte/model/radio-bear...
src/devices/lte/model/radio-bearer-instance.h:122: void SetQoSParameters
(Ptr<BearerQoSParameters> qosParameters);
On 2010/10/21 17:29:36, Nicola Baldo wrote:
> coding style: should be SetQosParameters

Done.