LTE module - Giuseppe Piro

[Nicola Baldo, 2010-07-20 12:41:41]

Hi Giuseppe,

I went through most of the code that you uploaded for review. 
There are some parts that I still have to examine in detail (for instance, the
newly added propagation models), but I think that we have already enough issues
to discuss and solve before. I suggest to keep the discussion on the
ns-3-reviews mailing list, so that other developers can join if interested.

Nicola

http://codereview.appspot.com/1866042/diff/1/7
File examples/spectrum/adhoc-ofdm-aloha-with-microwave-oven.cc (right):

http://codereview.appspot.com/1866042/diff/1/7#newcode21
examples/spectrum/adhoc-ofdm-aloha-with-microwave-oven.cc:21: 
Why does this file appear in the review of the LTE code?

http://codereview.appspot.com/1866042/diff/1/8
File examples/spectrum/adhoc-ofdm-aloha.cc (right):

http://codereview.appspot.com/1866042/diff/1/8#newcode21
examples/spectrum/adhoc-ofdm-aloha.cc:21: 
why does this file appear in the review?

http://codereview.appspot.com/1866042/diff/1/10
File src/common/spectrum-propagation-loss-model.h (left):

http://codereview.appspot.com/1866042/diff/1/10#oldcode106
src/common/spectrum-propagation-loss-model.h:106: #endif /*
SPECTRUM_PROPAGATION_LOSS_MODEL_H */
whitespace change?

http://codereview.appspot.com/1866042/diff/1/29
File src/devices/lte/amc-module.h (right):

http://codereview.appspot.com/1866042/diff/1/29#newcode56
src/devices/lte/amc-module.h:56: int* GetCqi ();
About all these functions that return a pointer to your global C arrays: it
seems you don't use them in your code, so I think you should remove them. 

Furthermore, C arrays are evil:
http://www.parashift.com/c++-faq-lite/containers.html#faq-34.1
so, while I think it can be ok if you use C arrays in your implementation only
(.cc files), I argue that C arrays should absolutely not be used for the API (.h
files).

http://codereview.appspot.com/1866042/diff/1/29#newcode81
src/devices/lte/amc-module.h:81: * Get a proper CQI for the spectrale
s is approximated to the nearest value? or to the upper (or lower) value?

http://codereview.appspot.com/1866042/diff/1/30
File src/devices/lte/bearer-manager.cc (right):

http://codereview.appspot.com/1866042/diff/1/30#newcode44
src/devices/lte/bearer-manager.cc:44: m_downlinkGbrBearersContainer = new
BearersContainer ();
memory leak here ("new" without corresponding "delete").
That's one good reason why you should use member variables of type
BearersContainer rather than BearersContainer*: it will save you memory
management headaches.

http://codereview.appspot.com/1866042/diff/1/31
File src/devices/lte/bearer-manager.h (right):

http://codereview.appspot.com/1866042/diff/1/31#newcode56
src/devices/lte/bearer-manager.h:56: BearersContainer* GetDownlinkGbrBearers
(void) const;
It seems to me that these Get*Bearers() methods are not used in any part of the
code, so I think you should remove them.

http://codereview.appspot.com/1866042/diff/1/31#newcode98
src/devices/lte/bearer-manager.h:98: BearersContainer*
m_downlinkGbrBearersContainer;
It seems to me that you allocate these BearersContainer only once upon
construction of BearerManager, so why don't you just make these variables of
type BearersContainer rather than BearersContainer* ?

http://codereview.appspot.com/1866042/diff/1/33
File src/devices/lte/bearer.h (right):

http://codereview.appspot.com/1866042/diff/1/33#newcode34
src/devices/lte/bearer.h:34: * a data flow between UE and eNB will be mapped.
Why don't you rename this class to RadioBearerInstance?

http://codereview.appspot.com/1866042/diff/1/33#newcode51
src/devices/lte/bearer.h:51: BEARER_TYPE_GBR, BEARER_TYPE_NGBR
Why don't you use your QosParameters class to represent this information?

http://codereview.appspot.com/1866042/diff/1/33#newcode73
src/devices/lte/bearer.h:73: STATE_INITIALIZED, STATE_DEACTIVED, STATE_ACTIVED
"ACTIVED" --> "ACTIVE"
"DEACTIVED" --> "DEACTIVATED" or "INACTIVE"
any reference to the standard for these states?

http://codereview.appspot.com/1866042/diff/1/33#newcode78
src/devices/lte/bearer.h:78: SCHEDULER_STATE_TO_SCHEDULE, SCHEDULER_STATE_FROZEN
any reference to the standard for these states?

http://codereview.appspot.com/1866042/diff/1/40
File src/devices/lte/enb-net-device.cc (right):

