Wifi helpers and api refactoring

It has been decided to refactor Wi-Fi helpers () as well as aggregation and block ack APIs.
See bugs 2116 and 2213.

The wifi mac helpers have been reduced to a single helper (non QoS).
When the user configure 802.11n or 802.11ac standard, it enables QoS, HT and VHT (if 802.11ac is selected).
If the user wants to work with QoS with legacy versions (a/b/g), he needs to manually enableQoS using attributes.

Aggregation and block ack APIs have been changed to make them simpler. The result is that aggregation and block ack parameters can now be configured similarly as other MAC layer parameters. Besides, the selection of 802.11n/ac automatically enables MPDU aggregation for AC_BE and AC_VI as mostly observed in the field. Note that 802.11ac default values are identical to 802.11n default values since we lack information about 802.11ac values that are commonly set as default in the field.

[Tom Henderson, 2016-01-07 20:33:24]

I think this is an improvement but would like to ask if more could be done.  I
left some comments in the code but have a couple of additional high-level
comments and questions:

1) what do you think about leaving the existing helpers in place for a release
cycle or two, with NS_DEPRECATED tag?  Will it cause any harm?  Will they still
work as before?

2) I think we need an example that more clearly shows how to enable or disable
aggregation.  I still find the simple-mpdu-aggregation example too complicated
for this.

How about an example that shows a few independent Wi-Fi networks with different
configurations?  The first configuration is a default 802.11n, where the comment
is made that only SetStandard() is needed to enable MPDU by default for BE and
VI.  The second configuration is one in which 802.11n has SetStandard() but the
user wants to disable MPDU aggregation-- show how that is done.  Can it be done
simply by setting the default value of BE_AmpduSize to zero?  The third
configuration is one in which MPDU aggregation is added for another class (BK)--
this shows the necessary block ack configuration and BK_AmpduSize configuration.
 Can it be done by setting BK_AmpduSize and other BK attributes to non-zero
values, via Config::SetDefault()?

If so, the msdu and two-level examples could follow this approach also.

https://codereview.appspot.com/281230043/diff/20001/examples/energy/energy-mo...
File examples/energy/energy-model-example.cc (right):

https://codereview.appspot.com/281230043/diff/20001/examples/energy/energy-mo...
examples/energy/energy-model-example.cc:192: // Add an upper MAC and disable
rate control
add a MAC, not just an upper MAC

https://codereview.appspot.com/281230043/diff/20001/src/wifi/helper/default-m...
File src/wifi/helper/default-mac-helper.h (right):

https://codereview.appspot.com/281230043/diff/20001/src/wifi/helper/default-m...
src/wifi/helper/default-mac-helper.h:18: * Author: Mirko Banchi
<mk.banchi@gmail.com>

to document the history of this file, since it is not one that Mirko actually
created, I suggest to add your name and your copyright, and to add a comment
below that this file represents the merge of two previous helpers (nqos and qos)
that Mirko originally contributed.

https://codereview.appspot.com/281230043/diff/20001/src/wifi/helper/default-m...
src/wifi/helper/default-mac-helper.h:31: * This class can create MACs of type
ns3::ApWifiMac, ns3::StaWifiMac and ns3::AdhocWifiMac.
state more clearly what the default configuraiton is (i.e. Adhoc, no QoS).

state more clearly the typical usage which is to set type and attribute values,
and then hand this object over to the WifiHelper that finishes the job of
installing.

state that the main point of this helper is to allow a WifiHelper to configure
and install WifiMac objects on a collection of nodes, and that the WifiMac
objects themselves are composed of TxMiddle, RxMiddle, DcfManager, MacLow,
WifiRemoteStationManager, MpduAggregator, etc. objects, so this helper offers
the opportunity to configure attribute values away from their default values, on
a per-NodeContainer basis.

state that valid values of the type field must be a type subclassed from
WifiMac.

state that this class may be further subclassed, and that the WaveMacHelper is
an example of this (in the src/wave module).

https://codereview.appspot.com/281230043/diff/20001/src/wifi/helper/default-m...
src/wifi/helper/default-mac-helper.h:33: class DefaultWifiMacHelper : public
WifiMacHelper
Why not move class WifiMacHelper out of wifi-helper.{cc,h} into a new file
wifi-mac-helper.{cc,h} and move this code into that file?

It seems to me that we are moving away from a virtual WifiMacHelper that is to
be subclassed, to a single (yet still subclassable, see Wave module)
WifiMacHelper that is configured by type/attribute.  So, I'd suggest that we not
call it a "Default" helper since its usage typically is to actually configure a
non-default.  I realize that other wifi helpers use the Default() pattern but we
can work on that API over time.

In user code, what about this:

  WifiMacHelper wifiMac;
  wifiMac.SetType ("ns3::ApWifiMac",
                   "Ssid", SsidValue (ssid));

instead of this:

  DefaultWifiMacHelper wifiMac = DefaultWifiMacHelper::Default ();
  wifiMac.SetType ("ns3::ApWifiMac",
                   "Ssid", SsidValue (ssid));

and place the default configuration in the constructor of WifiMacHelper, and
remove Default() method?

