ns-3 GSoC 2013 (LTE) - Final code review

Hi,

This is the final deliverables of the ns-3 GSoC 2013 project, LTE UE measurements. The objective of this code review is to let the ns-3 community review the code and provide some feedback.

This issue contains a collection of self-contained patch sets, which is a subset of the full changes. To keep each patch set short and straight to the point, I only include the changes that I deem essential and worth reviewing. Thus, these patch sets most likely won't work when you download them. If you're interested with the full list of changes, or need a working code, please refer to another code review issue [1], which is intended for merging purpose.

Each patch set contains a GSOC_README.txt file which has a brief introduction to the content of the patch set. There are 4 patch sets:
 - Patch Set 1: Models - UE measurements
 - Patch Set 2: Models - Initial cell selection
 - Patch Set 3: Models - ANR and handover algorithm
 - Patch Set 4: Tests, examples, and Sphinx documentation

=== UPDATE (23rd October 2013). Added Patch Set 5, containing only the updates from the provided feedback.

Note: the compiled Sphinx documentation is also available for review, e.g., to see the figures and formatting. It is provided in HTML [2] and PDF [3] formats. These links lead to the whole ns-3 Model Library documentation, which is very large, so please jump to the related sections, as listed below:
 - Design Documentation > RRC > Initial Cell Selection
 - Design Documentation > RRC > UE RRC Measurements Model
 - Design Documentation > RRC > Handover
 - Design Documentation > RRC > Neighbour Relation
 - Design Documentation > X2 (only the part mentioning handover)
 - User Documentation > Network Attachment (about initial cell selection)
 - User Documentation > Configure UE measurements
 - User Documentation > X2-based handover
 - User Documentation > Reference scenarios > Handover simulation campaign
 - Testing Documentation > UE measurement configuration tests
 - Testing Documentation > Initial cell selection
 - Testing Documentation > Selection of target cell in handover algorithm

Comments and feedback are very much appreciated.

Regards,
-budi-

[1] https://codereview.appspot.com/13728043/
[2] http://www.nsnam.org/~buherman/sphinx_gsoc2013/html/lte.html
[3] http://www.nsnam.org/~buherman/sphinx_gsoc2013/latex/ns-3-model-library.pdf