http://codereview.appspot.com/1866042/diff/1/40#newcode169
src/devices/lte/enb-net-device.cc:169: Ptr<Bearer> bearer = GetIpClassifier
()->Classify (packet);
you should put a comment here saying that this is a simplification due to the
fact that a core network implementation is not available yet. In fact, according
to the standard, the classification of traffic in DL is done by the PGW (not by
the eNB).

http://codereview.appspot.com/1866042/diff/1/40#newcode260
src/devices/lte/enb-net-device.cc:260: Ptr<PacketBurst> pb = new PacketBurst ();
you should use CreateObject<PacketBurst> () instead of "new"

note that you have several places in your code where you use "new" instead of
CreateObject<>() or Create<>(). Please search all your code for these instances
and fix them whenever needed. Hint: whenever "new" appears on the right hand
side of an assignment to a Ptr<> variable, it's most likely wrong.

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

http://codereview.appspot.com/1866042/diff/1/41#newcode42
src/devices/lte/enb-net-device.h:42: 
I see that a lot of functionality is still implemented in the EnbNetDevice, in
spite of the fact that we had agreed some time ago to put most of the
functionality in dedicated classes, such as the Mac entity and the Phy. Below,
I'll try to point out which methods I think need to be moved.

http://codereview.appspot.com/1866042/diff/1/41#newcode100
src/devices/lte/enb-net-device.h:100: void CalcChannelQualityForUe (std::vector
<double> sinr, Ptr<UeNetDevice> ue);
this should be in the PHY. In fact, channel measurements are a functionality
that the PHY provides to the MAC, see TS 36.321, section 4.3.2

http://codereview.appspot.com/1866042/diff/1/41#newcode107
src/devices/lte/enb-net-device.h:107: //void ReceiveCqiFeedbacks
(LteNetDevice::CqiFeedbacks c, Ptr<UeNetDevice> ue);
this should go in the MAC, since it is the MAC that receives the CQI
measurements provided by the PHY. See above reference.

http://codereview.appspot.com/1866042/diff/1/41#newcode112
src/devices/lte/enb-net-device.h:112: void StartFrame (void);
the handling of frames and sub-frames should be done by the PHY. See TS 36.200,
Section 5 "Physical Layer for E-UTRA".

http://codereview.appspot.com/1866042/diff/1/41#newcode134
src/devices/lte/enb-net-device.h:134: virtual void SendIdealControlMessage
(Ptr<IdealControlMessage> msg);
I would handle this in the PHY. I think a good API would be such that the MAC
doesn't need to care whether the control channels are ideal or realistic.

http://codereview.appspot.com/1866042/diff/1/69
File src/devices/lte/lte-spectrum-phy.h (right):

http://codereview.appspot.com/1866042/diff/1/69#newcode83
src/devices/lte/lte-spectrum-phy.h:83: * (Watt, Pascal...) per Hz.
I guess in this case it is definitely W/Hz...

http://codereview.appspot.com/1866042/diff/1/69#newcode159
src/devices/lte/lte-spectrum-phy.h:159: SpectrumInterference
GetSpectrumInterference (void);
why return by value?

http://codereview.appspot.com/1866042/diff/1/70
File src/devices/lte/lte-spectrum-value-helper.cc (right):

http://codereview.appspot.com/1866042/diff/1/70#newcode38
src/devices/lte/lte-spectrum-value-helper.cc:38: for (int i = 0; i < 100; ++i)
only 100 channels? TS 36.300 says that we have 110*12 subcarriers for downlink
(section 5.1.1) and the same number for downlink (section 5.2.1).

http://codereview.appspot.com/1866042/diff/1/70#newcode41
src/devices/lte/lte-spectrum-value-helper.cc:41: double centralFrequencyOfPRB =
1.99109 + (i * 0.00018);
this is in GHz, right? and 0.00018 is subcarrier_spacing*num_subcarriers_per_RB,
right? You should mention this. Also you should mention what 1.99109 refers to.

http://codereview.appspot.com/1866042/diff/1/70#newcode42
src/devices/lte/lte-spectrum-value-helper.cc:42: freqs.push_back
(centralFrequencyOfPRB * 1e9);
please provide a reference for these values

http://codereview.appspot.com/1866042/diff/1/70#newcode57
src/devices/lte/lte-spectrum-value-helper.cc:57: Ptr<SpectrumValue> txPsd =
Create <SpectrumValue> (LteSpectrumModel);
I still do not agree that you use the same SpectrumModel for UL and DL. In FDD
mode, UL and DL are transmitted in different frequencies, so I do not understand
how you model this in your code.

http://codereview.appspot.com/1866042/diff/1/71
File src/devices/lte/lte-spectrum-value-helper.h (right):

