Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(60)

Issue 4901055: NSoC final code review 2011 :LTE RRC extension

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 10 months ago by pankaj
Modified:
10 years, 10 months ago
Reviewers:
Piro Giuseppe
CC:
Lalith Suresh, pankaj, Tom Henderson
Visibility:
Public.

Patch Set 1 #

Total comments: 90
Unified diffs Side-by-side diffs Delta from patch set Stats (+1918 lines, -107 lines) Patch
M src/lte/model/enb-net-device.cc View 4 chunks +4 lines, -3 lines 4 comments Download
A src/lte/model/enb-rrc-cp-entity.h View 1 chunk +172 lines, -0 lines 13 comments Download
A src/lte/model/enb-rrc-cp-entity.cc View 1 chunk +208 lines, -0 lines 11 comments Download
M src/lte/model/ideal-control-messages.h View 1 chunk +1 line, -1 line 0 comments Download
M src/lte/model/rrc-entity.h View 3 chunks +53 lines, -0 lines 8 comments Download
M src/lte/model/rrc-entity.cc View 3 chunks +52 lines, -10 lines 15 comments Download
A src/lte/model/rrc-ideal-control-messages.h View 1 chunk +522 lines, -0 lines 0 comments Download
A src/lte/model/rrc-ideal-control-messages.cc View 1 chunk +230 lines, -0 lines 1 comment Download
M src/lte/model/ue-net-device.h View 2 chunks +2 lines, -3 lines 3 comments Download
M src/lte/model/ue-net-device.cc View 2 chunks +4 lines, -3 lines 0 comments Download
M src/lte/model/ue-record.h View 2 chunks +95 lines, -60 lines 11 comments Download
M src/lte/model/ue-record.cc View 1 chunk +20 lines, -21 lines 2 comments Download
A src/lte/model/ue-rrc-cp-entity.h View 1 chunk +160 lines, -0 lines 3 comments Download
A src/lte/model/ue-rrc-cp-entity.cc View 1 chunk +193 lines, -0 lines 16 comments Download
A src/lte/test/lte-test-rrc-messages.cc View 1 chunk +142 lines, -0 lines 1 comment Download
M src/lte/wscript View 3 chunks +9 lines, -6 lines 0 comments Download
M src/point-to-point/bindings/modulegen__gcc_ILP32.py View 2 chunks +51 lines, -0 lines 2 comments Download

Messages

Total messages: 2
Piro Giuseppe
Hi Pankaj, I have reviewed your code. Unfortunately the code must be improved a lot ...
10 years, 10 months ago (2011-08-22 11:30:37 UTC) #1
pankaj
10 years, 10 months ago (2011-09-06 06:05:39 UTC) #2
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b