GSoC 2014 FFR algorithms mid-term code review

GSoC 2014 FFR algorithms mid-term code review

[Nicola Baldo, 2014-06-24 21:00:29]

Hi Piotr, 

here are some preliminary comments focusing mostly on the newly introduced
interfaces; more comments to come later.

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
File src/lte/model/lte-ffr-algorithm.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.cc:55: .AddAttribute ("UlBandwidth",
If this UlBandwidth is meant to be an exact duplicate of
ns3::LteEnbNetDevice::UlBandwidth, then I am absolutely against making it an
attribute that is configurable by the user. 

Instead, the value shall be configured by the LteEnbRrc upon execution of
LteEnbRrc::ConfigureCell() by calling a dedicated method of LteFfrRrcSapProvider
(to be implemented)

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.cc:67: .AddAttribute ("FfrCellId",
Please explain in the description what this attribute does and how it shall be
used. 

I wonder whether this attribute really has exactly the same meaning for any
possible FFR algorithm, or rather only makes sense for some algorithms (perhaps
many, but not all). In the latter case, I'd move it to sub-classes, even if it
means duplicating it a few times.

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
File src/lte/model/lte-ffr-algorithm.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.h:37: class LteFfrAlgorithm : public Object
please add doxygen

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-rrc-sap.h
File src/lte/model/lte-ffr-rrc-sap.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-rrc-sap...
src/lte/model/lte-ffr-rrc-sap.h:29: class LteFfrRrcSapProvider
please add doxygen

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-rrc-sap...
src/lte/model/lte-ffr-rrc-sap.h:42: class LteFfrRrcSapUser
please add doxygen

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-sap.h
File src/lte/model/lte-ffr-sap.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-sap.h#n...
src/lte/model/lte-ffr-sap.h:30: class LteFfrSapProvider
please add doxygen

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-sap.h#n...
src/lte/model/lte-ffr-sap.h:44: class LteFfrSapUser
please add doxygen

https://codereview.appspot.com/110020049/diff/1/src/lte/test/lte-simple-spect...
File src/lte/test/lte-simple-spectrum-phy.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/test/lte-simple-spect...
src/lte/test/lte-simple-spectrum-phy.h:38: * The LteSimpleSpectrumPhy models the
physical layer of LTE
please revise this comment explaining the actual purpose of this class

[gawlowicz.p, 2014-06-26 22:50:24]

code improvements

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
File src/lte/model/lte-ffr-algorithm.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.cc:55: .AddAttribute ("UlBandwidth",
On 2014/06/24 21:00:29, Nicola Baldo wrote:
> If this UlBandwidth is meant to be an exact duplicate of
> ns3::LteEnbNetDevice::UlBandwidth, then I am absolutely against making it an
> attribute that is configurable by the user. 
> 
> Instead, the value shall be configured by the LteEnbRrc upon execution of
> LteEnbRrc::ConfigureCell() by calling a dedicated method of
LteFfrRrcSapProvider
> (to be implemented) 

Done.

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.cc:67: .AddAttribute ("FfrCellId",
On 2014/06/24 21:00:29, Nicola Baldo wrote:
> Please explain in the description what this attribute does and how it shall be
> used. 
> 
> I wonder whether this attribute really has exactly the same meaning for any
> possible FFR algorithm, or rather only makes sense for some algorithms
(perhaps
> many, but not all). In the latter case, I'd move it to sub-classes, even if it
> means duplicating it a few times.

It is useful in each implemented FR algorithm so far. Maybe the name is a bit
confusing. Will FrConfigurationId be better?

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
File src/lte/model/lte-ffr-algorithm.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.h:37: class LteFfrAlgorithm : public Object
On 2014/06/24 21:00:29, Nicola Baldo wrote:
> please add doxygen

Done.

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-rrc-sap.h
File src/lte/model/lte-ffr-rrc-sap.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-rrc-sap...
src/lte/model/lte-ffr-rrc-sap.h:29: class LteFfrRrcSapProvider
On 2014/06/24 21:00:29, Nicola Baldo wrote:
> please add doxygen

Done.

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-rrc-sap...
src/lte/model/lte-ffr-rrc-sap.h:42: class LteFfrRrcSapUser
On 2014/06/24 21:00:29, Nicola Baldo wrote:
> please add doxygen

Done.

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-sap.h
File src/lte/model/lte-ffr-sap.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-sap.h#n...
src/lte/model/lte-ffr-sap.h:30: class LteFfrSapProvider
On 2014/06/24 21:00:29, Nicola Baldo wrote:
> please add doxygen

Done.

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-sap.h#n...
src/lte/model/lte-ffr-sap.h:44: class LteFfrSapUser
On 2014/06/24 21:00:29, Nicola Baldo wrote:
> please add doxygen

Done.

https://codereview.appspot.com/110020049/diff/1/src/lte/test/lte-simple-spect...
File src/lte/test/lte-simple-spectrum-phy.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/test/lte-simple-spect...
src/lte/test/lte-simple-spectrum-phy.h:38: * The LteSimpleSpectrumPhy models the
physical layer of LTE
On 2014/06/24 21:00:29, Nicola Baldo wrote:
> please revise this comment explaining the actual purpose of this class

Done.

[Ruben Martinez, 2014-07-02 09:53:03]

Hello Piotr,

The code looks fairly well done. I just have a few minor comments, mostly
regarding logging and documentation.

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

https://codereview.appspot.com/110020049/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:244: {
NS_LOG_FUNCTION(this);

https://codereview.appspot.com/110020049/diff/1/src/lte/model/cqa-ff-mac-sche...
File src/lte/model/cqa-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/cqa-ff-mac-sche...
src/lte/model/cqa-ff-mac-scheduler.cc:353: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/cqa-ff-mac-sche...
src/lte/model/cqa-ff-mac-scheduler.cc:360: return m_ffrSapUser;
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/cqa-ff-mac-sche...
File src/lte/model/cqa-ff-mac-scheduler.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/cqa-ff-mac-sche...
src/lte/model/cqa-ff-mac-scheduler.h:249: // FFR SAPs
Is smart pointer usage not possible here?

https://codereview.appspot.com/110020049/diff/1/src/lte/model/fdmt-ff-mac-sch...
File src/lte/model/fdmt-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/fdmt-ff-mac-sch...
src/lte/model/fdmt-ff-mac-scheduler.cc:299: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/fdmt-ff-mac-sch...
src/lte/model/fdmt-ff-mac-scheduler.cc:305: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/fdtbfq-ff-mac-s...
File src/lte/model/fdtbfq-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/fdtbfq-ff-mac-s...
src/lte/model/fdtbfq-ff-mac-scheduler.cc:322: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/fdtbfq-ff-mac-s...
src/lte/model/fdtbfq-ff-mac-scheduler.cc:328: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
File src/lte/model/lte-ffr-algorithm.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.cc:35: };  // see table 7.1.6.1-1 of 36.213
It may be obvious for someone familiar with LTE but It wasn't for me. I would
recommend something more detailed like:

see table 7.1.6.1-1 of 3GPP TS 36.213

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-soft-al...
File src/lte/model/lte-ffr-soft-algorithm.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-soft-al...
src/lte/model/lte-ffr-soft-algorithm.cc:39: }
g_ffrSoftDownlinkDefaultConfiguration[] = {
Are these values defined somewhere? If so, could you provide reference?

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-soft-al...
src/lte/model/lte-ffr-soft-algorithm.cc:54: static const struct
FfrSoftUplinkDefaultConfiguration
Same

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-fr-hard-alg...
File src/lte/model/lte-fr-hard-algorithm.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-fr-hard-alg...
src/lte/model/lte-fr-hard-algorithm.cc:227: if(1) {
Is this for debug purposes?

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-spectrum-ph...
File src/lte/model/lte-spectrum-phy.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-spectrum-ph...
src/lte/model/lte-spectrum-phy.cc:1126: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-spectrum-phy.h
File src/lte/model/lte-spectrum-phy.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-spectrum-ph...
src/lte/model/lte-spectrum-phy.h:335: *
No \brief provided

https://codereview.appspot.com/110020049/diff/1/src/lte/model/pf-ff-mac-sched...
File src/lte/model/pf-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/pf-ff-mac-sched...
src/lte/model/pf-ff-mac-scheduler.cc:302: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/pf-ff-mac-sched...
src/lte/model/pf-ff-mac-scheduler.cc:308: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/pss-ff-mac-sche...
File src/lte/model/pss-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/pss-ff-mac-sche...
src/lte/model/pss-ff-mac-scheduler.cc:313: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/pss-ff-mac-sche...
src/lte/model/pss-ff-mac-scheduler.cc:319: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/rr-ff-mac-sched...
File src/lte/model/rr-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/rr-ff-mac-sched...
src/lte/model/rr-ff-mac-scheduler.cc:301: RrFfMacScheduler::SetLteFfrSapProvider
(LteFfrSapProvider* s)
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/rr-ff-mac-sched...
src/lte/model/rr-ff-mac-scheduler.cc:308: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/tdbet-ff-mac-sc...
File src/lte/model/tdbet-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/tdbet-ff-mac-sc...
src/lte/model/tdbet-ff-mac-scheduler.cc:300: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/tdbet-ff-mac-sc...
src/lte/model/tdbet-ff-mac-scheduler.cc:306: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/tdmt-ff-mac-sch...
File src/lte/model/tdmt-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/tdmt-ff-mac-sch...
src/lte/model/tdmt-ff-mac-scheduler.cc:299: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/tdmt-ff-mac-sch...
src/lte/model/tdmt-ff-mac-scheduler.cc:305: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/tdtbfq-ff-mac-s...
File src/lte/model/tdtbfq-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/tdtbfq-ff-mac-s...
src/lte/model/tdtbfq-ff-mac-scheduler.cc:322: {
NS_LOG_FUNCTION

https://codereview.appspot.com/110020049/diff/1/src/lte/model/tdtbfq-ff-mac-s...
src/lte/model/tdtbfq-ff-mac-scheduler.cc:328: {
NS_LOG_FUNCTION

[gawlowicz.p, 2014-07-04 22:39:47]

Hi Ruben,

Thank you for your time! I have addressed your comments.

Best regards,
Piotr

https://codereview.appspot.com/110020049/diff/1/src/lte/model/cqa-ff-mac-sche...
File src/lte/model/cqa-ff-mac-scheduler.h (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/cqa-ff-mac-sche...
src/lte/model/cqa-ff-mac-scheduler.h:249: // FFR SAPs
Probably it is possible, but all SAPs use raw pointers

On 2014/07/02 09:53:02, Ruben Martinez wrote:
> Is smart pointer usage not possible here?

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
File src/lte/model/lte-ffr-algorithm.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-algorit...
src/lte/model/lte-ffr-algorithm.cc:35: };  // see table 7.1.6.1-1 of 36.213
On 2014/07/02 09:53:02, Ruben Martinez wrote:
> It may be obvious for someone familiar with LTE but It wasn't for me. I would
> recommend something more detailed like:
> 
> see table 7.1.6.1-1 of 3GPP TS 36.213 
> 

Done.

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-soft-al...
File src/lte/model/lte-ffr-soft-algorithm.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-ffr-soft-al...
src/lte/model/lte-ffr-soft-algorithm.cc:39: }
g_ffrSoftDownlinkDefaultConfiguration[] = {
No I have not found these values anywhere, it is only example/default
configuration

On 2014/07/02 09:53:02, Ruben Martinez wrote:
> Are these values defined somewhere? If so, could you provide reference?

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-fr-hard-alg...
File src/lte/model/lte-fr-hard-algorithm.cc (right):

https://codereview.appspot.com/110020049/diff/1/src/lte/model/lte-fr-hard-alg...
src/lte/model/lte-fr-hard-algorithm.cc:227: if(1) {
Yes, I will clean it 

On 2014/07/02 09:53:02, Ruben Martinez wrote:
> Is this for debug purposes?