GSoC 2014 LTE FFR algorithms project

GSOC 2014 LTE FFR Final code review

The review covers all the changes from the beginning of the project, so my patch contains:
-FfrAlgorithm abstract class
-Two SAPs FfrSchedulerSap and FfrRrcSap
-Updated all schedulers and eNB RRC with new SAPs
-Updated LteHelper
-Downlink Power Control (3 modes: Open Loop, Closed Loop Absolute, Closed Loop Accumulative)
-Updated RrcAsn1Header with PdschConfigDedicated structure
-Implementation of new CQI generation approach
-Code refactoring of LteSinrChunkProcessor
-Connected FR algorithms with PF, PSS, CQA, FD-TBFQ, TD-TBFQ Schedulers
-Fixed HARQ bug
-Uplink Power Control
-Frequency Reuse Algorithms: 
	-NoOp FR algorithm
	-Hard FR algorithm
	-Strict FR algorithm
	-Soft FR algorithm
	-Soft FFR algorithm
	-Enhanced FFR algorithm
	-Distributed FFR algorithm
-Tests:
	-lte-downlink-power-control to check correctness of Downlink Power Control
	-lte-frequency-reuse to check if scheduler (PF, PSS, CQA, FD-TBFQ, TD-TBFQ) is not using forbidden RBGs, 
         and if UE is served with proper power and RBGs in downlink according to FFR algorithm policy
	-lte-interference-fr checks interference level with Hard and Strict FR algorithms
	-lte-cqi-generation check new CQI generation approach
	-lte-uplink-power-control to check correctness of Uplink Power Control in 3 modes
-Design, user and test documentation 
-Automatic Frequency Reuse configuration in DL and UL (cell can be configured with only 1 attribute)
-Fixed problem with uplink scheduling with frequency reuse algorithm
-Fixed 2 HARQ bugs in all MAC schedulers
-Examples:
	-lena-frequency-reuse
	-lena-distributed-ffr
	-lena-uplink-power-control

There are 5 test suites of interest, which can be run as follows:
     	./test.py -s lte-downlink-power-control
     	./test.py -s lte-frequency-reuse
     	./test.py -s lte-interference-fr
     	./test.py -s lte-cqi-generation
     	./test.py -s lte-uplink-power-control

More details can be found in the project Wiki page [1] and documentation page [2]. 
Source code is available for download in project Mercurial repository [3]. 

Please feel free to contact me if you have any comments or questions. 
Thanks in advance for reviewing.

[1] http://www.nsnam.org/wiki/GSOC2014LteFFR
[2] http://code.nsnam.org/pgawlowicz/ns-3-dev-lte-ffr/
[3] http://www.nsnam.org/~pgawlowicz/lte.html

[buherman, 2014-08-21 23:43:37]

Hi Piotr,

Well done for a tremendous piece of work. I especially like the new look of
chunk processor. Please find my comments below.

