ns-3: Correct selection of Wi-Fi control response rates

This patch aims to address Bug 853 in the ns-3 Bugzilla module.

The patch modifies the GetControlAnswerMode() method of class WifiRemoteStationManager to correctly identify rates within the same modulation class as the received frame, and then adds a search for a suitable mandatory rate in the case where no suitable (per IEEE 802.11-2007, Section 9.6) basic rate has been found.

Note that this change exposes issues in some of the test and example code that uses the Wi-Fi models. These issues are addressed in a separate changelist (which is also available for review here as Issue 861041)

[Nicola Baldo, 2010-06-22 13:47:39]

Hi Dean,

I am not sure whether I understand exactly your point, so I'll try to summarize
what I understood so far. Please correct me if I am wrong.

The fact is that in real life we have several sets of rates that we should
consider:

1) the set of all rates that the PHY supports (i.e., is able to TX/RX). This has
no name in the standard. In the simulator, this is YansWifiPhy::m_modes. Let's
call it DeviceRateSet

2) BasicRateSet, defined as
"The set of data rates that must be supported
by all STAs to join this BSS. The STA that is
creating the BSS must be able to receive and
transmit at each of the data rates listed in the set."

3) OperationalRateSet, defined as:
"The set of data rates that the STA desires to
use for communication within the BSS. The
STA must be able to receive at each of the
data rates listed in the set. This set is a
superset of the rates contained in the
BSSBasicRateSet parameter."

4) MandatoryRateSet
this is defined by the standard. I think we can hardcode this somewhere.


I agree with Mathieu that in order to get the MandatoryRateSet it is not
necessary to get it from the Phy instance. But, in fact, now I understand that
talking with the Phy instance is necessary to get the DeviceRateSet. So I am
fine with the proposed approach.

So the only thing that I would ask Dean to modify at this point is the
documentation of WifiPhy::GetNModes and GetMode, so that it is clear to the user
to which set of rates they refer to, and the relationship/differences with the
MandatoryRateSet and the OperationalRateSet.

http://codereview.appspot.com/855042/diff/1/2
File src/devices/wifi/wifi-remote-station-manager.cc (right):

http://codereview.appspot.com/855042/diff/1/2#newcode598
src/devices/wifi/wifi-remote-station-manager.cc:598: if (thismode.IsMandatory ()
this code effectively calculates the intersection between DeviceRateSet and
MandatoryRateSet, am I right?

http://codereview.appspot.com/855042/diff/1/2#newcode601
src/devices/wifi/wifi-remote-station-manager.cc:601: && thismode.GetStandard ()
== reqMode.GetStandard ())
this is now GetModulationClass, isn't it?

[Dean, 2010-06-22 14:15:20]

Nicola,

Thanks for looking at this.

> I am not sure whether I understand exactly your point, so I'll try to
summarize
> what I understood so far. Please correct me if I am wrong.
>
> The fact is that in real life we have several sets of rates that we should
> consider...

I think your summary of the different "sets of rates" is accurate and complete.
You've done a much better job of making this clear than I.

> 4) MandatoryRateSet
> this is defined by the standard. I think we can hardcode this somewhere.

Indeed, this is logically already the case - the construction of WifiModes by
the WifiPhy object marks those that are mandatory rates as such. As I think you
can see (based on your comments below), my approach was to simply look for this
marker when iterating through (what you have sensibly named) the DeviceRateSet.

> I agree with Mathieu that in order to get the MandatoryRateSet it is not
> necessary to get it from the Phy instance. But, in fact, now I understand that
> talking with the Phy instance is necessary to get the DeviceRateSet. So I am
> fine with the proposed approach.

Okay. So for avoidance of doubt, you are saying that it is fine for me to add
the GetPhy() method to WifiRemoteStationManager (I can make it private if that
helps) and use the GetNModes() and GetMode() methods to find the mandatory
subset of DeviceRateSet (i.e., m_modes in a YansWifiPhy)?

> So the only thing that I would ask Dean to modify at this point is the
> documentation of WifiPhy::GetNModes and GetMode, so that it is clear to the
user
> to which set of rates they refer to, and the relationship/differences with the
> MandatoryRateSet and the OperationalRateSet.

Understood. Will do.

Regards,
Dean.

http://codereview.appspot.com/855042/diff/1/2
File src/devices/wifi/wifi-remote-station-manager.cc (right):

