ns-3 GSoC 2013 UeMeasurementActiveIdle - Week 26

ns-3 GSoC 2013 UeMeasurementActiveIdle - Week 26

[Tom Henderson, 2013-06-28 18:18:03]

Great work so far!

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

https://codereview.appspot.com/10742043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:2119: whether the trigerring condition is
fulfilled in accordance with Section 5.5.4
triggering

https://codereview.appspot.com/10742043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:2120: of [TS36331]_. When at least one
trigerring condition from all the active
triggering

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-enb-rrc.cc#n...
src/lte/model/lte-enb-rrc.cc:1661: Simulator::Schedule (MilliSeconds (16),
&LteEnbRrc::SendSystemInformation, this);
where does '16' come from?

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-rrc-sap.h#ne...
src/lte/model/lte-rrc-sap.h:257: EVENT_A5 ///< Event A5: PCell becomes worse
than absolute `threshold1` AND Neighbour becomes better than another absolute
`threshold2`.
A general comment:  before I came upon this file, I had to search on the web for
definition of these events because it is kind of cryptic to a non-LTE expert. 
Here, you have defined what A1, A2, etc. are.

Can you update your wiki page (project plan) to define these events the first
time you reference them?

Also, when you send your weekly report to the list, I suggest that you elaborate
like:

 - addition of LTE measurement event A3 (Neighbour becomes offset better than
PCell)

instead of 

-  addition of Event A3 and A5;

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-rrc-sap.h#ne...
src/lte/model/lte-rrc-sap.h:320: ReportConfigEutra ()
perhaps this is a nit, but this constructor could be moved to the implementation
file.

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-ue-rrc.cc#ne...
src/lte/model/lte-ue-rrc.cc:701: //ms = EutranMeasurementMapping::QuantizeRsrp
(m_storedMeasValues[m_cellId].rsrp);
delete dead code?

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-ue-rrc.cc#ne...
src/lte/model/lte-ue-rrc.cc:855: double hys = (double)
reportConfigEutra.hysteresis * 0.5; // Hys, the hysteresis parameter for this
event. See 36.331 section 6.3.5 for the conversion.
recommend to use C++ static_cast in general, instead of C casts

[Nicola Baldo, 2013-07-01 11:44:04]

Overall, very good work! The effort in documenting the code is especially
appreciated. Please see some detailed comments below.

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-enb-rrc.cc#o...
src/lte/model/lte-enb-rrc.cc:1206: // Just intra-frequency measurements are
supported,
Is this functionality being moved somewhere else? I remember we had talked about
having a default UE measurement configuration...

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-enb-rrc.cc#o...
src/lte/model/lte-enb-rrc.cc:1502: .AddAttribute ("EventA2Threshold",
I understand that attributes like this would be moved to the class implementing
the handover algorithm, am I right?

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-enb-rrc.cc#n...
src/lte/model/lte-enb-rrc.cc:1661: Simulator::Schedule (MilliSeconds (16),
&LteEnbRrc::SendSystemInformation, this);
On 2013/06/28 18:18:03, Tom Henderson wrote:
> where does '16' come from?

whoops sorry for the magic number...
16ms is the periodicity of SIB2 according to 3GPP (the only SIB supported in the
latest LENA version). Actually if we start sending other SIBs (like SIB1 which I
think is needed in the GSoC) we should handle the difference in periodicity.

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-rrc-sap.h#ne...
src/lte/model/lte-rrc-sap.h:236: THRESHOLD_RSRP, ///< RSRP based threshold for
event evaluation. The actual value is (value - 140) dBm.
to be picky, the enum does not represent the threshold itself, but actually
whether RSRP or RSRQ is used for the threshold. I think the comment could be
misleading.

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-rrc-sap.h#ne...
src/lte/model/lte-rrc-sap.h:239: uint8_t range;
I would put the above comments on the value here. It would be good to refer to 
3GPP TS 36.133 section 9.1.4 and 9.1.7 here (if I remember correctly)

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-rrc-sap.h#ne...
src/lte/model/lte-rrc-sap.h:320: ReportConfigEutra ()
On 2013/06/28 18:18:03, Tom Henderson wrote:
> perhaps this is a nit, but this constructor could be moved to the
implementation
> file.

I agree, actually the ns-3 coding style is quite clear on this:

"Do not include inline implementations in header files; put all implementation
in a .cc file (unless implementation in the header file brings demonstrable and
significant performance improvement)."

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-ue-rrc.cc#ne...
src/lte/model/lte-ue-rrc.cc:701: //ms = EutranMeasurementMapping::QuantizeRsrp
(m_storedMeasValues[m_cellId].rsrp);
On 2013/06/28 18:18:03, Tom Henderson wrote:
> delete dead code?

fine with me, please delete the same line within the other events.

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-ue-rrc.cc#ne...
src/lte/model/lte-ue-rrc.cc:855: double hys = (double)
reportConfigEutra.hysteresis * 0.5; // Hys, the hysteresis parameter for this
event. See 36.331 section 6.3.5 for the conversion.
On 2013/06/28 18:18:03, Tom Henderson wrote:
> recommend to use C++ static_cast in general, instead of C casts

I suggest to move the conversion to a dedicated method, possibly in
lte-common.cc. See for instance EutranMeasurementMapping::RsrpRange2Dbm and
similar methods.

https://codereview.appspot.com/10742043/diff/1/src/lte/test/lte-test-ue-measu...
File src/lte/test/lte-test-ue-measurements.cc (right):

https://codereview.appspot.com/10742043/diff/1/src/lte/test/lte-test-ue-measu...
src/lte/test/lte-test-ue-measurements.cc:122: //RunOriginalTestCase ();
Two comments:

1) "Run" here is misnamed, since the tests are not expected to be run in teh
constructor of the test suite. They are just created here.