http://codereview.appspot.com/1866042/diff/1/71#newcode41
src/devices/lte/lte-spectrum-value-helper.h:41: * \param powerTx the power
transmission
say in which units

http://codereview.appspot.com/1866042/diff/1/71#newcode42
src/devices/lte/lte-spectrum-value-helper.h:42: * \param channels the list of
sub channels where the signal will be sent
what are the allowed values for channels?

http://codereview.appspot.com/1866042/diff/1/84
File src/devices/lte/qos-parameters.h (right):

http://codereview.appspot.com/1866042/diff/1/84#newcode61
src/devices/lte/qos-parameters.h:61: class QoSParameters : public Object
why don't you rename it to BearerParameters or something similar? I think it's
important to include the word "bearer" in the name.

http://codereview.appspot.com/1866042/diff/1/88
File src/devices/lte/rrc-entity.h (right):

http://codereview.appspot.com/1866042/diff/1/88#newcode34
src/devices/lte/rrc-entity.h:34: class BearerManager;
why do you need to have a BearerManager separated from the RRC? I see that your
RRC is only doing bearer management, so maybe it is better to just rename
BearerManager to RrcEntity. 
The term "bearer manager" is not used by the standard.

[GiuseppePiro, 2010-07-22 16:17:11]

Dear Nicola, 

thank you for your review. Hope I have responded correctly to all your feedback
and that I have fixed the code according to your review.

Just one consideration: I will resolve the question about Spectrum Model as soon
as possible. 

Regards,

Giuseppe Piro

http://codereview.appspot.com/1866042/diff/1/7
File examples/spectrum/adhoc-ofdm-aloha-with-microwave-oven.cc (right):

http://codereview.appspot.com/1866042/diff/1/7#newcode21
examples/spectrum/adhoc-ofdm-aloha-with-microwave-oven.cc:21: 
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> Why does this file appear in the review of the LTE code?

It was an error done during the marge with ns-3-dev --> fixed!

http://codereview.appspot.com/1866042/diff/1/8
File examples/spectrum/adhoc-ofdm-aloha.cc (right):

http://codereview.appspot.com/1866042/diff/1/8#newcode21
examples/spectrum/adhoc-ofdm-aloha.cc:21: 
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> why does this file appear in the review?

It was an error done during the marge with ns-3-dev --> fixed!

http://codereview.appspot.com/1866042/diff/1/10
File src/common/spectrum-propagation-loss-model.h (left):

http://codereview.appspot.com/1866042/diff/1/10#oldcode106
src/common/spectrum-propagation-loss-model.h:106: #endif /*
SPECTRUM_PROPAGATION_LOSS_MODEL_H */
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> whitespace change?

It was an error done during the marge with ns-3-dev --> fixed!

http://codereview.appspot.com/1866042/diff/1/29
File src/devices/lte/amc-module.h (right):

http://codereview.appspot.com/1866042/diff/1/29#newcode56
src/devices/lte/amc-module.h:56: int* GetCqi ();
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> About all these functions that return a pointer to your global C arrays: it
> seems you don't use them in your code, so I think you should remove them. 

Done! 

> 
> Furthermore, C arrays are evil:
> http://www.parashift.com/c++-faq-lite/containers.html#faq-34.1
> so, while I think it can be ok if you use C arrays in your implementation only
> (.cc files), I argue that C arrays should absolutely not be used for the API
(.h
> files).

http://codereview.appspot.com/1866042/diff/1/29#newcode81
src/devices/lte/amc-module.h:81: * Get a proper CQI for the spectrale
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> s is approximated to the nearest value? or to the upper (or lower) value?


It is the lower value. In this way infact, since the chosen of the CQI is
conservative, we can assure a fewer Block Error Rate. (Furthermore, this
behaviour cannot be observed  for the current implementation of the module: the
error model will be implemented after GSoC)

http://codereview.appspot.com/1866042/diff/1/30
File src/devices/lte/bearer-manager.cc (right):

http://codereview.appspot.com/1866042/diff/1/30#newcode44
src/devices/lte/bearer-manager.cc:44: m_downlinkGbrBearersContainer = new
BearersContainer ();
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> memory leak here ("new" without corresponding "delete").
> That's one good reason why you should use member variables of type
> BearersContainer rather than BearersContainer*: it will save you memory
> management headaches.

Fixed. In the current implementation, for each "new" corrensponds a "delete"

http://codereview.appspot.com/1866042/diff/1/31
File src/devices/lte/bearer-manager.h (right):

http://codereview.appspot.com/1866042/diff/1/31#newcode56
src/devices/lte/bearer-manager.h:56: BearersContainer* GetDownlinkGbrBearers
(void) const;
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> It seems to me that these Get*Bearers() methods are not used in any part of
the
> code, so I think you should remove them.

