ns-3-dev-lte-ffr code review

ns-3-dev-lte-ffr code review

[Marco Miozzo, 2014-06-25 08:12:19]

Here my comments.

BR, marco.

https://codereview.appspot.com/104160043/diff/1/src/lte/model/cqa-ff-mac-sche...
File src/lte/model/cqa-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/model/cqa-ff-mac-sche...
src/lte/model/cqa-ff-mac-scheduler.cc:363: void
What about FFR?
We agreed to have FFR in PF, PSS and CQA.

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

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.h:93: int GetRbgSize (int dlbandwidth);
misleading name for the variable, check it.

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

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-ffr-soft-al...
src/lte/model/lte-ffr-soft-algorithm.cc:39: }
g_ffrSoftDownlinkDefaultConfiguration[] = {
What about for 15 RB (i.e., 3MHz)?

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-ffr-soft-al...
src/lte/model/lte-ffr-soft-algorithm.cc:210: NS_ASSERT_MSG (m_ulBandwidth >
24,"UlBandwidth must be at least 25 to use FFR algorithms");
same as above, I understand for 6, but what about 15?

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-hard-alg...
File src/lte/model/lte-fr-hard-algorithm.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-hard-alg...
src/lte/model/lte-fr-hard-algorithm.cc:38: { 1, 25, 0, 8},
15 RBs?

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-hard-alg...
src/lte/model/lte-fr-hard-algorithm.cc:251: if(1) {
if (1) ??

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-soft-alg...
File src/lte/model/lte-fr-soft-algorithm.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-soft-alg...
src/lte/model/lte-fr-soft-algorithm.cc:39: { 1, 25, 0, 8},
15 RBs?

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-soft-alg...
src/lte/model/lte-fr-soft-algorithm.cc:259: if(1) {
"if (1)" ??

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

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-strict-a...
src/lte/model/lte-fr-strict-algorithm.cc:40: { 1, 25, 6, 0, 6},
15 RBs?

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

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-trivial-...
src/lte/model/lte-fr-trivial-algorithm.h:42: class LteFrTrivialAlgorithm :
public LteFfrAlgorithm
Trivial might generate confusion, why not move to a name where is clear that the
algorithm is not performing any FR solution, e.g., LteNoFrAlgorithm or
LteFrNoOpAlgorithm.

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-spectrum-ph...
File src/lte/model/lte-spectrum-phy.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-spectrum-ph...
src/lte/model/lte-spectrum-phy.cc:1125:
LteSpectrumPhy::AddDataPowerChunkProcessor (Ptr<LteSinrChunkProcessor> p)
This is used only for testing purposes, am I right?

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-spectrum-va...
File src/lte/model/lte-spectrum-value-helper.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-spectrum-va...
src/lte/model/lte-spectrum-value-helper.cc:293: txPowerDensity = (powerTxW /
(txBandwidthConfiguration * 180000));
I would try to optimize a bit this code, since it is used many times during the
execution (i.e., avoiding this spurious calculation, it has been already
calculated before the txPowerDensity per RB basis in case of no power
allocation).

https://codereview.appspot.com/104160043/diff/1/src/lte/model/pss-ff-mac-sche...
File src/lte/model/pss-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/model/pss-ff-mac-sche...
src/lte/model/pss-ff-mac-scheduler.cc:323: void
What about FFR in PSS?
We agreed to have FFR in PF, PSS and CQA.

https://codereview.appspot.com/104160043/diff/1/src/lte/test/lte-ffr-simple.cc
File src/lte/test/lte-ffr-simple.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/test/lte-ffr-simple.c...
src/lte/test/lte-ffr-simple.cc:170: if(0){
"if(0)" ?

[gawlowicz.p, 2014-06-26 21:00:02]

code fixing

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

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.h:93: int GetRbgSize (int dlbandwidth);
On 2014/06/25 08:12:18, marco.miozzo wrote:
> misleading name for the variable, check it.
Can you explain more, please?

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

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-ffr-soft-al...
src/lte/model/lte-ffr-soft-algorithm.cc:39: }
g_ffrSoftDownlinkDefaultConfiguration[] = {
On 2014/06/25 08:12:18, marco.miozzo wrote:
> What about for 15 RB (i.e., 3MHz)?

Done.

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-ffr-soft-al...
src/lte/model/lte-ffr-soft-algorithm.cc:210: NS_ASSERT_MSG (m_ulBandwidth >
24,"UlBandwidth must be at least 25 to use FFR algorithms");
On 2014/06/25 08:12:18, marco.miozzo wrote:
> same as above, I understand for 6, but what about 15?

Done.

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-hard-alg...
File src/lte/model/lte-fr-hard-algorithm.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-hard-alg...
src/lte/model/lte-fr-hard-algorithm.cc:38: { 1, 25, 0, 8},
On 2014/06/25 08:12:18, marco.miozzo wrote:
> 15 RBs?

Done.

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-hard-alg...
src/lte/model/lte-fr-hard-algorithm.cc:251: if(1) {
On 2014/06/25 08:12:18, marco.miozzo wrote:
> if (1) ??

Done.

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-soft-alg...
File src/lte/model/lte-fr-soft-algorithm.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-soft-alg...
src/lte/model/lte-fr-soft-algorithm.cc:39: { 1, 25, 0, 8},
On 2014/06/25 08:12:18, marco.miozzo wrote:
> 15 RBs?

Done.

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-soft-alg...
src/lte/model/lte-fr-soft-algorithm.cc:259: if(1) {
On 2014/06/25 08:12:19, marco.miozzo wrote:
> "if (1)" ??

Done.

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

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-strict-a...
src/lte/model/lte-fr-strict-algorithm.cc:40: { 1, 25, 6, 0, 6},
On 2014/06/25 08:12:18, marco.miozzo wrote:
> 15 RBs?

Done.

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

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-fr-trivial-...
src/lte/model/lte-fr-trivial-algorithm.h:42: class LteFrTrivialAlgorithm :
public LteFfrAlgorithm
On 2014/06/25 08:12:19, marco.miozzo wrote:
> Trivial might generate confusion, why not move to a name where is clear that
the
> algorithm is not performing any FR solution, e.g., LteNoFrAlgorithm or
> LteFrNoOpAlgorithm. 

Done.

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-spectrum-ph...
File src/lte/model/lte-spectrum-phy.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-spectrum-ph...
src/lte/model/lte-spectrum-phy.cc:1125:
LteSpectrumPhy::AddDataPowerChunkProcessor (Ptr<LteSinrChunkProcessor> p)
On 2014/06/25 08:12:19, marco.miozzo wrote:
> This is used only for testing purposes, am I right?
Yes, only to compute power difference between PDSCH and PDCCH

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-spectrum-va...
File src/lte/model/lte-spectrum-value-helper.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/model/lte-spectrum-va...
src/lte/model/lte-spectrum-value-helper.cc:293: txPowerDensity = (powerTxW /
(txBandwidthConfiguration * 180000));
On 2014/06/25 08:12:19, marco.miozzo wrote:
> I would try to optimize a bit this code, since it is used many times during
the
> execution (i.e., avoiding this spurious calculation, it has been already
> calculated before the txPowerDensity per RB basis in case of no power
> allocation).

Done.

https://codereview.appspot.com/104160043/diff/1/src/lte/test/lte-ffr-simple.cc
File src/lte/test/lte-ffr-simple.cc (right):

https://codereview.appspot.com/104160043/diff/1/src/lte/test/lte-ffr-simple.c...
src/lte/test/lte-ffr-simple.cc:170: if(0){
On 2014/06/25 08:12:19, marco.miozzo wrote:
> "if(0)" ?

Done.