2) You already clearly defined different sets of test cases. Then, why don't you
just declare different test suites, one for each RunXxxx method, and move the
content of that method to the TestSuite constructor? I think the implementation
would be much more in line with the TestCase/TestSuite API, and you would get
the additional benefit of being able to run each set of tests selectively from
the command line.

https://codereview.appspot.com/10742043/diff/1/src/lte/test/lte-test-ue-measu...
src/lte/test/lte-test-ue-measurements.cc:684: // Setup pre-GSOC UE measurement
configuration to the eNodeBs
I think that as time passes the meaning of "pre-GSOC" will become unclear.
Instead, I'd suggest to mention simply that the test vector in this case is
determined based on the specific UE measurement configuration given below.

https://codereview.appspot.com/10742043/diff/1/src/lte/test/test-lte-x2-hando...
File src/lte/test/test-lte-x2-handover-measures.cc (right):

https://codereview.appspot.com/10742043/diff/1/src/lte/test/test-lte-x2-hando...
src/lte/test/test-lte-x2-handover-measures.cc:225: // Setup pre-GSOC UE
measurement configuration to the eNodeBs
same observation as in lte-test-ue-measurements.cc: just mention that the test
vector is valid for the particular UE measurement config below.

[Nicola Baldo, 2013-07-02 15:01:22]

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-enb-rrc.cc#n...
src/lte/model/lte-enb-rrc.cc:1661: Simulator::Schedule (MilliSeconds (16),
&LteEnbRrc::SendSystemInformation, this);
On 2013/07/01 11:44:04, Nicola Baldo wrote:
> On 2013/06/28 18:18:03, Tom Henderson wrote:
> > where does '16' come from?
> 
> whoops sorry for the magic number...
> 16ms is the periodicity of SIB2 according to 3GPP (the only SIB supported in
the
> latest LENA version). Actually if we start sending other SIBs (like SIB1 which
I
> think is needed in the GSoC) we should handle the difference in periodicity.

actually it's not even the periodicity, it's just an arbitrary time for "the
first time System Information is sent", as written in the existing comment. 

In other words, this command just schedules the first transmission; subsequent
transmissions are periodic according to m_systemInformationPeriodicity

[buherman, 2013-07-03 12:51:19]