I think that in the future, the implementation of several features (such as
packet schedulers etc..) can use these functions. 
So, I want to leave them into the code.

http://codereview.appspot.com/1866042/diff/1/31#newcode98
src/devices/lte/bearer-manager.h:98: BearersContainer*
m_downlinkGbrBearersContainer;
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> It seems to me that you allocate these BearersContainer only once upon
> construction of BearerManager, so why don't you just make these variables of
> type BearersContainer rather than BearersContainer* ?


As a consequence of the previous comment, use BearersContainer* instead of
BearersContainer can be a better solution.

http://codereview.appspot.com/1866042/diff/1/33
File src/devices/lte/bearer.h (right):

http://codereview.appspot.com/1866042/diff/1/33#newcode34
src/devices/lte/bearer.h:34: * a data flow between UE and eNB will be mapped.
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> Why don't you rename this class to RadioBearerInstance?
> 
> 

Done!

http://codereview.appspot.com/1866042/diff/1/33#newcode51
src/devices/lte/bearer.h:51: BEARER_TYPE_GBR, BEARER_TYPE_NGBR
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> Why don't you use your QoSParameters class to represent this information?

Ok, this information has been moved into the  QoSParameters.

http://codereview.appspot.com/1866042/diff/1/33#newcode73
src/devices/lte/bearer.h:73: STATE_INITIALIZED, STATE_DEACTIVED, STATE_ACTIVED
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> "ACTIVED" --> "ACTIVE"
> "DEACTIVED" --> "DEACTIVATED" or "INACTIVE"
> any reference to the standard for these states?

Since  I'm not sure on the reference of this information, for now I deleted it
from the code.

http://codereview.appspot.com/1866042/diff/1/33#newcode78
src/devices/lte/bearer.h:78: SCHEDULER_STATE_TO_SCHEDULE, SCHEDULER_STATE_FROZEN
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> any reference to the standard for these states?

Since  I'm not sure on the reference of this information, for now I deleted it
from the code.

http://codereview.appspot.com/1866042/diff/1/40
File src/devices/lte/enb-net-device.cc (right):

http://codereview.appspot.com/1866042/diff/1/40#newcode169
src/devices/lte/enb-net-device.cc:169: Ptr<Bearer> bearer = GetIpClassifier
()->Classify (packet);
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> you should put a comment here saying that this is a simplification due to the
> fact that a core network implementation is not available yet. In fact,
according
> to the standard, the classification of traffic in DL is done by the PGW (not
by
> the eNB).

Done!

http://codereview.appspot.com/1866042/diff/1/40#newcode260
src/devices/lte/enb-net-device.cc:260: Ptr<PacketBurst> pb = new PacketBurst ();
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> you should use CreateObject<PacketBurst> () instead of "new"
> 

Done!

> note that you have several places in your code where you use "new" instead of
> CreateObject<>() or Create<>(). Please search all your code for these
instances
> and fix them whenever needed. Hint: whenever "new" appears on the right hand
> side of an assignment to a Ptr<> variable, it's most likely wrong.

I will fix all these problems as soon as possible.

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

http://codereview.appspot.com/1866042/diff/1/41#newcode42
src/devices/lte/enb-net-device.h:42: 
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> I see that a lot of functionality is still implemented in the EnbNetDevice, in
> spite of the fact that we had agreed some time ago to put most of the
> functionality in dedicated classes, such as the Mac entity and the Phy. Below,
> I'll try to point out which methods I think need to be moved.
> 

Ok, I moved some functionalities into a proper entity, according to your
feedbacks

http://codereview.appspot.com/1866042/diff/1/41#newcode100
src/devices/lte/enb-net-device.h:100: void CalcChannelQualityForUe (std::vector
<double> sinr, Ptr<UeNetDevice> ue);
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> this should be in the PHY. In fact, channel measurements are a functionality
> that the PHY provides to the MAC, see TS 36.321, section 4.3.2

It  has been moved into the EnbPhy class.

http://codereview.appspot.com/1866042/diff/1/41#newcode107
src/devices/lte/enb-net-device.h:107: //void ReceiveCqiFeedbacks
(LteNetDevice::CqiFeedbacks c, Ptr<UeNetDevice> ue);
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> this should go in the MAC, since it is the MAC that receives the CQI
> measurements provided by the PHY. See above reference.

This function does not exist now,

http://codereview.appspot.com/1866042/diff/1/41#newcode112
src/devices/lte/enb-net-device.h:112: void StartFrame (void);
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> the handling of frames and sub-frames should be done by the PHY. See TS
36.200,
> Section 5 "Physical Layer for E-UTRA".
> 

Done!

