NSoC final code review 2011 :LTE RRC extension

[Piro Giuseppe, 2011-08-22 11:30:37]

Hi Pankaj, 

I have reviewed your code. 

Unfortunately the code must be improved a lot and, for now, it is very difficult
to decide to merge it into the ns-3-dev. Moreover, I think it isn't now
appropriate choose an external reviewer because of the code is difficult to
understand, it has a lot of errors and the patch for code review must be
obtained correctly (in the current issue there are several unnecessary diffs).


In general, RRC entities and control messages have been implemented discreetly. 

Rrc procedure are all incorrect. Also the test code is simple and incomplete. 

Hope that Pankaj will be able to improve the code in order to produce a mergeble
version of its NSoC project.


Best Regards,
Giuseppe

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-net-device.cc
File src/lte/model/enb-net-device.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-net-device.cc#...
src/lte/model/enb-net-device.cc:55: tid =
What is the reason of this patch ? 
Please, make sure that unnecessary changes will be not included into the next
review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-net-device.cc#...
src/lte/model/enb-net-device.cc:176: bearer = GetRrcEntity ()->GetDefaultBearer
();
What is the reason of this patch ? 
Please, make sure that unnecessary changes will be not included into the next
review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity.cc
File src/lte/model/enb-rrc-cp-entity.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:45: 
I see that not of all functions have logging functions. Please, fix it.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:80: Ptr<IdealControlMessage> msg = new
IdealControlMessage();
This method should create an rrc ideal control messages. Why you have created a
pointer to the IdealControlMessage ? You must, instead, create a pointer to the
proper rrc control message and set properly its parameters.

The same comment is true for all the following methods.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:88: mibmsg->SetMIParam(bandwidth, sfn);
Informations about the MIB are not included into the message!

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:130: Ptr<RrcIdealConnectionreq>
rrc_connreq_msg = DynamicCast<RrcIdealConnectionreq> (msg);
This method is called by the ReceiveRrcIdealControlMessage functionsif the
message is one for request a connection. 
This means that you already know the type of the message (it isn't
IdealCOntrolMessage but ConnectionRequestRrcIdealControlMessage).

