ns-3: Bring rate representation in line with IEEE 802.11

This patch attempts to lay some groundwork for enhancements to the Wi-Fi module by bringing the representation of transmit rates more in line with the standard.

The patch is based on ns-3-dev changeset 6266:35e7d9d585be, and is proposed as part of a fix for Bug 871 in the ns-3 Bugzilla module.

The key part of the patch is the introduction of a type that corresponds to the notion of Modulation Class described in IEEE 802.11-2007, Section 9.6.1, Table 9-2.

It also adds coding rate information to the WifiModes and centralises their construction into a single WifiModeFactory method. The rates are also renamed with reference to the part of the standard that defines them.

WifiModes no longer have a WifiStandard, but the latter type still exists and is used to imply the set of WifiModes that a MAC/PHY pair will support.

[Mathieu Lacage, 2010-05-19 06:57:11]

my initial reaction was that this patch did too many things at the same time:
changing the names, adding new methods and fields to WifiRate... Also, I was
worried about the name change but I guess that it makes sense if you really want
to support the extra standards...

To summarize, I can't find any good reason against this patch but nicola will
need to review this.

[Nicola Baldo, 2010-05-21 17:43:51]

Dean, thank you very much for your effort in trying to address this issue. I
think your code goes in the right direction, but there are a few issues that we
need to address. I think the most problematic is the issue of the TX duration
for the DSSS-OFDM vs ERP-OFDM case.

http://codereview.appspot.com/986041/diff/1/18
File src/devices/wifi/wifi-mode.cc (right):

http://codereview.appspot.com/986041/diff/1/18#newcode181
src/devices/wifi/wifi-mode.cc:181: item->modulation = WifiMode::DQPSK;
what about CCK modulation?

http://codereview.appspot.com/986041/diff/1/18#newcode271
src/devices/wifi/wifi-mode.cc:271: item->modClass = WIFI_MOD_CLASS_DSSS;
maybe better to define WIFI_MOD_CLASS_UNKNOWN

http://codereview.appspot.com/986041/diff/1/19
File src/devices/wifi/wifi-mode.h (right):

http://codereview.appspot.com/986041/diff/1/19#newcode57
src/devices/wifi/wifi-mode.h:57: * This enumeration defines the various coding
rates used for the OFDM
but you don't use it only for OFDM, right?

http://codereview.appspot.com/986041/diff/1/19#newcode199
src/devices/wifi/wifi-mode.h:199: * \param codingRate the convolutional coding
rate used
 I would not say "convolutional" here, because this parameter is to be specified
for all modulation classes

http://codereview.appspot.com/986041/diff/1/21
File src/devices/wifi/wifi-phy.cc (right):