http://codereview.appspot.com/855042/diff/1/2#newcode598
src/devices/wifi/wifi-remote-station-manager.cc:598: if (thismode.IsMandatory ()
On 2010/06/22 13:47:40, Nicola Baldo wrote:
> this code effectively calculates the intersection between DeviceRateSet and
> MandatoryRateSet, am I right?

Absolutely.

http://codereview.appspot.com/855042/diff/1/2#newcode601
src/devices/wifi/wifi-remote-station-manager.cc:601: && thismode.GetStandard ()
== reqMode.GetStandard ())
On 2010/06/22 13:47:40, Nicola Baldo wrote:
> this is now GetModulationClass, isn't it?

Yes, it is.

[Dean, 2010-06-22 14:33:45]

On 2010/06/22 14:15:20, Dean wrote:
> Okay. So for avoidance of doubt, you are saying that it is fine for me to add
> the GetPhy() method to WifiRemoteStationManager (I can make it private if that
> helps) and use the GetNModes() and GetMode() methods to find the mandatory
> subset of DeviceRateSet (i.e., m_modes in a YansWifiPhy)?

I guess if I'm going to make GetPhy() private then I might as well do away with
it entirely and just use the m_wifiPhy member I've added directly from
WifiRemoteStationManager::GetControlAnswerMode(). Thoughts?

Thanks,
Dean.

[Dean, 2010-06-22 16:27:28]

I've extrapolated a bit from the comments received on this so far in the updated
patch which I've just uploaded.

It is a slightly more extensive changeset that aims to address the outstanding
comments, and also undertakes some reorganisation of code (reduces to renaming
in some cases) in attempt to "improve" things.

I've moved YansWifiPhy::m_modes to the parent class because I believe that
logically it is not YansWifiPhy-specific. I've also renamed per Nicola's
suggested nomenclature so we now have the equivalent information stored in
WifiPhy::m_deviceRateSet.

Similarly, the YansWifiPhy::GetNModes() and YansWifiPhy::GetMode() accessors are
moved up to WifiPhy, and I've written some comment in attempt to make their
purpose clearer.

I've ditched the WifiRemoteStationManager::GetPhy() that was introduced in my
previously-proposed code, and instead make direct use of the
WifiRemoteStationManager::m_wifiPhy in that class.

I've also ditched WifiRemoteStationManager::BeginBasicModes() and
WifiRemoteStationManager::EndBasicModes(), because it is not clear to me that
they add anything above WifiRemoteStationManager::GetNBasicModes() and
WifiRemoteStationManager::GetBasicMode().

I've created WifiModeList (std::vector<WifiMode>) and WifiModeListIterator
(WifiModeList::const_iterator) to provide for the repeated need of such types. 
I also renamed WifiRemoteStationManager::m_basicModes to m_bssBasicRateSet.

I'm happy to be told to go away on all or any of these aspects if there is
controversy. My hope was that they would be deemed to be a useful tidy-up of the
code that reduced potential for future confusion.

Regards,
Dean.

[nbaldo_cttc.es, 2010-06-22 20:14:27]

Hi Dean,

thank you very much for your effort in addressing this issue. Please
find some generic comments below. I'll come up with a few more detailed 
comments on the code later.

On 06/22/2010 06:27 PM, deanarm@gmail.com wrote:
> I've extrapolated a bit from the comments received on this so far in
> the updated patch which I've just uploaded.
>
> It is a slightly more extensive changeset that aims to address the
> outstanding comments, and also undertakes some reorganisation of
> code (reduces to renaming in some cases) in attempt to "improve"
> things.
>
> I've moved YansWifiPhy::m_modes to the parent class because I
> believe that logically it is not YansWifiPhy-specific.
[...]
> Similarly, the YansWifiPhy::GetNModes() and YansWifiPhy::GetMode()
> accessors are moved up to WifiPhy, and I've written some comment in
> attempt to make their purpose clearer.

I agree that the management of DeviceRateSet logically is not 
YansWifiPhy-specific, but I think that for this purpose it is enough to 
have the definition of the accessor methods (GetNModes() and GetMode()) 
in WifiPhy as pure virtual methods, and their implementation in 
YansWifiPhy. This is in line with the rest of the current wifi code: all 
the interfaces are in WifiPhy, and all the implementations is in 
YansWifiPhy.

(actually this approach has been questioned recently, but I think it is 
a fairly complex issue to tackle. So I suggest to stick with the status 
quo for your patch)


> I've also renamed per Nicola's suggested nomenclature so we now have
> the equivalent information stored in WifiPhy::m_deviceRateSet.

