ns-3 GSoC 2013 (LTE) - Mid-term code review

This is the mid-term code review for the GSoC 2013 project for developing UE measurements in ACTIVE and IDLE mode for ns-3.

The review covers all the changes from the beginning of the project until the latest revision as of July 22nd (in specific, revision 9928 was the base, and revision 9984 was the head). This includes code, test scripts, and documentation of the completed "UE Measurements" block and the partially completed "Initial Cell Selection" block. More information on the detailed features can be found in the project Wiki page [1] or in the model library (Sphinx) documentation.

As a reference, below is the list of supported event-based triggering in the underlying UE measurements function.
 - Event A1: Serving cell becomes better than `threshold`
 - Event A2: Serving cell becomes worse than `threshold`
 - Event A3: Neighbour becomes `offset` dB better than serving cell
 - Event A4: Neighbour becomes better than `threshold`
 - Event A5: Serving becomes worse than `threshold1` *AND* neighbour becomes better than `threshold2`

Several notes about the code:

 - The "pre-GSOC" UE measurement configuration, found in some test suites and examples, exists as a temporary workaround before it is implemented as a handover algorithm during the second term of the project.

 - The UE measurements implementation still contains several blocks of `#ifdef NS3_ASSERT_ENABLE`, which were intended for my own testing. They will be eventually removed.

 - I forget to disable `LogComponentEnable` statements in the `lte-cell-selection` test suite. They will be disabled for sure.

 - The documentation on Initial Cell Selection might not reflect the current code, since I haven't updated it yet.

 - A full-blown `./test.py` results in failure for `lte-pathloss-model` test suite and crash for `buildings-pathloss-profiler` example. The former is caused by UE not entering CONNECTED mode. While the latter can probably be fixed when I pull in some bug fixes from lena-dev.

The whole set of code can be downloaded from the repository [2]. As of now, there are 5 test suites of interest, which can be run as follows:
    ./test.py -f EXTENSIVE -s lte-ue-measurements
    ./test.py -f TAKES_FOREVER -s lte-ue-measurements-piecewise-1
    ./test.py -f TAKES_FOREVER -s lte-ue-measurements-piecewise-2
    ./test.py -f TAKES_FOREVER -s lte-ue-measurements-handover
    ./test.py -s lte-cell-selection

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

-budi-

[1] https://www.nsnam.org/wiki/index.php/GSOC2013UeMeasurementActiveIdle
[2] http://code.nsnam.org/buherman/ns-3-gsoc-lte/

[Nicola Baldo, 2013-07-25 16:56:11]

Hi Budiarto,

I reviewed the model code only, I still have to take a look at the tests and the
documentation. 

Overall, good work! 

The main general comment that I have is that the configuration shall be
propagated among classes using the SAPs shown here:
http://lena.cttc.es/manual/lte-design.html#fig-lte-arch-ue-ctrl
and I spotted several cases in which you took shortcuts, e.g., having the PHY
configured directly by the NetDevice, or by the LteHelper. I think these cases
should be changed to use the SAP approach, for consistency.

Most of the other comments are minor, e.g., about naming, code organization or
to request some clarification.

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

https://codereview.appspot.com/11670043/diff/1/src/lte/helper/lte-helper.cc#n...
src/lte/helper/lte-helper.cc:593: uePhy->CellSearch ();
instead of this, please have the RRC put the UePhy in CELL_SEARCH whenever
needed, using a new LteUeCphySap::StartCellSearch () method

https://codereview.appspot.com/11670043/diff/1/src/lte/helper/lte-helper.cc#n...
src/lte/helper/lte-helper.cc:686: ueNas->Connect ();
Why do you need a new LteHelper::Connect(ueDev) method? ...can't you move
everything into the LteHelper::Attach() method?

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

https://codereview.appspot.com/11670043/diff/1/src/lte/helper/lte-helper.h#ne...
src/lte/helper/lte-helper.h:195: * Attach a set of UE devices to a single eNB
device
please add a comment here saying that the use of this method will disable the
cell selection procedure