https://codereview.appspot.com/281230043/diff/20001/src/wifi/model/regular-wi...
File src/wifi/model/regular-wifi-mac.cc (right):

https://codereview.appspot.com/281230043/diff/20001/src/wifi/model/regular-wi...
src/wifi/model/regular-wifi-mac.cc:977: SetQosSupported (true);
This SetStandard() approach still bothers me (overriding attribute defaults). 
It is not intuitive (and not documented, although you said that documentation
would follow) that setting the standard will overwrite the BE and VI Ampdu
attribute defaults (or any default value change that the user had configured).

Why not make these values the defaults, and only SetHtSupported (true) and
SetQosSupported (true)?  I think it may be because other parts of the code only
check these AMPDU values are nonzero and not whether AMPDU should be used.

What about a design where only SetHtSupported(true) or SetVhtSupported(true)
causes an MpduAggregator and MsduAggregator to be added, and only then does
aggregation happen?  The EDCA code should then check for the presence of an
aggregator before using it.

I then would like to see some documentation around WifiHelper::SetStandard()
that setting ac or n will cause the following attributes to be overwritten:
  HtSupported
  VhtSupported (if applicable)
  QosSupported

(among other side effects).

[S. Deronne, 2016-01-17 20:14:59]

On 2016/01/07 20:33:24, Tom Henderson wrote:
> I think this is an improvement but would like to ask if more could be done.  I
> left some comments in the code but have a couple of additional high-level
> comments and questions:
> 
> 1) what do you think about leaving the existing helpers in place for a release
> cycle or two, with NS_DEPRECATED tag?  Will it cause any harm?  Will they
still
> work as before?

I will change the code in this way, it should still work (with some changes);
and we'll get rid of those deprecated classes/functions in future releases.
 
> 2) I think we need an example that more clearly shows how to enable or disable
> aggregation.  I still find the simple-mpdu-aggregation example too complicated
> for this.

I find it much easier than it used to be, but ok I am maybe too much familiar
with it to realize its complexity :-)
 
> How about an example that shows a few independent Wi-Fi networks with
different
> configurations?  The first configuration is a default 802.11n, where the
comment
> is made that only SetStandard() is needed to enable MPDU by default for BE and
> VI.  The second configuration is one in which 802.11n has SetStandard() but
the
> user wants to disable MPDU aggregation-- show how that is done.  Can it be
done
> simply by setting the default value of BE_AmpduSize to zero?  The third
> configuration is one in which MPDU aggregation is added for another class
(BK)--
> this shows the necessary block ack configuration and BK_AmpduSize
configuration.
>  Can it be done by setting BK_AmpduSize and other BK attributes to non-zero
> values, via Config::SetDefault()?

Do you mean different networks running at the same time, or 3 cases (using a
"for" loop) showing the throughput for each case?
It would make a slight difference in the future, once we would support
inter-channel interference; so I think it is best to show the throughput for
different cases (a bit like ht-wifi-network for the different bitrate).

> If so, the msdu and two-level examples could follow this approach also.

[S. Deronne, 2016-01-17 20:15:06]

https://codereview.appspot.com/281230043/diff/20001/examples/energy/energy-mo...
File examples/energy/energy-model-example.cc (right):

https://codereview.appspot.com/281230043/diff/20001/examples/energy/energy-mo...
examples/energy/energy-model-example.cc:192: // Add an upper MAC and disable
rate control

Done (+ idem errors at various places)

https://codereview.appspot.com/281230043/diff/20001/src/wifi/helper/default-m...
File src/wifi/helper/default-mac-helper.h (right):

https://codereview.appspot.com/281230043/diff/20001/src/wifi/helper/default-m...
src/wifi/helper/default-mac-helper.h:18: * Author: Mirko Banchi
<mk.banchi@gmail.com>
On 2016/01/07 20:33:24, Tom Henderson wrote:
> 
> to document the history of this file, since it is not one that Mirko actually
> created, I suggest to add your name and your copyright, and to add a comment
> below that this file represents the merge of two previous helpers (nqos and
qos)
> that Mirko originally contributed.

Done.

https://codereview.appspot.com/281230043/diff/20001/src/wifi/helper/default-m...
src/wifi/helper/default-mac-helper.h:31: * This class can create MACs of type
ns3::ApWifiMac, ns3::StaWifiMac and ns3::AdhocWifiMac.
On 2016/01/07 20:33:24, Tom Henderson wrote:
> state more clearly what the default configuraiton is (i.e. Adhoc, no QoS).
> 
> state more clearly the typical usage which is to set type and attribute
values,
> and then hand this object over to the WifiHelper that finishes the job of
> installing.
> 
> state that the main point of this helper is to allow a WifiHelper to configure
> and install WifiMac objects on a collection of nodes, and that the WifiMac
> objects themselves are composed of TxMiddle, RxMiddle, DcfManager, MacLow,
> WifiRemoteStationManager, MpduAggregator, etc. objects, so this helper offers
> the opportunity to configure attribute values away from their default values,
on
> a per-NodeContainer basis.
> 
> state that valid values of the type field must be a type subclassed from
> WifiMac.
> 
> state that this class may be further subclassed, and that the WaveMacHelper is
> an example of this (in the src/wave module).

