11n all-in-one

This code adds the new 11n data rates, support for short guard interval, greenfield and mixed HT preambles and HT wifi mac. It also adds the HT-Capabilities information element It does not add HT operations information elelment, A-MPDU, or CTS-To-Self and is thus not yet full 802.11n support.

[Matías Richart, 2013-07-17 19:39:31]

Hi again Ghada.
I think I found a bug.

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/interference-he...
File src/wifi/model/interference-helper.cc (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/interference-he...
src/wifi/model/interference-helper.cc:514: else if (current >= plcpHeaderStart)
Doing some tests I found that although not using HT, the program reach here
causing that MFHeaderMode is passed to CalculateSnr but it is not initialized.
As I'm not using HT plcpHtTrainingSymbolsStart = plcpHsigHeaderStart =
plcpPayloadStart and obviously HeaderStart<PayloadStart
If previous < HeaderStart and current > HeaderStart but current < PayloadStart
then it satisfies this condition.

[Junling Bu, 2013-07-18 12:42:02]

I am not familiar with 802.11n, so I just give my personal thought after
reviewing. The work of this 802.11n is really good and hard.

https://codereview.appspot.com/11345043/diff/1/examples/wireless/ht-wifi-netw...
File examples/wireless/ht-wifi-network.cc (right):

https://codereview.appspot.com/11345043/diff/1/examples/wireless/ht-wifi-netw...
examples/wireless/ht-wifi-network.cc:28: //This is a simple example in order to
show how 802.11n frame aggregation feature (A-MSDU) works.
I think the introduction is copied from simple-wifi-frame-aggregation.cc, but if
they are the same, users maybe get confused, what's the difference between them?
I think it's better to introduce some difference.

And below "NS_LOG_COMPONENT_DEFINE ("DataRates");"
How about using a better name?

https://codereview.appspot.com/11345043/diff/1/examples/wireless/ht-wifi-netw...
examples/wireless/ht-wifi-network.cc:65: std::cout << "DataRate" <<"  " <<
"Throughput" << '\n';
I think the c++ prefer to std::endl rather than '\n'

https://codereview.appspot.com/11345043/diff/1/examples/wireless/ht-wifi-netw...
examples/wireless/ht-wifi-network.cc:78: wifiNodes.Create (1);
wifi station node is only one here, but Network topology is two station node.

https://codereview.appspot.com/11345043/diff/1/examples/wireless/ht-wifi-netw...
examples/wireless/ht-wifi-network.cc:241: std::cout << datarate <<"  " <<
throughput << '\n';
std::endl ?

https://codereview.appspot.com/11345043/diff/1/scratch/datarates.cc
File scratch/datarates.cc (right):

https://codereview.appspot.com/11345043/diff/1/scratch/datarates.cc#newcode2
scratch/datarates.cc:2: /*
The code of this file is complete same as examples/wireless/ht-wifi-network.cc ?
Should remove it ?

https://codereview.appspot.com/11345043/diff/1/src/wifi/doc/wifi.rst
File src/wifi/doc/wifi.rst (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/doc/wifi.rst#newcode188
src/wifi/doc/wifi.rst:188: wifiPhyHelper.Set
("GreenfieldEnabled",BooleanValue(true));
I am not familiar with 802.11n, my confusion is should we set both two
parameters to get 802.11n or sometimes can we set only one? If 802.11n means
both need to be set, maybe we can only provide one parameter.

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/ap-wifi-mac.cc
File src/wifi/model/ap-wifi-mac.cc (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/ap-wifi-mac.cc#...
src/wifi/model/ap-wifi-mac.cc:579: {//
remove "//" or add comments

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/cara-wifi-manag...
File src/wifi/model/cara-wifi-manager.cc (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/cara-wifi-manag...
src/wifi/model/cara-wifi-manager.cc:28: #define Min(a,b) ((a < b) ? a : b)
I see Min in some wifi-managers, how about std::min ?

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/ht-capabilities.cc
File src/wifi/model/ht-capabilities.cc (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/ht-capabilities...
src/wifi/model/ht-capabilities.cc:107: HtCapabilities::GetRxMcsBitmask()
I am confused whether here should return pointer

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/sta-wifi-mac.cc
File src/wifi/model/sta-wifi-mac.cc (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/sta-wifi-mac.cc...
src/wifi/model/sta-wifi-mac.cc:564: // if (rates.IsBasicMcs (mcs))
why comments are here ?
the codes needs or not ?

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/supported-rates.h
File src/wifi/model/supported-rates.h (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/supported-rates...
src/wifi/model/supported-rates.h:32: * allowed to have. Currently this number is
set for IEEE 802.11b/g
should here say the number is set for 802.11n also?

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/wifi-phy.h
File src/wifi/model/wifi-phy.h (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/wifi-phy.h#newc...
src/wifi/model/wifi-phy.h:1: 
this line should be better removed

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/wifi-phy.h#newc...
src/wifi/model/wifi-phy.h:531: * @param isShortPreamble true if short preamble
is used, false otherwise
miss param about txPower

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/wifi-tx-vector.h
File src/wifi/model/wifi-tx-vector.h (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/wifi-tx-vector....
src/wifi/model/wifi-tx-vector.h:37: *
I think TXVECTOR is phy-specific, different phys has different TXVECOR
parameters, WifiTxVector here is used and enough for "High Throughput (HT) PHY
specification", which maybe strange in other phy specification. Fox example, I
want to control data rate and txpower of 802.11a, and this WifiTxVector is OH,
but STBC could make me confused. My thought is can we find a more suitable to
implement TXVECTOR defined in the standard ?
Another issues is the introduction here is describe for DSSS Phy which is not
correct as the WifiTxVector actually do here.

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/yans-wifi-phy.cc
File src/wifi/model/yans-wifi-phy.cc (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/yans-wifi-phy.c...
src/wifi/model/yans-wifi-phy.cc:125: UintegerValue (2407),
why we need Frequency here? 
The YansWifiPhy defines m_channelStartingFrequency,
when Configure80211a method is called, m_channelStartingFrequency = 5e3,
when Configure80211b method is called,
m_channelStartingFrequency = 2407.
so when we configure 802.11n, I think it is enough to
to set channelStartingFrequency.

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/yans-wifi-phy.h
File src/wifi/model/yans-wifi-phy.h (right):

https://codereview.appspot.com/11345043/diff/1/src/wifi/model/yans-wifi-phy.h...
src/wifi/model/yans-wifi-phy.h:156: virtual void SetFreq (uint32_t freq);
I still think m_channelStartingFrequency is enough.

[gbadawy, 2013-07-18 15:56:09]

On 2013/07/17 19:39:31, Matías Richart wrote:
> Hi again Ghada.
> I think I found a bug.
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/interference-he...
> File src/wifi/model/interference-helper.cc (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/interference-he...
> src/wifi/model/interference-helper.cc:514: else if (current >=
plcpHeaderStart)
> Doing some tests I found that although not using HT, the program reach here
> causing that MFHeaderMode is passed to CalculateSnr but it is not initialized.
> As I'm not using HT plcpHtTrainingSymbolsStart = plcpHsigHeaderStart =
> plcpPayloadStart and obviously HeaderStart<PayloadStart
> If previous < HeaderStart and current > HeaderStart but current < PayloadStart
> then it satisfies this condition.

Hi Matias,
Yes it is a bug. Thanks alot. For some reason I wrote the code thinking
plcpHtTrainingSymbolsStart = plcpHsigHeaderStart =plcpHeaderStart.
I corrected it and will issue a patch soon. It was a hard bug to find. A fresh
eye is always a good one
Thanks,
Ghada

[gbadawy, 2013-07-18 17:13:06]

On 2013/07/18 12:42:02, Junling Bu wrote:
> I am not familiar with 802.11n, so I just give my personal thought after
> reviewing. The work of this 802.11n is really good and hard.
> 
>
https://codereview.appspot.com/11345043/diff/1/examples/wireless/ht-wifi-netw...
> File examples/wireless/ht-wifi-network.cc (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/examples/wireless/ht-wifi-netw...
> examples/wireless/ht-wifi-network.cc:28: //This is a simple example in order
to
> show how 802.11n frame aggregation feature (A-MSDU) works.
> I think the introduction is copied from simple-wifi-frame-aggregation.cc, but
if
> they are the same, users maybe get confused, what's the difference between
them?
> I think it's better to introduce some difference.
 I will change it

> And below "NS_LOG_COMPONENT_DEFINE ("DataRates");"
> How about using a better name?
I will use the same name as the file
 
>
https://codereview.appspot.com/11345043/diff/1/examples/wireless/ht-wifi-netw...
> examples/wireless/ht-wifi-network.cc:65: std::cout << "DataRate" <<"  " <<
> "Throughput" << '\n';
> I think the c++ prefer to std::endl rather than '\n'

>
https://codereview.appspot.com/11345043/diff/1/examples/wireless/ht-wifi-netw...
> examples/wireless/ht-wifi-network.cc:78: wifiNodes.Create (1);
> wifi station node is only one here, but Network topology is two station node.
 Will change it

>
https://codereview.appspot.com/11345043/diff/1/examples/wireless/ht-wifi-netw...
> examples/wireless/ht-wifi-network.cc:241: std::cout << datarate <<"  " <<
> throughput << '\n';
> std::endl ?
> 
> https://codereview.appspot.com/11345043/diff/1/scratch/datarates.cc
> File scratch/datarates.cc (right):
> 
> https://codereview.appspot.com/11345043/diff/1/scratch/datarates.cc#newcode2
> scratch/datarates.cc:2: /*
> The code of this file is complete same as examples/wireless/ht-wifi-network.cc
?
> Should remove it ?
Will remove it
> https://codereview.appspot.com/11345043/diff/1/src/wifi/doc/wifi.rst
> File src/wifi/doc/wifi.rst (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/doc/wifi.rst#newcode188
> src/wifi/doc/wifi.rst:188: wifiPhyHelper.Set
> ("GreenfieldEnabled",BooleanValue(true));
> I am not familiar with 802.11n, my confusion is should we set both two
> parameters to get 802.11n or sometimes can we set only one? If 802.11n means
> both need to be set, maybe we can only provide one parameter.

No you can set greenfield with no short guard interval

> https://codereview.appspot.com/11345043/diff/1/src/wifi/model/ap-wifi-mac.cc
> File src/wifi/model/ap-wifi-mac.cc (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/ap-wifi-mac.cc#...
> src/wifi/model/ap-wifi-mac.cc:579: {//
> remove "//" or add comments

Removed

>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/cara-wifi-manag...
> File src/wifi/model/cara-wifi-manager.cc (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/cara-wifi-manag...
> src/wifi/model/cara-wifi-manager.cc:28: #define Min(a,b) ((a < b) ? a : b)
> I see Min in some wifi-managers, how about std::min ?
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/ht-capabilities.cc
> File src/wifi/model/ht-capabilities.cc (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/ht-capabilities...
> src/wifi/model/ht-capabilities.cc:107: HtCapabilities::GetRxMcsBitmask()
> I am confused whether here should return pointer
I don't understand your comment why are you confused

> https://codereview.appspot.com/11345043/diff/1/src/wifi/model/sta-wifi-mac.cc
> File src/wifi/model/sta-wifi-mac.cc (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/sta-wifi-mac.cc...
> src/wifi/model/sta-wifi-mac.cc:564: // if (rates.IsBasicMcs (mcs))
> why comments are here ?
> the codes needs or not ?
Removed

>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/supported-rates.h
> File src/wifi/model/supported-rates.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/supported-rates...
> src/wifi/model/supported-rates.h:32: * allowed to have. Currently this number
is
> set for IEEE 802.11b/g
> should here say the number is set for 802.11n also?

Changed

> https://codereview.appspot.com/11345043/diff/1/src/wifi/model/wifi-phy.h
> File src/wifi/model/wifi-phy.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/wifi-phy.h#newc...
> src/wifi/model/wifi-phy.h:1: 
> this line should be better removed
Removed

>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/wifi-phy.h#newc...
> src/wifi/model/wifi-phy.h:531: * @param isShortPreamble true if short preamble
> is used, false otherwise
> miss param about txPower
Done
 
> https://codereview.appspot.com/11345043/diff/1/src/wifi/model/wifi-tx-vector.h
> File src/wifi/model/wifi-tx-vector.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/wifi-tx-vector....
> src/wifi/model/wifi-tx-vector.h:37: *
> I think TXVECTOR is phy-specific, different phys has different TXVECOR
> parameters, WifiTxVector here is used and enough for "High Throughput (HT) PHY
> specification", which maybe strange in other phy specification. Fox example, I
> want to control data rate and txpower of 802.11a, and this WifiTxVector is OH,
> but STBC could make me confused. My thought is can we find a more suitable to
> implement TXVECTOR defined in the standard ?
> Another issues is the introduction here is describe for DSSS Phy which is not
> correct as the WifiTxVector actually do here.
> 
TxVector is used for any phy and if the phy is not HT then STBC is going to be
false

> https://codereview.appspot.com/11345043/diff/1/src/wifi/model/yans-wifi-phy.cc
> File src/wifi/model/yans-wifi-phy.cc (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/yans-wifi-phy.c...
> src/wifi/model/yans-wifi-phy.cc:125: UintegerValue (2407),
> why we need Frequency here? 
> The YansWifiPhy defines m_channelStartingFrequency,
> when Configure80211a method is called, m_channelStartingFrequency = 5e3,
> when Configure80211b method is called,
> m_channelStartingFrequency = 2407.
> so when we configure 802.11n, I think it is enough to
> to set channelStartingFrequency.

802.11n can work on wither 2.4GHz or 5GHz

> https://codereview.appspot.com/11345043/diff/1/src/wifi/model/yans-wifi-phy.h
> File src/wifi/model/yans-wifi-phy.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/1/src/wifi/model/yans-wifi-phy.h...
> src/wifi/model/yans-wifi-phy.h:156: virtual void SetFreq (uint32_t freq);
> I still think m_channelStartingFrequency is enough.

Same as before

[Daniel L., 2013-07-28 23:07:42]

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h
File src/wifi/model/wifi-phy.h (right):

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
src/wifi/model/wifi-phy.h:254: static uint32_t
GetPlcpHtTrainingSymboldDurationMicroSeconds (WifiMode payloadMode, WifiPreamble
preamble, WifiTxVector txvector);
Symbol

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
src/wifi/model/wifi-phy.h:548: virtual void SetFreq (uint32_t freq)=0;
Frequency

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
src/wifi/model/wifi-phy.h:570: virtual uint32_t GetFreq (void) const=0;
Frequency

I think you usually pair Set/Get together. May I suggest that you follow that
convention instead of grouping Sets/Gets?

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
src/wifi/model/wifi-phy.h:572: virtual uint32_t GetNumberOfTx (void) const=0;
Transmitter / Receiver

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote...
File src/wifi/model/wifi-remote-station-manager.h (right):

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote...
src/wifi/model/wifi-remote-station-manager.h:103: bool GetHtSupported (void)
const;
Is/Has instead of Get

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote...
src/wifi/model/wifi-remote-station-manager.h:329: uint32_t GetNoOfTransmitters
(void);
GetNumberOfTransmitters

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote...
src/wifi/model/wifi-remote-station-manager.h:342: uint32_t GetNoOfRx (const
WifiRemoteStation *station) const;
Number Receivers

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-tx-vec...
File src/wifi/model/wifi-tx-vector.h (right):

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-tx-vec...
src/wifi/model/wifi-tx-vector.h:74: uint8_t  m_txPowerLevel;  /**< The
TXPWR_LEVEL parameter in Table 15-4.
I think you are not trying to aligned variable declarations so these should
follow the same convention.

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
File src/wifi/model/yans-wifi-phy.h (right):

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
src/wifi/model/yans-wifi-phy.h:158: virtual void SetFreq (uint32_t freq);
Frequency

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
src/wifi/model/yans-wifi-phy.h:244: uint32_t m_tx;
m_numberOfTransmitters/Receivers

[gbadawy, 2013-07-29 15:36:54]

On 2013/07/28 23:07:42, Daniel L. wrote:
> https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h
> File src/wifi/model/wifi-phy.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
> src/wifi/model/wifi-phy.h:254: static uint32_t
> GetPlcpHtTrainingSymboldDurationMicroSeconds (WifiMode payloadMode,
WifiPreamble
> preamble, WifiTxVector txvector);
> Symbol
 Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
> src/wifi/model/wifi-phy.h:548: virtual void SetFreq (uint32_t freq)=0;
> Frequency
Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
> src/wifi/model/wifi-phy.h:570: virtual uint32_t GetFreq (void) const=0;
> Frequency
> 
> I think you usually pair Set/Get together. May I suggest that you follow that
> convention instead of grouping Sets/Gets?

Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
> src/wifi/model/wifi-phy.h:572: virtual uint32_t GetNumberOfTx (void) const=0;
> Transmitter / Receiver

Done
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote...
> File src/wifi/model/wifi-remote-station-manager.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote...
> src/wifi/model/wifi-remote-station-manager.h:103: bool GetHtSupported (void)
> const;
> Is/Has instead of Get
Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote...
> src/wifi/model/wifi-remote-station-manager.h:329: uint32_t GetNoOfTransmitters
> (void);
> GetNumberOfTransmitters

Done
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote...
> src/wifi/model/wifi-remote-station-manager.h:342: uint32_t GetNoOfRx (const
> WifiRemoteStation *station) const;
> Number Receivers
Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-tx-vec...
> File src/wifi/model/wifi-tx-vector.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-tx-vec...
> src/wifi/model/wifi-tx-vector.h:74: uint8_t  m_txPowerLevel;  /**< The
> TXPWR_LEVEL parameter in Table 15-4.
> I think you are not trying to aligned variable declarations so these should
> follow the same convention.

I don't understand what are you trying to say?

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
> File src/wifi/model/yans-wifi-phy.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
> src/wifi/model/yans-wifi-phy.h:158: virtual void SetFreq (uint32_t freq);
> Frequency
Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
> src/wifi/model/yans-wifi-phy.h:244: uint32_t m_tx;
> m_numberOfTransmitters/Receivers
Done

[Daniel L., 2013-07-29 15:42:09]

Hi Ghada,

It's just how the variable declarations are aligned, for example:

aligned:
uint32_t a
bool       x

versus

unaligned:
uint32_t a
bool x

I'm not sure if it's the code review formatting or they are actually not
aligned. I think you have both aligned and unaligned in wifi-tx-vector.h

WifiMode m_mode
uint8_t m_txPowerLevel
uint8_t m_retries


bool m_shortGuardInterval
uint8_t m_nss
uint8_t m_ness
bool m_stbc


On Mon, Jul 29, 2013 at 8:36 AM, <gbadawy@gmail.com> wrote:

> On 2013/07/28 23:07:42, Daniel L. wrote:
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-phy.h<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h>
>
>> File src/wifi/model/wifi-phy.h (right):
>>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-phy.h#**newcode254<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#newcode254>
>
>> src/wifi/model/wifi-phy.h:254: static uint32_t
>> GetPlcpHtTrainingSymboldDurati**onMicroSeconds (WifiMode payloadMode,
>>
> WifiPreamble
>
>> preamble, WifiTxVector txvector);
>> Symbol
>>
>  Done
>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-phy.h#**newcode548<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#newcode548>
>
>> src/wifi/model/wifi-phy.h:548: virtual void SetFreq (uint32_t freq)=0;
>> Frequency
>>
> Done
>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-phy.h#**newcode570<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#newcode570>
>
>> src/wifi/model/wifi-phy.h:570: virtual uint32_t GetFreq (void)
>>
> const=0;
>
>> Frequency
>>
>
>  I think you usually pair Set/Get together. May I suggest that you
>>
> follow that
>
>> convention instead of grouping Sets/Gets?
>>
>
> Done
>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-phy.h#**newcode572<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#newcode572>
>
>> src/wifi/model/wifi-phy.h:572: virtual uint32_t GetNumberOfTx (void)
>>
> const=0;
>
>> Transmitter / Receiver
>>
>
> Done
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-remote-**station-manager.h<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote-station-manager.h>
>
>> File src/wifi/model/wifi-remote-**station-manager.h (right):
>>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-remote-**station-manager.h#newcode103<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote-station-manager.h#newcode103>
>
>> src/wifi/model/wifi-remote-**station-manager.h:103: bool GetHtSupported
>>
> (void)
>
>> const;
>> Is/Has instead of Get
>>
> Done
>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-remote-**station-manager.h#newcode329<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote-station-manager.h#newcode329>
>
>> src/wifi/model/wifi-remote-**station-manager.h:329: uint32_t
>>
> GetNoOfTransmitters
>
>> (void);
>> GetNumberOfTransmitters
>>
>
> Done
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-remote-**station-manager.h#newcode342<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-remote-station-manager.h#newcode342>
>
>> src/wifi/model/wifi-remote-**station-manager.h:342: uint32_t GetNoOfRx
>>
> (const
>
>> WifiRemoteStation *station) const;
>> Number Receivers
>>
> Done
>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-tx-vector.h<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-tx-vector.h>
>
>> File src/wifi/model/wifi-tx-vector.**h (right):
>>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/wifi-tx-vector.h#**newcode74<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-tx-vector.h#newcode74>
>
>> src/wifi/model/wifi-tx-vector.**h:74: uint8_t  m_txPowerLevel;  /**< The
>> TXPWR_LEVEL parameter in Table 15-4.
>> I think you are not trying to aligned variable declarations so these
>>
> should
>
>> follow the same convention.
>>
>
> I don't understand what are you trying to say?
>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/yans-wifi-phy.h<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-phy.h>
>
>> File src/wifi/model/yans-wifi-phy.h (right):
>>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/yans-wifi-phy.h#**newcode158<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-phy.h#newcode158>
>
>> src/wifi/model/yans-wifi-phy.**h:158: virtual void SetFreq (uint32_t
>>
> freq);
>
>> Frequency
>>
> Done
>
>
>
> https://codereview.appspot.**com/11345043/diff/22001/src/**
>
wifi/model/yans-wifi-phy.h#**newcode244<https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-phy.h#newcode244>
>
>> src/wifi/model/yans-wifi-phy.**h:244: uint32_t m_tx;
>> m_numberOfTransmitters/**Receivers
>>
> Done
>
>
>
https://codereview.appspot.**com/11345043/<https://codereview.appspot.com/113...
>

[Nicola Baldo, 2013-07-30 18:21:50]

First of all, thanks for this contribution! Clearly there is a significant
amount of work behind this patch. Plus, I am glad that someone finally found the
TxVector patch useful :-)

As a general question, it would be good to have some tests for some key
functionalities which are being modified/enhanced, such as the TX duration
calculation and the interference calculation. 

Please find below some detailed comments on the code.

Regards,

Nicola

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/ht-capabili...
File src/wifi/model/ht-capabilities.cc (right):

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/ht-capabili...
src/wifi/model/ht-capabilities.cc:3: * Copyright (c) 2010 CTTC
really? :-)

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/interferenc...
File src/wifi/model/interference-helper.cc (right):

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/interferenc...
src/wifi/model/interference-helper.cc:319: /* else if (current
>=plcpHtTrainingSymbolsStart)
kill dead code

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy-st...
File src/wifi/model/wifi-phy-standard.h (right):

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy-st...
src/wifi/model/wifi-phy-standard.h:52: //added by Ghada Badawy
These comments are not needed. We use mercurial's history for this purpose.

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h
File src/wifi/model/wifi-phy.h (right):

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
src/wifi/model/wifi-phy.h:363: virtual uint32_t GetBssMembershipSelector
(uint32_t selector) const=0;
please add doxygen for all these public methods

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
src/wifi/model/wifi-phy.h:566: virtual void SetStbc (bool stbc)=0;
please add doxygen to all public methods

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-tx-vec...
File src/wifi/model/wifi-tx-vector.h (right):

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-tx-vec...
src/wifi/model/wifi-tx-vector.h:52: WifiMode GetMode (void) const;
please add doxygen for public members

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
File src/wifi/model/yans-wifi-phy.h (right):

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
src/wifi/model/yans-wifi-phy.h:158: virtual void SetFreq (uint32_t freq);
What is the relation between SetFreq and SetChannelNumber()? Are both really
needed?

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
src/wifi/model/yans-wifi-phy.h:162: virtual void SetNumberOfTx (uint32_t tx);
is "number of transmitters" a standard naming? If it's not, I'd propose a more
meaningful "number of transmitting antennas".

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
src/wifi/model/yans-wifi-phy.h:166: virtual void SetNumberOfRx (uint32_t rx) ;
same as previous comment

https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
src/wifi/model/yans-wifi-phy.h:174: * \paramif LDPC is supported or not
\paramif ?

[gbadawy, 2013-08-01 15:37:13]

> First of all, thanks for this contribution! Clearly there is a significant
> amount of work behind this patch. Plus, I am glad that someone finally found
the
> TxVector patch useful :-)
> 
> As a general question, it would be good to have some tests for some key
> functionalities which are being modified/enhanced, such as the TX duration
> calculation and the interference calculation. 
> 
> Please find below some detailed comments on the code.
> 
> Regards,
> 
> Nicola

Hi Nicola,
Thanks for the txVecto patch it was very useful :) Also thank you for the test
idea I needed some I will upload a new patch with at txDuration test case and I
am working on the interference one

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/ht-capabili...
> File src/wifi/model/ht-capabilities.cc (right):
> 
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/ht-capabili...
> src/wifi/model/ht-capabilities.cc:3: * Copyright (c) 2010 CTTC
> really? :-)
Deleted

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/interferenc...
> File src/wifi/model/interference-helper.cc (right):
> 
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/interferenc...
> src/wifi/model/interference-helper.cc:319: /* else if (current
> >=plcpHtTrainingSymbolsStart)
> kill dead code
Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy-st...
> File src/wifi/model/wifi-phy-standard.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy-st...
> src/wifi/model/wifi-phy-standard.h:52: //added by Ghada Badawy
> These comments are not needed. We use mercurial's history for this purpose.
Done

> https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h
> File src/wifi/model/wifi-phy.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
> src/wifi/model/wifi-phy.h:363: virtual uint32_t GetBssMembershipSelector
> (uint32_t selector) const=0;
> please add doxygen for all these public methods
Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-phy.h#...
> src/wifi/model/wifi-phy.h:566: virtual void SetStbc (bool stbc)=0;
> please add doxygen to all public methods
Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-tx-vec...
> File src/wifi/model/wifi-tx-vector.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/wifi-tx-vec...
> src/wifi/model/wifi-tx-vector.h:52: WifiMode GetMode (void) const;
> please add doxygen for public members
Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
> File src/wifi/model/yans-wifi-phy.h (right):
> 
>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
> src/wifi/model/yans-wifi-phy.h:158: virtual void SetFreq (uint32_t freq);
> What is the relation between SetFreq and SetChannelNumber()? Are both really
> needed?
In all previous IEEE standards the standard defined one operation frequency
until 11n where the standard defines 2 the 2.4 GHz and the 5GHz so I created
SetFrequency() to set the m_channelstartingfrequency depending on the user
input. Before there was no access to m_channelstartingfrequency it was set
automatically in ConfigureX() where X represents the chosen standard. I had a
variable called m_frequency. I deleted that one as it is a duplicate to
m_channelstartingfrequency. So does that make sense now?

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
> src/wifi/model/yans-wifi-phy.h:162: virtual void SetNumberOfTx (uint32_t tx);
> is "number of transmitters" a standard naming? If it's not, I'd propose a more
> meaningful "number of transmitting antennas".
changed to NumberOfTransmitAntennas as in the standard

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
> src/wifi/model/yans-wifi-phy.h:166: virtual void SetNumberOfRx (uint32_t rx) ;
> same as previous comment
Done

>
https://codereview.appspot.com/11345043/diff/22001/src/wifi/model/yans-wifi-p...
> src/wifi/model/yans-wifi-phy.h:174: * \paramif LDPC is supported or not
> \paramif ?
Changed

Thanks
Ghada