Code review - Issue 862041: ns-3: Initial support for the 802.11g PHYhttps://codereview.appspot.com/2010-08-26T11:02:36+00:00rietveld
Message from unknown
2010-03-31T15:33:13+00:00Deanurn:md5:b617cb811e73420a55bce5f30b1433c1
Message from nicobaldo@gmail.com
2010-04-12T17:43:08+00:00Nicola Baldourn:md5:15efaa0be0b5a60cc59a9927bd8d2819
Most of the patch is ok, the main thing that I would like you to clarify a little bit is the issue of DSSS-OFDM vs ERP-OFDM.
http://codereview.appspot.com/862041/diff/8001/9003
File src/devices/wifi/interference-helper.cc (right):
http://codereview.appspot.com/862041/diff/8001/9003#newcode276
src/devices/wifi/interference-helper.cc:276: case WIFI_PHY_STANDARD_80211g:
This is correct for pure 802.11g (ERP-OFDM), but for 802.11b/g (DSSS-OFDM) the preamble is that of 802.11b.
http://codereview.appspot.com/862041/diff/8001/9003#newcode358
src/devices/wifi/interference-helper.cc:358: case WIFI_PHY_STANDARD_80211g:
The calculation that you are using here is for OFDM modes only. If I understood correctly, your idea is to model 802.11bg (i.e., DSSS-OFDM), so this means that even when payloadMode.GetStandard () == WIFI_PHY_STANDARD_80211g, the mode could refer to one of the DSSS modulations, and so the calculation would be wrong.
http://codereview.appspot.com/862041/diff/8001/9009
File src/devices/wifi/supported-rates.cc (right):
http://codereview.appspot.com/862041/diff/8001/9009#newcode38
src/devices/wifi/supported-rates.cc:38: NS_ASSERT (m_nRates < MAX_SUPPORTED_RATES);
This is not correct, see section 7.3.2.2 in the standard:
"If the number of rates in the Operational Rate
Set exceeds eight, then an Extended Supported Rate element shall be generated to specify the remaining
supported rates"
That's why a number of 8 was hard-coded.
http://codereview.appspot.com/862041/diff/8001/9013
File src/devices/wifi/wifi-phy-standard.h (right):
http://codereview.appspot.com/862041/diff/8001/9013#newcode28
src/devices/wifi/wifi-phy-standard.h:28: WIFI_PHY_STANDARD_80211g,
I get the impression that you refer to the DSSS-OFDM PHY specification in the IEEE Std. 802.11-2007. If so, maybe you should call it "80211bg"
http://codereview.appspot.com/862041/diff/8001/9017
File src/devices/wifi/yans-wifi-phy.cc (right):
http://codereview.appspot.com/862041/diff/8001/9017#newcode568
src/devices/wifi/yans-wifi-phy.cc:568: m_modes.push_back (WifiPhy::Get2mbb ());
mixing b modes with g modes? What will WifiMode::GetStandard() return for those b modes?
If you mix b and g modes, then it means that you refer to the DSSS-OFDM specification, right?
Message from deanarm@gmail.com
2010-04-22T21:54:42+00:00Deanurn:md5:1d48fa929d5752edcc21b47fa83dbec3
Nicola,
Thank you for your comments. Some responses below...
> ...the main thing I would like you to clarify a little bit
> is the issue of DSSS-OFDM vs ERP-OFDM.
Sure. I now realise that I didn't make this clear at all.
My intention was (and still is) to firstly provide support for
modeling consumer "802.11g" devices as are readily available
worldwide. Specifically, such devices implement support for the
physical layer described in Clause 19 of IEEE 802.11-2007 at least as
far as is necessary to gain Wi-Fi Certification.
To be compliant with Clause 19, these devices must support not only
the 6, 12, and 24 Mbps ERP-OFDM rates (Modulation Class 6, per Table
9-2 in IEEE 802.11-2007), but also the 1, 2, 5.5, and 11 Mbps
(HR/)DSSS rates defined in Clauses 15 and 18 (Modulation Class 3).
All other rates are optional, though most real-world devices support
the remaining ERP-OFDM rates (9, 18, 36, 48, and 54 Mbps).
To my knowledge, few (if any) real-world devices support the optional
ERP-PBCC and DSSS-OFDM rates (of Modulation Classes 4 and 5; much akin
to the general lack of support for other optional features like the IR
and FHSS PHYs, and PCF...). For this reason my personal focus is not
on providing for these modes of operation, though I'd like to see
things designed such that support for these (and other extensions I do
care about, like "802.11n") is not difficult to add.
So whenever I have said 802.11b/g in posted code or correspondence, I
have been referring to a device as described above - with support for
(at least some) rates of Modulation Classes 3 and 6. In the future I
will use less ambiguous terminology.
In light of this clarification I've not explicitly responded to your
comments that assumed I meant to support DSSS-OFDM, rather than
ERP-OFDM.
> http://codereview.appspot.com/862041/diff/8001/9009#newcode38
> src/devices/wifi/supported-rates.cc:38: NS_ASSERT (m_nRates <
> MAX_SUPPORTED_RATES);
> This is not correct, see section 7.3.2.2 in the standard:
> "If the number of rates in the Operational Rate
> Set exceeds eight, then an Extended Supported Rate element shall be
> generated to specify the remaining supported rates" That's why a
> number of 8 was hard-coded.
Okay - I missed this; thanks.
I'm not sure that the current handling of supported rates lends itself
to implementation of the Extended Supported Rates element. I've raised
Bug 881 (and proposed some code there) in attempt to provide
groundwork that might help make this (and other extensions) easier in
future. I suspect that (as long as the general approach to fixing Bug
881 is not too contentious) Bug 881 should probably block Bug 852 (for
which the code under review here is proposed).
> http://codereview.appspot.com/862041/diff/8001/9017#newcode568
> src/devices/wifi/yans-wifi-phy.cc:568: m_modes.push_back (WifiPhy::Get2mbb ());
> mixing b modes with g modes? What will WifiMode::GetStandard()
> return for those b modes?
You made a comment in the bug corresponding to this code (Bug 852)
that Bug 871 was relevant. I think you're quite right and I think that
it would be tidiest for that work to be completed first (I'm going to
propose some code shortly under that bug).
I've explained above that my intent here was to allow modeling of the
predominantly available Wi-Fi 2.4 GHz devices available today, which
support rates of Modulation Classes 3 and 6. This is why the code you
were looking at added rates from Clauses 15, 18, and 19, to the
supported rate set of the PHY.
I'll defer to my pending comments on Bug 871 for an answer to your
"what will WifiMode::GetStandard() return" question. The gist of that
will be that there is no reason for a WifiMode to know it's "Standard"
and no sensible value that could be returned by a method which claims
to return this, and thus this method (and the guff behind it) should
disappear.
To summarise, I think that Bugs 871 and 881 should block Bug 852 in
order that work under those bugs provide the framework to tidily
address your valid comments.
I'll thus focus my attention on those for a while before posting an updated patch here.
Kind regards,
Dean.
Message from unknown
2010-08-25T09:23:25+00:00Deanurn:md5:c1b5c9acefa58257f112dd0e87c01a54
Message from deanarm@gmail.com
2010-08-25T09:27:51+00:00Deanurn:md5:a80bb4637ee1332d6f18497dc2f8ee93
I've updated the code proposed to address Bug 852 ("Add support for 802.11g devices").
The draft patch builds on the fixes for Bugs 871 and 881. This patch is also available (split into it's two constituent changesets) at the head of http://code.nsnam.org/deanarm/ns-3-dev-80211g
Regards,
Dean.
Message from deanarm@gmail.com
2010-08-25T16:04:36+00:00Deanurn:md5:af3c37a01931d2049b3f508bb1ed36a9
I've updated the code proposed to address Bug 852 ("Add support for 802.11g devices").
The draft patch builds on the fixes for Bugs 871 and 881. This patch is also available (split into it's two constituent changesets) at the head of http://code.nsnam.org/deanarm/ns-3-dev-80211g
Regards,
Dean.
Message from deanarm@gmail.com
2010-08-26T11:02:36+00:00Deanurn:md5:b7b0e468a2196066753963a9fcde2162
I've updated the code proposed to address Bug 852 ("Add support for 802.11g devices").
The draft patch builds on the fixes for Bugs 871 and 881. This patch is also available (split into it's two constituent changesets) at the head of http://code.nsnam.org/deanarm/ns-3-dev-80211g
(Nicola; I'm sorry you're getting this multiple times on cc - there is something funny going on between Rietveld and the ns-3-reviews group so the original one didn't get through. I'm retrying to test a change that Mathieu has made.)
Regards,
Dean.