http://codereview.appspot.com/1866042/diff/1/41#newcode134
src/devices/lte/enb-net-device.h:134: virtual void SendIdealControlMessage
(Ptr<IdealControlMessage> msg);
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> I would handle this in the PHY. I think a good API would be such that the MAC
> doesn't need to care whether the control channels are ideal or realistic.


Done!

http://codereview.appspot.com/1866042/diff/1/69
File src/devices/lte/lte-spectrum-phy.h (right):

http://codereview.appspot.com/1866042/diff/1/69#newcode83
src/devices/lte/lte-spectrum-phy.h:83: * (Watt, Pascal...) per Hz.
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> I guess in this case it is definitely W/Hz...

Yes, I added a comment into the code

http://codereview.appspot.com/1866042/diff/1/69#newcode159
src/devices/lte/lte-spectrum-phy.h:159: SpectrumInterference
GetSpectrumInterference (void);
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> why return by value?

Fixed.

http://codereview.appspot.com/1866042/diff/1/71
File src/devices/lte/lte-spectrum-value-helper.h (right):

http://codereview.appspot.com/1866042/diff/1/71#newcode41
src/devices/lte/lte-spectrum-value-helper.h:41: * \param powerTx the power
transmission
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> say in which units

Done, (it was in dBm)

http://codereview.appspot.com/1866042/diff/1/71#newcode42
src/devices/lte/lte-spectrum-value-helper.h:42: * \param channels the list of
sub channels where the signal will be sent
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> what are the allowed values for channels?

these values are not know ahead.
It depends from scheduling decision about the UL and the DL transmission.

http://codereview.appspot.com/1866042/diff/1/84
File src/devices/lte/qos-parameters.h (right):

http://codereview.appspot.com/1866042/diff/1/84#newcode61
src/devices/lte/qos-parameters.h:61: class QoSParameters : public Object
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> why don't you rename it to BearerParameters or something similar? I think it's
> important to include the word "bearer" in the name.

Done!

http://codereview.appspot.com/1866042/diff/1/88
File src/devices/lte/rrc-entity.h (right):

http://codereview.appspot.com/1866042/diff/1/88#newcode34
src/devices/lte/rrc-entity.h:34: class BearerManager;
On 2010/07/20 12:41:41, Nicola Baldo wrote:
> why do you need to have a BearerManager separated from the RRC? I see that
your
> RRC is only doing bearer management, so maybe it is better to just rename
> BearerManager to RrcEntity. 
> The term "bearer manager" is not used by the standard.

Done!

[Nicola Baldo, 2010-08-04 12:40:38]

Good work, most of the previous comments have been addressed. The most important
issue which is still present from the previous review is that of the
SpectrumModel for FDD.

For the rest, I am providing some more comments, especially with respect to the
interaction between MAC, RLC, RRC and IpClassifier.

http://codereview.appspot.com/1866042/diff/17001/18041
File src/devices/lte/enb-net-device.cc (right):

http://codereview.appspot.com/1866042/diff/17001/18041#newcode174
src/devices/lte/enb-net-device.cc:174: return Enqueue (packet, bearer);
why don't you just do
bearer->Enqueue (packet)
and delete the method LteNetDevice::Enqueue (bearer, packet)?

http://codereview.appspot.com/1866042/diff/17001/18049
File src/devices/lte/ip-classifier.cc (right):

http://codereview.appspot.com/1866042/diff/17001/18049#newcode80
src/devices/lte/ip-classifier.cc:80: return GetDevice ()->GetRrcEntity
()->GetDefaultBearer ();
I understand that implementing dedicated bearers is beyond scope of the GSoC,
and that for this reason it is good to stick with default bearers only. However,
I think it is important to be noted that there is one default bearer per UE,
i.e., the RRC at the eNB should not have a single default bearer, but rather a
separate default bearer for every connected UE.

http://codereview.appspot.com/1866042/diff/17001/18072
File src/devices/lte/lte-spectrum-value-helper.cc (right):

http://codereview.appspot.com/1866042/diff/17001/18072#newcode27
src/devices/lte/lte-spectrum-value-helper.cc:27: Ptr<SpectrumModel>
LteSpectrumModel;
There is still a single LteSpectrumModel used both for DL and UL. For FDD, there
should be two separates FddLteSpectrumModel, one for UL and one for DL. It is
only for TDD that the spectrum is the same for UL and DL.

http://codereview.appspot.com/1866042/diff/17001/18072#newcode42
src/devices/lte/lte-spectrum-value-helper.cc:42: freqs.push_back
(centralFrequencyOfPRB * 1e9);
You have exactly the same code in lte-spectrum-model.cc, please remove one of
these instances

http://codereview.appspot.com/1866042/diff/17001/18088
File src/devices/lte/radio-bearer-instance.h (right):

