LTE RRC Extension Review

[Piro Giuseppe, 2011-09-30 13:59:08]

Hi Pankaj,
I reviewed your code. It seems more well organized with respect to the previous
time. 

There are only few points to be addressed before consider your code "mergeable".


- the ns-3 coding style has not been applied anywhere. Please, fix this problem.

- RrcEntity::ReceiveRrcIdealControlMessage has been implemented following a
different approach for both ue and enb (have a look to comments to the code).
Use only one approach. 

- test code should be improved. But the used approach is ok.



Now, I don't know what can be next steps before the merging (hope that Tom and
Lalith can help you in this direction). 
Maybe documentation, example (not tests :-)), and valgrind tests are required. 


Best Regards,
Giuseppe

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:65: *m_rrcStateInfo = RRC_IDLE;
Previous code are unnecessary. See the comment in the .h file. 

Otherwise, avoid to use the pointer.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:74: {
Delete it. See the previous comment

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:88: mibmsg->SetMessageType(
IdealControlMessage::SYSTEM_INFO_MIB);
The previous line is unnecessary because the message type is set into the
constructor of such  message.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:97: Ptr<IdealControlMessage> msg = new
IdealControlMessage();
msg is not necessary. delete its definition

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:105: sibmsg->SetMessageType(
IdealControlMessage::SYSTEM_INFO_SIB);
The previous line is unnecessary because the message type is set into the
constructor of such  message.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:122: pagingmsg->SetMessageType(
IdealControlMessage::PAGING);
The previous line is unnecessary because the message type is set into the
constructor of such  message.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:178: //UeRecord* record;
delete commented code.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:194: void
EnbRrcEntity::ReceiveRrcIdealControlMessage(Ptr<IdealControlMessage> msg)
Regarding this method, I didn't understand why you have followed different
approach for both enb and ue. 

For the ue, you identify the message type, convert the message definition and
forward the pointer of such message to the proper procedure. 

For the enb, instead, you don't convert the definition of the message but
forward the pointer to the IdealControlmessage. 

I suggest to use the same approach for both devices. Both of them are correctly.
It is just a way to have a uniform code :-)

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.h#n...
src/lte/model/enb-rrc-entity.h:56: enum RrcStates {
Why you have defined this variable for the eNB ? 
In particular, what does "RRC_IDLE" mean ? What happen if you have more than one
active rrc connection ?  

Probably, for then moment we have to delete the variable describing the status.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.h#n...
src/lte/model/enb-rrc-entity.h:68: RrcStates* GetRrcState();
Maybe it should be deleted. 
Furthermore, why you have defined the status as a pointer ? Please, don't use
the pointer.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.h#n...
src/lte/model/enb-rrc-entity.h:141: RrcStates* m_rrcStateInfo;
I sugegst to delete this variable; otherwise, delete the pointer.

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:63: m_UeControlInfo = new
UeRrcControlInformation();
Here you have to define the default value of each parameter of m_UeControlInfo

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:95: UeRrcEntity::m_UeControlInfo->m_dlbandwidth =
MibRecord->m_DL_bandwidth;
The better way is to use the following code:

UeRrcControlInformation* info = GetUeControlInfo ();
info->m_dlbandwidth = MibRecord->m_DL_bandwidth;

Use the same code for all the following methods.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:134: reqmsg->SetMessageType(
IdealControlMessage::RRC_ESTABLISHMENT);
The previous line is unnecessary because the message type is set into the
constructor of such  message.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:152: rejmsg->SetMessageType(
IdealControlMessage::RRC_CONN_RELEASE);
The previous line is unnecessary because the message type is set into the
constructor of such  message.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:169: reconfigmsg->SetMessageType(
IdealControlMessage::RRC_CONN_RECONFIG);
The previous line is unnecessary because the message type is set into the
constructor of such  message.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:185: //for sending control message for different
procedures.
The previous declaration generate doubts. Clarify better the mean of this
function.

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.h#ne...
src/lte/model/ue-rrc-entity.h:61: enum RrcStates {
Why  do you use the term "States" instead of "State" ?  I think that the UE can
have only one RRC state at a time.

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

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:75: //lte.EnableLogComponents ();
delete commented code

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:111: //EnbNetDevice* enb;
delete commented code

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:116: Ptr<UeRrcEntity> ueRrc =
DynamicCast<UeRrcEntity> (uerrcEntity);
previous 4 lines can be substituted with only two lines

For example:
Ptr<UeRrcEntity> ueRrc = ue->GetRrcEntity()->GetObject<UeRrcEntity> ();

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:126: 
The previous code seems to be ok. In what follows, instead, you must rewrite the
core of this test. 

As Nicola suggested in his review, you should use test macros NS_TEST_ASSERT_*
instead of printing output to stdout

For example, regarding the first check,
you can write:

NS_TEST_ASSERT_MSG_EQ (ueRrc->GetUeControlInfo()->m_dlbandwidth,
SystemInfoMIB::N6, "m_dlbandwidth hasn't been set correctly");

Note that the test will work correctly if you have set the default rrc
parameters, as I have suggested to do.

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:175: ueRrc->SendProcessRrcReConfig(ue ,
enb);
Also in this case you must very that the enb have received messages and it have
modified properly parameters into the ue record.

You can use the same approach suggested in the previous case.

[Nicola Baldo, 2011-10-03 15:27:50]

Please find some detailed comments on the code below. Note that some issues that
I raised in my previous review (and that were for some reason marked as "done")
are still open (e.g.: tests, update of RRC state...)

I also have some generic concerns about this code, I'll come back on this issue
in a separate email.

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-net-device.h#n...
src/lte/model/enb-net-device.h:126: virtual void SetRrcEntity (Ptr<RrcEntity>
rrc) ;
doxygen missing

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.h:32: * This procedure is done when UE
camp on to the network or it is coming
how are these events modeled in the simulator?

http://codereview.appspot.com/5108041/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.h:33: * from idle-connected mode. In
this UE context (S-TMSI) is exchange
 what is "idle-connected mode"?

http://codereview.appspot.com/5108041/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.h:82: SignallingBearer
m_signallingBearer;
why this? Aren't RRC connection requests always sent on SRB0?

http://codereview.appspot.com/5108041/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.h:98: void SetRrcConcReqParam(Direction
direction, SignallingBearer bearer ,
"Conc"... a typo in the name of the method?

http://codereview.appspot.com/5108041/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.h:136: * \brief The Radio Resource
ReConfig message.
please in the doxygen use the correct naming according to 3GPP

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-net-device.h#ne...
src/lte/model/ue-net-device.h:115: virtual void SetRrcEntity (Ptr<RrcEntity>
rrc) ;
doxygen missing

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-record.h#newcod...
src/lte/model/ue-record.h:144: UeRrcControlInfo m_uerrcControlInfo;
should be m_ueRrcControlInfo

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:137: UeRrcEntity::m_UeControlInfo->m_rrcState =
RRC_CONNECTED;
This issue was already raised in the previous review:
http://codereview.appspot.com/4961075/diff/1/src/lte/model/ue-rrc-entity.cc#n...

I don't see why you marked it as "done" in the previous review, the problem is
still there: you should not change the state to RRC_CONNECTED upon just sending
the RRC connection request. You should do it when the UE receives a RRC
connection setup message from the eNB.

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

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:175: ueRrc->SendProcessRrcReConfig(ue ,
enb);
I agree with Giuseppe. Basically, all the RRC connection related messages are
untested. This was already pointed out in the previous review.

[nbaldo_cttc.es, 2011-10-03 17:00:38]

Hi Pankaj,


On 10/03/2011 05:27 PM, nicobaldo@gmail.com wrote:
> I also have some generic concerns about this code, I'll come back on
> this issue in a separate email.
>

here we are...

in general, I still miss the big picture of your code. You implemented
several RRC messages, but how are these used in the simulator? The only
code that I have seen in the simulator that causes the RRC messages to
be sent is in your test program. Are they used for anything else?

Furthermore, I see no interaction between the RRC and the rest of the
protocol stack. For example, when a new bearer is created after RRC
connection request or RRC connection reconfiguration, you should create
new RLC instances and tell the scheduler that a new bearer is allocated.
I don't see anything like this done in your code.


I also had a look at your documentation on the project wiki page:
  http://www.nsnam.org/wiki/index.php/LTE_RRC_Extension
This page constains lots of 3GPP terminology, but frankly speaking I
find it difficult to get a clear view of what your code is actually
doing. For example (citing content from the wiki page):

> Deliverables
>
> At the end of the project we will be able establish between UE and
> eNodeB the SRB0/SRB1/DRBs.

What do you mean? My understanding is that you implement SRB0 and SRB1
using IdealControlMessages, so they are assumed to be always
established.

As for DRBs, it's not clear to me what's your part of the
code that deals with them, and how this feature would be used e.g.
within a simulation program.


> And UE and EnodeB can able to perform following procedures Paging,

For paging I see you implemented the RRC message, but what is it used
for? in the wiki, you describe paging as:

> Paging: To inform for the incoming calls to UE paging messages are
> used.

which is correct. But in ns-3 we don't have incoming calls, so why do we
need paging messages?

> 1. System information: System configuration messages send to your
> from e-utran are system information messages. System information
> classified in following four broad categories. Master information
> block: Contains information parameter for initial access.

ok but the LTE module in ns-3-dev does not do "initial access", and to 
my understanding your code does not change this. So what is your MIB 
implementation useful for?


 > SIB1 contains information parameter for cell selection.

cell selection is not implemented, so why do you need this?


> SIB2 contains
> information parameter for common and shared channel.

what is the use for this information?



To summarize, as a next step for the review process, I would strongly
encourage you to document clearly what are the usable features of your
code, focusing on explaining what your code allows to do that it was not
possible to do before with ns-3, explaining how users will be able to
make use of this functionality, and how it will change the behavior of
the simulator and affect the simulation results. I think that providing
some clear information with this respect is a necessary condition for
the merge.


Regards,

Nicola

[pankaj, 2011-10-12 18:18:28]

Dear All,

Thanks for taking your time and effort in reviewing the code. 

I have updated the code and marked as done. However, there are certein issues
which have been raised in the reviews which need more discussion before things
to be close.
 
Please send your feedback and comments

Regards,
Pankaj k Gupta

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-net-device.h#n...
src/lte/model/enb-net-device.h:126: virtual void SetRrcEntity (Ptr<RrcEntity>
rrc) ;
removed as per the previous review comment.

http://codereview.appspot.com/4961075/diff/1/src/lte/model/enb-net-device.h

On 2011/10/03 15:27:50, Nicola Baldo wrote:
> doxygen missing

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:65: *m_rrcStateInfo = RRC_IDLE;
deleted.

On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> Previous code are unnecessary. See the comment in the .h file. 
> 
> Otherwise, avoid to use the pointer.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:74: {
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> Delete it. See the previous comment

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:88: mibmsg->SetMessageType(
IdealControlMessage::SYSTEM_INFO_MIB);
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> The previous line is unnecessary because the message type is set into the
> constructor of such  message. 

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:97: Ptr<IdealControlMessage> msg = new
IdealControlMessage();
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> msg is not necessary. delete its definition

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:105: sibmsg->SetMessageType(
IdealControlMessage::SYSTEM_INFO_SIB);
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> The previous line is unnecessary because the message type is set into the
> constructor of such  message. 

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:122: pagingmsg->SetMessageType(
IdealControlMessage::PAGING);
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> The previous line is unnecessary because the message type is set into the
> constructor of such  message. 

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:178: //UeRecord* record;
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> delete commented code.

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.cc#...
src/lte/model/enb-rrc-entity.cc:194: void
EnbRrcEntity::ReceiveRrcIdealControlMessage(Ptr<IdealControlMessage> msg)
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> Regarding this method, I didn't understand why you have followed different
> approach for both enb and ue. 
> 
> For the ue, you identify the message type, convert the message definition and
> forward the pointer of such message to the proper procedure. 
> 
> For the enb, instead, you don't convert the definition of the message but
> forward the pointer to the IdealControlmessage. 
> 
> I suggest to use the same approach for both devices. Both of them are
correctly.
> It is just a way to have a uniform code :-) 

Done.

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.h#n...
src/lte/model/enb-rrc-entity.h:56: enum RrcStates {
This enum is used in enb-rrc-entity class. It is better not to delete this.

On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> Why you have defined this variable for the eNB ? 
> In particular, what does "RRC_IDLE" mean ? What happen if you have more than
one
> active rrc connection ?  
> 
> Probably, for then moment we have to delete the variable describing the
status.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.h#n...
src/lte/model/enb-rrc-entity.h:68: RrcStates* GetRrcState();
As of now we are not using this function. So, we can delete this function.

On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> Maybe it should be deleted. 
> Furthermore, why you have defined the status as a pointer ? Please, don't use
> the pointer.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/enb-rrc-entity.h#n...
src/lte/model/enb-rrc-entity.h:141: RrcStates* m_rrcStateInfo;
deleted
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> I sugegst to delete this variable; otherwise, delete the pointer.

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.h:32: * This procedure is done when UE
camp on to the network or it is coming
I understand the concern regarding how it is going to help in simulation. The
messages has been implemented using IdealControlMessages which limits the actual
radio resource usage. Project was aimed to provide all this. However, due to
limited time and resources we settled down to provide a basic framework for
signalling messages which was missing in the current simulator. For the
simulation purpose one can always update it as per the scenerio.

further discussion is required.

On 2011/10/03 15:27:50, Nicola Baldo wrote:
> how are these events modeled in the simulator?

http://codereview.appspot.com/5108041/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.h:33: * from idle-connected mode. In
this UE context (S-TMSI) is exchange
typo, it should be "RRC_IDLE to RRC_CONNECTED mode.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.h:82: SignallingBearer
m_signallingBearer;
yes, RRC connection always go on SRB0.
This is just to store the information.

It is not neccesary to keep this.

On 2011/10/03 15:27:50, Nicola Baldo wrote:
> why this? Aren't RRC connection requests always sent on SRB0?

http://codereview.appspot.com/5108041/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.h:98: void SetRrcConcReqParam(Direction
direction, SignallingBearer bearer ,
On 2011/10/03 15:27:50, Nicola Baldo wrote:
> "Conc"... a typo in the name of the method?

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/rrc-ideal-control-...
src/lte/model/rrc-ideal-control-messages.h:136: * \brief The Radio Resource
ReConfig message.
edited
On 2011/10/03 15:27:50, Nicola Baldo wrote:
> please in the doxygen use the correct naming according to 3GPP

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-net-device.h#ne...
src/lte/model/ue-net-device.h:115: virtual void SetRrcEntity (Ptr<RrcEntity>
rrc) ;
It has been removed as per the previous review comments.

http://codereview.appspot.com/4961075/diff/1/src/lte/model/ue-net-device.h
 
These functions are defined as pure virtual functions in LteNetDevice and hence
the doxygen is updated added. 




On 2011/10/03 15:27:50, Nicola Baldo wrote:
> doxygen missing

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-record.h#newcod...
src/lte/model/ue-record.h:144: UeRrcControlInfo m_uerrcControlInfo;
On 2011/10/03 15:27:50, Nicola Baldo wrote:
> should be m_ueRrcControlInfo

Done.

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:63: m_UeControlInfo = new
UeRrcControlInformation();
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> Here you have to define the default value of each parameter of m_UeControlInfo

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:95: UeRrcEntity::m_UeControlInfo->m_dlbandwidth =
MibRecord->m_DL_bandwidth;
Updated.

On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> The better way is to use the following code:
> 
> UeRrcControlInformation* info = GetUeControlInfo ();
> info->m_dlbandwidth = MibRecord->m_DL_bandwidth;
> 
> Use the same code for all the following methods.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:134: reqmsg->SetMessageType(
IdealControlMessage::RRC_ESTABLISHMENT);
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> The previous line is unnecessary because the message type is set into the
> constructor of such  message. 

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:137: UeRrcEntity::m_UeControlInfo->m_rrcState =
RRC_CONNECTED;
I agree with the comments. It needs changes in the existing code which includes
addition of more messages as mentioned RRC connection setup message.

On 2011/10/03 15:27:50, Nicola Baldo wrote:
> This issue was already raised in the previous review:
>
http://codereview.appspot.com/4961075/diff/1/src/lte/model/ue-rrc-entity.cc#n...
> 
> I don't see why you marked it as "done" in the previous review, the problem is
> still there: you should not change the state to RRC_CONNECTED upon just
sending
> the RRC connection request. You should do it when the UE receives a RRC
> connection setup message from the eNB.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:152: rejmsg->SetMessageType(
IdealControlMessage::RRC_CONN_RELEASE);
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> The previous line is unnecessary because the message type is set into the
> constructor of such  message. 

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:169: reconfigmsg->SetMessageType(
IdealControlMessage::RRC_CONN_RECONFIG);
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> The previous line is unnecessary because the message type is set into the
> constructor of such  message. 

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.cc#n...
src/lte/model/ue-rrc-entity.cc:185: //for sending control message for different
procedures.
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> The previous declaration generate doubts. Clarify better the mean of this
> function. 

Done.

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

http://codereview.appspot.com/5108041/diff/1/src/lte/model/ue-rrc-entity.h#ne...
src/lte/model/ue-rrc-entity.h:61: enum RrcStates {
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> Why  do you use the term "States" instead of "State" ?  I think that the UE
can
> have only one RRC state at a time. 

Done.

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

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:75: //lte.EnableLogComponents ();
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> delete commented code

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:111: //EnbNetDevice* enb;
On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> delete commented code

Done.

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:116: Ptr<UeRrcEntity> ueRrc =
DynamicCast<UeRrcEntity> (uerrcEntity);
updated.

On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> previous 4 lines can be substituted with only two lines
> 
> For example:
> Ptr<UeRrcEntity> ueRrc = ue->GetRrcEntity()->GetObject<UeRrcEntity> ();

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:126: 
I have used NS_TEST_ASSERT_MSG_EQ. Considering other points mentioned in the
email, i have updated the test cases. 

On 2011/09/30 13:59:13, Piro Giuseppe wrote:
> The previous code seems to be ok. In what follows, instead, you must rewrite
the
> core of this test. 
> 
> As Nicola suggested in his review, you should use test macros NS_TEST_ASSERT_*
> instead of printing output to stdout
> 
> For example, regarding the first check,
> you can write:
> 
> NS_TEST_ASSERT_MSG_EQ (ueRrc->GetUeControlInfo()->m_dlbandwidth,
> SystemInfoMIB::N6, "m_dlbandwidth hasn't been set correctly");
> 
> Note that the test will work correctly if you have set the default rrc
> parameters, as I have suggested to do.
>

http://codereview.appspot.com/5108041/diff/1/src/lte/test/lte-test-rrc-messag...
src/lte/test/lte-test-rrc-messages.cc:175: ueRrc->SendProcessRrcReConfig(ue ,
enb);
Earlier there is no parameter is send from the SendRRC... APIs so, i was not
checking the parameter updation. 

Now, i have modified the signature of send APIs for these three and added
validation test as i did in other messages.

On 2011/10/03 15:27:50, Nicola Baldo wrote:
> I agree with Giuseppe. Basically, all the RRC connection related messages are
> untested. This was already pointed out in the previous review.