NS-3 GSoC 2017 LTE CA handover milestone 1

NS-3 GSoC 2017 LTE CA handover milestone 1

[Biljana Bojović, 2017-06-27 17:13:50]

Hi, 

please find my comments. I think that cc-helper function needs to be further
improved. I also didn't understand changes in lte-rrc-header. The rest is very
good.

Cheers,
Biljana

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

https://codereview.appspot.com/327850043/diff/1/src/lte/helper/cc-helper.cc#n...
src/lte/helper/cc-helper.cc:178: 
I agree with your approach. But I would change the calculation because one RB is
180KHz. I suggest to change the code taking into account the following: 

Spacing between contiguous CCs is equal to 180KHz * bandwidth_in_number_of_RBs.
Then the earfcn is the first following number that is multiple of 300KHz. 300KHz
is used because it is the least common multiple of 15KHz and 100KHz. Multiple of
15KHz needs to be fulfilled for orthogonality of subcarriers, and 100KHz to
respect channel raster of previous LTE release. E.g. if the bandwidth is 6RB,
spacing is 1080, but this is not possible, because it has to be multiple of
300KHz. E.g. 1100 would have ok channel raster, but would not have orthogonal
15KHz subcarriers, 1200 is ok because it respects both rules.

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

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-rrc-header....
src/lte/model/lte-rrc-header.cc:1742: 
Sorry, why is MAX_EARFCN changed to 65536?

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-rrc-header....
src/lte/model/lte-rrc-header.cc:2648: 
I do not follow these changes between 65536 and MAX_EARFCN...

[Tom Henderson, 2017-06-29 15:46:30]

This looks good; I have one comment (request) and one question.

request:  We need the LTE Sphinx documentation extended.  It is important to
clarify what features/functions are now being modelled with this addition.  The
'Technical Approach' section of your wiki page is a good start at this.  Please
find an appropriate place in LTE documentation to communicate to users exactly
what is now modelled (and what limitations or 'todo's remain).

question:  (for Biljana and Alexander)  Once review comments are addressed, what
prevents us, if anything, from proceeding to merge this chunk of code to
ns-3-dev?

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