Done.

https://codereview.appspot.com/281230043/diff/20001/src/wifi/helper/default-m...
src/wifi/helper/default-mac-helper.h:33: class DefaultWifiMacHelper : public
WifiMacHelper
I agree.

https://codereview.appspot.com/281230043/diff/20001/src/wifi/model/regular-wi...
File src/wifi/model/regular-wifi-mac.cc (right):

https://codereview.appspot.com/281230043/diff/20001/src/wifi/model/regular-wi...
src/wifi/model/regular-wifi-mac.cc:977: SetQosSupported (true);
I like the idea, I will go for it.

[Tom Henderson, 2016-01-20 18:07:17]

> 
> Do you mean different networks running at the same time, or 3 cases (using a
> "for" loop) showing the throughput for each case?
> It would make a slight difference in the future, once we would support
> inter-channel interference; so I think it is best to show the throughput for
> different cases (a bit like ht-wifi-network for the different bitrate).

I meant different networks running at the same time, on isolated channels.

I think that either a for loop or separate isolated networks can be used to do
this.  I don't have a strong preference.  I had suggested the isolated channels
approach with the thinking that the block of code could be directly copy-pasted
from that example into someone else's program in the future (i.e., a user who is
not interested in the code used to handle different configuration cases) but I
don't think it really matters much.

[S. Deronne, 2016-01-20 20:12:27]

On 2016/01/20 18:07:17, Tom Henderson wrote:
> > 
> > Do you mean different networks running at the same time, or 3 cases (using a
> > "for" loop) showing the throughput for each case?
> > It would make a slight difference in the future, once we would support
> > inter-channel interference; so I think it is best to show the throughput for
> > different cases (a bit like ht-wifi-network for the different bitrate).
> 
> I meant different networks running at the same time, on isolated channels.
> 
> I think that either a for loop or separate isolated networks can be used to do
> this.  I don't have a strong preference.  I had suggested the isolated
channels
> approach with the thinking that the block of code could be directly
copy-pasted
> from that example into someone else's program in the future (i.e., a user who
is
> not interested in the code used to handle different configuration cases) but I
> don't think it really matters much.

Ok, I will go for different networks.
Why not merging the 3 aggregation examples, and go for several different
networks in order to illustrate how enable/disable/configure MPDU, MSDU and
two-level aggregation, compared to results obtained with the default parameters?
Or are you still in favor to keep in 3 different examples, so that they are not
too loaded?

[Tom Henderson, 2016-01-24 20:13:20]

I just have some comments on the new example; otherwise, I think it is ready.

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
File examples/wireless/wifi-aggregation.cc (right):

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:31: // Each network countains one access
point and one station. Each station
contains

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:34: // Network topology:
s/Network topology/Network topology (numbers in parentheses are channel numbers)

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:43: // - station B doesn't use aggregation
(both A-MDU and A-MSDU are disabled);
A-MPDU

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:51: // Example: ./waf --run
"wifi-aggregation --distance=10 --enableRts=0 --simulationTime=20"
add also a comment about what the user should expect to see and briefly explain
why the numbers differ.  Include also what people should expect to see if they
increase the distance parameter, and if they enable RTS.

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:68: cmd.AddValue ("distance", "Distance in
meters between the station and the access point", distance);
please add also a pcap option (default false)

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:158: //Disable A-MPDU on STA B
add a comment that we must perform these Config::Set() operations _after_
Install() for values to take effect.

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:159: Config::Set
("/NodeList/5/DeviceList/*/$ns3::WifiNetDevice/Mac/BE_MaxAmpduSize",
UintegerValue (0));
From a documentation perspective, it may be better to add the
"$ns3::RegularWifiMac" to the path as a hint about which class this attribute
appears in.

/NodeList/[i]/DeviceList/[i]/$ns3::WifiNetDevice/Mac/$ns3::RegularWifiMac/BE_MaxAmpduSize

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:163: Config::Set
("/NodeList/6/DeviceList/*/$ns3::WifiNetDevice/Mac/BE_MaxAmsduSize",
UintegerValue (7935));
explain where value of 7935 comes from

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:165: //Enable A-MSDU and change A-MPDU
configuration on STA D (A-MDU + A-MSDU= two-level aggregation)
explain why these two constant values 32768 and 3839

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:168: 
For one of these networks, what about trying to apply the attribute through the
helper?  does the following work?

+  mac.SetType ("ns3::StaWifiMac",
+               "Ssid", SsidValue (ssid),
+               "ActiveProbing", BooleanValue (false),
+               "BE_MaxAmpduSize, UintegerValue (32768));

I wonder in this example program if it would be useful to show people that we
don't have to do everything through Config::Set-- the helpers can also be used.

https://codereview.appspot.com/281230043/diff/40001/examples/wireless/wifi-ag...
examples/wireless/wifi-aggregation.cc:174: //Set position for APs
add comment to reinforce that while we set these positions 10m apart, the
networks do not interact and the only variable that affects performance is STA
distance.