release bearer

[Nicola Baldo, 2014-11-28 18:00:38]

Overall, very good work almost ready for merge!
Please find some requested modifications to the code below. More comments in a
separate email.

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

https://codereview.appspot.com/176380043/diff/1/src/lte/helper/lte-helper.h#n...
src/lte/helper/lte-helper.h:338: void DeActivateDedicatedEpsBearer (Time
deActivateTime, Ptr<NetDevice> ueDevice, Ptr<NetDevice> enbDevice, uint8_t
bearerId);
For the sake of ease of use of the helper, I think we shall make the
DeActivateDedicatedEpsBearer have similar and compatible semantics with
ActivateDedicatedEpsBearer, which is pre-existing. To this aim, I suggest:

1) remove the Time parameter (ActivateDedicatedEpsBearer does not have it) and
make the method instantaneous. When a delay is needed, let the user program take
care of that by explicitly calling Simulator::Schedule(). 
(I understand there is a pre-existing method HandoverRequest() which has a
similar Time parameter, but in fact that method is not typically used (HO
algorithms are typically expected to be used instead) and I'd actually deprecate
it)

2) modify ActivateDedicatedEpsBearer to return the uint8_t bearerId, so that the
user program will be able to store it to eventually call
DeActivateDedicatedEpsBearer later

https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h
File src/lte/model/epc-s1ap-sap.h (right):

https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h#...
src/lte/model/epc-s1ap-sap.h:73: * \brief As per 3GPP TS 23.401 Release 9 V9.5.0
Figure 5.4.4.2-1  eNB sends indication of Bearer Release to MME
please also add mention to TS 36.413 section 8.2.3.2.2 where this method is
described

https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...
File src/lte/test/lte-test-deactivate-bearer.cc (right):

https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...
src/lte/test/lte-test-deactivate-bearer.cc:137: Config::SetDefault
("ns3::LteHelper::UseIdealRrc", BooleanValue (true));
it is crucial to test with both IdealRrc and RealRrc. Please add a "bool
useIdealRrc" parameter to the test case constructor, so that you can create two
instances of the test, one for each setting, similar to what done in
src/lte/test/test-lte-rrc.cc

[gaurav.sathe88, 2014-12-08 07:39:07]

Hi Nicola and Team,

I have done necessary changes in the code and closed all review comments.
When we are trying to push changes on 
'http://code.nsnam.org/nbaldo/ns-3-release-bearer/ ' repository, following 
error occurs:

"
root@tcsuser-ThinkPad-T420:/home/test/ns-3-release-bearer# hg commit -m 
'Closure commnets of Release Bearer Feature'
nothing changed
root@tcsuser-ThinkPad-T420:/home/test/ns-3-release-bearer# hg push
pushing to http://code.nsnam.org/nbaldo/ns-3-release-bearer/
searching for changes
remote: ssl required
"

Request you to please provide solution for the same.

Regards,
Gaurav Sathe

On Friday, November 28, 2014 11:30:38 PM UTC+5:30, Nicola Baldo wrote:
>
> Reviewers: , 
>
> Message: 
> Overall, very good work almost ready for merge! 
> Please find some requested modifications to the code below. More 
> comments in a separate email. 
>
>
> https://codereview.appspot.com/176380043/diff/1/src/lte/helper/lte-helper.h 
> File src/lte/helper/lte-helper.h (right): 
>
>
https://codereview.appspot.com/176380043/diff/1/src/lte/helper/lte-helper.h#n...

>
> src/lte/helper/lte-helper.h:338 
>
<https://codereview.appspot.com/176380043/diff/1/src/lte/helper/lte-helper.h#n...>:

> void DeActivateDedicatedEpsBearer (Time 
> deActivateTime, Ptr<NetDevice> ueDevice, Ptr<NetDevice> enbDevice, 
> uint8_t bearerId); 
> For the sake of ease of use of the helper, I think we shall make the 
> DeActivateDedicatedEpsBearer have similar and compatible semantics with 
> ActivateDedicatedEpsBearer, which is pre-existing. To this aim, I 
> suggest: 
>
> 1) remove the Time parameter (ActivateDedicatedEpsBearer does not have 
> it) and make the method instantaneous. When a delay is needed, let the 
> user program take care of that by explicitly calling 
> Simulator::Schedule(). 
> (I understand there is a pre-existing method HandoverRequest() which has 
> a similar Time parameter, but in fact that method is not typically used 
> (HO algorithms are typically expected to be used instead) and I'd 
> actually deprecate it) 
>
> 2) modify ActivateDedicatedEpsBearer to return the uint8_t bearerId, so 
> that the user program will be able to store it to eventually call 
> DeActivateDedicatedEpsBearer later 
>
>
> https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h 
> File src/lte/model/epc-s1ap-sap.h (right): 
>
>
https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h#...

