ns-3 LTE module from the LENA project

[Piro Giuseppe, 2011-10-16 07:05:40]

Dear Nicola and all,
I finished to see the code produced within the LENA project.

The first my personal comment is: "it is a lot of code!".
This means that I cannot ensure that all implemented features have been
developed correctly.
Probably I missed something during the review  but I hope taht in the near
future, other people (including myself)
will be able to test more and more the module and report to us possible
neglected bugs.

The summary of my review is reported in what follows:

- The code is well written.
- The NS-3 coding style  has been always respected.
- Regarding the documentation, instead, several parts of the MAC layer have not
been well documentated. I think it is not a real problem for the merging process
but it should be fixed as soon as possible (for an lte-inexperienced, the 
understanding the code represents a very hard work).
- the development of all LTE network devices and all protocol stack  entities
has been done with a particular attention, following guidelines provided by both
3gpp standards and femto forum suggestions.

Accordingly,  despite the amount of code, only few comments have been added to
the code.


The only big issue, related to the uplink channel quality, should be properly
discussed.

Firstly, the creation of CQI feedbacks is a feature handled by only the UE
device, not for the eNB. However, the function GenerateCquiFeedback has been
defined for both devices.
I saw that the eNB uses this method for estimating the uplink channel quality of
a given UE. This procedure does not create any feedback and, for this reason, it
is better to:
- use another function for doing this, defining the GenerateCqiFeedback method
only for the UE.
- delete the ideal control messages related to the uplink cqi feedback

Another point that I haven't understand is the management of the uplink channel
quality.
The uplink channel quality is different for all UEs.
In order to select the proper UL MCS, the eNB must store uplink channel
informations separately for each UE.
From the code, instead, it seams that the eNB uses only one variable to store
these informations, and this variable is the same for all UEs.

In details, when the eNB PHY interface receives a packet burst from the channel,
it calls the method EnbPhySapUser::UlCqiReport for creating the uplink channel
indicator and for storing them into the LteEnbMac::m_ulCqiReceived variable. I
have understood that this variable is the same for all UEs. This is not correct:
the eNb should store these information for all UEs separately.
If i have misinterpreted the code, please clarify this point. Otherwise, I think
it should be fixed.


Having said that, it is imporant to remark that the proposed code outperforms
features provided by the LTE module developed within the GSoC 2010. For this
reason, despite the lack of backward compatibility with it, I totally agree for
merging the LENA project into the ns-3-dev.


Best regards,
Giuseppe

http://codereview.appspot.com/4626069/diff/1/README
File README (right):

http://codereview.appspot.com/4626069/diff/1/README#newcode111
README:111: 
Why this file has been changed just for a white line ?

