GSoC 2015 - MidTerm Milestone - Carrier Aggregation

This is code review request for GSoC 2015, LTE CA Support.

This code is related to the first 5 weeks of the project (see https://www.nsnam.org/wiki/GSOC2015LTECA)

This code review is incremental wrt https://codereview.appspot.com/241320043/

[dabrignani, 2015-06-26 16:37:38]

This is code review request for GSoC 2015, LTE CA Support.

This code is related to the first 5 weeks of the project (see
https://www.nsnam.org/wiki/GSOC2015LTECA)

This patch is incremental with respect to
https://codereview.appspot.com/241320043/

[buherman, 2015-07-01 13:53:35]

Hi Danilo,

I have some small comments to your code.

I find that you commented out some existing code. The deliverable of your
project should not contain those commented code, because they may confuse users
and other developers. I'd rather remove those commented code from now before we
forget later.

I also find some cases where the indentation of an existing file has completely
changed, for example in lte-rrc-sap.cc. In this case, everything under
`namespace ns3` becomes indented, which is not necessary in my opinion. It also
blocks people from reviewing the actual changes.

Finally, I notice that you sometimes add a space immediately inside brackets,
e.g., `if ( x == 0 )`. For the sake of consistency with the rest of the code in
LTE module, it would be better if you use `if (x == 0)` instead.

And the following is some inline comments.

-budi-

https://codereview.appspot.com/249960043/diff/1/src/lte/examples/lena-simple.cc
File src/lte/examples/lena-simple.cc (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/examples/lena-simple....
src/lte/examples/lena-simple.cc:92: config.ConfigureAttributes ();
I think it's not a good idea to enable this.

https://codereview.appspot.com/249960043/diff/1/src/lte/examples/wscript
File src/lte/examples/wscript (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/examples/wscript#newc...
src/lte/examples/wscript:60: obj.source = 'lena-componentcarrierTest.cc'
This file does not exist.

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.cc#n...
src/lte/helper/cc-helper.cc:32: #define MAC_CC 2
MAC_CC -> MAX_CC

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.cc#n...
src/lte/helper/cc-helper.cc:58: .AddAttribute ("NoOfCcs",
As far as I know, I think the convention for this kind of attribute is
"NumberOf...", for example "NumberOfRaPreambles" in LteEnbMac.

And then "Ccs" may wrongly suggest "component carrier selection", so I suggest
expanding it to "ComponentCarriers".

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.h#ne...
src/lte/helper/cc-helper.h:60: class CcHelper : public Object
I've seen usage of either "ComponentCarrier" and "Cc" in names. I suggest to
make them consistent, e.g., by sticking to "ComponentCarrier". In this helper
class, for instance, that would be CcHelper -> ComponentCarrierHelper,
SetCcAttribute() -> SetComponentCarrierAttribute(), and so on.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.h#ne...
src/lte/helper/cc-helper.h:73: Ptr<ComponentCarrier> DoCreateSingleCc (uint16_t
ulBw, uint16_t dlBw, uint16_t ulFreq, uint16_t dlFreq, bool pc);
Why the split into CreateSingleCc() and DoCreateSingleCc()?

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.h#ne...
src/lte/helper/cc-helper.h:73: Ptr<ComponentCarrier> DoCreateSingleCc (uint16_t
ulBw, uint16_t dlBw, uint16_t ulFreq, uint16_t dlFreq, bool pc);
I think the arguments can be improved here. 
- Bandwidth is supposed to be uint8_t.
- "Frequency" may suggest Hz, so in this case I prefer "Earfcn"
- "Bw" -> "Bandwidth"
- "pc" -> "isPrimary"
The above also applies to the attributes of this class.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.h#ne...
src/lte/helper/cc-helper.h:119: * \param dlFreq downlink EARFCN - not control on
the validity at this point
Why not controlled? Will it be controlled at later point?

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:189: "If it is more than one and m_useCa is false,
it will rise an error ",
rise -> raise

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:651: tmpPhy->GetDlSpectrumPhy ()->SetDevice (dev);
Duplicate?

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.h#n...
src/lte/helper/lte-helper.h:113: void ChanelModelInitializedx (void);
Three comments here:
- Chanel -> Channel
- Why the 'x' at the end?
- Does this method has to be public?

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.h#n...
src/lte/helper/lte-helper.h:277: void SetEnbCc (std::map< uint8_t,
Ptr<ComponentCarrier> > ccmap);
I find that component carriers are sometimes held in a map (like above), while
at other times in a vector (e.g., in RRC). I'd go for vector for all, because
it's simpler and the key of the map is 0..n-1 anyway.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/radio-bearer-s...
File src/lte/helper/radio-bearer-stats-calculator.cc (left):

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/radio-bearer-s...
src/lte/helper/radio-bearer-stats-calculator.cc:29: namespace ns3 {
This seems like a full-blown indentation change to the file. From ns-3 style
perspective, this change is not necessary. Anyway, is there any other changes
besides indentation change?

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
File src/lte/model/component-carrier-ue.h (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier-ue.h:36: * ComponentCarrierUe Object, it defines
a single Carrier
I suggest to add a note here that the class is a duplicate of class
ComponentCarrier. Please also describe its difference with ComponentCarrier.
Have you also considered to create a base class on top of ComponentCarrier and
ComponentCarrierUe?

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
File src/lte/model/component-carrier.h (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier.h:136: bool GetPrimaryCarrier () const;
I think SetAsPrimary() and IsPrimary() would be better names.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier.h:141: Ptr<LteEnbPhy> GetPhy (void);
Add const.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier.h:157: //    bool m_isConfigured;
Most of the content of this class comes from the LteEnbNetDevice. However, the
above member variables are not relevant for this class, so they should be
removed.

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

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-enb-net-dev...
src/lte/model/lte-enb-net-device.cc:50: #include <ns3/simulator.h>
The above two includes are probably not necessary.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-enb-rrc.cc
File src/lte/model/lte-enb-rrc.cc (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-enb-rrc.cc#...
src/lte/model/lte-enb-rrc.cc:1486: "initial cell selection and EPC-enabled
simulation.",
Please replace the above attribute description.

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

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-spectrum-ph...
src/lte/model/lte-spectrum-phy.cc:1153: errorRate =
LteMiErrorModel::GetPcfichPdcchError (m_sinrPerceived);
This line was removed from ns-3-dev by Nicola some weeks ago, so it's weird to
find it here again as a change. There's probably something wrong with how you
submit the code review.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-ue-rrc.cc
File src/lte/model/lte-ue-rrc.cc (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-ue-rrc.cc#n...
src/lte/model/lte-ue-rrc.cc:171: m_cphySapUser.erase (m_cphySapUser.begin
(),m_cphySapUser.end ());
Is the above line still necessary? If yes, then it would be good if you explain
why using comments.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-ue-rrc.h
File src/lte/model/lte-ue-rrc.h (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-ue-rrc.h#ne...
src/lte/model/lte-ue-rrc.h:261: uint64_t GetCaSupport (void) const;
This method should return bool.

[dabrignani, 2015-07-02 14:12:30]

Hi Buherman,

I try to reply to all your comments. I hope you will found the answer
satisfactory.

Best,
Danilo

https://codereview.appspot.com/249960043/diff/1/src/lte/examples/lena-simple.cc
File src/lte/examples/lena-simple.cc (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/examples/lena-simple....
src/lte/examples/lena-simple.cc:92: config.ConfigureAttributes ();
On 2015/07/01 13:53:33, buherman wrote:
> I think it's not a good idea to enable this.
Yes, actually it was a mistake in the review file, I enabled these two lines and
I forgot to disable before pushing online the review.
Acknowledged.

https://codereview.appspot.com/249960043/diff/1/src/lte/examples/wscript
File src/lte/examples/wscript (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/examples/wscript#newc...
src/lte/examples/wscript:60: obj.source = 'lena-componentcarrierTest.cc'
On 2015/07/01 13:53:33, buherman wrote:
> This file does not exist.

I delete those line. Thanks

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.cc#n...
src/lte/helper/cc-helper.cc:32: #define MAC_CC 2
On 2015/07/01 13:53:33, buherman wrote:
> MAC_CC -> MAX_CC

Done.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.cc#n...
src/lte/helper/cc-helper.cc:58: .AddAttribute ("NoOfCcs",
On 2015/07/01 13:53:33, buherman wrote:
> As far as I know, I think the convention for this kind of attribute is
> "NumberOf...", for example "NumberOfRaPreambles" in LteEnbMac.
> 
> And then "Ccs" may wrongly suggest "component carrier selection", so I suggest
> expanding it to "ComponentCarriers".

Done.

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.h#ne...
src/lte/helper/cc-helper.h:60: class CcHelper : public Object
On 2015/07/01 13:53:34, buherman wrote:
> I've seen usage of either "ComponentCarrier" and "Cc" in names. I suggest to
> make them consistent, e.g., by sticking to "ComponentCarrier". In this helper
> class, for instance, that would be CcHelper -> ComponentCarrierHelper,
> SetCcAttribute() -> SetComponentCarrierAttribute(), and so on.

I decided to use Cc instead of ComponentCarrier to have shorter names. However,
of course I can change to more explicit naming.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.h#ne...
src/lte/helper/cc-helper.h:73: Ptr<ComponentCarrier> DoCreateSingleCc (uint16_t
ulBw, uint16_t dlBw, uint16_t ulFreq, uint16_t dlFreq, bool pc);
On 2015/07/01 13:53:33, buherman wrote:
> I think the arguments can be improved here. 
> - Bandwidth is supposed to be uint8_t.
> - "Frequency" may suggest Hz, so in this case I prefer "Earfcn"
> - "Bw" -> "Bandwidth"
> - "pc" -> "isPrimary"
> The above also applies to the attributes of this class.

Done.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.h#ne...
src/lte/helper/cc-helper.h:73: Ptr<ComponentCarrier> DoCreateSingleCc (uint16_t
ulBw, uint16_t dlBw, uint16_t ulFreq, uint16_t dlFreq, bool pc);
On 2015/07/01 13:53:34, buherman wrote:
> Why the split into CreateSingleCc() and DoCreateSingleCc()?

There is not a strong reason. However, my initial idea was to leave here the
possibility to extend the code with some validation routine, e.g. check the
validity of the dl/ul EARFCN or verify if others Component Carrier are using the
same portion of the spectrum and so on.
Of course, currently seems redundant, but I guess it doesn't change in terms of
code readability neither on code performances.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/cc-helper.h#ne...
src/lte/helper/cc-helper.h:119: * \param dlFreq downlink EARFCN - not control on
the validity at this point
On 2015/07/01 13:53:34, buherman wrote:
> Why not controlled? Will it be controlled at later point?
Yes, basically when both uplink and downlink spectrum object are initialized the
spectrum-value-helper checks if uplink EARFCN and downlink EARFCN are valid or
not.
In the second case, it rises an error and conclude the code execution.

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:189: "If it is more than one and m_useCa is false,
it will rise an error ",
On 2015/07/01 13:53:34, buherman wrote:
> rise -> raise

Done.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:651: tmpPhy->GetDlSpectrumPhy ()->SetDevice (dev);
On 2015/07/01 13:53:34, buherman wrote:
> Duplicate?
Yes, removed!
Done.

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.h#n...
src/lte/helper/lte-helper.h:113: void ChanelModelInitializedx (void);
On 2015/07/01 13:53:34, buherman wrote:
> Three comments here:
> - Chanel -> Channel
> - Why the 'x' at the end?
> - Does this method has to be public?

-- Chanel -> Channel - Corrected!
-- Why the 'x' at the end? - Removed
-- Does this method has to be public? - Actually no, so I moved to private
Done.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.h#n...
src/lte/helper/lte-helper.h:277: void SetEnbCc (std::map< uint8_t,
Ptr<ComponentCarrier> > ccmap);
On 2015/07/01 13:53:34, buherman wrote:
> I find that component carriers are sometimes held in a map (like above), while
> at other times in a vector (e.g., in RRC). I'd go for vector for all, because
> it's simpler and the key of the map is 0..n-1 anyway.

Actually, in RRC the vector is used to contains the sap
(CPhySap[User/Provider]), where there is one sap per phy object connected (some
references on the wiki page and on the documentation). While, Component Carriers
are always in a map. 
In the past weeks, I tried to use vectors and I experienced some problem so,
after a discussion with my mentors, I moved to map.
Moreover, it is similar to other existing objects in the code that are collected
within  a map, e.g. std::map <uint8_t, Ptr<LteDataRadioBearerInfo> > in the
UeManager class or in the LteEnbRrc.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/radio-bearer-s...
File src/lte/helper/radio-bearer-stats-calculator.cc (left):

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/radio-bearer-s...
src/lte/helper/radio-bearer-stats-calculator.cc:29: namespace ns3 {
On 2015/07/01 13:53:34, buherman wrote:
> This seems like a full-blown indentation change to the file. From ns-3 style
> perspective, this change is not necessary. Anyway, is there any other changes
> besides indentation change?

None at all. I guess was some problem in my editor configuration (Emacs), I just
correct automatically the indentation on a set of open files and erroneously
this was in between.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
File src/lte/model/component-carrier-ue.h (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier-ue.h:36: * ComponentCarrierUe Object, it defines
a single Carrier
On 2015/07/01 13:53:34, buherman wrote:
> I suggest to add a note here that the class is a duplicate of class
> ComponentCarrier. Please also describe its difference with ComponentCarrier.
> Have you also considered to create a base class on top of ComponentCarrier and
> ComponentCarrierUe?

Yes, I wrote in the report that I will create a top ComponentCarrier class and
then two ComponentCarrierEnb, ComponentCarrierUe. Since, right now, the only
difference is the LtePhy pointer.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
File src/lte/model/component-carrier.h (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier.h:136: bool GetPrimaryCarrier () const;
On 2015/07/01 13:53:34, buherman wrote:
> I think SetAsPrimary() and IsPrimary() would be better names.

Done.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier.h:141: Ptr<LteEnbPhy> GetPhy (void);
On 2015/07/01 13:53:34, buherman wrote:
> Add const.

Done.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier.h:157: //    bool m_isConfigured;
On 2015/07/01 13:53:34, buherman wrote:
> Most of the content of this class comes from the LteEnbNetDevice. However, the
> above member variables are not relevant for this class, so they should be
> removed.

I was thinking to remove those variables that currently are not needed. However,
I'm still thinking if removing it or not. I might need for other reasons, i.e.
checking if the Component Carrier is configured and connected both to
LteNetDevice and the LtePhy is initialized or not.
In other words, I would like to keep there a little more instead of reintroduce
in case. Of course, if there is no reason in the next weeks I will remove those.

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

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-enb-net-dev...
src/lte/model/lte-enb-net-device.cc:50: #include <ns3/simulator.h>
On 2015/07/01 13:53:34, buherman wrote:
> The above two includes are probably not necessary.
In fact weren't necessary, I removed!
Acknowledged.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-enb-rrc.cc
File src/lte/model/lte-enb-rrc.cc (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-enb-rrc.cc#...
src/lte/model/lte-enb-rrc.cc:1486: "initial cell selection and EPC-enabled
simulation.",
On 2015/07/01 13:53:34, buherman wrote:
> Please replace the above attribute description.

Done.

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

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-spectrum-ph...
src/lte/model/lte-spectrum-phy.cc:1153: errorRate =
LteMiErrorModel::GetPcfichPdcchError (m_sinrPerceived);
On 2015/07/01 13:53:35, buherman wrote:
> This line was removed from ns-3-dev by Nicola some weeks ago, so it's weird to
> find it here again as a change. There's probably something wrong with how you
> submit the code review.

I will check and try to understand what can be wrong... Actually, I didn't
modified this file.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-ue-rrc.cc
File src/lte/model/lte-ue-rrc.cc (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-ue-rrc.cc#n...
src/lte/model/lte-ue-rrc.cc:171: m_cphySapUser.erase (m_cphySapUser.begin
(),m_cphySapUser.end ());
On 2015/07/01 13:53:35, buherman wrote:
> Is the above line still necessary? If yes, then it would be good if you
explain
> why using comments.
No, it was redundant. I removed that.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-ue-rrc.h
File src/lte/model/lte-ue-rrc.h (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-ue-rrc.h#ne...
src/lte/model/lte-ue-rrc.h:261: uint64_t GetCaSupport (void) const;
On 2015/07/01 13:53:35, buherman wrote:
> This method should return bool.

Done.

[Marco Miozzo, 2015-07-02 14:47:11]

Hi Danilo,

very good work, and many thanks to Budiarto also for the valuable review.
I have just a few minor comments to add, the main one is on the pa value, which
you copy for all the CC actives. I'm a bit scared that this might generate
problem with the power control, since it is a parameter derived from channel
conditions. I would suggest to check this.

Best,
marco.

https://codereview.appspot.com/249960043/diff/1/src/lte/examples/wscript
File src/lte/examples/wscript (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/examples/wscript#newc...
src/lte/examples/wscript:60: obj.source = 'lena-componentcarrierTest.cc'
Yes, it does not compile the code (changeset 11425:fec4e7859e37)

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:182: "If true, Carrier Aggregation is enable and a
valid Component Carrier Map is expected "
enable -> enabled

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
File src/lte/model/component-carrier-ue.h (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier-ue.h:36: * ComponentCarrierUe Object, it defines
a single Carrier
On 2015/07/02 14:12:29, dabrignani wrote:
> On 2015/07/01 13:53:34, buherman wrote:
> > I suggest to add a note here that the class is a duplicate of class
> > ComponentCarrier. Please also describe its difference with ComponentCarrier.
> > Have you also considered to create a base class on top of ComponentCarrier
and
> > ComponentCarrierUe?
> 
> Yes, I wrote in the report that I will create a top ComponentCarrier class and
> then two ComponentCarrierEnb, ComponentCarrierUe. Since, right now, the only
> difference is the LtePhy pointer.

Acknowledged.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
File src/lte/model/component-carrier.h (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier.h:157: //    bool m_isConfigured;
On 2015/07/02 14:12:29, dabrignani wrote:
> On 2015/07/01 13:53:34, buherman wrote:
> > Most of the content of this class comes from the LteEnbNetDevice. However,
the
> > above member variables are not relevant for this class, so they should be
> > removed.
> 
> I was thinking to remove those variables that currently are not needed.
However,
> I'm still thinking if removing it or not. I might need for other reasons, i.e.
> checking if the Component Carrier is configured and connected both to
> LteNetDevice and the LtePhy is initialized or not.
> In other words, I would like to keep there a little more instead of
reintroduce
> in case. Of course, if there is no reason in the next weeks I will remove
those.

Acknowledged.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-enb-rrc.cc
File src/lte/model/lte-enb-rrc.cc (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-enb-rrc.cc#...
src/lte/model/lte-enb-rrc.cc:906: m_rrc->m_cphySapProvider.at (i)->SetPa
(m_rnti, paDouble);
A single pa for all the component carrier might generate problems to the power
control schemes.

[dabrignani, 2015-07-02 15:39:02]

Hi Marco,

thanks for the comments.
Regarding the pa value, I will check the possible solution and I will back with
a proposal asap.

Best,
Danilo

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:182: "If true, Carrier Aggregation is enable and a
valid Component Carrier Map is expected "
On 2015/07/02 14:47:10, Marco Miozzo wrote:
> enable -> enabled

Done.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-enb-rrc.cc
File src/lte/model/lte-enb-rrc.cc (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-enb-rrc.cc#...
src/lte/model/lte-enb-rrc.cc:906: m_rrc->m_cphySapProvider.at (i)->SetPa
(m_rnti, paDouble);
On 2015/07/02 14:47:11, Marco Miozzo wrote:
> A single pa for all the component carrier might generate problems to the power
> control schemes.
Hi will check whis and try to provide a solution.

[gawlowicz.p, 2015-07-05 00:25:09]

Hi Danilo,

First of all, it is very good work. I am looking forward to see working example
:)

One general comment:
I know it is sometime tricky, but try to avoid tracking unnecessary indentation,
what makes it hard to find intended changes in patch.

And some following inline comments.

Best,
Piotr

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:520: it->second->m_phy = phy;
Since ComponentCarrier only defines a single carrier, it should not be used to
store phy. But if I understand correctly, there will be ComponentCarrierEnb used
here?

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:712: std::map<uint8_t, Ptr<ComponentCarrierUe> >
m_ccMapUe;
In case user create UE nodes first, we have to check if CA is used and if
m_ccMap is not empty.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:845: tmpPhy->GetDlSpectrumPhy ()->SetDevice (dev);
Duplicate

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.h#o...
src/lte/helper/lte-helper.h:679: 
I know that having few different SpectrumChannel will reduce some computation on
receiving side. But I saw that guys are developing code for coexistence of LTE
and Wifi. As I checked, they are installing all LTE eNBs and UEs and then take
pointer to Channel and attach WiFi nodes to it. Probably, with several
SpectrumChannels, they need to attach WiFI nodes to all of them? or any other
ideas?

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
File src/lte/model/component-carrier.h (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier.h:141: Ptr<LteEnbPhy> GetPhy (void);
Will Phy be removed from base class? It should be used only to store single
carrier definition

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

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-ue-net-devi...
src/lte/model/lte-ue-net-device.h:35: #include <vector>
I think it is not needed here?

[dabrignani, 2015-07-05 11:12:49]

Dear Piotr,

thanks for your comments.
I tried to answer to all.

Let me know if the replies are satisfactory.

Best,
Danilo

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:520: it->second->m_phy = phy;
On 2015/07/05 00:25:08, gawlowicz.p wrote:
> Since ComponentCarrier only defines a single carrier, it should not be used to
> store phy. But if I understand correctly, there will be ComponentCarrierEnb
used
> here?

The idea is to change completely the architecture. In fact, in the first draft
of the documentation (and in the wiki page), the new architecture propose to
'deprecate' LteEnbPhy and LteUePhy from LteEnbNetDevice and LteUeNetDevice
respectively. For the sake of backward compatibility, I added this code within
the LteHelper in order to use the 'new architecture' with old simulation
scripts.
Yes, you understood correctly, in the next commit I will replace
ComponentCarrier with ComponentCarrieEnb.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:712: std::map<uint8_t, Ptr<ComponentCarrierUe> >
m_ccMapUe;
On 2015/07/05 00:25:08, gawlowicz.p wrote:
> In case user create UE nodes first, we have to check if CA is used and if
> m_ccMap is not empty.

Thanks to point it out. I will add the necessary code.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:712: std::map<uint8_t, Ptr<ComponentCarrierUe> >
m_ccMapUe;
On 2015/07/05 00:25:08, gawlowicz.p wrote:
> In case user create UE nodes first, we have to check if CA is used and if
> m_ccMap is not empty.

Thanks to point it out. I will add the necessary code.

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.cc#...
src/lte/helper/lte-helper.cc:845: tmpPhy->GetDlSpectrumPhy ()->SetDevice (dev);
On 2015/07/05 00:25:09, gawlowicz.p wrote:
> Duplicate 
Removed!
Done.

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

https://codereview.appspot.com/249960043/diff/1/src/lte/helper/lte-helper.h#o...
src/lte/helper/lte-helper.h:679: 
On 2015/07/05 00:25:09, gawlowicz.p wrote:
> I know that having few different SpectrumChannel will reduce some computation
on
> receiving side. But I saw that guys are developing code for coexistence of LTE
> and Wifi. As I checked, they are installing all LTE eNBs and UEs and then take
> pointer to Channel and attach WiFi nodes to it. Probably, with several
> SpectrumChannels, they need to attach WiFI nodes to all of them? or any other
> ideas?

Maybe I didn't understand your point. However, I will try to reply... We need
different Spectrum Channel because each component component is centered on
different frequency and potentially with different bandwidth. Moreover, since it
is possible to aggregate component carrier on different band (e.g. ~700 MHz and
~2 GHz), it could be needed different Pathloss models.

Finally, I guess that in case of coexistence Wifi should be attached only on the
SpectrumChannels on the same band. In other words, I guess that is reasonable to
say that Wifi will be attached only on one component carrier, i.e. the one on
the same band.

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
File src/lte/model/component-carrier.h (right):

https://codereview.appspot.com/249960043/diff/1/src/lte/model/component-carri...
src/lte/model/component-carrier.h:141: Ptr<LteEnbPhy> GetPhy (void);
On 2015/07/05 00:25:09, gawlowicz.p wrote:
> Will Phy be removed from base class? It should be used only to store single
> carrier definition

I will remove the attribute, however, I guess removing the method GetPhy is not
be possible, it will point to the LteEnbPhy of the Primary Component Carrier
(PCC).

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

https://codereview.appspot.com/249960043/diff/1/src/lte/model/lte-ue-net-devi...
src/lte/model/lte-ue-net-device.h:35: #include <vector>
On 2015/07/05 00:25:09, gawlowicz.p wrote:
> I think it is not needed here?

I will check, in case, I will remove that.

[dabrignani, 2015-07-05 11:12:53]

Dear Piotr,

thanks for your comments.
I tried to answer to all.

Let me know if the replies are satisfactory.

Best,
Danilo