Accordingly, you can avoid to convert the message and you must modify the
function for receiving the pointer to the correct message type.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:148: //configuring the UE parameters , RLC ,
MAC configuration
Configure!

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:186: {
convert the message before calling the procedure

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity.h
File src/lte/model/enb-rrc-cp-entity.h (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:1: /* -*- Mode:C++; c-file-style:"gnu";
indent-tabs-mode:nil; -*- */
Why did you chosen for these file the name "enb-rrc-cp-entity" ? I suggest to
substitute it with "enb-rrc-entity". The same comment is true also for the ue
rrc.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:31: //#include "ideal-control-messages.h"
if it is not necessary, delete it!

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:49: class EnbLteRrc : public RrcEntity
I suggest to use the name EnbRrcEntity

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:62: struct EnbRrcRecord {
A documentation to the utility of this record should be added.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:118: 
It is not necessary to add the prefix "ENB" to all the following functions

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:158: void
EnbRecieveRrcReConfig(Ptr<RrcConnectionReConfiguration> msg);
ReConfig -> do not use short name

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:166: RrcStates m_rrcStateInfo;
Define this object as a pointer and add it definition into the constructor.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc
File src/lte/model/rrc-entity.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:173: {
What is the reason of this patch ? 
Please, make sure that unnecessary changes will be not included into the next
review process.
Again, in my opinion these problems are due to the incorrect ,merging procedure.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:272: }
What is the reason of this patch ? 
Please, make sure that unnecessary changes will be not included into the next
review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:279: }
Delete. See the comment into the .h file

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:284: */
Note that the doxygen documentation must be inserted only into the .h file.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:290: }
Delete. See the comment into the .h file

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:301: }
Delete. See the comment into the .h file

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:312: }
Delete. See the comment into the .h file

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.h
File src/lte/model/rrc-entity.h (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.h#newco...
src/lte/model/rrc-entity.h:152: 
ll the previous methods must be deleted (these functions have been added into
the latest week without any my suggestion... my first question is: why did you
do this ? ). 

It is not correct to have a general RRC Entity class which contain pointers to
the RRC entity specialized for both UE and eNB devices. 

RrcEntity is the father class form which will be inherited UeRrcEntity and
EnbRrcEntity that represent the specialized rrc entity class for both UE and eNB
devices, respectively. 

Hence, 
- delete previous functions and pointers created in what follows; 
- make sure that EnbLteNetDevice and UeLteNetDevice will have a pointer to
UeRrcEntity and EnbRrcEntity objects, respectively;
-  delete the m_rrcEntity parameter and all methods related to it from the
LteNetDevice class
- define, as pure virtual function, SetRrcEntity and GetRrcEntity methods into
the LteNetDevice (these method will be  used by EnbLteNetDevice and
UeLteNetDevice for setting and getting the correct pointer to the rrc entity)
- make sure that into the constructor of both EnbLteNetDevice and UeLteNetDevice
a correct rrc entity is created.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.h#newco...
src/lte/model/rrc-entity.h:158: virtual void SendRrcIdealControlMessage(Ptr <
IdealControlMessage > msg) = 0;
Ptr < IdealControlMessage > msg
Are you sure that this is the ns-3 codeing style ? Have a look to this page 
http://www2.nsnam.org/codingstyle.html
and update the code accordingly.

Note that to have a code compliant to the coding style is one of the many
aspects to be addressed before merging the code to the official release.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.h#newco...
src/lte/model/rrc-entity.h:179: Ptr<UeLteRrc> m_uerrcEntity;
delete

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.h#newco...
src/lte/model/rrc-entity.h:180: Ptr<EnbLteRrc> m_enbrrcEntity;
delete

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-ideal-control-...
File src/lte/model/rrc-ideal-control-messages.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.cc:1: /* -*- Mode:C++;
c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
Just a general comment to the file. It seems that the ns-3 coding style has not
been respected.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-net-device.h
File src/lte/model/ue-net-device.h (left):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-net-device.h#ol...
src/lte/model/ue-net-device.h:45: *
What is the reason of this patch ? 
Please, make sure that unnecessary changes will be not included into the next
review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-net-device.h
File src/lte/model/ue-net-device.h (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-net-device.h#ne...
src/lte/model/ue-net-device.h:32: #include "rrc-entity.h"
What is the reason of this patch ? 
Please, make sure that unnecessary changes will be not included into the next
review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.cc
File src/lte/model/ue-record.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.cc#newco...
src/lte/model/ue-record.cc:23: NS_LOG_COMPONENT_DEFINE("UeRecord");
This file contains several changes.  What is the reason of these changes (I
think none of them are necessary) ?  
Please, make sure that unnecessary changes will be not included into the next
review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h
File src/lte/model/ue-record.h (left):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#oldcode62
src/lte/model/ue-record.h:62: typedef std::vector<struct CqiFeedback>
CqiFeedbacks;
I don't know why you have deleted this code. CqiFeedbacks represents an
important feature for the lte module.
Please, restore the old module.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h
File src/lte/model/ue-record.h (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#newcode44
src/lte/model/ue-record.h:44: ~UeRecord ();
What is the reason of this patch ? 
Please, make sure that unnecessary changes will be not included into the next
review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#newcode57
src/lte/model/ue-record.h:57: };
What is the reason of this patch ? 
Please, make sure that unnecessary changes will be not included into the next
review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#newcode64
src/lte/model/ue-record.h:64: struct UeRrcControlInfo {
The info stored into the UeRrcControlInfo seems to be correct. 

Unfortunately. it is impossible for a guy that doesn't know the code made a
review because of the patch is very complicated to read. Please, can you fix
this problem ?

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#newcod...
src/lte/model/ue-record.h:141: CqiFeedbacks GetCqiFeedbacks(void);
All the previous code (from the definition of the CqiFeedbacks to the
GetCqiFeedbacks function) must be not included as "new" of "modified" code with
respect to the one included into the ns-3-dev.
Make sure that this problem will be fixed for the new review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#newcod...
src/lte/model/ue-record.h:144: UeRrcControlInfo m_uerrcControlInfo;
I suggest to add methods for adding and getting parameters defined int the
UeRrcControlInfo structure.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#newcod...
src/lte/model/ue-record.h:149: Ptr<NetDevice> m_enb;
What is the reason of this patch ? 
Please, make sure that unnecessary changes will be not included into the next
review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#newcod...
src/lte/model/ue-record.h:151: CqiFeedbacks m_cqiFeedbacks;
What is the reason of this patch ? 
Please, make sure that unnecessary changes will be not included into the next
review process.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.cc
File src/lte/model/ue-rrc-cp-entity.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:75: {
The method is called after that you are sure that msg have a proper message
type. Hence, you can pass to the function the pointer to the proper message type
(and not th general pointer to IdealCOntrolMessage). 
The comment is true for all the following methods.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:75: {
This function doesn't take any parameter from the input object msg. Why ?
Please, implement correctly the procedure.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:90: {
This function doesn't take any parameter from the input object msg. Why ?
Please, implement correctly the procedure.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:102: {
This function doesn't take any parameter from the input object msg. Why ?
Please, implement correctly the procedure.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:114: {
This function doesn't take any parameter from the input object msg. Why ?
Please, implement correctly the procedure.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:149: //configuring the UE parameters , RLC ,
MAC configuration
Configure

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:168: //msg->GetMessageType()
Delete the previous line

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:173: 
As soon you check the message type, convert it and send the proper pointer to
the procedure

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.h
File src/lte/model/ue-rrc-cp-entity.h (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.h...
src/lte/model/ue-rrc-cp-entity.h:62: struct UeRrcControlInformation {
Add a documentation about the function of this structure

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.h...
src/lte/model/ue-rrc-cp-entity.h:107: 
It is not necessary to add the prefix "Ue" to all the following functions

http://codereview.appspot.com/4901055/diff/1/src/lte/test/lte-test-rrc-messag...
File src/lte/test/lte-test-rrc-messages.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:20: 
I haven't see the test because 
- procedures have not been implemented correctly and
- I cannot understand, for this reason,  how the test can be correct 

Moreover, I have asked to implement a test where eNB and ue exchange ALL rrc
control message and that, for each message, the test should verify that
- the message has been received
- after the reception, rrc control info have been stored correctly into the rrc
entity

http://codereview.appspot.com/4901055/diff/1/src/point-to-point/bindings/modu...
File src/point-to-point/bindings/modulegen__gcc_ILP32.py (left):

http://codereview.appspot.com/4901055/diff/1/src/point-to-point/bindings/modu...
src/point-to-point/bindings/modulegen__gcc_ILP32.py:2584: ## type-id.h (module
'core'): ns3::TypeId::TraceSourceInformation::TraceSourceInformation()
[constructor]
This ile should be not modified with respect to the one into the ns-3-dev
branch. Probably you have obtained it as a consequence of an incorrect merging
procedure. 
Please, update your repository in order to fix this problem.

[pankaj, 2011-09-06 06:05:39]

Hi Giuseppe,

I have uploaded the code after fixing the review comments where ever applicable.


code link
http://code.nsnam.org/pankaj/ns-3-lte-rrc/

Please revisit.

Thanks and Regards,
Pankaj

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-net-device.cc
File src/lte/model/enb-net-device.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-net-device.cc#...
src/lte/model/enb-net-device.cc:55: tid =
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> What is the reason of this patch ? 
> Please, make sure that unnecessary changes will be not included into the next
> review process.

becuase of code indentation . I used a tool .

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-net-device.cc#...
src/lte/model/enb-net-device.cc:176: bearer = GetRrcEntity ()->GetDefaultBearer
();
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> What is the reason of this patch ? 
> Please, make sure that unnecessary changes will be not included into the next
> review process.

code indentation.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity.cc
File src/lte/model/enb-rrc-cp-entity.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:45: 
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> I see that not of all functions have logging functions. Please, fix it.

done

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:80: Ptr<IdealControlMessage> msg = new
IdealControlMessage();
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> This method should create an rrc ideal control messages. Why you have created
a
> pointer to the IdealControlMessage ? You must, instead, create a pointer to
the
> proper rrc control message and set properly its parameters.
> 
> The same comment is true for all the following methods.


There was a problem of how to identify the destination and source of the
message. that can only be availble in the ideal control message not in the rrc
ideal control messages.
Hence, i used the pointer of ideal control message so that in the recieve
processing src address and destination address can be used.

In case i have to use only rrc ideal control messages , what can be the
alternative ? How should enodeB able to send the messages to a particular UE.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:88: mibmsg->SetMIParam(bandwidth, sfn);
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Informations about the MIB are not included into the message! 


bandwidth and sfn are the only parameters we are taking for MIB . I have
considered both of them.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:148: //configuring the UE parameters , RLC ,
MAC configuration
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Configure!

Because of other limitations i have left it for the future implementation .Can
be deleted.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.cc:186: {
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> convert the message before calling the procedure

ok

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity.h
File src/lte/model/enb-rrc-cp-entity.h (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:1: /* -*- Mode:C++; c-file-style:"gnu";
indent-tabs-mode:nil; -*- */
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Why did you chosen for these file the name "enb-rrc-cp-entity" ? I suggest to
> substitute it with "enb-rrc-entity". The same comment is true also for the ue
> rrc.  

cp is for the control plane. It can be changed.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:31: //#include "ideal-control-messages.h"
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> if it is not necessary, delete it!

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:49: class EnbLteRrc : public RrcEntity
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> I suggest to use the name EnbRrcEntity

done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:118: 
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> It is not necessary to add the prefix "ENB" to all the following functions

ok

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:158: void
EnbRecieveRrcReConfig(Ptr<RrcConnectionReConfiguration> msg);
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> ReConfig -> do not use short name

updated

http://codereview.appspot.com/4901055/diff/1/src/lte/model/enb-rrc-cp-entity....
src/lte/model/enb-rrc-cp-entity.h:166: RrcStates m_rrcStateInfo;
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Define this object as a pointer and add it definition into the constructor. 

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc
File src/lte/model/rrc-entity.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:173: {
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> What is the reason of this patch ? 
> Please, make sure that unnecessary changes will be not included into the next
> review process.
> Again, in my opinion these problems are due to the incorrect ,merging
procedure.
> 

indentation.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:173: {
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> What is the reason of this patch ? 
> Please, make sure that unnecessary changes will be not included into the next
> review process.
> Again, in my opinion these problems are due to the incorrect ,merging
procedure.
> 

Removed

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:272: }
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> What is the reason of this patch ? 
> Please, make sure that unnecessary changes will be not included into the next
> review process.

ok

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:279: }
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Delete. See the comment into the .h file

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:284: */
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Note that the doxygen documentation must be inserted only into the .h file.

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:290: }
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Delete. See the comment into the .h file

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:301: }
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Delete. See the comment into the .h file

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.cc#newc...
src/lte/model/rrc-entity.cc:312: }
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Delete. See the comment into the .h file

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.h
File src/lte/model/rrc-entity.h (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.h#newco...
src/lte/model/rrc-entity.h:152: 
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> ll the previous methods must be deleted (these functions have been added into
> the latest week without any my suggestion... my first question is: why did you
> do this ? ). 
> 
> It is not correct to have a general RRC Entity class which contain pointers to
> the RRC entity specialized for both UE and eNB devices. 
> 
> RrcEntity is the father class form which will be inherited UeRrcEntity and
> EnbRrcEntity that represent the specialized rrc entity class for both UE and
eNB
> devices, respectively. 
> 
> Hence, 
> - delete previous functions and pointers created in what follows; 
> - make sure that EnbLteNetDevice and UeLteNetDevice will have a pointer to
> UeRrcEntity and EnbRrcEntity objects, respectively;
> -  delete the m_rrcEntity parameter and all methods related to it from the
> LteNetDevice class
> - define, as pure virtual function, SetRrcEntity and GetRrcEntity methods into
> the LteNetDevice (these method will be  used by EnbLteNetDevice and
> UeLteNetDevice for setting and getting the correct pointer to the rrc entity)
> - make sure that into the constructor of both EnbLteNetDevice and
UeLteNetDevice
> a correct rrc entity is created.

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.h#newco...
src/lte/model/rrc-entity.h:158: virtual void SendRrcIdealControlMessage(Ptr <
IdealControlMessage > msg) = 0;
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Ptr < IdealControlMessage > msg
> Are you sure that this is the ns-3 codeing style ? Have a look to this page 
> http://www2.nsnam.org/codingstyle.html
> and update the code accordingly.
> 
> Note that to have a code compliant to the coding style is one of the many
> aspects to be addressed before merging the code to the official release.

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.h#newco...
src/lte/model/rrc-entity.h:179: Ptr<UeLteRrc> m_uerrcEntity;
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> delete

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/rrc-entity.h#newco...
src/lte/model/rrc-entity.h:180: Ptr<EnbLteRrc> m_enbrrcEntity;
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> delete

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-net-device.h
File src/lte/model/ue-net-device.h (left):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-net-device.h#ol...
src/lte/model/ue-net-device.h:45: *
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> What is the reason of this patch ? 
> Please, make sure that unnecessary changes will be not included into the next
> review process.

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.cc
File src/lte/model/ue-record.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.cc#newco...
src/lte/model/ue-record.cc:23: NS_LOG_COMPONENT_DEFINE("UeRecord");
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> This file contains several changes.  What is the reason of these changes (I
> think none of them are necessary) ?  
> Please, make sure that unnecessary changes will be not included into the next
> review process.

it is because of code indentation .

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h
File src/lte/model/ue-record.h (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#newcod...
src/lte/model/ue-record.h:141: CqiFeedbacks GetCqiFeedbacks(void);
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> All the previous code (from the definition of the CqiFeedbacks to the
> GetCqiFeedbacks function) must be not included as "new" of "modified" code
with
> respect to the one included into the ns-3-dev.
> Make sure that this problem will be fixed for the new review process.

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#newcod...
src/lte/model/ue-record.h:149: Ptr<NetDevice> m_enb;
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> What is the reason of this patch ? 
> Please, make sure that unnecessary changes will be not included into the next
> review process.

code indentation.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-record.h#newcod...
src/lte/model/ue-record.h:151: CqiFeedbacks m_cqiFeedbacks;
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> What is the reason of this patch ? 
> Please, make sure that unnecessary changes will be not included into the next
> review process.

code indentation.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.cc
File src/lte/model/ue-rrc-cp-entity.cc (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:75: {
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> This function doesn't take any parameter from the input object msg. Why ?
> Please, implement correctly the procedure.

updated

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:75: {
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> The method is called after that you are sure that msg have a proper message
> type. Hence, you can pass to the function the pointer to the proper message
type
> (and not th general pointer to IdealCOntrolMessage). 
> The comment is true for all the following methods. 

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:90: {
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> This function doesn't take any parameter from the input object msg. Why ?
> Please, implement correctly the procedure.

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:102: {
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> This function doesn't take any parameter from the input object msg. Why ?
> Please, implement correctly the procedure.

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:102: {
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> This function doesn't take any parameter from the input object msg. Why ?
> Please, implement correctly the procedure.

UE is making the procedure . It does not required any extra parameter. same is
the case with other procedures as well.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:149: //configuring the UE parameters , RLC ,
MAC configuration
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> Configure

This is left for the future use.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:173: 
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> As soon you check the message type, convert it and send the proper pointer to
> the procedure

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.c...
src/lte/model/ue-rrc-cp-entity.cc:173: 
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> As soon you check the message type, convert it and send the proper pointer to
> the procedure

Done.

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.h
File src/lte/model/ue-rrc-cp-entity.h (right):

http://codereview.appspot.com/4901055/diff/1/src/lte/model/ue-rrc-cp-entity.h...
src/lte/model/ue-rrc-cp-entity.h:107: 
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> It is not necessary to add the prefix "Ue" to all the following functions

Done.

http://codereview.appspot.com/4901055/diff/1/src/point-to-point/bindings/modu...
File src/point-to-point/bindings/modulegen__gcc_ILP32.py (left):

http://codereview.appspot.com/4901055/diff/1/src/point-to-point/bindings/modu...
src/point-to-point/bindings/modulegen__gcc_ILP32.py:2584: ## type-id.h (module
'core'): ns3::TypeId::TraceSourceInformation::TraceSourceInformation()
[constructor]
On 2011/08/22 11:30:37, Piro Giuseppe wrote:
> This ile should be not modified with respect to the one into the ns-3-dev
> branch. Probably you have obtained it as a consequence of an incorrect merging
> procedure. 
> Please, update your repository in order to fix this problem. 

yes it comes because of improper merging.