http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc
File src/lte/helper/lena-helper.cc (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc#ne...
src/lte/helper/lena-helper.cc:61: m_uplinkChannel =
CreateObject<SingleModelSpectrumChannel> ();
Just a comment on the creation of DL and UL channels. In this implementation
only one DL/UL channel is created for all cells. This can increase the
computational load in scenario composed by several cells and several users (you
can think for example that a packet sent by one eNB will be received/processed
by all users, also if their are connected to another base station).
Maybe create a couple of DL/UL channel for each cell can avoid this problem. 

Obviously, this can cause some problem to the implementation of the interference
model that should be properly analyzed. 

Note that this is only my personal opinion. The current implementation is ok for
the merge.

http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc#ne...
src/lte/helper/lena-helper.cc:143: Start ();  // will run DoStart () if
necessary
Why you do not call Start () into the constructor ? In this way the DoStart ()
method is called always at the beginning.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ff-mac-sched-sap.h
File src/lte/model/ff-mac-sched-sap.h (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ff-mac-sched-sap.h...
src/lte/model/ff-mac-sched-sap.h:199: // (See 4.2 for description of the
primitives)
sooner or later we have to add documentation also for these methods.

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

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:138: * Direction for which the message is
created
Previous lines are the same. What is the utility of this diff ?

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:152: /** the sub channel */
Previous lines are the same. What is the utility of this diff ?

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:223: int m_idSubChannel;
Previous lines are the same. What is the utility of this diff ?

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:355: * The Fempto forum downlink
CqiIdealControlMessage defines an ideal list of
Fempto -> Femto

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:387: #define
ULCQI_IDEAL_CONTROL_MESSAGES_H
I didn't understand the utility of this message. 
For the downlink, the UE estimates the channel quality and sends CQi feedbacks
to the eNB. In this case it is explained the utility of the
DlCqiIdealControlMessage class.

For the uplink, instead, the eNB does not send any CQI fedbacks. The eNB is able
to estimate the uplink channel quality from the pilot signals that UEs send
through the uplink channel (and this feature has been already handled by
LteEnbPhy::CreateUlCqiReport).


I have also noted that this message is never created into the code. So, it
should be deleted.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:444: * The Fempto forum uplink
BsrIdealControlMessage defines the specific
Fempto -> Femto

http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-...
File src/lte/model/jakes-fading-loss-model.cc (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-...
src/lte/model/jakes-fading-loss-model.cc:114: // mobility = GetPhy
()->GetDownlinkSpectrumPhy ()->GetMobility ()->GetObject<MobilityModel> ();
What is the reason of this choose ? The fast fading depends from the user speed.


If you set the mobility to NULL, the following code will not work. Did you
deleted the impact of fast fading from the complete channel model realization ? 

If so, why ? Make sure that in the future the fast fading will be included into
the channel model. It is necessary for differentiate the channel quality
estimated by UE that are at the same distance from the eNB

http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-...
File src/lte/model/jakes-fading-loss-model.h (left):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-...
src/lte/model/jakes-fading-loss-model.h:60: * \brief Set the physical layer
Maybe this patch is unnecessary

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

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-amc.h#newcode39
src/lte/model/lte-amc.h:39: * need to create and manage instances of this class.
Why did you choose this strategy ? 
I think the better is that each node should have its amc module, defined at the
mac layer. 
Note that some information, such as the tbsize, depend from the PHY parameters
(i.e., number of antennas, other ?). Different devices can use different PHY
configurations. In this case the tables defined into the amc module should be
different. 

How do you think to manage this situation with the current implementation ?

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-amc.h#newcode59
src/lte/model/lte-amc.h:59: static int GetTbSizeFromMcs (int mcs, int nprb);
I agree with the new definition of the tbsize table. However, I think it depends
from a particular PHY interface configuration (tx antennas, rx antennas and so
on). 
What happen if this configuration changes ? 
What happen if different nodes have different PHY configurations ?

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-mac.h
File src/lte/model/lte-enb-mac.h (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-mac.h#newc...
src/lte/model/lte-enb-mac.h:140: private:
sooner or later we have to add documentation also for these methods.

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

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-net-device...
src/lte/model/lte-enb-net-device.h:160: Ptr<FfMacScheduler> m_scheduler;
The packet scheduler is a functionality of the MAC entity. Maybe it is more
correct ti define this object into the LteEnbMac

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc
File src/lte/model/lte-enb-phy.cc (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc#new...
src/lte/model/lte-enb-phy.cc:446: Ptr<LteEnbNetDevice> thisDevice = GetDevice
()->GetObject<LteEnbNetDevice> ();
What is the role of thisDevice ?

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc#new...
src/lte/model/lte-enb-phy.cc:449: 
This function is used for computing the UL channel quality of a given UE, after
that the eNB have received a signal from it. 

I think is more correct  to modify the name of this function. For example, use
GenerateCqiFeedback for the UE and GenerateUlChannelIndicator for the enb. In
other words, delete the word "feedback" because the enb does not feedback this
value to any devices.

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

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode216
src/lte/model/lte-phy.h:216: * \param p queue control message to be sent
the parameter is m not p

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode231
src/lte/model/lte-phy.h:231: virtual void  GenerateCqiFeedback (const
SpectrumValue& sinr) = 0;
Why you insert this function here ? The generation of CQI feedback is done only
at the UE side. 
The eNB will never generate any CQI feedback. 

I saw that you create also an ideal message for uplink cqi. Maybe the two things
are connected... 

I suggest to move this functionality only at the UE and delete ideal control
message for the uplink cqi.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode256
src/lte/model/lte-phy.h:256: std::vector< std::list<Ptr<IdealControlMessage> > >
m_controlMessagesQueue;
It is not clear at all why do you create a vector of PacketBurst and a vector of
list of control messages. 
I didn't find any description into the documentation. Since this is a dedicated
point, I think that a more accurate description should be added. 

What is the relation among these variables and m_macChTtiDelay (In  the
constructor of LtePhy you create both vectors according to the m_macChTtiDelay
variable) ?

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.cc
File src/lte/model/lte-ue-phy.cc (left):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.cc#oldc...
src/lte/model/lte-ue-phy.cc:152: {
I suggest to move this functionality to the MAC layer.

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

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.h#newco...
src/lte/model/lte-ue-phy.h:192: 
The channel quality report is a MAC functionality. Why do you include
specifications related to this feature into the PHY object ?

http://codereview.appspot.com/4626069/diff/1/src/lte/test/lte-test-pf-ff-mac-...
File src/lte/test/lte-test-pf-ff-mac-scheduler.cc (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/test/lte-test-pf-ff-mac-...
src/lte/test/lte-test-pf-ff-mac-scheduler.cc:24: 
This test crashs (it is the only case among all implemented tests)


host-001:ns-3-lena-dev giuseppepiro$ ./test.py > out
Waf: Entering directory
`/Users/giuseppepiro/Documents/work/NS-3/repos/ns-3-lena-dev/build'
Waf: Leaving directory
`/Users/giuseppepiro/Documents/work/NS-3/repos/ns-3-lena-dev/build'
'build' finished successfully (0.437s)
host-001:ns-3-lena-dev giuseppepiro$ grep CRASH out 
CRASH: TestSuite lte-pf-ff-mac-scheduler
host-001:ns-3-lena-dev giuseppepiro$ 

I didn't understend the reason.

[Nicola Baldo, 2012-03-05 16:48:07]

Hi Giuseppe, 

thank you very much for your effort in reviewing the LENA code. I've gone
through your comments and provided a reply for each. We revised the code to
address some of your comments. For the  other comments where we think that a
revision of the code is not necessary, I motivated in the reply the reason why.

Also, please find right below my replies to the questions that you raised in
your previous review.

Kind Regards,

Nicola



> - Regarding the documentation, instead, several parts of the MAC layer have
not
> been well documentated. I think it is not a real problem for the merging
process
> but it should be fixed as soon as possible (for an lte-inexperienced, the 
> understanding the code represents a very hard work).

The LTE module "manual" (i.e., this one: http://lena.cttc.es/manual/ ) has been
greatly improved since your review, hopefully the description of the code
(including the MAC layer) is satisfactory now. Please see the detailed comments
for those cases where you pointed out specific items missing in the
documentation.


> Firstly, the creation of CQI feedbacks is a feature handled by only the UE
> device, not for the eNB. However, the function GenerateCquiFeedback has been
> defined for both devices.
> I saw that the eNB uses this method for estimating the uplink channel quality
of
> a given UE. This procedure does not create any feedback and, for this reason,
it
> is better to:
> - use another function for doing this, defining the GenerateCqiFeedback method
> only for the UE.
> - delete the ideal control messages related to the uplink cqi feedback

The use of a single method for both the UE and the eNB is motivated by the
desire to present a single interface to the interference calculation code (in
particular the class  LteCqiSinrChunkProcessor), which then can be shared
between he UE and the eNB. 
I think the misundertanding was mostly due to the ambiguous use of the term
"feedback", we renamed the method to GenerateCqiReport to avoid the ambiguity. 
As for the ideal control message for UL CQI, we removed it. 
Please see the replies to your comments in the code for further details.


> 
> Another point that I haven't understand is the management of the uplink
channel
> quality.
> The uplink channel quality is different for all UEs.
> In order to select the proper UL MCS, the eNB must store uplink channel
> informations separately for each UE.
> From the code, instead, it seams that the eNB uses only one variable to store
> these informations, and this variable is the same for all UEs.
> 
> In details, when the eNB PHY interface receives a packet burst from the
channel,
> it calls the method EnbPhySapUser::UlCqiReport for creating the uplink channel
> indicator and for storing them into the LteEnbMac::m_ulCqiReceived variable. I
> have understood that this variable is the same for all UEs. This is not
correct:
> the eNb should store these information for all UEs separately.
> If i have misinterpreted the code, please clarify this point. Otherwise, I
think
> it should be fixed.

This approach is in conformance with the FemtoForum MAC Scheduler API, see in
particular the primitive 4.2.12 SCHED_UL_CQI_INFO_REQ. 
In detail, for the UL, a single vector of CQI values is passed to the scheduler
for all users at the end of each TTI. This is not a limitation; in fact, the
scheduler knows to which RB each UE was allocated (by keeping track of the past
allocations), and can hence determine the CQI values in the CQI vector that
correspond to each UE.

http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc
File src/lte/helper/lena-helper.cc (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc#ne...
src/lte/helper/lena-helper.cc:61: m_uplinkChannel =
CreateObject<SingleModelSpectrumChannel> ();
We use a single channel exactly because we want to account for inter-cell
interference. If someone wants to have one channel per cell in order to speedup
simulations, it can be done simply by using separate LenaHelper instances in the
simulation program.

http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc#ne...
src/lte/helper/lena-helper.cc:143: Start ();  // will run DoStart () if
necessary
On 2011/10/16 07:05:40, Piro Giuseppe wrote:
> Why you do not call Start () into the constructor ? In this way the DoStart ()
> method is called always at the beginning. 

because if we do that then it won't work if the user calls
lenaHelper->SetXxxAttribute (). Hence, DoStart () needs to be delayed.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ff-mac-sched-sap.h
File src/lte/model/ff-mac-sched-sap.h (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ff-mac-sched-sap.h...
src/lte/model/ff-mac-sched-sap.h:199: // (See 4.2 for description of the
primitives)
On 2011/10/16 07:05:40, Piro Giuseppe wrote:
> sooner or later we have to add documentation also for these methods.

the whole class refers to the FemtoForum LTE MAC Scheduler Interface
Specification, as noted in the doxygen description of the class. The comment
above specifies that these methods are explained in section 4.2 of that
document. I think that document is pretty much exaustive and hence it is not
needed to add more detailed doxygen here.

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

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:138: * Direction for which the message is
created
I think the whitespace change was caused by running check-style.py

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:152: /** the sub channel */
I think the whitespace change was caused by running check-style.py

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:223: int m_idSubChannel;
I think the whitespace change was caused by running check-style.py

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:355: * The Fempto forum downlink
CqiIdealControlMessage defines an ideal list of
On 2011/10/16 07:05:40, Piro Giuseppe wrote:
> Fempto -> Femto

thanks, fixed.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:387: #define
ULCQI_IDEAL_CONTROL_MESSAGES_H
Thanks, we removed the unused message.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-mess...
src/lte/model/ideal-control-messages.h:444: * The Fempto forum uplink
BsrIdealControlMessage defines the specific
thanks, fixed.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-...
File src/lte/model/jakes-fading-loss-model.cc (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-...
src/lte/model/jakes-fading-loss-model.cc:114: // mobility = GetPhy
()->GetDownlinkSpectrumPhy ()->GetMobility ()->GetObject<MobilityModel> ();
in the revision of the code, we replaced the fading model with a completely new
one, so this comment does not apply any more.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-...
File src/lte/model/jakes-fading-loss-model.h (left):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-...
src/lte/model/jakes-fading-loss-model.h:60: * \brief Set the physical layer
I think the whitespace change was produced by check-style.py

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

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-amc.h#newcode39
src/lte/model/lte-amc.h:39: * need to create and manage instances of this class.
in the code you reviewed, this situation was managed by passing the
configuration parameters as function parameters. But eventually we found that
there was some parameter (e.g., the eta variable holding the target BER) that
was better managed as an object attribute, so in the revised version of the code
the LteAmc became an Object again.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-amc.h#newcode59
src/lte/model/lte-amc.h:59: static int GetTbSizeFromMcs (int mcs, int nprb);
As noted in the comment, the table comes from 3GPP TS 36.213, 7.1.7.2.1 which is
titled "Transport blocks not mapped to two-layer spatial multiplexing". It is
valid for all PHY configurations that use resource allocation type 0 and that do
not use spatial multiplexing. We currently do not support neither spatial
multiplexing nor any allocation type other than 0, so this is fine. 

When we will eventually support some of these other features, we'll need to add
more tables, see sections 7.1.7.2.2 and 7.1.7.2.3 for instance.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-mac.h
File src/lte/model/lte-enb-mac.h (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-mac.h#newc...
src/lte/model/lte-enb-mac.h:140: private:
These are private methods so I think it's fine to leave them undocumented.

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

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-net-device...
src/lte/model/lte-enb-net-device.h:160: Ptr<FfMacScheduler> m_scheduler;
We refer to the architecture described in the FemtoForum "LTE MAC Scheduler
Interface Specification". See for instance Figure 1 in that doc, the Scheduler
is an object on its own with specific interfaces to interact with the MAC.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc
File src/lte/model/lte-enb-phy.cc (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc#new...
src/lte/model/lte-enb-phy.cc:446: Ptr<LteEnbNetDevice> thisDevice = GetDevice
()->GetObject<LteEnbNetDevice> ();
Good catch, that's useless. Removed in the revision.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc#new...
src/lte/model/lte-enb-phy.cc:449: 
I agree that the naming is not very much appropriate for the UL. We used the
same name that for the DL because this method is a virtual method of LtePhy
which is overloaded by both EnbLtePhy and UeLtePhy. To avoid the inappropriate
naming, the method was renamed to LtePhy::GenerateCqiReport()

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

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode216
src/lte/model/lte-phy.h:216: * \param p queue control message to be sent
thanks, fixed.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode231
src/lte/model/lte-phy.h:231: virtual void  GenerateCqiFeedback (const
SpectrumValue& sinr) = 0;
The method is here because it is convenient to have the same interface for both
the UE and the eNB, so that for instance the interference calculation code can
be the same in the UE and the eNB. The method was renamed to GenerateCqiReport()
to avoid using the ambiguous term "feedback" (which would apply to DL only)

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode256
src/lte/model/lte-phy.h:256: std::vector< std::list<Ptr<IdealControlMessage> > >
m_controlMessagesQueue;

To model the latency of real MAC and PHY implementations, the PHY model
simulates a MAC-to-channel delay in multiples of TTIs (1ms). The transmission of
both data and control packets are delayed by this amount.

The vectors that you see implement this delay. We added the above explanation to
the design documentation.

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.cc
File src/lte/model/lte-ue-phy.cc (left):

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.cc#oldc...
src/lte/model/lte-ue-phy.cc:152: {
I think it is correct to have the CQI reporting implementation in the PHY, since
it is described in 3GPP TS 36.213 "Physical layer procedures".

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

http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.h#newco...
src/lte/model/lte-ue-phy.h:192: 

I don't agree that CQI report is a MAC functionality. In fact it is described in
3GPP TS 36.213 which is titled "Physical layer procedures".

http://codereview.appspot.com/4626069/diff/1/src/lte/test/lte-test-pf-ff-mac-...
File src/lte/test/lte-test-pf-ff-mac-scheduler.cc (right):

http://codereview.appspot.com/4626069/diff/1/src/lte/test/lte-test-pf-ff-mac-...
src/lte/test/lte-test-pf-ff-mac-scheduler.cc:24: 
All tests pass in the latest revision

[Piro Giuseppe, 2012-03-06 09:36:19]

Dear Nicola,
After reading your answers, I have no further issues to discuss about this
review. The work is good and, in my opinion, it could be merged into
ns-3-dev.

Best Regards
Giuseppe


Il giorno 05 marzo 2012 17:48, <nicobaldo@gmail.com> ha scritto:

> Reviewers: Piro Giuseppe,
>
> Message:
> Hi Giuseppe,
>
> thank you very much for your effort in reviewing the LENA code. I've
> gone through your comments and provided a reply for each. We revised the
> code to address some of your comments. For the  other comments where we
> think that a revision of the code is not necessary, I motivated in the
> reply the reason why.
>
> Also, please find right below my replies to the questions that you
> raised in your previous review.
>
> Kind Regards,
>
> Nicola
>
>
>
>  - Regarding the documentation, instead, several parts of the MAC layer
>>
> have not
>
>> been well documentated. I think it is not a real problem for the
>>
> merging process
>
>> but it should be fixed as soon as possible (for an lte-inexperienced,
>>
> the
>
>> understanding the code represents a very hard work).
>>
>
> The LTE module "manual" (i.e., this one: http://lena.cttc.es/manual/ )
> has been greatly improved since your review, hopefully the description
> of the code (including the MAC layer) is satisfactory now. Please see
> the detailed comments for those cases where you pointed out specific
> items missing in the documentation.
>
>
>  Firstly, the creation of CQI feedbacks is a feature handled by only
>>
> the UE
>
>> device, not for the eNB. However, the function GenerateCquiFeedback
>>
> has been
>
>> defined for both devices.
>> I saw that the eNB uses this method for estimating the uplink channel
>>
> quality of
>
>> a given UE. This procedure does not create any feedback and, for this
>>
> reason, it
>
>> is better to:
>> - use another function for doing this, defining the
>>
> GenerateCqiFeedback method
>
>> only for the UE.
>> - delete the ideal control messages related to the uplink cqi feedback
>>
>
> The use of a single method for both the UE and the eNB is motivated by
> the desire to present a single interface to the interference calculation
> code (in particular the class  LteCqiSinrChunkProcessor), which then can
> be shared between he UE and the eNB.
> I think the misundertanding was mostly due to the ambiguous use of the
> term "feedback", we renamed the method to GenerateCqiReport to avoid the
> ambiguity.
> As for the ideal control message for UL CQI, we removed it.
> Please see the replies to your comments in the code for further details.
>
>
>
>  Another point that I haven't understand is the management of the
>>
> uplink channel
>
>> quality.
>> The uplink channel quality is different for all UEs.
>> In order to select the proper UL MCS, the eNB must store uplink
>>
> channel
>
>> informations separately for each UE.
>>  From the code, instead, it seams that the eNB uses only one variable
>>
> to store
>
>> these informations, and this variable is the same for all UEs.
>>
>
>  In details, when the eNB PHY interface receives a packet burst from
>>
> the channel,
>
>> it calls the method EnbPhySapUser::UlCqiReport for creating the uplink
>>
> channel
>
>> indicator and for storing them into the LteEnbMac::m_ulCqiReceived
>>
> variable. I
>
>> have understood that this variable is the same for all UEs. This is
>>
> not correct:
>
>> the eNb should store these information for all UEs separately.
>> If i have misinterpreted the code, please clarify this point.
>>
> Otherwise, I think
>
>> it should be fixed.
>>
>
> This approach is in conformance with the FemtoForum MAC Scheduler API,
> see in particular the primitive 4.2.12 SCHED_UL_CQI_INFO_REQ.
> In detail, for the UL, a single vector of CQI values is passed to the
> scheduler for all users at the end of each TTI. This is not a
> limitation; in fact, the scheduler knows to which RB each UE was
> allocated (by keeping track of the past allocations), and can hence
> determine the CQI values in the CQI vector that correspond to each UE.
>
>
>
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/helper/**
>
lena-helper.cc<http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc>
> File src/lte/helper/lena-helper.cc (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/helper/**
>
lena-helper.cc#newcode61<http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc#newcode61>
> src/lte/helper/lena-helper.cc:**61: m_uplinkChannel =
> CreateObject<**SingleModelSpectrumChannel> ();
> We use a single channel exactly because we want to account for
> inter-cell interference. If someone wants to have one channel per cell
> in order to speedup simulations, it can be done simply by using separate
> LenaHelper instances in the simulation program.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/helper/**
>
lena-helper.cc#newcode143<http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc#newcode143>
> src/lte/helper/lena-helper.cc:**143: Start ();  // will run DoStart () if
> necessary
> On 2011/10/16 07:05:40, Piro Giuseppe wrote:
>
>> Why you do not call Start () into the constructor ? In this way the
>>
> DoStart ()
>
>> method is called always at the beginning.
>>
>
> because if we do that then it won't work if the user calls
> lenaHelper->SetXxxAttribute (). Hence, DoStart () needs to be delayed.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
ff-mac-sched-sap.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ff-mac-sched-sap.h>
> File src/lte/model/ff-mac-sched-**sap.h (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
ff-mac-sched-sap.h#newcode199<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ff-mac-sched-sap.h#newcode199>
> src/lte/model/ff-mac-sched-**sap.h:199: // (See 4.2 for description of the
> primitives)
> On 2011/10/16 07:05:40, Piro Giuseppe wrote:
>
>> sooner or later we have to add documentation also for these methods.
>>
>
> the whole class refers to the FemtoForum LTE MAC Scheduler Interface
> Specification, as noted in the doxygen description of the class. The
> comment above specifies that these methods are explained in section 4.2
> of that document. I think that document is pretty much exaustive and
> hence it is not needed to add more detailed doxygen here.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
ideal-control-messages.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h>
> File src/lte/model/ideal-control-**messages.h (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
ideal-control-messages.h#**newcode138<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode138>
> src/lte/model/ideal-control-**messages.h:138: * Direction for which the
> message is created
> I think the whitespace change was caused by running check-style.py
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
ideal-control-messages.h#**newcode152<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode152>
> src/lte/model/ideal-control-**messages.h:152: /** the sub channel */
> I think the whitespace change was caused by running check-style.py
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
ideal-control-messages.h#**newcode223<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode223>
> src/lte/model/ideal-control-**messages.h:223: int m_idSubChannel;
> I think the whitespace change was caused by running check-style.py
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
ideal-control-messages.h#**newcode355<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode355>
> src/lte/model/ideal-control-**messages.h:355: * The Fempto forum downlink
> CqiIdealControlMessage defines an ideal list of
> On 2011/10/16 07:05:40, Piro Giuseppe wrote:
>
>> Fempto -> Femto
>>
>
> thanks, fixed.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
ideal-control-messages.h#**newcode387<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode387>
> src/lte/model/ideal-control-**messages.h:387: #define
> ULCQI_IDEAL_CONTROL_MESSAGES_H
> Thanks, we removed the unused message.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
ideal-control-messages.h#**newcode444<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode444>
> src/lte/model/ideal-control-**messages.h:444: * The Fempto forum uplink
> BsrIdealControlMessage defines the specific
> thanks, fixed.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
jakes-fading-loss-model.cc<http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-model.cc>
> File src/lte/model/jakes-fading-**loss-model.cc (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
jakes-fading-loss-model.cc#**newcode114<http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-model.cc#newcode114>
> src/lte/model/jakes-fading-**loss-model.cc:114: // mobility = GetPhy
> ()->GetDownlinkSpectrumPhy ()->GetMobility ()->GetObject<MobilityModel>
> ();
> in the revision of the code, we replaced the fading model with a
> completely new one, so this comment does not apply any more.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
jakes-fading-loss-model.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-model.h>
> File src/lte/model/jakes-fading-**loss-model.h (left):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
jakes-fading-loss-model.h#**oldcode60<http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-model.h#oldcode60>
> src/lte/model/jakes-fading-**loss-model.h:60: * \brief Set the physical
> layer
> I think the whitespace change was produced by check-style.py
>
>
http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**lte-amc.h<http...
> File src/lte/model/lte-amc.h (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-amc.h#newcode39<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-amc.h#newcode39>
> src/lte/model/lte-amc.h:39: * need to create and manage instances of
> this class.
> in the code you reviewed, this situation was managed by passing the
> configuration parameters as function parameters. But eventually we found
> that there was some parameter (e.g., the eta variable holding the target
> BER) that was better managed as an object attribute, so in the revised
> version of the code the LteAmc became an Object again.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-amc.h#newcode59<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-amc.h#newcode59>
> src/lte/model/lte-amc.h:59: static int GetTbSizeFromMcs (int mcs, int
> nprb);
> As noted in the comment, the table comes from 3GPP TS 36.213, 7.1.7.2.1
> which is titled "Transport blocks not mapped to two-layer spatial
> multiplexing". It is valid for all PHY configurations that use resource
> allocation type 0 and that do not use spatial multiplexing. We currently
> do not support neither spatial multiplexing nor any allocation type
> other than 0, so this is fine.
>
> When we will eventually support some of these other features, we'll need
> to add more tables, see sections 7.1.7.2.2 and 7.1.7.2.3 for instance.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-enb-mac.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-mac.h>
> File src/lte/model/lte-enb-mac.h (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-enb-mac.h#newcode140<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-mac.h#newcode140>
> src/lte/model/lte-enb-mac.h:**140: private:
> These are private methods so I think it's fine to leave them
> undocumented.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-enb-net-device.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-net-device.h>
> File src/lte/model/lte-enb-net-**device.h (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-enb-net-device.h#**newcode160<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-net-device.h#newcode160>
> src/lte/model/lte-enb-net-**device.h:160: Ptr<FfMacScheduler> m_scheduler;
> We refer to the architecture described in the FemtoForum "LTE MAC
> Scheduler Interface Specification". See for instance Figure 1 in that
> doc, the Scheduler is an object on its own with specific interfaces to
> interact with the MAC.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-enb-phy.cc<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc>
> File src/lte/model/lte-enb-phy.cc (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-enb-phy.cc#newcode446<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc#newcode446>
> src/lte/model/lte-enb-phy.cc:**446: Ptr<LteEnbNetDevice> thisDevice =
> GetDevice ()->GetObject<LteEnbNetDevice> ();
> Good catch, that's useless. Removed in the revision.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-enb-phy.cc#newcode449<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc#newcode449>
> src/lte/model/lte-enb-phy.cc:**449:
> I agree that the naming is not very much appropriate for the UL. We used
> the same name that for the DL because this method is a virtual method of
> LtePhy which is overloaded by both EnbLtePhy and UeLtePhy. To avoid the
> inappropriate naming, the method was renamed to
> LtePhy::GenerateCqiReport()
>
>
http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**lte-phy.h<http...
> File src/lte/model/lte-phy.h (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-phy.h#newcode216<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode216>
> src/lte/model/lte-phy.h:216: * \param p queue control message to be sent
> thanks, fixed.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-phy.h#newcode231<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode231>
> src/lte/model/lte-phy.h:231: virtual void  GenerateCqiFeedback (const
> SpectrumValue& sinr) = 0;
> The method is here because it is convenient to have the same interface
> for both the UE and the eNB, so that for instance the interference
> calculation code can be the same in the UE and the eNB. The method was
> renamed to GenerateCqiReport() to avoid using the ambiguous term
> "feedback" (which would apply to DL only)
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-phy.h#newcode256<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode256>
> src/lte/model/lte-phy.h:256: std::vector<
> std::list<Ptr<**IdealControlMessage> > > m_controlMessagesQueue;
>
> To model the latency of real MAC and PHY implementations, the PHY model
> simulates a MAC-to-channel delay in multiples of TTIs (1ms). The
> transmission of both data and control packets are delayed by this
> amount.
>
> The vectors that you see implement this delay. We added the above
> explanation to the design documentation.
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-ue-phy.cc<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.cc>
> File src/lte/model/lte-ue-phy.cc (left):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-ue-phy.cc#oldcode152<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.cc#oldcode152>
> src/lte/model/lte-ue-phy.cc:**152: {
> I think it is correct to have the CQI reporting implementation in the
> PHY, since it is described in 3GPP TS 36.213 "Physical layer
> procedures".
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-ue-phy.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.h>
> File src/lte/model/lte-ue-phy.h (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>
lte-ue-phy.h#newcode192<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.h#newcode192>
> src/lte/model/lte-ue-phy.h:**192:
>
> I don't agree that CQI report is a MAC functionality. In fact it is
> described in 3GPP TS 36.213 which is titled "Physical layer procedures".
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/test/**
>
lte-test-pf-ff-mac-scheduler.**cc<http://codereview.appspot.com/4626069/diff/1/src/lte/test/lte-test-pf-ff-mac-scheduler.cc>
> File src/lte/test/lte-test-pf-ff-**mac-scheduler.cc (right):
>
> http://codereview.appspot.com/**4626069/diff/1/src/lte/test/**
>
lte-test-pf-ff-mac-scheduler.**cc#newcode24<http://codereview.appspot.com/4626069/diff/1/src/lte/test/lte-test-pf-ff-mac-scheduler.cc#newcode24>
> src/lte/test/lte-test-pf-ff-**mac-scheduler.cc:24:
> All tests pass in the latest revision
>
>
>
> Please review this at
http://codereview.appspot.com/**4626069/<http://codereview.appspot.com/4626069/>
>
> Affected files:
>  M README
>  M src/lte/AUTHORS
>  A src/lte/RELEASE_NOTES
>  A src/lte/examples/inter-cell-**interference.cc
>  A src/lte/examples/lena-first-**sim.cc
>  A src/lte/examples/lena-rlc-**calculator.cc
>  R src/lte/examples/lte-amc.cc
>  R src/lte/examples/lte-channel-**model.cc
>  R src/lte/examples/lte-device.cc
>  R src/lte/examples/lte-multiple-**flows.cc
>  R src/lte/examples/lte-phy-**downlink.cc
>  R src/lte/examples/lte-phy-**uplink.cc
>  A src/lte/examples/profiling-**reference.cc
>  M src/lte/examples/wscript
>  A src/lte/helper/lena-helper.cc
>  A src/lte/helper/lena-helper.h
>  R src/lte/helper/lte-helper.cc
>  R src/lte/helper/lte-helper.h
>  A src/lte/helper/mac-stats-**calculator.cc
>  A src/lte/helper/mac-stats-**calculator.h
>  A src/lte/helper/rlc-stats-**calculator.cc
>  A src/lte/helper/rlc-stats-**calculator.h
>  R src/lte/model/bearer-qos-**parameters.cc
>  R src/lte/model/bearer-qos-**parameters.h
>  R src/lte/model/enb-lte-**spectrum-phy.cc
>  R src/lte/model/enb-lte-**spectrum-phy.h
>  R src/lte/model/enb-mac-entity.**cc
>  R src/lte/model/enb-mac-entity.h
>  A src/lte/model/eps-bearer.cc
>  A src/lte/model/eps-bearer.h
>  A src/lte/model/ff-mac-common.h
>  A src/lte/model/ff-mac-csched-**sap.cc
>  A src/lte/model/ff-mac-csched-**sap.h
>  A src/lte/model/ff-mac-sched-**sap.cc
>  A src/lte/model/ff-mac-sched-**sap.h
>  A src/lte/model/ff-mac-**scheduler.cc
>  A src/lte/model/ff-mac-**scheduler.h
>  M src/lte/model/ideal-control-**messages.cc
>  M src/lte/model/ideal-control-**messages.h
>  M src/lte/model/jakes-fading-**loss-model.cc
>  M src/lte/model/jakes-fading-**loss-model.h
>  M src/lte/model/lte-amc.cc
>  M src/lte/model/lte-amc.h
>  A src/lte/model/lte-common.cc
>  A src/lte/model/lte-common.h
>  A src/lte/model/lte-enb-cmac-**sap.cc
>  A src/lte/model/lte-enb-cmac-**sap.h
>  A src/lte/model/lte-enb-mac.cc
>  A src/lte/model/lte-enb-mac.h
>  M src/lte/model/lte-enb-net-**device.cc
>  M src/lte/model/lte-enb-net-**device.h
>  A src/lte/model/lte-enb-phy-sap.**cc
>  A src/lte/model/lte-enb-phy-sap.**h
>  M src/lte/model/lte-enb-phy.cc
>  M src/lte/model/lte-enb-phy.h
>  A src/lte/model/lte-enb-rrc.cc
>  A src/lte/model/lte-enb-rrc.h
>  A src/lte/model/lte-**interference.cc
>  A src/lte/model/lte-**interference.h
>  R src/lte/model/lte-mac-header.**cc
>  R src/lte/model/lte-mac-header.h
>  R src/lte/model/lte-mac-queue.cc
>  R src/lte/model/lte-mac-queue.h
>  A src/lte/model/lte-mac-sap.cc
>  A src/lte/model/lte-mac-sap.h
>  A src/lte/model/lte-mac-tag.cc
>  A src/lte/model/lte-mac-tag.h
>  M src/lte/model/lte-net-device.**cc
>  M src/lte/model/lte-net-device.h
>  A src/lte/model/lte-phy-tag.cc
>  A src/lte/model/lte-phy-tag.h
>  M src/lte/model/lte-phy.cc
>  M src/lte/model/lte-phy.h
>  A src/lte/model/lte-rlc-tag.cc
>  A src/lte/model/lte-rlc-tag.h
>  A src/lte/model/lte-rlc.cc
>  A src/lte/model/lte-rlc.h
>  A src/lte/model/lte-sinr-chunk-**processor.cc
>  A src/lte/model/lte-sinr-chunk-**processor.h
>  M src/lte/model/lte-spectrum-**phy.cc
>  M src/lte/model/lte-spectrum-**phy.h
>  M src/lte/model/lte-spectrum-**value-helper.cc
>  M src/lte/model/lte-spectrum-**value-helper.h
>  A src/lte/model/lte-ue-cmac-sap.**cc
>  A src/lte/model/lte-ue-cmac-sap.**h
>  A src/lte/model/lte-ue-mac.cc
>  A src/lte/model/lte-ue-mac.h
>  M src/lte/model/lte-ue-net-**device.cc
>  M src/lte/model/lte-ue-net-**device.h
>  A src/lte/model/lte-ue-phy-sap.**cc
>  A src/lte/model/lte-ue-phy-sap.h
>  M src/lte/model/lte-ue-phy.cc
>  M src/lte/model/lte-ue-phy.h
>  A src/lte/model/lte-ue-rrc.cc
>  A src/lte/model/lte-ue-rrc.h
>  R src/lte/model/mac-entity.cc
>  R src/lte/model/mac-entity.h
>  R src/lte/model/packet-**scheduler.cc
>  R src/lte/model/packet-**scheduler.h
>  A src/lte/model/pf-ff-mac-**scheduler.cc
>  A src/lte/model/pf-ff-mac-**scheduler.h
>  R src/lte/model/radio-bearer-**instance.cc
>  R src/lte/model/radio-bearer-**instance.h
>  R src/lte/model/rlc-entity.cc
>  R src/lte/model/rlc-entity.h
>  A src/lte/model/rr-ff-mac-**scheduler.cc
>  A src/lte/model/rr-ff-mac-**scheduler.h
>  R src/lte/model/rrc-entity.cc
>  R src/lte/model/rrc-entity.h
>  R src/lte/model/simple-packet-**scheduler.cc
>  R src/lte/model/simple-packet-**scheduler.h
>  R src/lte/model/ue-lte-spectrum-**phy.cc
>  R src/lte/model/ue-lte-spectrum-**phy.h
>  R src/lte/model/ue-mac-entity.cc
>  R src/lte/model/ue-mac-entity.h
>  R src/lte/model/ue-manager.cc
>  R src/lte/model/ue-manager.h
>  R src/lte/model/ue-record.cc
>  R src/lte/model/ue-record.h
>  A src/lte/test/examples-to-run.**py
>  R src/lte/test/lte-bearer-test.**cc
>  R src/lte/test/lte-device-test.**cc
>  R src/lte/test/lte-phy-test.cc
>  R src/lte/test/lte-propagation-**loss-model-test.cc
>  A src/lte/test/lte-test-**downlink-sinr.cc
>  A src/lte/test/lte-test-**downlink-sinr.h
>  A src/lte/test/lte-test-earfcn.**cc
>  A src/lte/test/lte-test-**interference.cc
>  A src/lte/test/lte-test-**interference.h
>  A src/lte/test/lte-test-link-**adaptation.cc
>  A src/lte/test/lte-test-link-**adaptation.h
>  A src/lte/test/lte-test-pf-ff-**mac-scheduler.cc
>  A src/lte/test/lte-test-pf-ff-**mac-scheduler.h
>  A src/lte/test/lte-test-rr-ff-**mac-scheduler.cc
>  A src/lte/test/lte-test-rr-ff-**mac-scheduler.h
>  A src/lte/test/lte-test-sinr-**chunk-processor.cc
>  A src/lte/test/lte-test-sinr-**chunk-processor.h
>  A src/lte/test/lte-test-**spectrum-value-helper.cc
>  A src/lte/test/lte-test-ue-phy.**cc
>  A src/lte/test/lte-test-ue-phy.h
>  A src/lte/test/lte-test-uplink-**sinr.cc
>  A src/lte/test/lte-test-uplink-**sinr.h
>  A src/lte/test/reference/gain_**freespace.m
>  A src/lte/test/reference/**generate_test_data_lte_sinr.m
>  A src/lte/test/reference/**generate_test_data_lte_**spectrum_model.m
>  A src/lte/test/reference/**generate_test_data_lte_**
> spectrum_value_noise.m
>  A src/lte/test/reference/**generate_test_data_lte_**
> spectrum_value_txpsd.m
>  A src/lte/test/reference/lte-**mcs-index.dat
>  A src/lte/test/reference/lte-**mcs-index.eps
>  A src/lte/test/reference/lte-**mcs-index.gnuplot
>  A src/lte/test/reference/lte-**mcs-index.pdf
>  A src/lte/test/reference/lte-**mcs-index.png
>  A src/lte/test/reference/lte_**amc.m
>  A src/lte/test/reference/lte_**link_budget.m
>  A src/lte/test/reference/lte_**link_budget_interference.m
>  A src/lte/test/reference/print_**C_vector.m
>  M src/lte/wscript
>
>
>
>


-- 

Giuseppe Piro
Ph.D. Student at Politecnico di Bari | via Orabona 4 | 70125 Bari | Italy
Email: peppe@giuseppepiro.com | g.piro@poliba.it
Phone: +39 080 5963301
Fax:   +39 080 5963410
Web:   www.giuseppepiro.com | telematics.poliba.it/piro

[abdulmehsen, 2012-03-28 15:10:59]

Hi,

I have been monitoring the development of the LTE code in ns-3 for a while 
now, and I have some comments on the interference model that is being used, 
which I may address later in a detailed post in the general user list. But 
right now, I have a specific comment regarding this new LENA code under 
review.  I think there maybe an issue with the way the CQI feedbacks are 
generated. Looking at the latest LENA code and documentation, it seems that 
the CQI is estimated by the UE based on the received SINR of the actual 
data TB sent by the eNB which is not accurate. In reality, CQI are based on 
reference (pilot) signals which are periodically transmitted by the eNB. 

Can you look again into this and clarify if this is the intended design or 
just some overlooked error in the documentation?

/Abdul
EE Dept.
University of Washington
Seattle, WA 
USA

 

On Tuesday, March 6, 2012 1:35:58 AM UTC-8, Giuseppe Piro wrote:
>
> Dear Nicola,
> After reading your answers, I have no further issues to discuss about 
> this review. The work is good and, in my opinion, it could be merged into 
> ns-3-dev. 
>
> Best Regards
> Giuseppe
>
>
> Il giorno 05 marzo 2012 17:48, <nicobaldo@gmail.com> ha scritto:
>
>> Reviewers: Piro Giuseppe,
>>
>> Message:
>> Hi Giuseppe,
>>
>> thank you very much for your effort in reviewing the LENA code. I've
>> gone through your comments and provided a reply for each. We revised the
>> code to address some of your comments. For the  other comments where we
>> think that a revision of the code is not necessary, I motivated in the
>> reply the reason why.
>>
>> Also, please find right below my replies to the questions that you
>> raised in your previous review.
>>
>> Kind Regards,
>>
>> Nicola
>>
>>
>>
>>  - Regarding the documentation, instead, several parts of the MAC layer
>>>
>> have not
>>
>>> been well documentated. I think it is not a real problem for the
>>>
>> merging process
>>
>>> but it should be fixed as soon as possible (for an lte-inexperienced,
>>>
>> the
>>
>>> understanding the code represents a very hard work).
>>>
>>
>> The LTE module "manual" (i.e., this one: http://lena.cttc.es/manual/ )
>> has been greatly improved since your review, hopefully the description
>> of the code (including the MAC layer) is satisfactory now. Please see
>> the detailed comments for those cases where you pointed out specific
>> items missing in the documentation.
>>
>>
>>  Firstly, the creation of CQI feedbacks is a feature handled by only
>>>
>> the UE
>>
>>> device, not for the eNB. However, the function GenerateCquiFeedback
>>>
>> has been
>>
>>> defined for both devices.
>>> I saw that the eNB uses this method for estimating the uplink channel
>>>
>> quality of
>>
>>> a given UE. This procedure does not create any feedback and, for this
>>>
>> reason, it
>>
>>> is better to:
>>> - use another function for doing this, defining the
>>>
>> GenerateCqiFeedback method
>>
>>> only for the UE.
>>> - delete the ideal control messages related to the uplink cqi feedback
>>>
>>
>> The use of a single method for both the UE and the eNB is motivated by
>> the desire to present a single interface to the interference calculation
>> code (in particular the class  LteCqiSinrChunkProcessor), which then can
>> be shared between he UE and the eNB.
>> I think the misundertanding was mostly due to the ambiguous use of the
>> term "feedback", we renamed the method to GenerateCqiReport to avoid the
>> ambiguity.
>> As for the ideal control message for UL CQI, we removed it.
>> Please see the replies to your comments in the code for further details.
>>
>>
>>
>>  Another point that I haven't understand is the management of the
>>>
>> uplink channel
>>
>>> quality.
>>> The uplink channel quality is different for all UEs.
>>> In order to select the proper UL MCS, the eNB must store uplink
>>>
>> channel
>>
>>> informations separately for each UE.
>>>  From the code, instead, it seams that the eNB uses only one variable
>>>
>> to store
>>
>>> these informations, and this variable is the same for all UEs.
>>>
>>
>>  In details, when the eNB PHY interface receives a packet burst from
>>>
>> the channel,
>>
>>> it calls the method EnbPhySapUser::UlCqiReport for creating the uplink
>>>
>> channel
>>
>>> indicator and for storing them into the LteEnbMac::m_ulCqiReceived
>>>
>> variable. I
>>
>>> have understood that this variable is the same for all UEs. This is
>>>
>> not correct:
>>
>>> the eNb should store these information for all UEs separately.
>>> If i have misinterpreted the code, please clarify this point.
>>>
>> Otherwise, I think
>>
>>> it should be fixed.
>>>
>>
>> This approach is in conformance with the FemtoForum MAC Scheduler API,
>> see in particular the primitive 4.2.12 SCHED_UL_CQI_INFO_REQ.
>> In detail, for the UL, a single vector of CQI values is passed to the
>> scheduler for all users at the end of each TTI. This is not a
>> limitation; in fact, the scheduler knows to which RB each UE was
>> allocated (by keeping track of the past allocations), and can hence
>> determine the CQI values in the CQI vector that correspond to each UE.
>>
>>
>>
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/helper/**
>>
lena-helper.cc<http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc>
>> File src/lte/helper/lena-helper.cc (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/helper/**
>>
lena-helper.cc#newcode61<http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc#newcode61>
>> src/lte/helper/lena-helper.cc:**61: m_uplinkChannel =
>> CreateObject<**SingleModelSpectrumChannel> ();
>> We use a single channel exactly because we want to account for
>> inter-cell interference. If someone wants to have one channel per cell
>> in order to speedup simulations, it can be done simply by using separate
>> LenaHelper instances in the simulation program.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/helper/**
>>
lena-helper.cc#newcode143<http://codereview.appspot.com/4626069/diff/1/src/lte/helper/lena-helper.cc#newcode143>
>> src/lte/helper/lena-helper.cc:**143: Start ();  // will run DoStart () if
>> necessary
>> On 2011/10/16 07:05:40, Piro Giuseppe wrote:
>>
>>> Why you do not call Start () into the constructor ? In this way the
>>>
>> DoStart ()
>>
>>> method is called always at the beginning.
>>>
>>
>> because if we do that then it won't work if the user calls
>> lenaHelper->SetXxxAttribute (). Hence, DoStart () needs to be delayed.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
ff-mac-sched-sap.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ff-mac-sched-sap.h>
>> File src/lte/model/ff-mac-sched-**sap.h (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
ff-mac-sched-sap.h#newcode199<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ff-mac-sched-sap.h#newcode199>
>> src/lte/model/ff-mac-sched-**sap.h:199: // (See 4.2 for description of 
>> the
>> primitives)
>> On 2011/10/16 07:05:40, Piro Giuseppe wrote:
>>
>>> sooner or later we have to add documentation also for these methods.
>>>
>>
>> the whole class refers to the FemtoForum LTE MAC Scheduler Interface
>> Specification, as noted in the doxygen description of the class. The
>> comment above specifies that these methods are explained in section 4.2
>> of that document. I think that document is pretty much exaustive and
>> hence it is not needed to add more detailed doxygen here.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
ideal-control-messages.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h>
>> File src/lte/model/ideal-control-**messages.h (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
ideal-control-messages.h#**newcode138<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode138>
>> src/lte/model/ideal-control-**messages.h:138: * Direction for which the
>> message is created
>> I think the whitespace change was caused by running check-style.py
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
ideal-control-messages.h#**newcode152<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode152>
>> src/lte/model/ideal-control-**messages.h:152: /** the sub channel */
>> I think the whitespace change was caused by running check-style.py
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
ideal-control-messages.h#**newcode223<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode223>
>> src/lte/model/ideal-control-**messages.h:223: int m_idSubChannel;
>> I think the whitespace change was caused by running check-style.py
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
ideal-control-messages.h#**newcode355<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode355>
>> src/lte/model/ideal-control-**messages.h:355: * The Fempto forum downlink
>> CqiIdealControlMessage defines an ideal list of
>> On 2011/10/16 07:05:40, Piro Giuseppe wrote:
>>
>>> Fempto -> Femto
>>>
>>
>> thanks, fixed.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
ideal-control-messages.h#**newcode387<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode387>
>> src/lte/model/ideal-control-**messages.h:387: #define
>> ULCQI_IDEAL_CONTROL_MESSAGES_H
>> Thanks, we removed the unused message.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
ideal-control-messages.h#**newcode444<http://codereview.appspot.com/4626069/diff/1/src/lte/model/ideal-control-messages.h#newcode444>
>> src/lte/model/ideal-control-**messages.h:444: * The Fempto forum uplink
>> BsrIdealControlMessage defines the specific
>> thanks, fixed.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
jakes-fading-loss-model.cc<http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-model.cc>
>> File src/lte/model/jakes-fading-**loss-model.cc (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
jakes-fading-loss-model.cc#**newcode114<http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-model.cc#newcode114>
>> src/lte/model/jakes-fading-**loss-model.cc:114: // mobility = GetPhy
>> ()->GetDownlinkSpectrumPhy ()->GetMobility ()->GetObject<MobilityModel>
>> ();
>> in the revision of the code, we replaced the fading model with a
>> completely new one, so this comment does not apply any more.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
jakes-fading-loss-model.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-model.h>
>> File src/lte/model/jakes-fading-**loss-model.h (left):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
jakes-fading-loss-model.h#**oldcode60<http://codereview.appspot.com/4626069/diff/1/src/lte/model/jakes-fading-loss-model.h#oldcode60>
>> src/lte/model/jakes-fading-**loss-model.h:60: * \brief Set the physical
>> layer
>> I think the whitespace change was produced by check-style.py
>>
>>
http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**lte-amc.h<http...
>> File src/lte/model/lte-amc.h (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-amc.h#newcode39<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-amc.h#newcode39>
>> src/lte/model/lte-amc.h:39: * need to create and manage instances of
>> this class.
>> in the code you reviewed, this situation was managed by passing the
>> configuration parameters as function parameters. But eventually we found
>> that there was some parameter (e.g., the eta variable holding the target
>> BER) that was better managed as an object attribute, so in the revised
>> version of the code the LteAmc became an Object again.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-amc.h#newcode59<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-amc.h#newcode59>
>> src/lte/model/lte-amc.h:59: static int GetTbSizeFromMcs (int mcs, int
>> nprb);
>> As noted in the comment, the table comes from 3GPP TS 36.213, 7.1.7.2.1
>> which is titled "Transport blocks not mapped to two-layer spatial
>> multiplexing". It is valid for all PHY configurations that use resource
>> allocation type 0 and that do not use spatial multiplexing. We currently
>> do not support neither spatial multiplexing nor any allocation type
>> other than 0, so this is fine.
>>
>> When we will eventually support some of these other features, we'll need
>> to add more tables, see sections 7.1.7.2.2 and 7.1.7.2.3 for instance.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-enb-mac.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-mac.h>
>> File src/lte/model/lte-enb-mac.h (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-enb-mac.h#newcode140<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-mac.h#newcode140>
>> src/lte/model/lte-enb-mac.h:**140: private:
>> These are private methods so I think it's fine to leave them
>> undocumented.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-enb-net-device.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-net-device.h>
>> File src/lte/model/lte-enb-net-**device.h (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-enb-net-device.h#**newcode160<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-net-device.h#newcode160>
>> src/lte/model/lte-enb-net-**device.h:160: Ptr<FfMacScheduler> 
>> m_scheduler;
>> We refer to the architecture described in the FemtoForum "LTE MAC
>> Scheduler Interface Specification". See for instance Figure 1 in that
>> doc, the Scheduler is an object on its own with specific interfaces to
>> interact with the MAC.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-enb-phy.cc<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc>
>> File src/lte/model/lte-enb-phy.cc (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-enb-phy.cc#newcode446<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc#newcode446>
>> src/lte/model/lte-enb-phy.cc:**446: Ptr<LteEnbNetDevice> thisDevice =
>> GetDevice ()->GetObject<LteEnbNetDevice> ();
>> Good catch, that's useless. Removed in the revision.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-enb-phy.cc#newcode449<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-enb-phy.cc#newcode449>
>> src/lte/model/lte-enb-phy.cc:**449:
>> I agree that the naming is not very much appropriate for the UL. We used
>> the same name that for the DL because this method is a virtual method of
>> LtePhy which is overloaded by both EnbLtePhy and UeLtePhy. To avoid the
>> inappropriate naming, the method was renamed to
>> LtePhy::GenerateCqiReport()
>>
>>
http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**lte-phy.h<http...
>> File src/lte/model/lte-phy.h (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-phy.h#newcode216<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode216>
>> src/lte/model/lte-phy.h:216: * \param p queue control message to be sent
>> thanks, fixed.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-phy.h#newcode231<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode231>
>> src/lte/model/lte-phy.h:231: virtual void  GenerateCqiFeedback (const
>> SpectrumValue& sinr) = 0;
>> The method is here because it is convenient to have the same interface
>> for both the UE and the eNB, so that for instance the interference
>> calculation code can be the same in the UE and the eNB. The method was
>> renamed to GenerateCqiReport() to avoid using the ambiguous term
>> "feedback" (which would apply to DL only)
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-phy.h#newcode256<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-phy.h#newcode256>
>> src/lte/model/lte-phy.h:256: std::vector<
>> std::list<Ptr<**IdealControlMessage> > > m_controlMessagesQueue;
>>
>> To model the latency of real MAC and PHY implementations, the PHY model
>> simulates a MAC-to-channel delay in multiples of TTIs (1ms). The
>> transmission of both data and control packets are delayed by this
>> amount.
>>
>> The vectors that you see implement this delay. We added the above
>> explanation to the design documentation.
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-ue-phy.cc<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.cc>
>> File src/lte/model/lte-ue-phy.cc (left):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-ue-phy.cc#oldcode152<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.cc#oldcode152>
>> src/lte/model/lte-ue-phy.cc:**152: {
>> I think it is correct to have the CQI reporting implementation in the
>> PHY, since it is described in 3GPP TS 36.213 "Physical layer
>> procedures".
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-ue-phy.h<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.h>
>> File src/lte/model/lte-ue-phy.h (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/model/**
>>
lte-ue-phy.h#newcode192<http://codereview.appspot.com/4626069/diff/1/src/lte/model/lte-ue-phy.h#newcode192>
>> src/lte/model/lte-ue-phy.h:**192:
>>
>> I don't agree that CQI report is a MAC functionality. In fact it is
>> described in 3GPP TS 36.213 which is titled "Physical layer procedures".
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/test/**
>>
lte-test-pf-ff-mac-scheduler.**cc<http://codereview.appspot.com/4626069/diff/1/src/lte/test/lte-test-pf-ff-mac-scheduler.cc>
>> File src/lte/test/lte-test-pf-ff-**mac-scheduler.cc (right):
>>
>> http://codereview.appspot.com/**4626069/diff/1/src/lte/test/**
>>
lte-test-pf-ff-mac-scheduler.**cc#newcode24<http://codereview.appspot.com/4626069/diff/1/src/lte/test/lte-test-pf-ff-mac-scheduler.cc#newcode24>
>> src/lte/test/lte-test-pf-ff-**mac-scheduler.cc:24:
>> All tests pass in the latest revision
>>
>>
>>
>> Please review this at
http://codereview.appspot.com/**4626069/<http://codereview.appspot.com/4626069/>
>>
>> Affected files:
>>  M README
>>  M src/lte/AUTHORS
>>  A src/lte/RELEASE_NOTES
>>  A src/lte/examples/inter-cell-**interference.cc
>>  A src/lte/examples/lena-first-**sim.cc
>>  A src/lte/examples/lena-rlc-**calculator.cc
>>  R src/lte/examples/lte-amc.cc
>>  R src/lte/examples/lte-channel-**model.cc
>>  R src/lte/examples/lte-device.cc
>>  R src/lte/examples/lte-multiple-**flows.cc
>>  R src/lte/examples/lte-phy-**downlink.cc
>>  R src/lte/examples/lte-phy-**uplink.cc
>>  A src/lte/examples/profiling-**reference.cc
>>  M src/lte/examples/wscript
>>  A src/lte/helper/lena-helper.cc
>>  A src/lte/helper/lena-helper.h
>>  R src/lte/helper/lte-helper.cc
>>  R src/lte/helper/lte-helper.h
>>  A src/lte/helper/mac-stats-**calculator.cc
>>  A src/lte/helper/mac-stats-**calculator.h
>>  A src/lte/helper/rlc-stats-**calculator.cc
>>  A src/lte/helper/rlc-stats-**calculator.h
>>  R src/lte/model/bearer-qos-**parameters.cc
>>  R src/lte/model/bearer-qos-**parameters.h
>>  R src/lte/model/enb-lte-**spectrum-phy.cc
>>  R src/lte/model/enb-lte-**spectrum-phy.h
>>  R src/lte/model/enb-mac-entity.**cc
>>  R src/lte/model/enb-mac-entity.h
>>  A src/lte/model/eps-bearer.cc
>>  A src/lte/model/eps-bearer.h
>>  A src/lte/model/ff-mac-common.h
>>  A src/lte/model/ff-mac-csched-**sap.cc
>>  A src/lte/model/ff-mac-csched-**sap.h
>>  A src/lte/model/ff-mac-sched-**sap.cc
>>  A src/lte/model/ff-mac-sched-**sap.h
>>  A src/lte/model/ff-mac-**scheduler.cc
>>  A src/lte/model/ff-mac-**scheduler.h
>>  M src/lte/model/ideal-control-**messages.cc
>>  M src/lte/model/ideal-control-**messages.h
>>  M src/lte/model/jakes-fading-**loss-model.cc
>>  M src/lte/model/jakes-fading-**loss-model.h
>>  M src/lte/model/lte-amc.cc
>>  M src/lte/model/lte-amc.h
>>  A src/lte/model/lte-common.cc
>>  A src/lte/model/lte-common.h
>>  A src/lte/model/lte-enb-cmac-**sap.cc
>>  A src/lte/model/lte-enb-cmac-**sap.h
>>  A src/lte/model/lte-enb-mac.cc
>>  A src/lte/model/lte-enb-mac.h
>>  M src/lte/model/lte-enb-net-**device.cc
>>  M src/lte/model/lte-enb-net-**device.h
>>  A src/lte/model/lte-enb-phy-sap.**cc
>>  A src/lte/model/lte-enb-phy-sap.**h
>>  M src/lte/model/lte-enb-phy.cc
>>  M src/lte/model/lte-enb-phy.h
>>  A src/lte/model/lte-enb-rrc.cc
>>  A src/lte/model/lte-enb-rrc.h
>>  A src/lte/model/lte-**interference.cc
>>  A src/lte/model/lte-**interference.h
>>  R src/lte/model/lte-mac-header.**cc
>>  R src/lte/model/lte-mac-header.h
>>  R src/lte/model/lte-mac-queue.cc
>>  R src/lte/model/lte-mac-queue.h
>>  A src/lte/model/lte-mac-sap.cc
>>  A src/lte/model/lte-mac-sap.h
>>  A src/lte/model/lte-mac-tag.cc
>>  A src/lte/model/lte-mac-tag.h
>>  M src/lte/model/lte-net-device.**cc
>>  M src/lte/model/lte-net-device.h
>>  A src/lte/model/lte-phy-tag.cc
>>  A src/lte/model/lte-phy-tag.h
>>  M src/lte/model/lte-phy.cc
>>  M src/lte/model/lte-phy.h
>>  A src/lte/model/lte-rlc-tag.cc
>>  A src/lte/model/lte-rlc-tag.h
>>  A src/lte/model/lte-rlc.cc
>>  A src/lte/model/lte-rlc.h
>>  A src/lte/model/lte-sinr-chunk-**processor.cc
>>  A src/lte/model/lte-sinr-chunk-**processor.h
>>  M src/lte/model/lte-spectrum-**phy.cc
>>  M src/lte/model/lte-spectrum-**phy.h
>>  M src/lte/model/lte-spectrum-**value-helper.cc
>>  M src/lte/model/lte-spectrum-**value-helper.h
>>  A src/lte/model/lte-ue-cmac-sap.**cc
>>  A src/lte/model/lte-ue-cmac-sap.**h
>>  A src/lte/model/lte-ue-mac.cc
>>  A src/lte/model/lte-ue-mac.h
>>  M src/lte/model/lte-ue-net-**device.cc
>>  M src/lte/model/lte-ue-net-**device.h
>>  A src/lte/model/lte-ue-phy-sap.**cc
>>  A src/lte/model/lte-ue-phy-sap.h
>>  M src/lte/model/lte-ue-phy.cc
>>  M src/lte/model/lte-ue-phy.h
>>  A src/lte/model/lte-ue-rrc.cc
>>  A src/lte/model/lte-ue-rrc.h
>>  R src/lte/model/mac-entity.cc
>>  R src/lte/model/mac-entity.h
>>  R src/lte/model/packet-**scheduler.cc
>>  R src/lte/model/packet-**scheduler.h
>>  A src/lte/model/pf-ff-mac-**scheduler.cc
>>  A src/lte/model/pf-ff-mac-**scheduler.h
>>  R src/lte/model/radio-bearer-**instance.cc
>>  R src/lte/model/radio-bearer-**instance.h
>>  R src/lte/model/rlc-entity.cc
>>  R src/lte/model/rlc-entity.h
>>  A src/lte/model/rr-ff-mac-**scheduler.cc
>>  A src/lte/model/rr-ff-mac-**scheduler.h
>>  R src/lte/model/rrc-entity.cc
>>  R src/lte/model/rrc-entity.h
>>  R src/lte/model/simple-packet-**scheduler.cc
>>  R src/lte/model/simple-packet-**scheduler.h
>>  R src/lte/model/ue-lte-spectrum-**phy.cc
>>  R src/lte/model/ue-lte-spectrum-**phy.h
>>  R src/lte/model/ue-mac-entity.cc
>>  R src/lte/model/ue-mac-entity.h
>>  R src/lte/model/ue-manager.cc
>>  R src/lte/model/ue-manager.h
>>  R src/lte/model/ue-record.cc
>>  R src/lte/model/ue-record.h
>>  A src/lte/test/examples-to-run.**py
>>  R src/lte/test/lte-bearer-test.**cc
>>  R src/lte/test/lte-device-test.**cc
>>  R src/lte/test/lte-phy-test.cc
>>  R src/lte/test/lte-propagation-**loss-model-test.cc
>>  A src/lte/test/lte-test-**downlink-sinr.cc
>>  A src/lte/test/lte-test-**downlink-sinr.h
>>  A src/lte/test/lte-test-earfcn.**cc
>>  A src/lte/test/lte-test-**interference.cc
>>  A src/lte/test/lte-test-**interference.h
>>  A src/lte/test/lte-test-link-**adaptation.cc
>>  A src/lte/test/lte-test-link-**adaptation.h
>>  A src/lte/test/lte-test-pf-ff-**mac-scheduler.cc
>>  A src/lte/test/lte-test-pf-ff-**mac-scheduler.h
>>  A src/lte/test/lte-test-rr-ff-**mac-scheduler.cc
>>  A src/lte/test/lte-test-rr-ff-**mac-scheduler.h
>>  A src/lte/test/lte-test-sinr-**chunk-processor.cc
>>  A src/lte/test/lte-test-sinr-**chunk-processor.h
>>  A src/lte/test/lte-test-**spectrum-value-helper.cc
>>  A src/lte/test/lte-test-ue-phy.**cc
>>  A src/lte/test/lte-test-ue-phy.h
>>  A src/lte/test/lte-test-uplink-**sinr.cc
>>  A src/lte/test/lte-test-uplink-**sinr.h
>>  A src/lte/test/reference/gain_**freespace.m
>>  A src/lte/test/reference/**generate_test_data_lte_sinr.m
>>  A src/lte/test/reference/**generate_test_data_lte_**spectrum_model.m
>>  A src/lte/test/reference/**generate_test_data_lte_**
>> spectrum_value_noise.m
>>  A src/lte/test/reference/**generate_test_data_lte_**
>> spectrum_value_txpsd.m
>>  A src/lte/test/reference/lte-**mcs-index.dat
>>  A src/lte/test/reference/lte-**mcs-index.eps
>>  A src/lte/test/reference/lte-**mcs-index.gnuplot
>>  A src/lte/test/reference/lte-**mcs-index.pdf
>>  A src/lte/test/reference/lte-**mcs-index.png
>>  A src/lte/test/reference/lte_**amc.m
>>  A src/lte/test/reference/lte_**link_budget.m
>>  A src/lte/test/reference/lte_**link_budget_interference.m
>>  A src/lte/test/reference/print_**C_vector.m
>>  M src/lte/wscript
>>
>>
>>
>>
>
>
> -- 
>
> Giuseppe Piro
> Ph.D. Student at Politecnico di Bari | via Orabona 4 | 70125 Bari | Italy
> Email: peppe@giuseppepiro.com | g.piro@poliba.it 
> Phone: +39 080 5963301
> Fax:   +39 080 5963410
> Web:   www.giuseppepiro.com | telematics.poliba.it/piro
>
>  

[Marco Miozzo, 2012-04-23 12:56:28]

Hi,

the CQI are now evaluated according to the power received with the TB data since
there is no RS implemented and considering that RS signals share most of their
resource with the one of the data in the resource block. As soon as the RS will
be implemented the CQI evaluation will be updated accordingly.

Regards,
Marco Miozzo.

[Marco Miozzo, 2012-04-23 13:50:22]

To conclude,

I modified the documentation section on CQI generation in order clarify this
issue.

Regards,
Marco Miozzo.


On 2012/04/23 12:56:28, marco.miozzo wrote:
> Hi,
> 
> the CQI are now evaluated according to the power received with the TB data
since
> there is no RS implemented and considering that RS signals share most of their
> resource with the one of the data in the resource block. As soon as the RS
will
> be implemented the CQI evaluation will be updated accordingly.
> 
> Regards,
> Marco Miozzo.

[abdulmehsen, 2012-04-23 16:54:03]

Hi Marco,

Thanks for the clarification. There are a couple of issues in this approach:
1- First, it invalidates the Gaussian interference model which is assumed in the
SINR to CQI mapping process. In the Gaussian model, we assume there are many
interference sources that have the same order of magnitude. If you extract the
CQI from the received TB's SINR, this assumption does not hold because there
could be only one or two interference sources depending on the scheduling/ICIC
dynamics. Extracting CQI from SINR of pilot (RS) signals does not suffer from
this issue, because a pilot by definition is always transmitted from all cells
(independent of TBs) which implies many interference sources at the receiver. 

2- You can see from the above point that there will be a strong undesirable
interaction between the CQI feedback process and the scheduling/interference
coordination process. For example, suppose in time slot #1, cell A assigns RB#1
to user x and RB#2 to user y. Further, suppose an adjacent cell B assigns RB#1
to user z.  So in this time slot, RB#1 has interference and RB#2 does not. Based
on the extracted CQI from the TB, cell A will avoid using RB#1 in the next slot
which will results in wasting resources if cell B does not transmit on (or does
not choose) RB#1 in the next slot. Basically, this renders the information in
the CQI useless for the the scheduling/interference coordination (ICIC) process.


You don't have to implement RS signals in the TB using the detailed LTE resource
grid. I suggest using an out-of-band control channel solely for this purpose.
All eNBs must be transmitting on this channel continuously over all RB's so that
all users can estimate the SINR and CQI correctly and independently from any
interaction from scheduling/interference coordination that may be running in the
network. In fact this is what is done in practice, because these pre-configured
periodic Reference Signals (RS) essentially create a virtual control channel.  

I hope this is helpful.

regards
/Abdul
EE Dept.
University of Washington
Seattle, WA 
USA

[Marco Miozzo, 2012-04-27 09:17:17]

Hi Abdul,

many thanks for your comment and suggestion.

We know about the limitations of this assumption and we are going to solve
it in the next future.

Regards,
marco.



On Mon, Apr 23, 2012 at 6:54 PM, <abdulmehsen@gmail.com> wrote:

> Hi Marco,
>
> Thanks for the clarification. There are a couple of issues in this
> approach:
> 1- First, it invalidates the Gaussian interference model which is
> assumed in the SINR to CQI mapping process. In the Gaussian model, we
> assume there are many interference sources that have the same order of
> magnitude. If you extract the CQI from the received TB's SINR, this
> assumption does not hold because there could be only one or two
> interference sources depending on the scheduling/ICIC dynamics.
> Extracting CQI from SINR of pilot (RS) signals does not suffer from this
> issue, because a pilot by definition is always transmitted from all
> cells (independent of TBs) which implies many interference sources at
> the receiver.
>
> 2- You can see from the above point that there will be a strong
> undesirable interaction between the CQI feedback process and the
> scheduling/interference coordination process. For example, suppose in
> time slot #1, cell A assigns RB#1 to user x and RB#2 to user y. Further,
> suppose an adjacent cell B assigns RB#1 to user z.  So in this time
> slot, RB#1 has interference and RB#2 does not. Based on the extracted
> CQI from the TB, cell A will avoid using RB#1 in the next slot which
> will results in wasting resources if cell B does not transmit on (or
> does not choose) RB#1 in the next slot. Basically, this renders the
> information in the CQI useless for the the scheduling/interference
> coordination (ICIC) process.
>
> You don't have to implement RS signals in the TB using the detailed LTE
> resource grid. I suggest using an out-of-band control channel solely for
> this purpose. All eNBs must be transmitting on this channel continuously
> over all RB's so that all users can estimate the SINR and CQI correctly
> and independently from any interaction from scheduling/interference
> coordination that may be running in the network. In fact this is what is
> done in practice, because these pre-configured periodic Reference
> Signals (RS) essentially create a virtual control channel.
>
> I hope this is helpful.
>
> regards
> /Abdul
> EE Dept.
> University of Washington
> Seattle, WA
> USA
>
>
>
http://codereview.appspot.com/**4626069/<http://codereview.appspot.com/4626069/>
>