Code review - Issue 327850043: NS-3 GSoC 2017 LTE CA handover milestone 1https://codereview.appspot.com/2017-07-11T08:57:19+00:00rietveld
Message from unknown
2017-06-23T22:26:29+00:00ilabdsfurn:md5:6a30ad27609282cbb538817157e68a4b
Message from Biljana.Bojovic@gmail.com
2017-06-27T17:13:50+00:00Biljana Bojovićurn:md5:97df8f32a750f98a7224b21e988500b7
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#newcode178
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.cc#newcode1742
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.cc#newcode2648
src/lte/model/lte-rrc-header.cc:2648:
I do not follow these changes between 65536 and MAX_EARFCN...
Message from tomh.org@gmail.com
2017-06-29T15:46:30+00:00Tom Hendersonurn:md5:cd712a23e2e289220848db63f2d67cfb
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#newcode531
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#newcode2131
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#newcode899
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.cc#newcode2731
src/lte/model/lte-rrc-header.cc:2731: NS_ASSERT(!nulOpt[2]); // crossCarrierSchedulingConfig-r10 NOT IMplemented
s/NOT IMpl/Not Impl/
Message from Biljana.Bojovic@gmail.com
2017-06-30T11:38:25+00:00Biljana Bojovićurn:md5:fa92f9b9b30d986dd14f467976e81afe
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.
Message from ilabdsf@gmail.com
2017-06-30T12:25:33+00:00ilabdsfurn:md5:17227c95dd973002dc03890c644b1ec5
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#newcode178
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#newcode531
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#newcode2131
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.cc#newcode1742
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.cc#newcode2648
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.cc#newcode2731
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.
Message from zorazeali@gmail.com
2017-07-04T14:53:03+00:00zorazealiurn:md5:6167d64a9996f27950ad1a3931d88b26
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#newcode179
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.
Message from Biljana.Bojovic@gmail.com
2017-07-11T08:57:19+00:00Biljana Bojovićurn:md5:03f758910ab2983e57aa2d0113a765c2
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#newcode179
> 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.