>
> src/lte/model/epc-s1ap-sap.h:73 
>
<https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h#...>:

> * \brief As per 3GPP TS 23.401 Release 
> 9 V9.5.0 Figure 5.4.4.2-1  eNB sends indication of Bearer Release to MME 
> please also add mention to TS 36.413 section 8.2.3.2.2 where this method 
> is described 
>
>
>
https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...

> File src/lte/test/lte-test-deactivate-bearer.cc (right): 
>
>
https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...

>
> src/lte/test/lte-test-deactivate-bearer.cc:137 
>
<https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...>:

> Config::SetDefault 
> ("ns3::LteHelper::UseIdealRrc", BooleanValue (true)); 
> it is crucial to test with both IdealRrc and RealRrc. Please add a "bool 
> useIdealRrc" parameter to the test case constructor, so that you can 
> create two instances of the test, one for each setting, similar to what 
> done in src/lte/test/test-lte-rrc.cc 
>
>
>
> Please review this at https://codereview.appspot.com/176380043/ 
>
> Affected files (+1081, -58 lines): 
>    M AUTHORS 
>    M RELEASE_NOTES 
>    M src/lte/bindings/modulegen__gcc_ILP32.py 
>    A src/lte/examples/lena-deactivate-bearer.cc 
>    M src/lte/examples/wscript 
>    M src/lte/helper/lte-helper.h 
>    M src/lte/helper/lte-helper.cc 
>    M src/lte/helper/radio-bearer-stats-calculator.cc 
>    M src/lte/model/epc-enb-application.h 
>    M src/lte/model/epc-enb-application.cc 
>    M src/lte/model/epc-enb-s1-sap.h 
>    M src/lte/model/epc-mme.h 
>    M src/lte/model/epc-mme.cc 
>    M src/lte/model/epc-s11-sap.h 
>    M src/lte/model/epc-s1ap-sap.h 
>    M src/lte/model/epc-sgw-pgw-application.h 
>    M src/lte/model/epc-sgw-pgw-application.cc 
>    M src/lte/model/lte-enb-mac.cc 
>    M src/lte/model/lte-enb-rrc.h 
>    M src/lte/model/lte-enb-rrc.cc 
>    M src/lte/model/lte-ue-mac.cc 
>    M src/lte/model/lte-ue-rrc.cc 
>    A src/lte/test/lte-test-deactivate-bearer.h 
>    A src/lte/test/lte-test-deactivate-bearer.cc 
>    M src/lte/wscript 
>
>
>

[gaurav.sathe88, 2014-12-10 11:45:52]

Hi Nicola,

All the comments provided on https://codereview.appspot.com/176380043/  are
closed and changes are pushed on bit bucket. Following is the link to access
directory:
"https://gauravsathe@bitbucket.org/gauravsathe/ns3_release_bearer_feature"

I have followed below steps to push the changes on bit bucket:
1) Cloned code from http://code.nsnam.org/nbaldo/ns-3-release-bearer/ 
2) Closed the code comments 
3) Copied the code into Repository created using bitbucket
4) Pushed code into bit bucket directory
Let me know If you need any other details.

One  more query--
As mentioned in our initial emails, apart from Release Bearer Feature, our team
also implemented Modify Bearer Feature which is invoked when RRM (Radio resource
manager) triggers the pre-emption of extra resources from GBR bearers ( over and
above the GBR value). 
For this we have written few new functions like ModifyDataRadioBearer,
ReConfigureLC etc.
Will this contribution be helpful for NS3 as a future enhancement?

Regards,
Gaurav Sathe