http://codereview.appspot.com/1866042/diff/17001/18088#newcode202
src/devices/lte/radio-bearer-instance.h:202: Ptr<RlcEntity> GetRlcEntiry (void);
Entiry --> Entity

http://codereview.appspot.com/1866042/diff/17001/18088#newcode218
src/devices/lte/radio-bearer-instance.h:218: Ptr<IpClassifierRecord>
m_classifierRecord;
Why does the RlcEntity need to know the corresponding IpClassifierRecord? I
thought that the mapping between the two was to be managed by the IpClassifier
class only.

http://codereview.appspot.com/1866042/diff/17001/18090
File src/devices/lte/rlc-entity.h (right):

http://codereview.appspot.com/1866042/diff/17001/18090#newcode42
src/devices/lte/rlc-entity.h:42: RLC_MODE_TM, RLC_MODEUM, RLC_MODE_AM
this should be done by class inheritance

http://codereview.appspot.com/1866042/diff/17001/18090#newcode60
src/devices/lte/rlc-entity.h:60: 
there should be methods realizing the interface between RadioBearerInstance as
well as with the Mac. I mean, now the PacketScheduler directly calls the
Dequeue() method of RadioBearerInstance, so in fact the RLC is useless. 

You should modify the API to improve the interface between RadioBearer and RLC,
as well as between RLC and MAC. In particular:

1)  RadioBearerInstance should forward the MSDUs to the associated RlcEntity,
using a dedicated method of RlcEntity

2) the MAC should provide methods so that the RLC can forward the MPDUs to the
MAC. The API should provide the following functionality mentioned in TS 36.322:

4.3.2 Services expected from lower layers
The following services are expected by RLC from lower layer (i.e. MAC):
-data transfer;
-notification of a transmission opportunity, together with the total size of the
RLC PDU(s) to be transmitted in the
transmission opportunity;

3) Since for the GSoC only RLC/TM is expected, the actual implementation of
RlcEntity will be very simple.

http://codereview.appspot.com/1866042/diff/17001/18091
File src/devices/lte/rrc-entity.cc (right):

http://codereview.appspot.com/1866042/diff/17001/18091#newcode108
src/devices/lte/rrc-entity.cc:108: return false;
you mean return 0?

[GiuseppePiro, 2010-08-06 14:09:47]

Hi Nicola, thanks for your review.
Hope that all problems have been fixed well!

Regards

Giuseppe Piro

http://codereview.appspot.com/1866042/diff/17001/18041
File src/devices/lte/enb-net-device.cc (right):

http://codereview.appspot.com/1866042/diff/17001/18041#newcode174
src/devices/lte/enb-net-device.cc:174: return Enqueue (packet, bearer);
On 2010/08/04 12:40:38, Nicola Baldo wrote:
> why don't you just do
> bearer->Enqueue (packet)
> and delete the method LteNetDevice::Enqueue (bearer, packet)?

Done.

http://codereview.appspot.com/1866042/diff/17001/18049
File src/devices/lte/ip-classifier.cc (right):

http://codereview.appspot.com/1866042/diff/17001/18049#newcode80
src/devices/lte/ip-classifier.cc:80: return GetDevice ()->GetRrcEntity
()->GetDefaultBearer ();
On 2010/08/04 12:40:38, Nicola Baldo wrote:
> I understand that implementing dedicated bearers is beyond scope of the GSoC,
> and that for this reason it is good to stick with default bearers only.
However,
> I think it is important to be noted that there is one default bearer per UE,
> i.e., the RRC at the eNB should not have a single default bearer, but rather a
> separate default bearer for every connected UE.


In order to implement one dedicated radio bearer for each UE it is necessary do
complete the development of the ip-classifier, the ue-manager and so on. For
this reason, this feature will be implemented after GSoC.

http://codereview.appspot.com/1866042/diff/17001/18072
File src/devices/lte/lte-spectrum-value-helper.cc (right):

http://codereview.appspot.com/1866042/diff/17001/18072#newcode27
src/devices/lte/lte-spectrum-value-helper.cc:27: Ptr<SpectrumModel>
LteSpectrumModel;
On 2010/08/04 12:40:38, Nicola Baldo wrote:
> There is still a single LteSpectrumModel used both for DL and UL. For FDD,
there
> should be two separates FddLteSpectrumModel, one for UL and one for DL. It is
> only for TDD that the spectrum is the same for UL and DL.

Done.

http://codereview.appspot.com/1866042/diff/17001/18072#newcode42
src/devices/lte/lte-spectrum-value-helper.cc:42: freqs.push_back
(centralFrequencyOfPRB * 1e9);
On 2010/08/04 12:40:38, Nicola Baldo wrote:
> You have exactly the same code in lte-spectrum-model.cc, please remove one of
> these instances