Regards,
-budi-

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
File src/lte/doc/source/lte-design.rst (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
src/lte/doc/source/lte-design.rst:3627: \end{Bmatrix} [dBm]
I'd prefer "\min" and "\log" instead of "min" and "log". There are several
similar occurrences in the rest of this document.

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
src/lte/doc/source/lte-design.rst:3696: serving cell :math:`c` on subframe
:math:`i − K PUSCH`; :math:`K_{PUSCH} = 4` for FDD.
K PUSCH -> K_{PUSCH}

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
src/lte/doc/source/lte-design.rst:3710: serving cell :math:`c` on subframe
:math:`i − K PUSCH`; :math:`K_{PUSCH} = 4` for FDD.
(idem)

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
src/lte/doc/source/lte-design.rst:3845: divided into few (typically 3,4 or 7)
disjoint sub-bands. Adjacent eNBs are
3,4, or 7 -> 3, 4, or 7

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
src/lte/doc/source/lte-design.rst:3869: Stric Frequency Reuse scheme is
combination of Full and Hard Frequency Reuse
Stric -> Strict

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-testin...
File src/lte/doc/source/lte-testing.rst (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-testin...
src/lte/doc/source/lte-testing.rst:1430: UE uses Uplink Power Control to
autamatically change Tx Power level for Uplink
autamatically -> automatically

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-testin...
src/lte/doc/source/lte-testing.rst:1431: Physical Channels. Tx Power is computed
based on path-loss, number of RB used for transmition,
transmition -> transmission

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-testin...
src/lte/doc/source/lte-testing.rst:1454: *
LteUplinkClosedLoopPowerControlAccumulatedModeTestCase Closed Loop Uplink Power
Control
Closed Loop -> test case check(s)

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-user.rst
File src/lte/doc/source/lte-user.rst (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-user.r...
src/lte/doc/source/lte-user.rst:1530: cell bandwidth greater or equal than 15.
This limitation is caused by requirement
Do you mean 15 RBs or 15 MHz?

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-user.r...
src/lte/doc/source/lte-user.rst:1550: its ``SetHandoverAlgorithmType`` method as
shown below::
SetHandoverAlgorithmType -> SetFfrAlgorithmType

https://codereview.appspot.com/130130043/diff/1/src/lte/examples/lena-distrib...
File src/lte/examples/lena-distributed-ffr.cc (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/examples/lena-distrib...
src/lte/examples/lena-distributed-ffr.cc:379: }
This code block doesn't follow ns-3 indentation style.

Similar issues can be found in other places as well. You may need to increase
the level of check-style.py to catch this kind of issues (e.g., using -l3
argument).

As an alternative:
hg diff -r 10785:tip | grep -n '^+.*)\s*{'
hg diff -r 10785:tip | grep -n '^+\s*for('
hg diff -r 10785:tip | grep -n '^+\s*if('

https://codereview.appspot.com/130130043/diff/1/src/lte/examples/lena-x2-hand...
File src/lte/examples/lena-x2-handover.cc (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/examples/lena-x2-hand...
src/lte/examples/lena-x2-handover.cc:152: Config::SetDefault
("ns3::LteUePhy::EnableUplinkPowerControl", BooleanValue (false));
I assume uplink power control will be considered as a standard feature of ns-3
LTE module. Therefore, IMHO, we don't have to disable it in examples and tests.

https://codereview.appspot.com/130130043/diff/1/src/lte/helper/lte-helper.cc
File src/lte/helper/lte-helper.cc (left):

https://codereview.appspot.com/130130043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:157: "of any class inheriting from
ns3::HandoverAlgorithm.",
ns3::HandoverAlgorithm -> ns3::LteHandoverAlgorithm

https://codereview.appspot.com/130130043/diff/1/src/lte/helper/lte-helper.cc
File src/lte/helper/lte-helper.cc (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:158: "of any class inheriting from
ns3::FfrAlgorithm.",
ns3::FfrAlgorithm -> ns3::LteFfrAlgorithm

https://codereview.appspot.com/130130043/diff/1/src/lte/helper/radio-environm...
File src/lte/helper/radio-environment-map-helper.cc (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/helper/radio-environm...
src/lte/helper/radio-environment-map-helper.cc:147: MakeIntegerChecker<int32_t>
())
Extra leading spaces.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-chunk-proce...
File src/lte/model/lte-chunk-processor.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-chunk-proce...
src/lte/model/lte-chunk-processor.h:20: *        (move from CQI to Ctrl and Data
SINR Chunk processors)
Since this file (and the .cc file) has changed significantly, how about adding a
historical comment here.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-chunk-proce...
src/lte/model/lte-chunk-processor.h:33: #include <ns3/lte-spectrum-phy.h>
It's possible to trim the above #include list. SpectrumValue can be converted to
forward declaration. Secondly, packet-h, lte-phy.h, and lte-spectrum-phy.h can
be removed.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-chunk-proce...
src/lte/model/lte-chunk-processor.h:54: virtual void End ();
Would be great to have Doxygen documentation for each of the methods above.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-algorit...
File src/lte/model/lte-ffr-algorithm.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.h:29: #include "ns3/uinteger.h"
Not necessary.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.h:118: * \param FrCellTypeId for automatic FR
configuration
FrCellTypeId -> cellTypeId. Also check the rest of the methods in this class for
incorrect names in \param. There are at least seven of them in this file.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-distrib...
File src/lte/model/lte-ffr-distributed-algorithm.cc (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-distrib...
src/lte/model/lte-ffr-distributed-algorithm.cc:157: NS_LOG_LOGIC (this << "
requesting Event A4 measurements"
A4 -> A1 and A4. Please check other algorithms as well.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-distrib...
src/lte/model/lte-ffr-distributed-algorithm.cc:186: NS_LOG_FUNCTION (this);
Double.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-distrib...
File src/lte/model/lte-ffr-distributed-algorithm.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-distrib...
src/lte/model/lte-ffr-distributed-algorithm.h:23: #define
LTE_FR_DISTRIBUTED_ALGORITHM_H
FR -> FFR

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-soft-al...
File src/lte/model/lte-ffr-soft-algorithm.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-soft-al...
src/lte/model/lte-ffr-soft-algorithm.h:35: * \brief Soft Frequency Reuse
algorithm implementation
+Fractional

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-fr-no-op-al...
File src/lte/model/lte-fr-no-op-algorithm.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-fr-no-op-al...
src/lte/model/lte-fr-no-op-algorithm.h:23: #define LTE_FFR_NO_OP_ALGORITHM_H
FFR -> FR

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-fr-strict-a...
File src/lte/model/lte-fr-strict-algorithm.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-fr-strict-a...
src/lte/model/lte-fr-strict-algorithm.h:35: * \brief Soft Frequency Reuse
algorithm implementation
Soft -> Strict

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-rrc-header.h
File src/lte/model/lte-rrc-header.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-rrc-header....
src/lte/model/lte-rrc-header.h:67: 
Is it still possible to remove this very minimal change?

https://codereview.appspot.com/130130043/diff/1/src/lte/test/lte-test-interfe...
File src/lte/test/lte-test-interference-fr.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/test/lte-test-interfe...
src/lte/test/lte-test-interference-fr.h:20: *                                   
                                          Nicola Baldo <nbaldo@cttc.es>
Too many leading spaces.

https://codereview.appspot.com/130130043/diff/1/src/lte/test/lte-test-sinr-ch...
File src/lte/test/lte-test-sinr-chunk-processor.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/test/lte-test-sinr-ch...
src/lte/test/lte-test-sinr-chunk-processor.h:52: Ptr<LtePhy> m_phy;
It seems that m_phy is never used. Shall we remove it?

[gawlowicz.p, 2014-09-01 22:48:45]

Hi Budiarto,

Thank you for code review. 

Sorry for delay with fixing your comments, but I was finishing my MSc thesis
during last week.

Regards,
Piotr

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
File src/lte/doc/source/lte-design.rst (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
src/lte/doc/source/lte-design.rst:3627: \end{Bmatrix} [dBm]
On 2014/08/21 23:43:36, buherman wrote:
> I'd prefer "\min" and "\log" instead of "min" and "log". There are several
> similar occurrences in the rest of this document.

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
src/lte/doc/source/lte-design.rst:3696: serving cell :math:`c` on subframe
:math:`i − K PUSCH`; :math:`K_{PUSCH} = 4` for FDD.
On 2014/08/21 23:43:36, buherman wrote:
> K PUSCH -> K_{PUSCH}

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
src/lte/doc/source/lte-design.rst:3710: serving cell :math:`c` on subframe
:math:`i − K PUSCH`; :math:`K_{PUSCH} = 4` for FDD.
On 2014/08/21 23:43:36, buherman wrote:
> (idem)

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
src/lte/doc/source/lte-design.rst:3845: divided into few (typically 3,4 or 7)
disjoint sub-bands. Adjacent eNBs are
On 2014/08/21 23:43:36, buherman wrote:
> 3,4, or 7 -> 3, 4, or 7

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-design...
src/lte/doc/source/lte-design.rst:3869: Stric Frequency Reuse scheme is
combination of Full and Hard Frequency Reuse
On 2014/08/21 23:43:36, buherman wrote:
> Stric -> Strict

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-user.rst
File src/lte/doc/source/lte-user.rst (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-user.r...
src/lte/doc/source/lte-user.rst:1530: cell bandwidth greater or equal than 15.
This limitation is caused by requirement
RBs 
On 2014/08/21 23:43:37, buherman wrote:
> Do you mean 15 RBs or 15 MHz?

https://codereview.appspot.com/130130043/diff/1/src/lte/doc/source/lte-user.r...
src/lte/doc/source/lte-user.rst:1550: its ``SetHandoverAlgorithmType`` method as
shown below::
On 2014/08/21 23:43:37, buherman wrote:
> SetHandoverAlgorithmType -> SetFfrAlgorithmType

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/examples/lena-x2-hand...
File src/lte/examples/lena-x2-handover.cc (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/examples/lena-x2-hand...
src/lte/examples/lena-x2-handover.cc:152: Config::SetDefault
("ns3::LteUePhy::EnableUplinkPowerControl", BooleanValue (false));
But test were created without Uplink Power Control and do not pass with it. Test
needs to be updated and then this line will be removed.

On 2014/08/21 23:43:36, buherman wrote:
> I assume uplink power control will be considered as a standard feature of ns-3
> LTE module. Therefore, IMHO, we don't have to disable it in examples and
tests.

https://codereview.appspot.com/130130043/diff/1/src/lte/helper/lte-helper.cc
File src/lte/helper/lte-helper.cc (left):

https://codereview.appspot.com/130130043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:157: "of any class inheriting from
ns3::HandoverAlgorithm.",
On 2014/08/21 23:43:36, buherman wrote:
> ns3::HandoverAlgorithm -> ns3::LteHandoverAlgorithm

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/helper/lte-helper.cc
File src/lte/helper/lte-helper.cc (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:158: "of any class inheriting from
ns3::FfrAlgorithm.",
On 2014/08/21 23:43:36, buherman wrote:
> ns3::FfrAlgorithm -> ns3::LteFfrAlgorithm

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-algorit...
File src/lte/model/lte-ffr-algorithm.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.h:29: #include "ns3/uinteger.h"
On 2014/08/21 23:43:37, buherman wrote:
> Not necessary.

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.h:118: * \param FrCellTypeId for automatic FR
configuration
On 2014/08/21 23:43:36, buherman wrote:
> FrCellTypeId -> cellTypeId. Also check the rest of the methods in this class
for
> incorrect names in \param. There are at least seven of them in this file.

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-distrib...
File src/lte/model/lte-ffr-distributed-algorithm.cc (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-distrib...
src/lte/model/lte-ffr-distributed-algorithm.cc:157: NS_LOG_LOGIC (this << "
requesting Event A4 measurements"
On 2014/08/21 23:43:36, buherman wrote:
> A4 -> A1 and A4. Please check other algorithms as well.

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-distrib...
src/lte/model/lte-ffr-distributed-algorithm.cc:186: NS_LOG_FUNCTION (this);
On 2014/08/21 23:43:36, buherman wrote:
> Double.

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-soft-al...
File src/lte/model/lte-ffr-soft-algorithm.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-ffr-soft-al...
src/lte/model/lte-ffr-soft-algorithm.h:35: * \brief Soft Frequency Reuse
algorithm implementation
On 2014/08/21 23:43:36, buherman wrote:
> +Fractional

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-fr-no-op-al...
File src/lte/model/lte-fr-no-op-algorithm.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-fr-no-op-al...
src/lte/model/lte-fr-no-op-algorithm.h:23: #define LTE_FFR_NO_OP_ALGORITHM_H
On 2014/08/21 23:43:37, buherman wrote:
> FFR -> FR

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-fr-strict-a...
File src/lte/model/lte-fr-strict-algorithm.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-fr-strict-a...
src/lte/model/lte-fr-strict-algorithm.h:35: * \brief Soft Frequency Reuse
algorithm implementation
On 2014/08/21 23:43:36, buherman wrote:
> Soft -> Strict

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/test/lte-test-interfe...
File src/lte/test/lte-test-interference-fr.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/test/lte-test-interfe...
src/lte/test/lte-test-interference-fr.h:20: *                                   
                                          Nicola Baldo <nbaldo@cttc.es>
On 2014/08/21 23:43:36, buherman wrote:
> Too many leading spaces.

Done.

[Nicola Baldo, 2014-09-03 17:00:52]

Some further comments on issues raised by Budi and still open...

https://codereview.appspot.com/130130043/diff/1/src/lte/examples/lena-x2-hand...
File src/lte/examples/lena-x2-handover.cc (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/examples/lena-x2-hand...
src/lte/examples/lena-x2-handover.cc:152: Config::SetDefault
("ns3::LteUePhy::EnableUplinkPowerControl", BooleanValue (false));
I think Budi is right here, we should test with UPC enabled. Otherwise, if
handover fails with UPC enabled, and UPC defaults to enabled, we'll start
getting lots of complaints by users that handover doesn't work anymore.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-chunk-proce...
File src/lte/model/lte-chunk-processor.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-chunk-proce...
src/lte/model/lte-chunk-processor.h:33: #include <ns3/lte-spectrum-phy.h>
On 2014/09/01 22:47:30, gawlowicz.p wrote:
> Remove #include <ns3/spectrum-value.h> in here and add it in .cc file? 

Yes. IIRC the ns-3 coding style says to use forward declarations wherever
possible

[gawlowicz.p, 2014-09-05 00:12:29]

Hi,

I have fixed last two comments.

Piotr

https://codereview.appspot.com/130130043/diff/1/src/lte/examples/lena-x2-hand...
File src/lte/examples/lena-x2-handover.cc (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/examples/lena-x2-hand...
src/lte/examples/lena-x2-handover.cc:152: Config::SetDefault
("ns3::LteUePhy::EnableUplinkPowerControl", BooleanValue (false));
On 2014/09/03 17:00:52, Nicola Baldo wrote:
> I think Budi is right here, we should test with UPC enabled. Otherwise, if
> handover fails with UPC enabled, and UPC defaults to enabled, we'll start
> getting lots of complaints by users that handover doesn't work anymore.

Done.

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-chunk-proce...
File src/lte/model/lte-chunk-processor.h (right):

https://codereview.appspot.com/130130043/diff/1/src/lte/model/lte-chunk-proce...
src/lte/model/lte-chunk-processor.h:33: #include <ns3/lte-spectrum-phy.h>
On 2014/09/03 17:00:52, Nicola Baldo wrote:
> On 2014/09/01 22:47:30, gawlowicz.p wrote:
> > Remove #include <ns3/spectrum-value.h> in here and add it in .cc file? 
> 
> Yes. IIRC the ns-3 coding style says to use forward declarations wherever
> possible

Done.