Hi Tom and Nicola,

Thanks a lot for your review. I've updated the code based on your feedback.
Please check comments below.

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

https://codereview.appspot.com/10742043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:2119: whether the trigerring condition is
fulfilled in accordance with Section 5.5.4
On 2013/06/28 18:18:03, Tom Henderson wrote:
> triggering

Done.

https://codereview.appspot.com/10742043/diff/1/src/lte/doc/source/lte-design....
src/lte/doc/source/lte-design.rst:2120: of [TS36331]_. When at least one
trigerring condition from all the active
On 2013/06/28 18:18:03, Tom Henderson wrote:
> triggering

Done.

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-enb-rrc.cc#o...
src/lte/model/lte-enb-rrc.cc:1206: // Just intra-frequency measurements are
supported,
On 2013/07/01 11:44:04, Nicola Baldo wrote:
> Is this functionality being moved somewhere else? I remember we had talked
about
> having a default UE measurement configuration...

My understanding is that UE measurements configuration is completely dependent
on the existence of consumers behind the eNodeB; i.e. if there is no consumer,
there should be no UE measurements at all.

However, eNodeB will have default consumer(s), e.g. a default handover
algorithm. Therefore will be default UE measurements configuration too (assuming
that default consumer is requesting some UE measurements).

Thus, I think BuildMeasConfig can be safely removed (albeit with some impact to
the existing test cases and examples, which I've already fixed with temporary
workaround).