https://codereview.appspot.com/11670043/diff/1/src/lte/helper/lte-helper.h#ne...
src/lte/helper/lte-helper.h:199: */
also here

https://codereview.appspot.com/11670043/diff/1/src/lte/model/epc-ue-nas.cc
File src/lte/model/epc-ue-nas.cc (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/epc-ue-nas.cc#ne...
src/lte/model/epc-ue-nas.cc:136: // since RRC Idle Mode cell selection is not
supported yet, we
please update this comment

https://codereview.appspot.com/11670043/diff/1/src/lte/model/epc-ue-nas.h
File src/lte/model/epc-ue-nas.h (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/epc-ue-nas.h#new...
src/lte/model/epc-ue-nas.h:94: * attempt to connect at the earliest possible
time (e.g. after it camps to a
It's good to have this paragraph "If this function..." in the documentation, but
I think the appropriate place for it is LteAsSapProvider::Connect. Please move
it there. 

For EpcUeNas::Connect(), it might suffice to say that (among other things) it
causes the NAS to tell the Access Stratum (AS) to get connected.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-common.h
File src/lte/model/lte-common.h (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-common.h#new...
src/lte/model/lte-common.h:224: static double GetActualHysteresis (uint8_t
hysteresis);
For consistency with the other methods in this file, I would suggest to rename
this to IeValue2ActualHysteresis or something similar.

Not that I like very much the naming of the pre-existing methods here... feel
free to propose a renaming for all of them if you have a better idea.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-common.h#new...
src/lte/model/lte-common.h:233: static double GetActualA3Offset (int8_t
a3Offset);
same as previous comment

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

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-enb-rrc.h#ne...
src/lte/model/lte-enb-rrc.h:852: LteRrcSap::MeasConfig m_ueMeasConfigList;
"List" in the name is a bit misleading, what about calling it just
"m_ueMeasConfig"? 

I mean, the name would be ok if the type were std::list<MeasConfig>, but that's
not the case.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-enb-rrc.h#ne...
src/lte/model/lte-enb-rrc.h:857: uint8_t m_numOfUeMeasConfig;
The naming is misleading - if it were number of identities, it would be called
m_numOfUeMeasId, and actually you use to count the number of MeasReportConfig,
which in general is not the same as the number of MeasId.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-net-devic...
File src/lte/model/lte-ue-net-device.cc (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-net-devic...
src/lte/model/lte-ue-net-device.cc:191: m_phy->SetDlEarfcn (earfcn);
The LteUeNetDevice shall not configure the PHY directly. Instead, the
configuration of the PHY should be done by the RRC using the LteUeCphySap
interface.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc
File src/lte/model/lte-ue-phy.cc (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc#ne...
src/lte/model/lte-ue-phy.cc:902: if (m_cellId == 0)
If you need this, I'd use "switch (m_state)" instead

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc#ne...
src/lte/model/lte-ue-phy.cc:906: * value is not available and
GenerateCtrlCqiReport will not be called.
I understand why you replicated the RSRP code here; still, I wonder if there is
any way to reduce the code duplication. For example, what about leaving the 
RSRP measurements here (regardless of wheter the state is CELL_SEARCH or
SYNCHRONIZED), leaving only the RSRQ part in LteUePhy::GenerateCtrlCqiReport
(thus removing the RSRP code there)?

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc#ne...
src/lte/model/lte-ue-phy.cc:932: (*itMeasMap).second.rsrqNum++;
if RSRQ is not available, you should not increment this. Otherwise if RSRQ
suddenly becomes available, the average will be incorrect.

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

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.h#new...
src/lte/model/lte-ue-phy.h:148: void SetDlEarfcn (uint16_t earfcn);
please make this an LteUeCphySap method

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.h#new...
src/lte/model/lte-ue-phy.h:186: void CellSearch ();
please make this an LteUeCphySap method

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

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-rrc.cc#ne...
src/lte/model/lte-ue-rrc.cc:871: m_acceptableCell.insert (cellId);
Frankly, I would not model the selection of acceptable cells at all - are you
really interested in simulating emergency calls?

[Marco Miozzo, 2013-07-26 10:57:21]

I have two comments on the modification in LteUePhy.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc
File src/lte/model/lte-ue-phy.cc (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc#ne...
src/lte/model/lte-ue-phy.cc:428: LteUePhy::GenerateCtrlCqiReport (const
SpectrumValue& sinr)
GenerateCtrlCqiReport is called each time a PDCCH is received, how do you manage
with this change that the UE is not generating sporious CQIs (i.e., CQIs with
m_rnti equal to 0 when the UE is not yet attached or even with different
bandwidth respect to the eNB).

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc#ne...
src/lte/model/lte-ue-phy.cc:906: * value is not available and
GenerateCtrlCqiReport will not be called.
I agree with Nicola, let's try to limit code duplication. The split proposed by
Nicola seems to be reasonable. 
Moreover, RSRQ spans from -19.5 to -3 dB, by putting 0 you are using a not valid
value, which anyway will be truncated by UE RRC to -3. Thus, it will be possibly
used by the eNB RRC as a valid (good) values for its estimation in attaching the
UE and therefore compared with real RSRQ values.

[Nicola Baldo, 2013-07-29 11:11:38]

Hi Budi,

please find below some comments on the .rst documentation.

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

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:1969: Reception of system information is
detrimental to the transition of UE RRC
I think "detrimental" is incorrect here. Maybe fundamental?

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:1980: UE in `IDLE_CELL_SELECTION` state, e.g.
in the beginning of simulation, will
at the beginning

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:2022: (based on RSRP) and connect to it. This
is done by issuing a contention-based
I'd say no, connection will be done only if explicitly requested. Otherwise the
UE will stay IDLE_CAMPED_NORMALLY

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:2100: - since carrier aggregation is not
supported in general, the following
"not supported by the LTE module"

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:2109: Overall design
This whole subsection requires further discussion. It's ok, as this belongs to
the Handover Algorithm interface design, which is expected to be the third and
final part of the GSoC.

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:2297: Custom handover algorithm
Also this part is to be rediscussed in the final part of the GSoC

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

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-testing...
src/lte/doc/source/lte-testing.rst:217: UE measurement configuration tests
Very good work with this documentation!

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

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-user.rs...
src/lte/doc/source/lte-user.rst:948: the eNodeB antenna direction should be
considered as well. Besides that, we
direction -> directivity

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-user.rs...
src/lte/doc/source/lte-user.rst:959: Attachment based on received signal
I would suggest to use this title:
"Automatic attachment with the Idle Mode Cell Selection procedure"

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-user.rs...
src/lte/doc/source/lte-user.rst:973: After the method is called, the UE will
spend 200 ms (by default) to measure the
I'd just say "will spend some time", the actual time depends on many factors
(not only measurements but also System Information reception).

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-user.rs...
src/lte/doc/source/lte-user.rst:981: An interesting use case of the initial cell
selection process is to setup a
This needs to be revisited after our recent discussion on the use of PLMN for
cell selection.

[buherman, 2013-08-02 15:51:17]

Hi Nicola and Marco,

Thanks a lot for your feedback. It took a while for me to go through all of
them, but finally made the fix to the code. More details are in my comments
below.

Regards,
-budi-

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

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:1969: Reception of system information is
detrimental to the transition of UE RRC
On 2013/07/29 11:11:39, Nicola Baldo wrote:
> I think "detrimental" is incorrect here. Maybe fundamental? 

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:1980: UE in `IDLE_CELL_SELECTION` state, e.g.
in the beginning of simulation, will
On 2013/07/29 11:11:39, Nicola Baldo wrote:
> at the beginning

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:2022: (based on RSRP) and connect to it. This
is done by issuing a contention-based
On 2013/07/29 11:11:39, Nicola Baldo wrote:
> I'd say no, connection will be done only if explicitly requested. Otherwise
the
> UE will stay IDLE_CAMPED_NORMALLY

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:2100: - since carrier aggregation is not
supported in general, the following
On 2013/07/29 11:11:39, Nicola Baldo wrote:
> "not supported by the LTE module"

Done.

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

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-user.rs...
src/lte/doc/source/lte-user.rst:948: the eNodeB antenna direction should be
considered as well. Besides that, we
On 2013/07/29 11:11:39, Nicola Baldo wrote:
> direction -> directivity

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-user.rs...
src/lte/doc/source/lte-user.rst:959: Attachment based on received signal
On 2013/07/29 11:11:39, Nicola Baldo wrote:
> I would suggest to use this title:
> "Automatic attachment with the Idle Mode Cell Selection procedure"

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-user.rs...
src/lte/doc/source/lte-user.rst:973: After the method is called, the UE will
spend 200 ms (by default) to measure the
On 2013/07/29 11:11:39, Nicola Baldo wrote:
> I'd just say "will spend some time", the actual time depends on many factors
> (not only measurements but also System Information reception).

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/doc/source/lte-user.rs...
src/lte/doc/source/lte-user.rst:981: An interesting use case of the initial cell
selection process is to setup a
On 2013/07/29 11:11:39, Nicola Baldo wrote:
> This needs to be revisited after our recent discussion on the use of PLMN for
> cell selection.

Done.

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

https://codereview.appspot.com/11670043/diff/1/src/lte/helper/lte-helper.cc#n...
src/lte/helper/lte-helper.cc:593: uePhy->CellSearch ();
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> instead of this, please have the RRC put the UePhy in CELL_SEARCH whenever
> needed, using a new LteUeCphySap::StartCellSearch () method

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/helper/lte-helper.cc#n...
src/lte/helper/lte-helper.cc:686: ueNas->Connect ();
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> Why do you need a new LteHelper::Connect(ueDev) method? ...can't you move
> everything into the LteHelper::Attach() method?

Sure, I'll combine them together.

In the future when we have Idle mode ready, I think we should have them
separated. And that's why I decided to separate them. But now when I think about
it again, it may cause unnecessary confusion to users.

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

https://codereview.appspot.com/11670043/diff/1/src/lte/helper/lte-helper.h#ne...
src/lte/helper/lte-helper.h:195: * Attach a set of UE devices to a single eNB
device
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> please add a comment here saying that the use of this method will disable the
> cell selection procedure

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/helper/lte-helper.h#ne...
src/lte/helper/lte-helper.h:199: */
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> also here

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/epc-ue-nas.cc
File src/lte/model/epc-ue-nas.cc (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/epc-ue-nas.cc#ne...
src/lte/model/epc-ue-nas.cc:136: // since RRC Idle Mode cell selection is not
supported yet, we
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> please update this comment

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/epc-ue-nas.h
File src/lte/model/epc-ue-nas.h (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/epc-ue-nas.h#new...
src/lte/model/epc-ue-nas.h:94: * attempt to connect at the earliest possible
time (e.g. after it camps to a
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> It's good to have this paragraph "If this function..." in the documentation,
but
> I think the appropriate place for it is LteAsSapProvider::Connect. Please move
> it there. 
> 
> For EpcUeNas::Connect(), it might suffice to say that (among other things) it
> causes the NAS to tell the Access Stratum (AS) to get connected.

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-common.h
File src/lte/model/lte-common.h (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-common.h#new...
src/lte/model/lte-common.h:224: static double GetActualHysteresis (uint8_t
hysteresis);
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> For consistency with the other methods in this file, I would suggest to rename
> this to IeValue2ActualHysteresis or something similar.
> 
> Not that I like very much the naming of the pre-existing methods here... feel
> free to propose a renaming for all of them if you have a better idea.

Done.

I found that X2Y and XToY patterns are the most common across ns-3. Well, for
now I'll stick with X2Y.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-common.h#new...
src/lte/model/lte-common.h:233: static double GetActualA3Offset (int8_t
a3Offset);
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> same as previous comment

Done.

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

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-enb-rrc.h#ne...
src/lte/model/lte-enb-rrc.h:852: LteRrcSap::MeasConfig m_ueMeasConfigList;
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> "List" in the name is a bit misleading, what about calling it just
> "m_ueMeasConfig"? 
> 
> I mean, the name would be ok if the type were std::list<MeasConfig>, but
that's
> not the case.
> 

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-enb-rrc.h#ne...
src/lte/model/lte-enb-rrc.h:857: uint8_t m_numOfUeMeasConfig;
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> The naming is misleading - if it were number of identities, it would be called
> m_numOfUeMeasId, and actually you use to count the number of MeasReportConfig,
> which in general is not the same as the number of MeasId.

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-net-devic...
File src/lte/model/lte-ue-net-device.cc (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-net-devic...
src/lte/model/lte-ue-net-device.cc:191: m_phy->SetDlEarfcn (earfcn);
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> The LteUeNetDevice shall not configure the PHY directly. Instead, the
> configuration of the PHY should be done by the RRC using the LteUeCphySap
> interface. 

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc
File src/lte/model/lte-ue-phy.cc (right):

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc#ne...
src/lte/model/lte-ue-phy.cc:428: LteUePhy::GenerateCtrlCqiReport (const
SpectrumValue& sinr)
On 2013/07/26 10:57:21, marco.miozzo wrote:
> GenerateCtrlCqiReport is called each time a PDCCH is received, how do you
manage
> with this change that the UE is not generating sporious CQIs (i.e., CQIs with
> m_rnti equal to 0 when the UE is not yet attached or even with different
> bandwidth respect to the eNB).

You're right, I didn't realize this. I added back ``if (m_dlConfigured &&
m_ulConfigured && (m_rnti > 0))`` before P10 and A30 CQI calculation.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc#ne...
src/lte/model/lte-ue-phy.cc:902: if (m_cellId == 0)
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> If you need this, I'd use "switch (m_state)" instead

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc#ne...
src/lte/model/lte-ue-phy.cc:906: * value is not available and
GenerateCtrlCqiReport will not be called.
On 2013/07/26 10:57:21, marco.miozzo wrote:
> I agree with Nicola, let's try to limit code duplication. The split proposed
by
> Nicola seems to be reasonable. 
> Moreover, RSRQ spans from -19.5 to -3 dB, by putting 0 you are using a not
valid
> value, which anyway will be truncated by UE RRC to -3. Thus, it will be
possibly
> used by the eNB RRC as a valid (good) values for its estimation in attaching
the
> UE and therefore compared with real RSRQ values.

Done as suggested.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.cc#ne...
src/lte/model/lte-ue-phy.cc:932: (*itMeasMap).second.rsrqNum++;
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> if RSRQ is not available, you should not increment this. Otherwise if RSRQ
> suddenly becomes available, the average will be incorrect.

Done.

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

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.h#new...
src/lte/model/lte-ue-phy.h:148: void SetDlEarfcn (uint16_t earfcn);
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> please make this an LteUeCphySap method

Done.

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-phy.h#new...
src/lte/model/lte-ue-phy.h:186: void CellSearch ();
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> please make this an LteUeCphySap method

Done.

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

https://codereview.appspot.com/11670043/diff/1/src/lte/model/lte-ue-rrc.cc#ne...
src/lte/model/lte-ue-rrc.cc:871: m_acceptableCell.insert (cellId);
On 2013/07/25 16:56:11, Nicola Baldo wrote:
> Frankly, I would not model the selection of acceptable cells at all - are you
> really interested in simulating emergency calls?

As I've explained in another review session, this variable is used in
SynchronizeToStrongestCell() to prevent UE from blindly retrying the same cell
again. The approach is to avoid retrying to cells which have been found to have
different CSG ID. This definition matches with 3GPP's definition of "acceptable
cell", hence the name of the variable. But by choosing this name I don't mean to
introduce functionality related to acceptable cell (i.e. no emergency call). I
updated the documentation to state this.