https://codereview.appspot.com/327850043/diff/1/src/lte/helper/lte-helper.h#n...
src/lte/helper/lte-helper.h:531: void HandoverRequest (Time hoTime,
Ptr<NetDevice> ueDev,
If ueDev must be of the type LteUeNetDevice, why is this a Ptr to generic
NetDevice?

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-enb-rrc.cc
File src/lte/model/lte-enb-rrc.cc (right):

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-enb-rrc.cc#...
src/lte/model/lte-enb-rrc.cc:2131: NS_LOG_FUNCTION (this << componentCarrierId);
whenever logging a uint8_t, cast it to uint16_t to have it printed as an integer
and not character.

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

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-enb-rrc.h#n...
src/lte/model/lte-enb-rrc.h:899: uint16_t ComponentCarrierToCellId (uint8_t
componentCarrierId);
can the above two be const methods?

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

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-rrc-header....
src/lte/model/lte-rrc-header.cc:2731: NS_ASSERT(!nulOpt[2]); //
crossCarrierSchedulingConfig-r10 NOT IMplemented
s/NOT IMpl/Not Impl/

[Biljana Bojović, 2017-06-30 11:38:25]

I agree with Tom's request. 

Regarding the question, I think that there is basically nothing preventing us
from pushing this code to ns-3-dev. 

However, it cannot include the new test, since it does not pass yet the test,
this is the part of milestone2, and there is some minor issues in cc-helper that
I would like to be addressed before committing that part of code.

I would also like that Alexander clarifies changes in lte-rrc-header, either
here on adding some comments directly in the code.

[ilabdsf, 2017-06-30 12:25:33]

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

https://codereview.appspot.com/327850043/diff/1/src/lte/helper/cc-helper.cc#n...
src/lte/helper/cc-helper.cc:178: 
On 2017/06/27 17:13:50, Biljana Bojović wrote:
> I agree with your approach. But I would change the calculation because one RB
is
> 180KHz. I suggest to change the code taking into account the following: 
> 
> Spacing between contiguous CCs is equal to 180KHz *
bandwidth_in_number_of_RBs.
> Then the earfcn is the first following number that is multiple of 300KHz.
300KHz
> is used because it is the least common multiple of 15KHz and 100KHz. Multiple
of
> 15KHz needs to be fulfilled for orthogonality of subcarriers, and 100KHz to
> respect channel raster of previous LTE release. E.g. if the bandwidth is 6RB,
> spacing is 1080, but this is not possible, because it has to be multiple of
> 300KHz. E.g. 1100 would have ok channel raster, but would not have orthogonal
> 15KHz subcarriers, 1200 is ok because it respects both rules.

Thanks, I will fix it and some test for 6RB case then.

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

https://codereview.appspot.com/327850043/diff/1/src/lte/helper/lte-helper.h#n...
src/lte/helper/lte-helper.h:531: void HandoverRequest (Time hoTime,
Ptr<NetDevice> ueDev,
On 2017/06/29 15:46:30, Tom Henderson wrote:
> If ueDev must be of the type LteUeNetDevice, why is this a Ptr to generic
> NetDevice?

This function is a more specific version of 4-parameter HandoverRequest.

Thanks for suggestion, I will fix both of them as a separate commit to avoid
GetObject (which should have been a Dynamic cast btw) in DoHandoverRequest.

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-enb-rrc.cc
File src/lte/model/lte-enb-rrc.cc (right):

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-enb-rrc.cc#...
src/lte/model/lte-enb-rrc.cc:2131: NS_LOG_FUNCTION (this << componentCarrierId);
On 2017/06/29 15:46:30, Tom Henderson wrote:
> whenever logging a uint8_t, cast it to uint16_t to have it printed as an
integer
> and not character.

I will change it to "+componentCarrierId".

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

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-rrc-header....
src/lte/model/lte-rrc-header.cc:1742: 
On 2017/06/27 17:13:50, Biljana Bojović wrote:
> Sorry, why is MAX_EARFCN changed to 65536?

physCellId is a physical cell id (PCI), not an EARFCN. MAX_EARFCN was 65536
previously and this code worked correctly despite this bug, but now MAX_EARFCN
is 262143.

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-rrc-header....
src/lte/model/lte-rrc-header.cc:2648: 
On 2017/06/27 17:13:50, Biljana Bojović wrote:
> I do not follow these changes between 65536 and MAX_EARFCN...

Here I am deserializing dl-CarrierFreq, which is an EARFCN. Previously
MAX_EARFCN was 65536 and this code was correct, but then MAX_EARFCN changed and
this code is broken, because we use MAX_EARFCN constant during serialization.

Essentially, all frequencies are searialized as EARFCN and MAX_EARFCN should be
used for them.

https://codereview.appspot.com/327850043/diff/1/src/lte/model/lte-rrc-header....
src/lte/model/lte-rrc-header.cc:2731: NS_ASSERT(!nulOpt[2]); //
crossCarrierSchedulingConfig-r10 NOT IMplemented
On 2017/06/29 15:46:30, Tom Henderson wrote:
> s/NOT IMpl/Not Impl/

Done.

[zorazeali, 2017-07-04 14:53:03]

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

https://codereview.appspot.com/327850043/diff/1/src/lte/helper/cc-helper.cc#n...
src/lte/helper/cc-helper.cc:179: // One RB is 200 kHz wide, while increment by 1
in corresponds to 100 kHz shift
I think the other way to do this is by following the nominal channel spacing
formula given by 3GPP. Following is my suggestion. 

Let us suppose bw_pCC and bw_sCC are the bandwidths of primary and secondary
component carriers in MHz, e.g., if both bandwidths are 5000000 Hz following
function will take CarrierAggregationChannelSpacing(5, 5)

double CarrierAggregationChannelSpacing(bw_pCC, bw_sCC)

 {
    % Nominal channel spacing as defined in TS36.101 5.7.1A

    spacing = 1.0e6 * (floor((bw_pCC+bw_sCC-0.1*abs(bw_pCC-
              bw_sCC))/0.6)*0.3);
    return spacing;
 }

Now we will find the carrier frequency and the EARFCN of secondary component
carrier (Fc_sCC and dlEarfcn_sCC).
    
First we find the band number of primary component carrier to find the Fc_sCC
and dlEarfcn_sCC with in the same band.

    bandNumber = GetDownlinkCarrierBand (dlEarfcn_pCC)

    fc_pCC=GetCarrierFrequency (uint32_t dlEarfcn_pCC)
  
    Fc_sCC= fc_pCC + spacing (Note: Here Fc_sCC would be in Hz)

   % Function to find the EARFCN from carrier frequency. Formula assumes
frequencies in MHz so we do 

    Fc_sCC = Fc_sCC/1.0e6

    dlEarfcn_sCC = (Fc_sCC - g_eutraChannelNumbers[bandNumber].fDlLow + 0.1 *
g_eutraChannelNumbers[bandNumber].nOffsDl)/0.1


Now you have the correct Dl EARFCN number of the secondary component carrier
whose carrier frequency spacing is multiple of 300 KHz. You 
can generalize these functions to be used for both downlink and uplink.

[Biljana Bojović, 2017-07-11 08:57:19]

Thanks Zoraze for sharing the formula. The formula that you shared could be
useful in a general case. However, the function in CcHelper had the original
idea to create equally spaced carriers. But, the function had a bug which was
spotted by Alexander (overlapping of contiguous bands). Alexander provided a
patch according to which the central frequencies are calculated correctly, and
additionally he added constraint to have 300KHz spacing.

It would be nice also to have the generic function that can create different
combinations of contiguous carriers of different bandwidths, but this was not
planned under this GSoC project plan and I think it will not be needed for tests
and examples of this GSoC project.


On 2017/07/04 14:53:03, zorazeali wrote:
> https://codereview.appspot.com/327850043/diff/1/src/lte/helper/cc-helper.cc
> File src/lte/helper/cc-helper.cc (right):
> 
>
https://codereview.appspot.com/327850043/diff/1/src/lte/helper/cc-helper.cc#n...
> src/lte/helper/cc-helper.cc:179: // One RB is 200 kHz wide, while increment by
1
> in corresponds to 100 kHz shift
> I think the other way to do this is by following the nominal channel spacing
> formula given by 3GPP. Following is my suggestion. 
> 
> Let us suppose bw_pCC and bw_sCC are the bandwidths of primary and secondary
> component carriers in MHz, e.g., if both bandwidths are 5000000 Hz following
> function will take CarrierAggregationChannelSpacing(5, 5)
> 
> double CarrierAggregationChannelSpacing(bw_pCC, bw_sCC)
> 
>  {
>     % Nominal channel spacing as defined in TS36.101 5.7.1A
> 
>     spacing = 1.0e6 * (floor((bw_pCC+bw_sCC-0.1*abs(bw_pCC-
>               bw_sCC))/0.6)*0.3);
>     return spacing;
>  }
> 
> Now we will find the carrier frequency and the EARFCN of secondary component
> carrier (Fc_sCC and dlEarfcn_sCC).
>     
> First we find the band number of primary component carrier to find the Fc_sCC
> and dlEarfcn_sCC with in the same band.
> 
>     bandNumber = GetDownlinkCarrierBand (dlEarfcn_pCC)
> 
>     fc_pCC=GetCarrierFrequency (uint32_t dlEarfcn_pCC)
>   
>     Fc_sCC= fc_pCC + spacing (Note: Here Fc_sCC would be in Hz)
> 
>    % Function to find the EARFCN from carrier frequency. Formula assumes
> frequencies in MHz so we do 
> 
>     Fc_sCC = Fc_sCC/1.0e6
> 
>     dlEarfcn_sCC = (Fc_sCC - g_eutraChannelNumbers[bandNumber].fDlLow + 0.1 *
> g_eutraChannelNumbers[bandNumber].nOffsDl)/0.1
> 
> 
> Now you have the correct Dl EARFCN number of the secondary component carrier
> whose carrier frequency spacing is multiple of 300 KHz. You 
> can generalize these functions to be used for both downlink and uplink.