Done.

http://codereview.appspot.com/1866042/diff/17001/18088
File src/devices/lte/radio-bearer-instance.h (right):

http://codereview.appspot.com/1866042/diff/17001/18088#newcode202
src/devices/lte/radio-bearer-instance.h:202: Ptr<RlcEntity> GetRlcEntiry (void);
On 2010/08/04 12:40:38, Nicola Baldo wrote:
> Entiry --> Entity

Done.

http://codereview.appspot.com/1866042/diff/17001/18088#newcode218
src/devices/lte/radio-bearer-instance.h:218: Ptr<IpClassifierRecord>
m_classifierRecord;
On 2010/08/04 12:40:38, Nicola Baldo wrote:
> Why does the RlcEntity need to know the corresponding IpClassifierRecord? I
> thought that the mapping between the two was to be managed by the IpClassifier
> class only.

Ok, it has been fixed.

http://codereview.appspot.com/1866042/diff/17001/18090
File src/devices/lte/rlc-entity.h (right):

http://codereview.appspot.com/1866042/diff/17001/18090#newcode42
src/devices/lte/rlc-entity.h:42: RLC_MODE_TM, RLC_MODEUM, RLC_MODE_AM
On 2010/08/04 12:40:38, Nicola Baldo wrote:
> this should be done by class inheritance

Done.

http://codereview.appspot.com/1866042/diff/17001/18090#newcode60
src/devices/lte/rlc-entity.h:60: 
On 2010/08/04 12:40:38, Nicola Baldo wrote:
> there should be methods realizing the interface between RadioBearerInstance as
> well as with the Mac. I mean, now the PacketScheduler directly calls the
> Dequeue() method of RadioBearerInstance, so in fact the RLC is useless. 
> 
> You should modify the API to improve the interface between RadioBearer and
RLC,
> as well as between RLC and MAC. In particular:
> 
> 1)  RadioBearerInstance should forward the MSDUs to the associated RlcEntity,
> using a dedicated method of RlcEntity
> 
> 2) the MAC should provide methods so that the RLC can forward the MPDUs to the
> MAC. The API should provide the following functionality mentioned in TS
36.322:
> 
> 4.3.2 Services expected from lower layers
> The following services are expected by RLC from lower layer (i.e. MAC):
> -data transfer;
> -notification of a transmission opportunity, together with the total size of
the
> RLC PDU(s) to be transmitted in the
> transmission opportunity;
> 
> 3) Since for the GSoC only RLC/TM is expected, the actual implementation of
> RlcEntity will be very simple.


I improved the interaction among MAC, RLC and RRC

http://codereview.appspot.com/1866042/diff/17001/18091
File src/devices/lte/rrc-entity.cc (right):

http://codereview.appspot.com/1866042/diff/17001/18091#newcode108
src/devices/lte/rrc-entity.cc:108: return false;
On 2010/08/04 12:40:38, Nicola Baldo wrote:
> you mean return 0?

Done.

[g.piro_poliba.it, 2010-08-07 10:08:04]

The code has been moved to http://codereview.appspot.com/1869054/show

for final review.

Regards,

G.
-- 
Giuseppe Piro
Ph.D. Student
DEE - Politecnico di Bari
v. Orabona 4, 70125 - Bari, Italy
e-mail:    g.piro@poliba.it
Phone:   +39 0805963301
Web:       http://telematics.poliba.it/piro



Citando peppe.piro2@gmail.com:

> Hi Nicola, thanks for your review.
> Hope that all problems have been fixed well!
>
> Regards
>
> Giuseppe Piro
>
>
> http://codereview.appspot.com/1866042/diff/17001/18041
> File src/devices/lte/enb-net-device.cc (right):
>
> http://codereview.appspot.com/1866042/diff/17001/18041#newcode174
> src/devices/lte/enb-net-device.cc:174: return Enqueue (packet, bearer);
> On 2010/08/04 12:40:38, Nicola Baldo wrote:
>> why don't you just do
>> bearer->Enqueue (packet)
>> and delete the method LteNetDevice::Enqueue (bearer, packet)?
>
> Done.
>
> http://codereview.appspot.com/1866042/diff/17001/18049
> File src/devices/lte/ip-classifier.cc (right):
>
> http://codereview.appspot.com/1866042/diff/17001/18049#newcode80
> src/devices/lte/ip-classifier.cc:80: return GetDevice ()->GetRrcEntity
> ()->GetDefaultBearer ();
> On 2010/08/04 12:40:38, Nicola Baldo wrote:
>> I understand that implementing dedicated bearers is beyond scope of
> the GSoC,
>> and that for this reason it is good to stick with default bearers
> only. However,
>> I think it is important to be noted that there is one default bearer
> per UE,
>> i.e., the RRC at the eNB should not have a single default bearer, but
> rather a
>> separate default bearer for every connected UE.
>
>
> In order to implement one dedicated radio bearer for each UE it is
> necessary do complete the development of the ip-classifier, the
> ue-manager and so on. For this reason, this feature will be implemented
> after GSoC.
>
> http://codereview.appspot.com/1866042/diff/17001/18072
> File src/devices/lte/lte-spectrum-value-helper.cc (right):
>
> http://codereview.appspot.com/1866042/diff/17001/18072#newcode27
> src/devices/lte/lte-spectrum-value-helper.cc:27: Ptr<SpectrumModel>
> LteSpectrumModel;
> On 2010/08/04 12:40:38, Nicola Baldo wrote:
>> There is still a single LteSpectrumModel used both for DL and UL. For
> FDD, there
>> should be two separates FddLteSpectrumModel, one for UL and one for
> DL. It is
>> only for TDD that the spectrum is the same for UL and DL.
>
> Done.
>
> http://codereview.appspot.com/1866042/diff/17001/18072#newcode42
> src/devices/lte/lte-spectrum-value-helper.cc:42: freqs.push_back
> (centralFrequencyOfPRB * 1e9);
> On 2010/08/04 12:40:38, Nicola Baldo wrote:
>> You have exactly the same code in lte-spectrum-model.cc, please remove
> one of
>> these instances
>
> Done.
>
> http://codereview.appspot.com/1866042/diff/17001/18088
> File src/devices/lte/radio-bearer-instance.h (right):
>
> http://codereview.appspot.com/1866042/diff/17001/18088#newcode202
> src/devices/lte/radio-bearer-instance.h:202: Ptr<RlcEntity> GetRlcEntiry
> (void);
> On 2010/08/04 12:40:38, Nicola Baldo wrote:
>> Entiry --> Entity
>
> Done.
>
> http://codereview.appspot.com/1866042/diff/17001/18088#newcode218
> src/devices/lte/radio-bearer-instance.h:218: Ptr<IpClassifierRecord>
> m_classifierRecord;
> On 2010/08/04 12:40:38, Nicola Baldo wrote:
>> Why does the RlcEntity need to know the corresponding
> IpClassifierRecord? I
>> thought that the mapping between the two was to be managed by the
> IpClassifier
>> class only.
>
> Ok, it has been fixed.
>
> http://codereview.appspot.com/1866042/diff/17001/18090
> File src/devices/lte/rlc-entity.h (right):
>
> http://codereview.appspot.com/1866042/diff/17001/18090#newcode42
> src/devices/lte/rlc-entity.h:42: RLC_MODE_TM, RLC_MODEUM, RLC_MODE_AM
> On 2010/08/04 12:40:38, Nicola Baldo wrote:
>> this should be done by class inheritance
>
> Done.
>
> http://codereview.appspot.com/1866042/diff/17001/18090#newcode60
> src/devices/lte/rlc-entity.h:60:
> On 2010/08/04 12:40:38, Nicola Baldo wrote:
>> there should be methods realizing the interface between
> RadioBearerInstance as
>> well as with the Mac. I mean, now the PacketScheduler directly calls
> the
>> Dequeue() method of RadioBearerInstance, so in fact the RLC is
> useless.
>
>> You should modify the API to improve the interface between RadioBearer
> and RLC,
>> as well as between RLC and MAC. In particular:
>
>> 1)  RadioBearerInstance should forward the MSDUs to the associated
> RlcEntity,
>> using a dedicated method of RlcEntity
>
>> 2) the MAC should provide methods so that the RLC can forward the
> MPDUs to the
>> MAC. The API should provide the following functionality mentioned in
> TS 36.322:
>
>> 4.3.2 Services expected from lower layers
>> The following services are expected by RLC from lower layer (i.e.
> MAC):
>> -data transfer;
>> -notification of a transmission opportunity, together with the total
> size of the
>> RLC PDU(s) to be transmitted in the
>> transmission opportunity;
>
>> 3) Since for the GSoC only RLC/TM is expected, the actual
> implementation of
>> RlcEntity will be very simple.
>
>
> I improved the interaction among MAC, RLC and RRC
>
> http://codereview.appspot.com/1866042/diff/17001/18091
> File src/devices/lte/rrc-entity.cc (right):
>
> http://codereview.appspot.com/1866042/diff/17001/18091#newcode108
> src/devices/lte/rrc-entity.cc:108: return false;
> On 2010/08/04 12:40:38, Nicola Baldo wrote:
>> you mean return 0?
>
> Done.
>
> http://codereview.appspot.com/1866042/show



----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.