[bpswenson, 2013-09-25 17:30:51]

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.cc
File src/lte/model/lte-common.cc (right):

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:245: double
This method and others:  No use of NS_LOG_FUNCTION

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:259:
EutranMeasurementMapping::ActualHysteresis2IeValue (double hysteresisDb)
Personally I think it's better to use To instead of 2 in method names.  I think
it reads better and it's not like there is a character limit and the savings of
1 char is vital.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:260: {
If hysteresisDb is 15 which will pass this check, ieValue will be 30 which will
fail the NS_ASSERT below.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:288:
EutranMeasurementMapping::ActualA3Offset2IeValue (double a3OffsetDb)
The same thing here.  You're checking to make sure a value is within a specific
range, multiplying the value by 2 and then checking that the result fits into
the same range again.

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

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-enb-rrc....
src/lte/model/lte-enb-rrc.cc:1582: LteEnbRrc::AddUeMeasReportConfig
(LteRrcSap::ReportConfigEutra config)
NS_LOG_FUNCTION

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

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:474: VarMeasReportListClear
(measIdIt->second.measId);
This isn't specific to this particular patch however the use of meas as an
abbreviation to measurement seems to violate this part of the coding standard:

A long descriptive name which requires a lot of typing is always better than a
short name which is hard to decipher. Do not use abbreviations in names unless
the abbreviation is really unambiguous and obvious to everyone (e.g., use "size"
over "sz").

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:1168: NS_LOG_LOGIC (this << " CellId " << cellId <<
" RSRP " << rsrp << " RSRQ " << rsrq);
Should be NS_LOG_FUNCTION

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:1215: LteUeRrc::MeasurementReportTriggering (uint8_t
measId)
NS_LOG_FUNCTION for this and other class methods

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h
File src/lte/model/lte-ue-rrc.h (right):

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h...
src/lte/model/lte-ue-rrc.h:391: void VarMeasReportListAdd (uint8_t measId,
ConcernedCells_t enteringCells);
Comment methods for doxygen

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h...
src/lte/model/lte-ue-rrc.h:418: void CancelEnteringTrigger (uint8_t measId);
Comment methods for doxygen

[bpswenson, 2013-09-25 18:51:13]

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

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/epc-ue-nas.c...
src/lte/model/epc-ue-nas.cc:106: EpcUeNas::SetCsgId (uint32_t csgId)
Need NS_LOG_FUNCTION?

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/epc-ue-nas.c...
src/lte/model/epc-ue-nas.cc:238: // TODO
TODO what?

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

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/lte-common.h...
src/lte/model/lte-common.h:224: static double IeValue2ActualHysteresis (uint8_t
hysteresis);
Again I think this and other similar method names here should be
IeValueToActualHysteresis instead of 2.  Per the ns3 coding standard.

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/lte-control-...
File src/lte/model/lte-control-messages.h (right):

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/lte-control-...
src/lte/model/lte-control-messages.h:470: 
Is there a reason why every class in this header file has its own set of include
guards?

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

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/lte-ue-phy.h...
src/lte/model/lte-ue-phy.h:227: State GetState ();
const?  GetState doesn't modify this pointer

[bpswenson, 2013-09-25 19:55:04]

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
File src/lte/model/a2-a4-rsrq-handover-algorithm.cc (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
src/lte/model/a2-a4-rsrq-handover-algorithm.cc:228: // TODO
TODO what?

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
File src/lte/model/a2-a4-rsrq-handover-algorithm.h (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
src/lte/model/a2-a4-rsrq-handover-algorithm.h:103: // TODO instead of class, try
using struct or SimpleRefCount
See:
http://www.nsnam.org/docs/release/3.18/manual/html/object-model.html

Since you're not using object aggregation or the attribute system for this
class, simpleRefCount may be better.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a3-rsrp-hand...
File src/lte/model/a3-rsrp-handover-algorithm.cc (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a3-rsrp-hand...
src/lte/model/a3-rsrp-handover-algorithm.cc:178: // TODO
TODO what?

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a3-rsrp-hand...
File src/lte/model/a3-rsrp-handover-algorithm.h (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a3-rsrp-hand...
src/lte/model/a3-rsrp-handover-algorithm.h:84: LteHandoverManagementSapUser*
m_handoverManagementSapUser;
Can/should these be wrapped in smartpointers?

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr-sap.h
File src/lte/model/lte-anr-sap.h (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr-sap....
src/lte/model/lte-anr-sap.h:65: virtual bool GetNoRemove (uint16_t cellId) = 0;
const method?

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr-sap....
src/lte/model/lte-anr-sap.h:84: virtual bool GetNoX2 (uint16_t cellId) = 0;
const method?

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.cc
File src/lte/model/lte-anr.cc (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.cc#n...
src/lte/model/lte-anr.cc:129: NS_LOG_LOGIC (this << " requesting Event A4
measurements"
NS_LOG_FUNCTION

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.cc#n...
src/lte/model/lte-anr.cc:209: LteAnr::DoAddNeighbourRelation (uint16_t cellId)
NS_LOG_FUNCTION

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.h
File src/lte/model/lte-anr.h (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.h#ne...
src/lte/model/lte-anr.h:105: LteAnrSapUser* m_anrSapUser;
Wrap with smartpointers?

[buherman, 2013-09-26 07:02:18]

https://codereview.appspot.com/13720052/diff/53001/src/lte/test/lte-test-cell...
File src/lte/test/lte-test-cell-selection.cc (right):

https://codereview.appspot.com/13720052/diff/53001/src/lte/test/lte-test-cell...
src/lte/test/lte-test-cell-selection.cc:165: Config::SetGlobal ("RngRun",
IntegerValue (m_rngRun));
One question. Is it actually safe to change RngRun from within a test case? Will
it also change the RngRun of the other test cases that will run after this test
case?

[buherman, 2013-09-26 11:42:35]

Hi Brian,

Thanks for your comments. I've worked on some of them, as listed below.

-budi-

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.cc
File src/lte/model/lte-common.cc (right):

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:245: double
On 2013/09/25 17:30:51, bpswenson wrote:
> This method and others:  No use of NS_LOG_FUNCTION

This is actually a question that bothers me. There are many existing methods in
ns-3 which don't have NS_LOG_FUNCTION. Is there a rule of thumb used by
developers for using or not to using NS_LOG_FUNCTION?

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:259:
EutranMeasurementMapping::ActualHysteresis2IeValue (double hysteresisDb)
On 2013/09/25 17:30:51, bpswenson wrote:
> Personally I think it's better to use To instead of 2 in method names.  I
think
> it reads better and it's not like there is a character limit and the savings
of
> 1 char is vital.

It seems that the "X2Y" convention has been more commonly used in LTE module,
while other ns-3 modules are more toward "XToY" format.

Me and my mentors have been considering to change this. I've identified 21 of
such methods, which are used in different places in LTE module. I'd prefer to
keep this change outside of the GSoC scope, so later I'll post a new issue in
Bugzilla on this, and address it after GSoC ends.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:260: {
On 2013/09/25 17:30:51, bpswenson wrote:
> If hysteresisDb is 15 which will pass this check, ieValue will be 30 which
will
> fail the NS_ASSERT below.

Done.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:288:
EutranMeasurementMapping::ActualA3Offset2IeValue (double a3OffsetDb)
On 2013/09/25 17:30:51, bpswenson wrote:
> The same thing here.  You're checking to make sure a value is within a
specific
> range, multiplying the value by 2 and then checking that the result fits into
> the same range again.

Done.

Yes, exactly, this is a stupid mistake from me. I decided to remove the
NS_ASSERT, because I think they are useless.

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

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-enb-rrc....
src/lte/model/lte-enb-rrc.cc:1582: LteEnbRrc::AddUeMeasReportConfig
(LteRrcSap::ReportConfigEutra config)
On 2013/09/25 17:30:51, bpswenson wrote:
> NS_LOG_FUNCTION

Done.

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

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:474: VarMeasReportListClear
(measIdIt->second.measId);
On 2013/09/25 17:30:51, bpswenson wrote:
> This isn't specific to this particular patch however the use of meas as an
> abbreviation to measurement seems to violate this part of the coding standard:
> 
> A long descriptive name which requires a lot of typing is always better than a
> short name which is hard to decipher. Do not use abbreviations in names unless
> the abbreviation is really unambiguous and obvious to everyone (e.g., use
"size"
> over "sz").

I agree with your point. However, this one and also some other cryptic
abbreviations (e.g. "AddMod", "Var") are defined this way in 3GPP specification.
As far as I know, the specification doesn't expand these abbreviations. So maybe
it's not appropriate if we come up with our own expansion (even though I know
for sure that "meas" stands for "measurements"). Just my two cents.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:1168: NS_LOG_LOGIC (this << " CellId " << cellId <<
" RSRP " << rsrp << " RSRQ " << rsrq);
On 2013/09/25 17:30:51, bpswenson wrote:
> Should be NS_LOG_FUNCTION

Done.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:1215: LteUeRrc::MeasurementReportTriggering (uint8_t
measId)
On 2013/09/25 17:30:51, bpswenson wrote:
> NS_LOG_FUNCTION for this and other class methods

Done.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:1961: {
Added NS_LOG_FUNCTION here.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:1983: {
Added NS_LOG_FUNCTION here.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:2019: {
Added NS_LOG_FUNCTION here.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:2041: {
Added NS_LOG_FUNCTION here.

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

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/epc-ue-nas.c...
src/lte/model/epc-ue-nas.cc:106: EpcUeNas::SetCsgId (uint32_t csgId)
On 2013/09/25 18:51:14, bpswenson wrote:
> Need NS_LOG_FUNCTION?

Done here and in all the other methods.

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/epc-ue-nas.c...
src/lte/model/epc-ue-nas.cc:238: // TODO
On 2013/09/25 18:51:14, bpswenson wrote:
> TODO what?

Done. It now reads:
/**
 * \todo Currently not implemented, action by NAS and upper layers after UE
 *       fails to switch to CONNNECTED mode. Maybe a retry, or just stop here
 *       and fire a trace to let user know.
 */

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

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/lte-ue-phy.h...
src/lte/model/lte-ue-phy.h:227: State GetState ();
On 2013/09/25 18:51:14, bpswenson wrote:
> const?  GetState doesn't modify this pointer

Done here and also in other GetState in LteEnbRrc, LteUePhy, and EpcUeNas.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
File src/lte/model/a2-a4-rsrq-handover-algorithm.cc (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
src/lte/model/a2-a4-rsrq-handover-algorithm.cc:228: // TODO
On 2013/09/25 19:55:04, bpswenson wrote:
> TODO what?

Done. It now reads:
/**
 * \todo In the future, this function can be expanded to validate whether the
 *       neighbour cell is a valid target cell, e.g., taking into account the
 *       NRT in ANR and whether it is a CSG cell with closed access.
 */

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a3-rsrp-hand...
File src/lte/model/a3-rsrp-handover-algorithm.cc (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a3-rsrp-hand...
src/lte/model/a3-rsrp-handover-algorithm.cc:178: // TODO
On 2013/09/25 19:55:04, bpswenson wrote:
> TODO what?

Done. It now reads:
/**
 * \todo In the future, this function can be expanded to validate whether the
 *       neighbour cell is a valid target cell, e.g., taking into account the
 *       NRT in ANR and whether it is a CSG cell with closed access.
 */

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a3-rsrp-hand...
File src/lte/model/a3-rsrp-handover-algorithm.h (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a3-rsrp-hand...
src/lte/model/a3-rsrp-handover-algorithm.h:84: LteHandoverManagementSapUser*
m_handoverManagementSapUser;
On 2013/09/25 19:55:04, bpswenson wrote:
> Can/should these be wrapped in smartpointers?

Honestly I don't know. All the other SAP interfaces in LTE module are written
this way.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr-sap.h
File src/lte/model/lte-anr-sap.h (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr-sap....
src/lte/model/lte-anr-sap.h:65: virtual bool GetNoRemove (uint16_t cellId) = 0;
On 2013/09/25 19:55:04, bpswenson wrote:
> const method?

Done.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr-sap....
src/lte/model/lte-anr-sap.h:74: virtual bool GetNoHo (uint16_t cellId) = 0;
Also added const here.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr-sap....
src/lte/model/lte-anr-sap.h:84: virtual bool GetNoX2 (uint16_t cellId) = 0;
On 2013/09/25 19:55:04, bpswenson wrote:
> const method?

Done.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.cc
File src/lte/model/lte-anr.cc (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.cc#n...
src/lte/model/lte-anr.cc:129: NS_LOG_LOGIC (this << " requesting Event A4
measurements"
On 2013/09/25 19:55:04, bpswenson wrote:
> NS_LOG_FUNCTION

Done.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.cc#n...
src/lte/model/lte-anr.cc:209: LteAnr::DoAddNeighbourRelation (uint16_t cellId)
On 2013/09/25 19:55:04, bpswenson wrote:
> NS_LOG_FUNCTION

Done here and in all the other methods.

[buherman, 2013-09-26 11:49:30]

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a3-rsrp-hand...
File src/lte/model/a3-rsrp-handover-algorithm.cc (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a3-rsrp-hand...
src/lte/model/a3-rsrp-handover-algorithm.cc:37: {
Added NS_LOG_FUNCTION here and in all the other methods.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/no-op-handov...
File src/lte/model/no-op-handover-algorithm.cc (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/no-op-handov...
src/lte/model/no-op-handover-algorithm.cc:34: {
Added NS_LOG_FUNCTION here and in all the other methods.

[Nicola Baldo, 2013-10-04 12:25:01]

I went through the code. The major issue to be fixed is the Helper API issue for
the CsgId and QRxLev settings. The other issues are minor, see the detailed
comments.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.cc
File src/lte/model/lte-common.cc (right):

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:259:
EutranMeasurementMapping::ActualHysteresis2IeValue (double hysteresisDb)
I agree with handling this as a LTE-wide bug later.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:288:
EutranMeasurementMapping::ActualA3Offset2IeValue (double a3OffsetDb)
I'd leave the assertions there. I don't think that they are useless.

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

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.c...
src/lte/model/lte-ue-rrc.cc:474: VarMeasReportListClear
(measIdIt->second.measId);
I agree with Budi, this abbreviation is defined by 3GPP, so let's stick with it

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h
File src/lte/model/lte-ue-rrc.h (right):

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h...
src/lte/model/lte-ue-rrc.h:46: * This delay exists because of racing condition
between several UE measurements
I don't understand well this issue of the race condition and the proposed
solution:

- the layer-1 measurements are passed to the RRC all at once upon call to
LteUeRrc::DoReportUeMeasurements

- layer 3 filtering is always performed within LteUeRrc::DoReportUeMeasurements,
i.e., upon receiving the new layer-1 measurements

- I agree that there is a race condition with time-to-trigger. However, what
does delaying the report improve? Wouldn't you get just another race condition
at the later time you send the report? Why can't we live with such race
condition?

Note that if there are strong arguments I would be fine with keeping this, I
just want to verify these arguments.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h...
src/lte/model/lte-ue-rrc.h:55: static const Time UE_MEASUREMENTS_DELAY =
MicroSeconds (1);
You use this only for measurement reports, right? If so, please rename to
USE_MEASUREMENT_REPORT_DELAY

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

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:854: for (NetDeviceContainer::Iterator i =
enbDevices.Begin ();
this method should be deleted, see my comment in lte-helper.h

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:873: Ptr<LteEnbRrc> rrc = enbLteDevice->GetRrc ();
Please move this code into a new method LteEnbRrc::SetCsgId (uint32_t csgId)

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:883: for (NetDeviceContainer::Iterator i =
enbDevices.Begin ();
this method should be deleted, see my comment in lte-helper.h

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:902: LteRrcSap::SystemInformationBlockType1 sib1 =
rrc->GetSystemInformationBlockType1 ();
Please move this code into a new method LteEnbRrc::SetQRxLevMin (uint32_t
csgId).

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:911: i != ueDevices.End (); ++i)
this method should be deleted, see my comment in lte-helper.h

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:928: ueLteDevice->GetNas ()->SetCsgId (csgId);
I suggest that the LteUeNetDevice should have a CsgId attribute, and its setter
should be in charge of communicating the CsgId to the NAS

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

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.h:386: void SetEnbCsgId (NetDeviceContainer
enbDevices, uint32_t csgId,
This approach of the various Set{Enb,Ue}{CsgId,QrxLev} (...) methods is not
consistent with the usual Helper API. 

The regular approach to apply a setting to multiple eNB or UE devices is this
one:

NodeContainer enbNodes;
enbNodes.Create (10);
lteHelper->SetEnbDeviceAttribute ("CsgId", UintegerValue (4));
lteHelper->SetEnbDeviceAttribute ("QRxLevMin", UintegerValue (2));
NetDeviceContainer enbDevs  = lteHelper->InstallEnbDevice (homeEnbs);

Please remove the newly added setter methods and use Set{Enb,Ue}DeviceAttribute
instead for the CsgId setting.

As for QRxLevMin, since it is clearly an RRC detail, I suggest to just make it
an attribute of LteEnbRrc. I expect that people would just set it globally with
Config::SetDefault (and related means). If you wish, you could add a
LteHelper::SetEnbRrcAttribute() to allow an alternative means for setting this
(and other) RRC attribute - we currently have a similar approach for e.g.
scheduler attributes.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
File src/lte/model/a2-a4-rsrq-handover-algorithm.cc (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
src/lte/model/a2-a4-rsrq-handover-algorithm.cc:18: * Author: Budiarto Herman
<budiarto.herman@magister.fi>
This file includes the code of the pre-existent handover algorithm, right? then
you should include the original copyright and authors.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
File src/lte/model/a2-a4-rsrq-handover-algorithm.h (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
src/lte/model/a2-a4-rsrq-handover-algorithm.h:103: // TODO instead of class, try
using struct or SimpleRefCount
I agree

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.h
File src/lte/model/lte-anr.h (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.h#ne...
src/lte/model/lte-anr.h:49: virtual void DoDispose (void);
I think DoDispose should be protected, like DoInitialize

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.h#ne...
src/lte/model/lte-anr.h:105: LteAnrSapUser* m_anrSapUser;
On 2013/09/25 19:55:04, bpswenson wrote:
> Wrap with smartpointers?

I think this is not necessary: SAP instances have exactly the same lifetime as
the objects to which they provide access, so it's straightforward to construct
and destroy them.

Note that there are many similar SAPs in the pre-existing LTE code, so
eventually, if you believe that smart pointers are a must, this should be
treated as an LTE-wide issue. I wouldn't consider it for the merge of the GSoC
code anyway.

[buherman, 2013-10-06 18:54:30]

Hi All,

Thanks a lot for the review. Well, I haven't got the time to process all of
them. But let me first reply to these 2 comments (please see inline below).

Regards,
-budi-

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h
File src/lte/model/lte-ue-rrc.h (right):

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h...
src/lte/model/lte-ue-rrc.h:46: * This delay exists because of racing condition
between several UE measurements
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> I don't understand well this issue of the race condition and the proposed
> solution:
> 
> - the layer-1 measurements are passed to the RRC all at once upon call to
> LteUeRrc::DoReportUeMeasurements
> 
> - layer 3 filtering is always performed within
LteUeRrc::DoReportUeMeasurements,
> i.e., upon receiving the new layer-1 measurements
> 
> - I agree that there is a race condition with time-to-trigger. However, what
> does delaying the report improve? Wouldn't you get just another race condition
> at the later time you send the report? Why can't we live with such race
> condition?
> 
> Note that if there are strong arguments I would be fine with keeping this, I
> just want to verify these arguments.

At that time when I discovered the racing condition, I concluded that adding a
delay would be the best solution. I'm not aware about another racing condition
that can occur at the later time that you pointed out.

Well, to start with, I added a new section at the end of the project's Wiki to
explain my thought on this issue. Please refer to
https://www.nsnam.org/wiki/index.php/GSOC2013UeMeasurementActiveIdle#Delay_in...

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h...
src/lte/model/lte-ue-rrc.h:55: static const Time UE_MEASUREMENTS_DELAY =
MicroSeconds (1);
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> You use this only for measurement reports, right? If so, please rename to
> USE_MEASUREMENT_REPORT_DELAY

Yes, that would be a better name. But, did you mean
"UE_MEASUREMENT_REPORT_DELAY"?

[buherman, 2013-10-06 18:54:44]

Hi All,

Thanks a lot for the review. Well, I haven't got the time to process all of
them. But let me first reply to these 2 comments (please see inline below).

Regards,
-budi-

[Nicola Baldo, 2013-10-10 09:57:04]

Hi Budi,


On 2013/10/06 18:54:30, buherman wrote:
> At that time when I discovered the racing condition, I concluded that adding a
> delay would be the best solution. I'm not aware about another racing condition
> that can occur at the later time that you pointed out.
> 
> Well, to start with, I added a new section at the end of the project's Wiki to
> explain my thought on this issue. Please refer to
>
https://www.nsnam.org/wiki/index.php/GSOC2013UeMeasurementActiveIdle#Delay_in...


Thanks for the very nice and explanatory figure!

Thinking more about it, the other racing condition would only occur if the
reportInterval could take any value. But since it is limited to multiples of
120ms, if you use a delay of 1ms, there will be no racing condition. 

As a side note, I think there is a race condition also in the first case in the
figure (120ms report interval). The fact is that the ns-3 scheduler does not
give any guarantee on the order of execution of events scheduled at the same
time. What probably happens is that with the default scheduler, because of the
order in which events are scheduled, the 120ms case happens to be executed in
the good order, while the 240ms happens to be executed with another one. Maybe
with a non-default scheduler the behavior would change.

Anyway your solution addresses both cases, so after this discussion I agree in
keeping it.



> 
>
https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h...
> src/lte/model/lte-ue-rrc.h:55: static const Time UE_MEASUREMENTS_DELAY =
> MicroSeconds (1);
> On 2013/10/04 12:25:02, Nicola Baldo wrote:
> > You use this only for measurement reports, right? If so, please rename to
> > USE_MEASUREMENT_REPORT_DELAY
> 
> Yes, that would be a better name. But, did you mean
> "UE_MEASUREMENT_REPORT_DELAY"?



yes sure, sorry for the typo.

[buherman, 2013-10-23 15:37:51]

Hi All,

Thanks for the feedback. I've finally completed implementing them to the
repository. Patch Set 5 is now posted, which contains the changes related to the
feedback.

I've also merged the GSoC repository and ns-3-dev again. The result is in the
other code review: https://codereview.appspot.com/13728043/.

Aside to Nicola, could you please double check lte-test-phy-error-model.cc? The
test passes, but there were plenty of conflicts when I merged it. The one posted
here is my proposed merge.

Regards,
-budi-

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.cc
File src/lte/model/lte-common.cc (right):

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-common.c...
src/lte/model/lte-common.cc:288:
EutranMeasurementMapping::ActualA3Offset2IeValue (double a3OffsetDb)
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> I'd leave the assertions there. I don't think that they are useless.

No problem. I've put them back.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h
File src/lte/model/lte-ue-rrc.h (right):

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h...
src/lte/model/lte-ue-rrc.h:391: void VarMeasReportListAdd (uint8_t measId,
ConcernedCells_t enteringCells);
On 2013/09/25 17:30:51, bpswenson wrote:
> Comment methods for doxygen

Done.

https://codereview.appspot.com/13720052/diff/45001/src/lte/model/lte-ue-rrc.h...
src/lte/model/lte-ue-rrc.h:418: void CancelEnteringTrigger (uint8_t measId);
On 2013/09/25 17:30:51, bpswenson wrote:
> Comment methods for doxygen

Done.

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

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:854: for (NetDeviceContainer::Iterator i =
enbDevices.Begin ();
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> this method should be deleted, see my comment in lte-helper.h

Done.

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:873: Ptr<LteEnbRrc> rrc = enbLteDevice->GetRrc ();
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> Please move this code into a new method LteEnbRrc::SetCsgId (uint32_t csgId)  

Done.

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:883: for (NetDeviceContainer::Iterator i =
enbDevices.Begin ();
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> this method should be deleted, see my comment in lte-helper.h

Done.

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:902: LteRrcSap::SystemInformationBlockType1 sib1 =
rrc->GetSystemInformationBlockType1 ();
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> Please move this code into a new method LteEnbRrc::SetQRxLevMin (uint32_t
> csgId). 

Done.

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:911: i != ueDevices.End (); ++i)
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> this method should be deleted, see my comment in lte-helper.h

Done.

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.cc:928: ueLteDevice->GetNas ()->SetCsgId (csgId);
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> I suggest that the LteUeNetDevice should have a CsgId attribute, and its
setter
> should be in charge of communicating the CsgId to the NAS

Done.

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

https://codereview.appspot.com/13720052/diff/48001/src/lte/helper/lte-helper....
src/lte/helper/lte-helper.h:386: void SetEnbCsgId (NetDeviceContainer
enbDevices, uint32_t csgId,
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> This approach of the various Set{Enb,Ue}{CsgId,QrxLev} (...) methods is not
> consistent with the usual Helper API. 
> 
> The regular approach to apply a setting to multiple eNB or UE devices is this
> one:
> 
> NodeContainer enbNodes;
> enbNodes.Create (10);
> lteHelper->SetEnbDeviceAttribute ("CsgId", UintegerValue (4));
> lteHelper->SetEnbDeviceAttribute ("QRxLevMin", UintegerValue (2));
> NetDeviceContainer enbDevs  = lteHelper->InstallEnbDevice (homeEnbs);
> 
> Please remove the newly added setter methods and use
Set{Enb,Ue}DeviceAttribute
> instead for the CsgId setting.
> 
> As for QRxLevMin, since it is clearly an RRC detail, I suggest to just make it
> an attribute of LteEnbRrc. I expect that people would just set it globally
with
> Config::SetDefault (and related means). If you wish, you could add a
> LteHelper::SetEnbRrcAttribute() to allow an alternative means for setting this
> (and other) RRC attribute - we currently have a similar approach for e.g.
> scheduler attributes.

Done. The new attributes are:
- ns3::LteUeNetDevice::CsgId
- ns3::LteEnbNetDevice::CsgId
- ns3::LteEnbNetDevice::CsgIndication
- ns3::LteEnbRrc::QRxLevMin

The SetEnbRrcAttribute is actually an interesting idea. But since
Config::SetDefault also works, I'd prefer to postpone this idea for later.

But this raises another question. If I understand correctly, both approaches
(lteHelper->SetSomethingAttribute and Config::SetDefault) have the same purpose:
"all the devices installed after this statement will have this attribute value."
It looks redundant. But let's discuss this later.

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/lte-control-...
File src/lte/model/lte-control-messages.h (right):

https://codereview.appspot.com/13720052/diff/48001/src/lte/model/lte-control-...
src/lte/model/lte-control-messages.h:470: 
On 2013/09/25 18:51:14, bpswenson wrote:
> Is there a reason why every class in this header file has its own set of
include
> guards?

Not that I know of. Done. Removed all of them.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
File src/lte/model/a2-a4-rsrq-handover-algorithm.cc (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
src/lte/model/a2-a4-rsrq-handover-algorithm.cc:18: * Author: Budiarto Herman
<budiarto.herman@magister.fi>
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> This file includes the code of the pre-existent handover algorithm, right?
then
> you should include the original copyright and authors.

Yes, sorry I forgot to change this earlier. I checked other existing files for
some examples on writing this notice, then I came up with the following changes
proposal.

The copyright notice now reads:

 * Copyright (c) 2011 Centre Tecnologic de Telecomunicacions de Catalunya (CTTC)
 * Copyright (c) 2013 Budiarto Herman

And the author list now reads:

 * Original work authors (from lte-enb-rrc.cc):
 * - Nicola Baldo <nbaldo@cttc.es>
 * - Marco Miozzo <mmiozzo@cttc.es>
 * - Manuel Requena <manuel.requena@cttc.es>
 *
 * Converted to handover algorithm interface by:
 * - Budiarto Herman <budiarto.herman@magister.fi>

This would be applied to the .h and .cc of a2-a4-rsrq-handover-algorithm and
lte-anr.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
File src/lte/model/a2-a4-rsrq-handover-algorithm.h (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/a2-a4-rsrq-h...
src/lte/model/a2-a4-rsrq-handover-algorithm.h:103: // TODO instead of class, try
using struct or SimpleRefCount
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> I agree

Done.

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.h
File src/lte/model/lte-anr.h (right):

https://codereview.appspot.com/13720052/diff/51001/src/lte/model/lte-anr.h#ne...
src/lte/model/lte-anr.h:49: virtual void DoDispose (void);
On 2013/10/04 12:25:02, Nicola Baldo wrote:
> I think DoDispose should be protected, like DoInitialize

Done. I didn't realize this.

Anyway, I just want to note that many classes in LTE module apparently still
have public DoDispose. For now, I only fix the new classes that I wrote during
GSoC.

[Nicola Baldo, 2013-11-07 18:10:27]

Hi Budi,

thank you very much for the update. I've reviewed your resolution of the
previous open issues, and I find all of them satisfactory. 

I've also checked explicitly lte-test-phy-errror-model.cc as you requested. I
actually found the diff on code review a bit misleading, so in the end I
kompared the file in your repository at 
http://code.nsnam.org/buherman/ns-3-gsoc-lte
with the one on lena-dev, and I confirm that the changes are ok.

So, to summarize, I think we are now ready to merge!

The only big last doubt that I have is about your mercurial repository. I've
seen that the latest changeset is this one:

changeset:   10074:bab468e8fe1d
tag:         tip
user:        Budiarto Herman <budiarto.herman@magister.fi>
date:        Wed Oct 23 14:42:19 2013 +0300
summary:     Transition merge from ns-3-dev branch to GSoC branch

and looking at the history it is evident that all recent changeset are yours,
and that the most recent one which is not yours is 048edf672cef, which dates
back to June 04!! Can you please explain exactly what you did with your latest
merge changeset bab468e8fe1d? I had expected you would just pull changes from
ns-3-dev, do hg merge, solve conflicts, and then commit, but it seems you did
something different...

Regards,

Nicola



On 2013/10/23 15:37:51, buherman wrote:
> Hi All,
> 
> Thanks for the feedback. I've finally completed implementing them to the
> repository. Patch Set 5 is now posted, which contains the changes related to
the
> feedback.
> 
> I've also merged the GSoC repository and ns-3-dev again. The result is in the
> other code review: https://codereview.appspot.com/13728043/.
> 
> Aside to Nicola, could you please double check lte-test-phy-error-model.cc?
The
> test passes, but there were plenty of conflicts when I merged it. The one
posted
> here is my proposed merge.
> 
> Regards,
> -budi-
>

[buherman, 2013-11-07 19:59:36]

Hi Nicola,

Please see my comments below.

Regards,
-budi-


On 2013/11/07 18:10:27, Nicola Baldo wrote:
> Hi Budi,
> 
> thank you very much for the update. I've reviewed your resolution of the
> previous open issues, and I find all of them satisfactory. 
> 
> I've also checked explicitly lte-test-phy-errror-model.cc as you requested. I
> actually found the diff on code review a bit misleading, so in the end I
> kompared the file in your repository at 
> http://code.nsnam.org/buherman/ns-3-gsoc-lte
> with the one on lena-dev, and I confirm that the changes are ok.
> 
> So, to summarize, I think we are now ready to merge!
> 
> The only big last doubt that I have is about your mercurial repository. I've
> seen that the latest changeset is this one:
> 
> changeset:   10074:bab468e8fe1d
> tag:         tip
> user:        Budiarto Herman <mailto:budiarto.herman@magister.fi>
> date:        Wed Oct 23 14:42:19 2013 +0300
> summary:     Transition merge from ns-3-dev branch to GSoC branch
> 
> and looking at the history it is evident that all recent changeset are yours,
> and that the most recent one which is not yours is 048edf672cef, which dates
> back to June 04!! Can you please explain exactly what you did with your latest
> merge changeset bab468e8fe1d? I had expected you would just pull changes from
> ns-3-dev, do hg merge, solve conflicts, and then commit, but it seems you did
> something different...

Revision 9928:048edf672cef of June 4th was the latest lena-dev revision when I
created the ns-3-gsoc-lte repository (it was by cloning from lena-dev). After
that point, I've committed my changes, but have never pulled any newer revision
from either ns-3-dev or lena-dev.

The pull was done in the other repository, ns-3-gsoc-lte-merge. I created it as
a clone of the latest ns-3-dev, and then pulled the changes from ns-3-gsoc-lte
(so it's the other way around from what you mentioned above).

I've created and recreated ns-3-gsoc-lte-merge several times already for code
review purpose. So I had to resolve the same conflicts again and again. Some of
them are not straightforward to resolve, such as the reordering of files in
doc/models/Makefile, typos in some long paragraphs in RST files, and so on. So I
decided to manually apply some of these changes from ns-3-dev to ns-3-gsoc-lte
and commit it as the latest revision 10074:bab468e8fe1d, hoping that the next
time I merge I would have easier time dealing with those conflicts.

I'm not completely sure if this is the right way to do it, but at least from my
point of view, the merge has become easier after this. As an alternative, you
can safely ignore this latest revision (because its content is already in
ns-3-dev) and pull only up to the revision before it:

changeset:   10073:665af58c863b
user:        Budiarto Herman <budiarto.herman@magister.fi>
date:        Tue Oct 22 20:36:56 2013 +0300
summary:     Fix to SetCsgId in LteEnbRrc for not updating SIB1 in PHY

I hope that clears up the confusion.

> 
> Regards,
> 
> Nicola
>

[Nicola Baldo, 2013-11-08 12:08:25]

Hi Budi,

On 2013/11/07 19:59:36, buherman wrote:
> Revision 9928:048edf672cef of June 4th was the latest lena-dev revision when I
> created the ns-3-gsoc-lte repository (it was by cloning from lena-dev). After
> that point, I've committed my changes, but have never pulled any newer
revision
> from either ns-3-dev or lena-dev.
> 
> The pull was done in the other repository, ns-3-gsoc-lte-merge. I created it
as
> a clone of the latest ns-3-dev, and then pulled the changes from ns-3-gsoc-lte
> (so it's the other way around from what you mentioned above).

ok, anyway the "other way around" affects only the ordering of the changesets,
and which parent changeset is considered the default (e.g., when doing "hg
diff"). The actual result does not vary.

> 
> I've created and recreated ns-3-gsoc-lte-merge several times already for code
> review purpose. So I had to resolve the same conflicts again and again. 

actually there was no need to delete and recreate the merge repository several
time. You could have just committed different merge operations subsequently, and
still you would be able to generate a proper patch for code review by selecting
the appropriate revision to diff against.
Maybe I should have been more explict on this issue before, sorry for this. I
understand that re-doing the merge repeatedly is time consuming and very
annoying.

> Some of
> them are not straightforward to resolve, such as the reordering of files in
> doc/models/Makefile, typos in some long paragraphs in RST files, and so on. So
I
> decided to manually apply some of these changes from ns-3-dev to ns-3-gsoc-lte
> and commit it as the latest revision 10074:bab468e8fe1d, hoping that the next
> time I merge I would have easier time dealing with those conflicts.
> 
> I'm not completely sure if this is the right way to do it, but at least from
my
> point of view, the merge has become easier after this. 

no this is not the right way of doing it. I agree that the resulting code is
correct, and effectively the merge becomes easier, but as a side effect you are
overwriting the information about the author of each change which you merged
manually. As a result, for example, "hg annotate" will show that you are the
author for those changes by someone else that you applied manually.



> As an alternative, you
> can safely ignore this latest revision (because its content is already in
> ns-3-dev) and pull only up to the revision before it:
> 
> changeset:   10073:665af58c863b
> user:        Budiarto Herman <mailto:budiarto.herman@magister.fi>
> date:        Tue Oct 22 20:36:56 2013 +0300
> summary:     Fix to SetCsgId in LteEnbRrc for not updating SIB1 in PHY
> 


I just did that, then I merged with ns-3-dev, and I resolved the conflicts by
taking manually the resolved files from bab468e8fe1d in your repo (without
actually pulling in that changeset). I committed this as 

changeset:   10386:0156b589bf78
tag:         tip
parent:      10240:dd5bd38d3587
parent:      10385:665af58c863b
user:        Budiarto Herman <mailto:budiarto.herman@magister.fi>
date:        Fri Nov 08 12:09:58 2013 +0100
summary:     merge from ns-3-dev branch to GSoC branch

Note I put you as the author as the resolved conflict files came from you - I
did not have to add anything else.
I've just pushed all this to ns-3-dev, so that we are in time for the feature
freeze for the ns-3.19 release.

As a final remark, here is an example of the "hg annotate" issues that I
mentioned before. 
This is the result of the merge operation as I've done it:
http://code.nsnam.org/ns-3-dev/annotate/tip/src/lte/examples/lena-dual-stripe...
and this is how it resulted from your manual application of the changesets:
http://code.nsnam.org/buherman/ns-3-gsoc-lte-merge/annotate/6d30b5f127b4/src/...

[buherman, 2013-11-09 14:14:14]

Hi Nicola,

On 2013/11/08 12:08:25, Nicola Baldo wrote:
> ok, anyway the "other way around" affects only the ordering of the changesets,
> and which parent changeset is considered the default (e.g., when doing "hg
> diff"). The actual result does not vary.
> 
> > 
> > I've created and recreated ns-3-gsoc-lte-merge several times already for
code
> > review purpose. So I had to resolve the same conflicts again and again. 
> 
> actually there was no need to delete and recreate the merge repository several
> time. You could have just committed different merge operations subsequently,
and
> still you would be able to generate a proper patch for code review by
selecting
> the appropriate revision to diff against.
> Maybe I should have been more explict on this issue before, sorry for this. I
> understand that re-doing the merge repeatedly is time consuming and very
> annoying.

Well, to be honest I still don't understand it either from the above explanation
:). I need to try it out see it myself, so I will do that next time.

> no this is not the right way of doing it. I agree that the resulting code is
> correct, and effectively the merge becomes easier, but as a side effect you
are
> overwriting the information about the author of each change which you merged
> manually. As a result, for example, "hg annotate" will show that you are the
> author for those changes by someone else that you applied manually.
> 
> 
> 
> > As an alternative, you
> > can safely ignore this latest revision (because its content is already in
> > ns-3-dev) and pull only up to the revision before it:
> > 
> > changeset:   10073:665af58c863b
> > user:        Budiarto Herman <mailto:budiarto.herman@magister.fi>
> > date:        Tue Oct 22 20:36:56 2013 +0300
> > summary:     Fix to SetCsgId in LteEnbRrc for not updating SIB1 in PHY
> > 
> 
> 
> I just did that, then I merged with ns-3-dev, and I resolved the conflicts by
> taking manually the resolved files from bab468e8fe1d in your repo (without
> actually pulling in that changeset). I committed this as 
> 
> changeset:   10386:0156b589bf78
> tag:         tip
> parent:      10240:dd5bd38d3587
> parent:      10385:665af58c863b
> user:        Budiarto Herman <mailto:budiarto.herman@magister.fi>
> date:        Fri Nov 08 12:09:58 2013 +0100
> summary:     merge from ns-3-dev branch to GSoC branch
> 
> Note I put you as the author as the resolved conflict files came from you - I
> did not have to add anything else.
> I've just pushed all this to ns-3-dev, so that we are in time for the feature
> freeze for the ns-3.19 release.
> 
> As a final remark, here is an example of the "hg annotate" issues that I
> mentioned before. 
> This is the result of the merge operation as I've done it:
>
http://code.nsnam.org/ns-3-dev/annotate/tip/src/lte/examples/lena-dual-stripe...
> and this is how it resulted from your manual application of the changesets:
>
http://code.nsnam.org/buherman/ns-3-gsoc-lte-merge/annotate/6d30b5f127b4/src/...

Hm... I wasn't aware about "hg annotate" before. Thanks for explaining this. And
yes, I agree that this is  not the right way to do.

Anyway, thanks for the effort reviewing and then merging the code. I guess the
issue is closed now. Looking forward to contributing again to ns-3 and LENA in
the near future.

Regards,
-budi-