On 2014/12/08 07:39:07, gaurav.sathe88 wrote:
> Hi Nicola and Team,
> 
> I have done necessary changes in the code and closed all review comments.
> When we are trying to push changes on 
> 'http://code.nsnam.org/nbaldo/ns-3-release-bearer/ ' repository, following 
> error occurs:
> 
> "
> root@tcsuser-ThinkPad-T420:/home/test/ns-3-release-bearer# hg commit -m 
> 'Closure commnets of Release Bearer Feature'
> nothing changed
> root@tcsuser-ThinkPad-T420:/home/test/ns-3-release-bearer# hg push
> pushing to http://code.nsnam.org/nbaldo/ns-3-release-bearer/
> searching for changes
> remote: ssl required
> "
> 
> Request you to please provide solution for the same.
> 
> Regards,
> Gaurav Sathe
> 
> On Friday, November 28, 2014 11:30:38 PM UTC+5:30, Nicola Baldo wrote:
> >
> > Reviewers: , 
> >
> > Message: 
> > Overall, very good work almost ready for merge! 
> > Please find some requested modifications to the code below. More 
> > comments in a separate email. 
> >
> >
> > https://codereview.appspot.com/176380043/diff/1/src/lte/helper/lte-helper.h 
> > File src/lte/helper/lte-helper.h (right): 
> >
> >
>
https://codereview.appspot.com/176380043/diff/1/src/lte/helper/lte-helper.h#n...
> 
> >
> > src/lte/helper/lte-helper.h:338 
> >
>
<https://codereview.appspot.com/176380043/diff/1/src/lte/helper/lte-helper.h#n...>:
> 
> > void DeActivateDedicatedEpsBearer (Time 
> > deActivateTime, Ptr<NetDevice> ueDevice, Ptr<NetDevice> enbDevice, 
> > uint8_t bearerId); 
> > For the sake of ease of use of the helper, I think we shall make the 
> > DeActivateDedicatedEpsBearer have similar and compatible semantics with 
> > ActivateDedicatedEpsBearer, which is pre-existing. To this aim, I 
> > suggest: 
> >
> > 1) remove the Time parameter (ActivateDedicatedEpsBearer does not have 
> > it) and make the method instantaneous. When a delay is needed, let the 
> > user program take care of that by explicitly calling 
> > Simulator::Schedule(). 
> > (I understand there is a pre-existing method HandoverRequest() which has 
> > a similar Time parameter, but in fact that method is not typically used 
> > (HO algorithms are typically expected to be used instead) and I'd 
> > actually deprecate it) 
> >
> > 2) modify ActivateDedicatedEpsBearer to return the uint8_t bearerId, so 
> > that the user program will be able to store it to eventually call 
> > DeActivateDedicatedEpsBearer later 
> >
> >
> > https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h

> > File src/lte/model/epc-s1ap-sap.h (right): 
> >
> >
>
https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h#...
> 
> >
> > src/lte/model/epc-s1ap-sap.h:73 
> >
>
<https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h#...>:
> 
> > * \brief As per 3GPP TS 23.401 Release 
> > 9 V9.5.0 Figure 5.4.4.2-1  eNB sends indication of Bearer Release to MME 
> > please also add mention to TS 36.413 section 8.2.3.2.2 where this method 
> > is described 
> >
> >
> >
>
https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...
> 
> > File src/lte/test/lte-test-deactivate-bearer.cc (right): 
> >
> >
>
https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...
> 
> >
> > src/lte/test/lte-test-deactivate-bearer.cc:137 
> >
>
<https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...>:
> 
> > Config::SetDefault 
> > ("ns3::LteHelper::UseIdealRrc", BooleanValue (true)); 
> > it is crucial to test with both IdealRrc and RealRrc. Please add a "bool 
> > useIdealRrc" parameter to the test case constructor, so that you can 
> > create two instances of the test, one for each setting, similar to what 
> > done in src/lte/test/test-lte-rrc.cc 
> >
> >
> >
> > Please review this at https://codereview.appspot.com/176380043/ 
> >
> > Affected files (+1081, -58 lines): 
> >    M AUTHORS 
> >    M RELEASE_NOTES 
> >    M src/lte/bindings/modulegen__gcc_ILP32.py 
> >    A src/lte/examples/lena-deactivate-bearer.cc 
> >    M src/lte/examples/wscript 
> >    M src/lte/helper/lte-helper.h 
> >    M src/lte/helper/lte-helper.cc 
> >    M src/lte/helper/radio-bearer-stats-calculator.cc 
> >    M src/lte/model/epc-enb-application.h 
> >    M src/lte/model/epc-enb-application.cc 
> >    M src/lte/model/epc-enb-s1-sap.h 
> >    M src/lte/model/epc-mme.h 
> >    M src/lte/model/epc-mme.cc 
> >    M src/lte/model/epc-s11-sap.h 
> >    M src/lte/model/epc-s1ap-sap.h 
> >    M src/lte/model/epc-sgw-pgw-application.h 
> >    M src/lte/model/epc-sgw-pgw-application.cc 
> >    M src/lte/model/lte-enb-mac.cc 
> >    M src/lte/model/lte-enb-rrc.h 
> >    M src/lte/model/lte-enb-rrc.cc 
> >    M src/lte/model/lte-ue-mac.cc 
> >    M src/lte/model/lte-ue-rrc.cc 
> >    A src/lte/test/lte-test-deactivate-bearer.h 
> >    A src/lte/test/lte-test-deactivate-bearer.cc 
> >    M src/lte/wscript 
> >
> >
> >

[gaurav.sathe88, 2014-12-19 11:56:34]

Hi Nicola,