Now that I've stumbled upon an NRT function that depends on the default Event A4
that was setup by BuildMeasConfig. I suppose I should convert this NRT function
to some sort of default handover algorithm too. Let's further discuss about this
in our next conference call.

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-enb-rrc.cc#o...
src/lte/model/lte-enb-rrc.cc:1502: .AddAttribute ("EventA2Threshold",
On 2013/07/01 11:44:04, Nicola Baldo wrote:
> I understand that attributes like this would be moved to the class
implementing
> the handover algorithm, am I right?

Yup.

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-enb-rrc.cc#n...
src/lte/model/lte-enb-rrc.cc:1661: Simulator::Schedule (MilliSeconds (16),
&LteEnbRrc::SendSystemInformation, this);
On 2013/07/02 15:01:22, Nicola Baldo wrote:
> On 2013/07/01 11:44:04, Nicola Baldo wrote:
> > On 2013/06/28 18:18:03, Tom Henderson wrote:
> > > where does '16' come from?
> > 
> > whoops sorry for the magic number...
> > 16ms is the periodicity of SIB2 according to 3GPP (the only SIB supported in
> the
> > latest LENA version). Actually if we start sending other SIBs (like SIB1
which
> I
> > think is needed in the GSoC) we should handle the difference in periodicity.
> 
> actually it's not even the periodicity, it's just an arbitrary time for "the
> first time System Information is sent", as written in the existing comment. 
> 
> In other words, this command just schedules the first transmission; subsequent
> transmissions are periodic according to m_systemInformationPeriodicity
> 
> 

Updated the comments to clarify.

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-rrc-sap.h#ne...
src/lte/model/lte-rrc-sap.h:236: THRESHOLD_RSRP, ///< RSRP based threshold for
event evaluation. The actual value is (value - 140) dBm.
On 2013/07/01 11:44:04, Nicola Baldo wrote:
> to be picky, the enum does not represent the threshold itself, but actually
> whether RSRP or RSRQ is used for the threshold. I think the comment could be
> misleading.

It was my mistake when copy-pasting from 3GPP. Now fixed.

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-rrc-sap.h#ne...
src/lte/model/lte-rrc-sap.h:239: uint8_t range;
On 2013/07/01 11:44:04, Nicola Baldo wrote:
> I would put the above comments on the value here. It would be good to refer to

> 3GPP TS 36.133 section 9.1.4 and 9.1.7 here (if I remember correctly)

Done.

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-rrc-sap.h#ne...
src/lte/model/lte-rrc-sap.h:257: EVENT_A5 ///< Event A5: PCell becomes worse
than absolute `threshold1` AND Neighbour becomes better than another absolute
`threshold2`.
On 2013/06/28 18:18:03, Tom Henderson wrote:
> A general comment:  before I came upon this file, I had to search on the web
for
> definition of these events because it is kind of cryptic to a non-LTE expert. 
> Here, you have defined what A1, A2, etc. are.
> 
> Can you update your wiki page (project plan) to define these events the first
> time you reference them?
> 
> Also, when you send your weekly report to the list, I suggest that you
elaborate
> like:
> 
>  - addition of LTE measurement event A3 (Neighbour becomes offset better than
> PCell)
> 
> instead of 
> 
> -  addition of Event A3 and A5; 

Done.

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-rrc-sap.h#ne...
src/lte/model/lte-rrc-sap.h:320: ReportConfigEutra ()
On 2013/06/28 18:18:03, Tom Henderson wrote:
> perhaps this is a nit, but this constructor could be moved to the
implementation
> file.

Done.

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

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-ue-rrc.cc#ne...
src/lte/model/lte-ue-rrc.cc:701: //ms = EutranMeasurementMapping::QuantizeRsrp
(m_storedMeasValues[m_cellId].rsrp);
On 2013/07/01 11:44:04, Nicola Baldo wrote:
> On 2013/06/28 18:18:03, Tom Henderson wrote:
> > delete dead code?
> 
> fine with me, please delete the same line within the other events.

Done.

https://codereview.appspot.com/10742043/diff/1/src/lte/model/lte-ue-rrc.cc#ne...
src/lte/model/lte-ue-rrc.cc:855: double hys = (double)
reportConfigEutra.hysteresis * 0.5; // Hys, the hysteresis parameter for this
event. See 36.331 section 6.3.5 for the conversion.
On 2013/07/01 11:44:04, Nicola Baldo wrote:
> On 2013/06/28 18:18:03, Tom Henderson wrote:
> > recommend to use C++ static_cast in general, instead of C casts
> 
> I suggest to move the conversion to a dedicated method, possibly in
> lte-common.cc. See for instance EutranMeasurementMapping::RsrpRange2Dbm and
> similar methods. 
> 
> 
> 

Created new static functions in EutranMeasurementMapping for hysteresis and
a3Offset, using static_cast as well. In addition, the functions implement input
validation as per 3GPP, e.g. [0..30] for hysteresis and [-30..30] for a3Offset.

https://codereview.appspot.com/10742043/diff/1/src/lte/test/lte-test-ue-measu...
File src/lte/test/lte-test-ue-measurements.cc (right):

https://codereview.appspot.com/10742043/diff/1/src/lte/test/lte-test-ue-measu...
src/lte/test/lte-test-ue-measurements.cc:122: //RunOriginalTestCase ();
On 2013/07/01 11:44:04, Nicola Baldo wrote:
> Two comments:
> 
> 1) "Run" here is misnamed, since the tests are not expected to be run in teh
> constructor of the test suite. They are just created here.
> 
> 2) You already clearly defined different sets of test cases. Then, why don't
you
> just declare different test suites, one for each RunXxxx method, and move the
> content of that method to the TestSuite constructor? I think the
implementation
> would be much more in line with the TestCase/TestSuite API, and you would get
> the additional benefit of being able to run each set of tests selectively from
> the command line. 

And I should have uncommented those lines before committing. They are real code,
but I commented them because of my own testing. Anyway, I've moved them to
multiple test suites as you suggested.

https://codereview.appspot.com/10742043/diff/1/src/lte/test/lte-test-ue-measu...
src/lte/test/lte-test-ue-measurements.cc:684: // Setup pre-GSOC UE measurement
configuration to the eNodeBs
On 2013/07/01 11:44:04, Nicola Baldo wrote:
> I think that as time passes the meaning of "pre-GSOC" will become unclear.
> Instead, I'd suggest to mention simply that the test vector in this case is
> determined based on the specific UE measurement configuration given below.

This is the temporary workaround that I've mentioned in another comments. This
chunk of code will be removed once handover algorithm is implemented.