http://codereview.appspot.com/986041/diff/1/21#newcode153
src/devices/wifi/wifi-phy.cc:153: WifiModeFactory::CreateWifiMode
("WiFiClause15Rate1Mbps",

I agree that the new naming convention that you are proposing
("WiFiClause15Rate1Mbps") is an improvement w.r.t. the current one, because it
refers to the current standard. However, I wonder if it would be possible to
find something more meaningful for the users... what about "WifiDsssRate1Mbps"
or something similar?

http://codereview.appspot.com/986041/diff/1/21#newcode183
src/devices/wifi/wifi-phy.cc:183: WifiModeFactory::CreateWifiMode
("WiFiClause18Rate5M5bps",
I understand that we can't use "5.5Mbps", but "5M5bps" is really awkward. What
about 5_5Mbps as in the old code?

http://codereview.appspot.com/986041/diff/1/21#newcode565
src/devices/wifi/wifi-phy.cc:565: ns3::WifiPhy::GetClause17Rate9MbpsBW10MHz ();
My understanding is that you define WifiModes only for the available payload
modes - i.e., we don't see any "Clause19" mode because they use the same payload
modes of Clause 15, 17 and 18.

However, I see a problem: the WifiMode is actually also used to determining the
frame duration, including not only the payload, but also the PLCP (see
InterferenceHelper::CalculateTxDuration(). For example, without being able to
tell DSSS-OFDM from ERP-OFDM, how do you calculate the right PLCP duration? 

One solution might be to replicate WifiModes for every ModulationClass. An
alternative approach is to transform the current "WifiPreamble" into "WifiPlcp",
and give it the ability to distinguish between DSSS-OFDM and ERP-OFDM. What do
you think?

[Dean, 2010-05-22 09:00:11]

Mathieu, Nicola,

Thanks for your comments.

Mathieu,

I'll endeavour to present smaller coherent pieces of change in future. If you
(or anyone else) wants me to break this one down for review purposes then I'll
do that, but if there's no specific request then I'll probably leave this one
with it's current scope to save myself some work.

Nicola,

Please find responses to your comments below. If you are happy with the
explanations and proposed changes then let me know and I'll spin another set of
changes for review.

Kind regards,
Dean.

http://codereview.appspot.com/986041/diff/1/18
File src/devices/wifi/wifi-mode.cc (right):

http://codereview.appspot.com/986041/diff/1/18#newcode181
src/devices/wifi/wifi-mode.cc:181: item->modulation = WifiMode::DQPSK;
> what about CCK modulation?

I was following the existing code here. Prior to my proposed changes, both 5.5
Mbps and 11 Mbps rates of Modulation Class DSSS are created with calls to the
old WifiModeFactory::CreateDqpsk() method, which does this. This seemed
reasonable to me as "CCK modulation" can be considered to logically comprise a
Complementary Code applied prior to modulation using DQPSK.

I can readily add a new ModulationType, but I'll note that this would shift us
from the current approach which appears to be that we use the underlying
constellation (whether OFD multiplexed or not) to set the "modulation" parameter
of each WifiMode.

http://codereview.appspot.com/986041/diff/1/18#newcode271
src/devices/wifi/wifi-mode.cc:271: item->modClass = WIFI_MOD_CLASS_DSSS;
> maybe better to define WIFI_MOD_CLASS_UNKNOWN

Okay. I will also add a sprinkling of assertions in suitable places to ensure
that this value does not propagate into simulation-proper.

http://codereview.appspot.com/986041/diff/1/19
File src/devices/wifi/wifi-mode.h (right):

http://codereview.appspot.com/986041/diff/1/19#newcode57
src/devices/wifi/wifi-mode.h:57: * This enumeration defines the various coding
rates used for the OFDM
> but you don't use it only for OFDM, right?

Yes, I do use it only for OFDM rates. Rates of Modulation Class DSSS (includes
Clause 15 and Clause 18 rates) do not have as explicit a coding stage as OFDM
rates, so for these the WIFI_CODE_RATE_UNCODED option is present (there is a
terse comment to this effect; perhaps your query is alluding to the fact that
this comment could be improved).

In principle, I could capture the CCK coding in this way too, but so far I've
chosen not to as the coding is (in contrast to OFDM) often lumped in with the
modulation. Are you suggesting that I revisit this decision?

http://codereview.appspot.com/986041/diff/1/19#newcode199
src/devices/wifi/wifi-mode.h:199: * \param codingRate the convolutional coding
rate used
>  I would not say "convolutional" here, because this parameter is to be
specified
> for all modulation classes

As mentioned above, the codingRate parameter is only currently used for OFDM, so
if valid it is always specifying the rate for a convolutional code. I can see,
however, that extra comment is needed to provide some certainty for those
dealing with rates without and explicit convolutional coding step.

I propose to change the comment to something like "\param codingRate if
convolutional coding is used for thie rate then this parameter specifies the
convolutional coding rate used. If there is no explicit convolutional coding
step (e.g., for DSSS rates) then the caller should set this parameter to
WIFI_CODE_RATE_UNCODED."

Of course, this would change further if I was to incorporate the coding for
rates of modulation class DSSS in this encoding.

http://codereview.appspot.com/986041/diff/1/21
File src/devices/wifi/wifi-phy.cc (right):

http://codereview.appspot.com/986041/diff/1/21#newcode153
src/devices/wifi/wifi-phy.cc:153: WifiModeFactory::CreateWifiMode
("WiFiClause15Rate1Mbps",
> I agree that the new naming convention that you are proposing
> ("WiFiClause15Rate1Mbps") is an improvement w.r.t. the current one, because it
> refers to the current standard. However, I wonder if it would be possible to
> find something more meaningful for the users... what about "WifiDsssRate1Mbps"
> or something similar?

Seems reasonable. Presumably what you are alluding to is the replacement of
"Clausexx" with a textual representation of the Modulation Class as defined in
IEEE 802.11-2007, Section 9.6.1, Table 9-2. e.g., WifiDsssRate1Mbps,
WifiOfdmRate6Mbps, (and WifiErpOfdmRate6Mbps, when support for Clause 19 rates
is added).

http://codereview.appspot.com/986041/diff/1/21#newcode183
src/devices/wifi/wifi-phy.cc:183: WifiModeFactory::CreateWifiMode
("WiFiClause18Rate5M5bps",
> I understand that we can't use "5.5Mbps", but "5M5bps" is really awkward. What
> about 5_5Mbps as in the old code?

Okay.

http://codereview.appspot.com/986041/diff/1/21#newcode565
src/devices/wifi/wifi-phy.cc:565: ns3::WifiPhy::GetClause17Rate9MbpsBW10MHz ();
> My understanding is that you define WifiModes only for the available payload
> modes - i.e., we don't see any "Clause19" mode because they use the same
payload
> modes of Clause 15, 17 and 18.

This changeset does not add support for the rates added by Clause 19. That is a
distinct bug and fix.

> However, I see a problem: the WifiMode is actually also used to determining
the
> frame duration, including not only the payload, but also the PLCP (see
> InterferenceHelper::CalculateTxDuration(). For example, without being able to
> tell DSSS-OFDM from ERP-OFDM, how do you calculate the right PLCP duration? 
> 
> One solution might be to replicate WifiModes for every ModulationClass. An
> alternative approach is to transform the current "WifiPreamble" into
"WifiPlcp",
> and give it the ability to distinguish between DSSS-OFDM and ERP-OFDM. What do
> you think?

DSSS-OFDM and ERP-OFDM rates have distinct Modulation Classes (you'll see these
enumerated in WifiModulationClass in wifi-mode.h), and thus are readily
distinguished when support for one or t'other is added (I have a subsequent cset
that adds support for ERP-OFDM).

[Mathieu Lacage, 2010-05-22 18:23:23]

On Sat, May 22, 2010 at 11:00 AM,  <deanarm@gmail.com> wrote:

> I'll endeavour to present smaller coherent pieces of change in future.
> If you (or anyone else) wants me to break this one down for review
> purposes then I'll do that, but if there's no specific request then I'll
> probably leave this one with it's current scope to save myself some
> work.

I am fine with the current patch. It was just really big and I know
that (some) users will scream about the mode name change because they
can't even catch it at compile-time. Maybe it would be worth adding
some 'alias' modes with the old names for 1 or 2 releases to avoid too
many screams. Some users will always complain about changes anyway so,
feel free to ignore this comment if nicola is fine with it.

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

[Nicola Baldo, 2010-05-26 15:38:53]

Hi Dean, 

thank you for your reply. Below some further comments...

http://codereview.appspot.com/986041/diff/1/18
File src/devices/wifi/wifi-mode.cc (right):

http://codereview.appspot.com/986041/diff/1/18#newcode181
src/devices/wifi/wifi-mode.cc:181: item->modulation = WifiMode::DQPSK;
On 2010/05/22 09:00:11, Dean wrote:
> > what about CCK modulation?
> 
> I was following the existing code here. Prior to my proposed changes, both 5.5
> Mbps and 11 Mbps rates of Modulation Class DSSS are created with calls to the
> old WifiModeFactory::CreateDqpsk() method, which does this. This seemed
> reasonable to me as "CCK modulation" can be considered to logically comprise a
> Complementary Code applied prior to modulation using DQPSK.
> 
> I can readily add a new ModulationType, but I'll note that this would shift us
> from the current approach which appears to be that we use the underlying
> constellation (whether OFD multiplexed or not) to set the "modulation"
parameter
> of each WifiMode.
> 


I checked current ns-3-dev to see where WifiMode::GetModulationType() is is
used. It turns out that the only piece of code using it is
WifiRemoteStationManager::GetControlAnswerMode(), which is incorrect as you
pointed out earlier (it should use the ModulationClass instead). 

I thought that error models would use WifiMode::GetModulationType(), but
actually the implementation of YansErrorRateModel::GetChunkSuccessRate() tests
for specific WifiModes, rather than for generic characteristics such as
modulation and code rate. This patch uses WifiMode::GetConstellationSize(), but
not GetModulationType().

So the current "modulation" parameter will not be necessary after your patch
introduces ModulationClass... what should we do with it? 

If we decide to leave the modulation type around, we need to make it consistent.
I would agree with either of the following:

1) using DQPSK for 5.5 and 11 modes, constellation size of 4, and code rates 1/4
and 1/2, respectively;

2) using a new defined CCK modulation, with PHY rate equal to 11e6/8 (8 chips
per symbol), constellation size equal to 4 and 8 for the 5.5 and 11 modes
respectively, and coding rate of 1.


Maybe it is just simpler to remove the modulation type completely, since we
don't use it in the end... what do you think?

http://codereview.appspot.com/986041/diff/1/19
File src/devices/wifi/wifi-mode.h (right):

http://codereview.appspot.com/986041/diff/1/19#newcode57
src/devices/wifi/wifi-mode.h:57: * This enumeration defines the various coding
rates used for the OFDM
On 2010/05/22 09:00:11, Dean wrote:
> > but you don't use it only for OFDM, right?
> 
> Yes, I do use it only for OFDM rates. Rates of Modulation Class DSSS (includes
> Clause 15 and Clause 18 rates) do not have as explicit a coding stage as OFDM
> rates, so for these the WIFI_CODE_RATE_UNCODED option is present (there is a
> terse comment to this effect; perhaps your query is alluding to the fact that
> this comment could be improved).
> 
> In principle, I could capture the CCK coding in this way too, but so far I've
> chosen not to as the coding is (in contrast to OFDM) often lumped in with the
> modulation. Are you suggesting that I revisit this decision?
> 

If you make WifiMode::GetCodeRate work also for DSSS modes, then I would ask you
to make it return a consistent value.

Otherwise, you could trigger an assertion, saying that this functionality is
only supported for OFDM, and return WIFI_CODE_RATE_UNDEFINED (rather than
UNCODED) so we handle correctly the case of optimized builds.

http://codereview.appspot.com/986041/diff/1/21
File src/devices/wifi/wifi-phy.cc (right):

http://codereview.appspot.com/986041/diff/1/21#newcode565
src/devices/wifi/wifi-phy.cc:565: ns3::WifiPhy::GetClause17Rate9MbpsBW10MHz ();
> 
> This changeset does not add support for the rates added by Clause 19. That is
a
> distinct bug and fix.

I agree with your approach of separating the two issues,  I just want to make
sure that this patch does not create problem for the subsequent Clause 19 patch.


> DSSS-OFDM and ERP-OFDM rates have distinct Modulation Classes (you'll see
these
> enumerated in WifiModulationClass in wifi-mode.h), and thus are readily
> distinguished when support for one or t'other is added (I have a subsequent
cset
> that adds support for ERP-OFDM).
> 

Thanks, I now understand how InterferenceHelper::CalculateTxDuration() would
work. 


To summarize, my comment boils down to the naming of the WifiMode getter
functions, which should become something like

WifiPhy::GetXXXXRateYYMbps

where XXXX is the name of the modulation class (e.g., "Dsss" rather than
"Clause15") and YY is the rate in Mbps as usual. This is similar to the naming
issues as per one of my earlier comments, but unlike before I think it necessary
to make this naming change here, in order to properly support future support for
Clause 19 modes. This because we will need different methods to create DSSS-OFDM
and ERP-OFDM WifiMode instances, and the "GetClause19RateYY" naming does not
make this distinction.

[Nicola Baldo, 2010-05-26 15:52:41]

On 2010/05/22 18:23:23, Mathieu Lacage wrote:

> I am fine with the current patch. It was just really big and I know
> that (some) users will scream about the mode name change because they
> can't even catch it at compile-time. Maybe it would be worth adding
> some 'alias' modes with the old names for 1 or 2 releases to avoid too
> many screams. Some users will always complain about changes anyway so,
> feel free to ignore this comment if nicola is fine with it.
> 

I agree with Mathieu that we should do something to warn users of the changing
in the naming of modes, maybe still making old names work but printing a
"deprecated" warning, or failing but displaying a meaningful message. 

What I don't know is how it could be done... I was thinking of modifying the
attribute checker of WifiMode, but it is implemented with a macro which I find
really obscure :-/

Nicola

[Mathieu Lacage, 2010-05-26 18:52:15]

On Wed, May 26, 2010 at 5:52 PM,  <nicobaldo@gmail.com> wrote:

> What I don't know is how it could be done... I was thinking of modifying
> the attribute checker of WifiMode, but it is implemented with a macro
> which I find really obscure :-/

You could define new modes with the old names which happen to have the
same definitions as the matching new modes with the new names.

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

[Dean, 2010-06-03 15:48:19]

Nicola,

Thanks for your review.

On 2010/05/26 15:38:53, Nicola Baldo wrote:
> http://codereview.appspot.com/986041/diff/1/18#newcode181
> src/devices/wifi/wifi-mode.cc:181: item->modulation = WifiMode::DQPSK;
[snip]
> Maybe it is just simpler to remove the modulation type completely, since we
> don't use it in the end... what do you think?

I agree. I have done this in the next draft patch.

> http://codereview.appspot.com/986041/diff/1/19#newcode57
> src/devices/wifi/wifi-mode.h:57: * This enumeration defines the various coding
> rates used for the OFDM
> On 2010/05/22 09:00:11, Dean wrote:
[snip]
> > > but you don't use it only for OFDM, right?
> > 
> > Yes, I do use it only for OFDM rates
[snip]
> Otherwise, you could trigger an assertion, saying that this functionality is
> only supported for OFDM, and return WIFI_CODE_RATE_UNDEFINED (rather than
> UNCODED) so we handle correctly the case of optimized builds.

I've added a test in WifiModeFactory::CreateWifiMode() to ensure that
WIFI_CODE_RATE_UNDEFINED is used if and only if the modulation class is DSSS. If
someone writes code that attempts to do otherwise then a fatal error will be
generated (with a helpful message).

> http://codereview.appspot.com/986041/diff/1/21#newcode565
> src/devices/wifi/wifi-phy.cc:565: ns3::WifiPhy::GetClause17Rate9MbpsBW10MHz
();
[snip]
> To summarize, my comment boils down to the naming of the WifiMode getter
> functions, which should become something like
> 
> WifiPhy::GetXXXXRateYYMbps
> 
> where XXXX is the name of the modulation class (e.g., "Dsss" rather than
> "Clause15") and YY is the rate in Mbps as usual. This is similar to the
naming...

Good suggestion. This is done in the forthcoming second draft of the patch.

Thanks,
Dean.

[Dean, 2010-06-03 15:49:39]

Nicola, Mathieu, All,

I've uploaded a new draft patch as Issue 986041 on Code Review. The new patch is
based on changeset 6330:af9c574246d5 of ns-3-dev.

I believe I've addressed all review comments on the previous proposed patch in
line with the discussions in ns-3-reviews.

There are two aspects which weren't specifically discussed that I would like to
draw attention to here.

1) I've added code to produce an error with a helpful message when a WifiMode
that does not exist is nominated in WifiModeFactory::Search(), but the message
isn't specific to the old-style "wifix-nmbs" rate names - instead it will be
printed if ever a rate cannot be found. This generalisation of what was being
suggested in the ns-3-reviews discussion seems to meet the requirement of
helping users get past the API change this patch brings about, but also improves
the error reporting for users who make (e.g.) typographical errors.

The stderr output looks something like this (hoping that the Code Review system
doesn't mess too much with my line endings):

Could not find match for WifiMode named "wifib-1mbs". Valid options are:
  DsssRate1Mbps
  DsssRate2Mbps
  DsssRate5_5Mbps
  DsssRate11Mbps
  OfdmRate6Mbps
  OfdmRate9Mbps
  OfdmRate12Mbps
  OfdmRate18Mbps
  OfdmRate24Mbps
  OfdmRate36Mbps
  OfdmRate48Mbps
  OfdmRate54Mbps
  OfdmRate3MbpsBW10MHz
  OfdmRate4_5MbpsBW10MHz
  OfdmRate6MbpsBW10MHz
  OfdmRate9MbpsBW10MHz
  OfdmRate12MbpsBW10MHz
  OfdmRate18MbpsBW10MHz
  OfdmRate24MbpsBW10MHz
  OfdmRate27MbpsBW10MHz
  OfdmRate1_5MbpsBW5MHz
  OfdmRate2_25MbpsBW5MHz
  OfdmRate3MbpsBW5MHz
  OfdmRate4_5MbpsBW5MHz
  OfdmRate6MbpsBW5MHz
  OfdmRate9MbpsBW5MHz
  OfdmRate12MbpsBW5MHz
  OfdmRate13_5MbpsBW5MHz


2) I've done away with the WifiMode named "Invalid-WifiMode", which was created
on first call to WifiModeFactory::GetFactory(). I could not see the point of
this mode and it was polluting my dump (shown above) of the list of available
WifiModes, so I took it out. Was there a good reason for it being there?

Regards,
Dean.

[Mathieu Lacage, 2010-06-04 08:58:46]

On Thu, Jun 3, 2010 at 5:49 PM,  <deanarm@gmail.com> wrote:
> 2) I've done away with the WifiMode named "Invalid-WifiMode", which was
> created on first call to WifiModeFactory::GetFactory(). I could not see
> the point of this mode and it was polluting my dump (shown above) of the
> list of available WifiModes, so I took it out. Was there a good reason
> for it being there?

I don't remember the details but I believe that it was introduced to
handle the case where you need to initialize a WifiMode variable
instance which needs to be overriden later and you want to make sure
that this WifiMode instance is invalid until it is overriden to catch
the case where you use it before initializing it.

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

[Dean, 2010-06-04 10:09:43]

On 2010/06/04 08:58:46, Mathieu Lacage wrote:
> On Thu, Jun 3, 2010 at 5:49 PM,  <mailto:deanarm@gmail.com> wrote:
> > 2) I've done away with the WifiMode named "Invalid-WifiMode", ...
[snip]
> > ...Was there a good reason for it being there?
> 
> I don't remember the details but I believe that it was introduced to
> handle the case where you need to initialize a WifiMode variable
> instance which needs to be overriden later and you want to make sure
> that this WifiMode instance is invalid until it is overriden to catch
> the case where you use it before initializing it.

Okay. Thanks for explaining this.

I've reinstated it in the third draft patch on Code Review.

Regards,
Dean.