Do we have any update on this? Are the code changes ready to merge?

Regards,
Gaurav Sathe

On Friday, November 28, 2014 11:30:38 PM UTC+5:30, Nicola Baldo wrote:
>
> Reviewers: , 
>
> Message: 
> Overall, very good work almost ready for merge! 
> Please find some requested modifications to the code below. More 
> comments in a separate email. 
>
>
> https://codereview.appspot.com/176380043/diff/1/src/lte/helper/lte-helper.h 
> File src/lte/helper/lte-helper.h (right): 
>
>
https://codereview.appspot.com/176380043/diff/1/src/lte/helper/lte-helper.h#n...

>
> src/lte/helper/lte-helper.h:338 
>
<https://codereview.appspot.com/176380043/diff/1/src/lte/helper/lte-helper.h#n...>:

> void DeActivateDedicatedEpsBearer (Time 
> deActivateTime, Ptr<NetDevice> ueDevice, Ptr<NetDevice> enbDevice, 
> uint8_t bearerId); 
> For the sake of ease of use of the helper, I think we shall make the 
> DeActivateDedicatedEpsBearer have similar and compatible semantics with 
> ActivateDedicatedEpsBearer, which is pre-existing. To this aim, I 
> suggest: 
>
> 1) remove the Time parameter (ActivateDedicatedEpsBearer does not have 
> it) and make the method instantaneous. When a delay is needed, let the 
> user program take care of that by explicitly calling 
> Simulator::Schedule(). 
> (I understand there is a pre-existing method HandoverRequest() which has 
> a similar Time parameter, but in fact that method is not typically used 
> (HO algorithms are typically expected to be used instead) and I'd 
> actually deprecate it) 
>
> 2) modify ActivateDedicatedEpsBearer to return the uint8_t bearerId, so 
> that the user program will be able to store it to eventually call 
> DeActivateDedicatedEpsBearer later 
>
>
> https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h 
> File src/lte/model/epc-s1ap-sap.h (right): 
>
>
https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h#...

>
> src/lte/model/epc-s1ap-sap.h:73 
>
<https://codereview.appspot.com/176380043/diff/1/src/lte/model/epc-s1ap-sap.h#...>:

> * \brief As per 3GPP TS 23.401 Release 
> 9 V9.5.0 Figure 5.4.4.2-1  eNB sends indication of Bearer Release to MME 
> please also add mention to TS 36.413 section 8.2.3.2.2 where this method 
> is described 
>
>
>
https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...

> File src/lte/test/lte-test-deactivate-bearer.cc (right): 
>
>
https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...

>
> src/lte/test/lte-test-deactivate-bearer.cc:137 
>
<https://codereview.appspot.com/176380043/diff/1/src/lte/test/lte-test-deactiv...>:

> Config::SetDefault 
> ("ns3::LteHelper::UseIdealRrc", BooleanValue (true)); 
> it is crucial to test with both IdealRrc and RealRrc. Please add a "bool 
> useIdealRrc" parameter to the test case constructor, so that you can 
> create two instances of the test, one for each setting, similar to what 
> done in src/lte/test/test-lte-rrc.cc 
>
>
>
> Please review this at https://codereview.appspot.com/176380043/ 
>
> Affected files (+1081, -58 lines): 
>    M AUTHORS 
>    M RELEASE_NOTES 
>    M src/lte/bindings/modulegen__gcc_ILP32.py 
>    A src/lte/examples/lena-deactivate-bearer.cc 
>    M src/lte/examples/wscript 
>    M src/lte/helper/lte-helper.h 
>    M src/lte/helper/lte-helper.cc 
>    M src/lte/helper/radio-bearer-stats-calculator.cc 
>    M src/lte/model/epc-enb-application.h 
>    M src/lte/model/epc-enb-application.cc 
>    M src/lte/model/epc-enb-s1-sap.h 
>    M src/lte/model/epc-mme.h 
>    M src/lte/model/epc-mme.cc 
>    M src/lte/model/epc-s11-sap.h 
>    M src/lte/model/epc-s1ap-sap.h 
>    M src/lte/model/epc-sgw-pgw-application.h 
>    M src/lte/model/epc-sgw-pgw-application.cc 
>    M src/lte/model/lte-enb-mac.cc 
>    M src/lte/model/lte-enb-rrc.h 
>    M src/lte/model/lte-enb-rrc.cc 
>    M src/lte/model/lte-ue-mac.cc 
>    M src/lte/model/lte-ue-rrc.cc 
>    A src/lte/test/lte-test-deactivate-bearer.h 
>    A src/lte/test/lte-test-deactivate-bearer.cc 
>    M src/lte/wscript 
>
>
>