Ok good. The name "DeviceRateSet" makes sense for me, but I do not claim 
that it is the best name possible... so if anybody has a better name to 
propose, please do so.


> I've ditched the WifiRemoteStationManager::GetPhy() that was
> introduced in my previously-proposed code, and instead make direct
> use of the WifiRemoteStationManager::m_wifiPhy in that class.

Ok

>
> I've also ditched WifiRemoteStationManager::BeginBasicModes() and
> WifiRemoteStationManager::EndBasicModes(), because it is not clear to
> me that they add anything above
> WifiRemoteStationManager::GetNBasicModes() and
> WifiRemoteStationManager::GetBasicMode().

I agree

>
> I've created WifiModeList (std::vector<WifiMode>) and
> WifiModeListIterator (WifiModeList::const_iterator) to provide for
> the repeated need of such types.

Ok


 > I also renamed
> WifiRemoteStationManager::m_basicModes to m_bssBasicRateSet.

Ok good point


>
> I'm happy to be told to go away on all or any of these aspects if
> there is controversy. My hope was that they would be deemed to be a
> useful tidy-up of the code that reduced potential for future
> confusion.

As you see, most of your proposals are ok for me. Thanks for the good work!


Nicola

[Dean, 2010-06-22 21:00:14]

Nicola,

Thanks for your notes.

On 2010/06/22 20:14:27, nbaldo_cttc.es wrote:
> > I've moved YansWifiPhy::m_modes to the parent class because I
> > believe that logically it is not YansWifiPhy-specific.
> [...]
> > Similarly, the YansWifiPhy::GetNModes() and YansWifiPhy::GetMode()
> > accessors are moved up to WifiPhy, and I've written some comment in
> > attempt to make their purpose clearer.
> 
> I agree that the management of DeviceRateSet logically is not 
> YansWifiPhy-specific, but I think that for this purpose it is enough to 
> have the definition of the accessor methods (GetNModes() and GetMode()) 
> in WifiPhy as pure virtual methods, and their implementation in 
> YansWifiPhy. This is in line with the rest of the current wifi code: all 
> the interfaces are in WifiPhy, and all the implementations is in 
> YansWifiPhy.
> 
> (actually this approach has been questioned recently, but I think it is 
> a fairly complex issue to tackle. So I suggest to stick with the status 
> quo for your patch)

Status quo stuck with in Patch Set 3 on The Issue.

I'll wade into the discussion on "approach" when it next arises.

Regards,
Dean.

[Nicola Baldo, 2010-06-23 01:01:00]

The patch is almost ok now, just some minor comments. You have my +1 to push to
ns-3-dev after these are fixed.

http://codereview.appspot.com/855042/diff/6001/7005
File src/devices/wifi/wifi-remote-station-manager.h (right):

http://codereview.appspot.com/855042/diff/6001/7005#newcode460
src/devices/wifi/wifi-remote-station-manager.h:460: WifiModeList m_modes;
this is the OperationalRateSet, am I right? If so, maybe it would be better to
change the name to reflect this.

http://codereview.appspot.com/855042/diff/9003/10003
File src/devices/wifi/wifi-remote-station-manager.cc (right):

http://codereview.appspot.com/855042/diff/9003/10003#newcode594
src/devices/wifi/wifi-remote-station-manager.cc:594: *
here you should briefly mention what you said in your email about the existence
of devices that do not support all mandatory rates.

[Dean, 2010-06-23 08:24:38]

Nicola,

Thanks for your comments. I've just pushed the updated code, but to provide
direct response to your comments...

> http://codereview.appspot.com/855042/diff/6001/7005#newcode460
> src/devices/wifi/wifi-remote-station-manager.h:460: WifiModeList m_modes;
> this is the OperationalRateSet, am I right? If so, maybe it would be better to
> change the name to reflect this.

Done.

> http://codereview.appspot.com/855042/diff/9003/10003
> File src/devices/wifi/wifi-remote-station-manager.cc (right):
> 
> http://codereview.appspot.com/855042/diff/9003/10003#newcode594
> src/devices/wifi/wifi-remote-station-manager.cc:594: *
> here you should briefly mention what you said in your email about the
existence
> of devices that do not support all mandatory rates.

I've added this to the comment for YansWifiPhy::m_deviceRateSet, as that seemed
a better place to have it - the point, after all, is that we need to remind
developers to avoid the assumption that this (and the derived
OperationalRateSet) will include all mandatory rates for a given
(standard-defined) PHY.

Regards,
Dean.