IEEE 802.11p MAC

This code is part of Junling Bu's GSoC midterm review.

http://www.nsnam.org/wiki/index.php/GSOC2013WAVE_MAC

This patch removes previous 802.11p-related code in src/wifi, which was based on an old and unsuitable 1609.4-2006 standard and only partially implemented.

This patch implements the MAC portion of the 802.11p-2010 standard, that allows stations to send data packets directly. The main features are 1) provide outside the context of a base station (OCB) mode of high MAC defined in the standard, 2) use specific EDCA parameters, 3) provide vendor specific action frames and allow users to send management information over VSAs.

Further study is needed to verify that this implementation of 802.11p is accurate and can undertake a large number of nodes.

[Tom Henderson, 2013-07-28 15:22:29]

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
File src/wave/examples/wave-simple-ocb.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
src/wave/examples/wave-simple-ocb.cc:46: *  WaveHelper.

In the documentation (.rst file) it should probably be clarified what is the
division of functionality between these helpers.

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
File src/wave/examples/wave-simple-vsa.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
src/wave/examples/wave-simple-vsa.cc:34: *  Normally the up layer can send
packets in different protocols
upper

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
src/wave/examples/wave-simple-vsa.cc:50: *  3. OcbWifiMac regists this
identifier and function
registers

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
src/wave/examples/wave-simple-vsa.cc:82: m_16093oi = oi_16093;
need to run check-style.py on this file, to fix indentation and spacing

https://codereview.appspot.com/11685043/diff/1/src/wave/helper/wifi-80211p-he...
File src/wave/helper/wifi-80211p-helper.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/helper/wifi-80211p-he...
src/wave/helper/wifi-80211p-helper.cc:97: /* Need REDO */
can you clarify what is the open issue here?

https://codereview.appspot.com/11685043/diff/1/src/wave/helper/wifi-80211p-he...
File src/wave/helper/wifi-80211p-helper.h (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/helper/wifi-80211p-he...
src/wave/helper/wifi-80211p-helper.h:43: * shall 802.11a
(WIFI_PHY_STANDARD_80211_10MHZ or WIFI_PHY_STANDARD_80211a).
can you clarify the last sentence further?

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.cc
File src/wave/model/ocb-wifi-mac.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.cc...
src/wave/model/ocb-wifi-mac.cc:263: NS_LOG_LOGIC ("packet is not for us");
change "packet is not for us" to "management packet is not for us"

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.cc...
src/wave/model/ocb-wifi-mac.cc:284: NS_LOG_DEBUG("vsc callback could not handle
the packet successfully");
statement needs braces, is the return associated with the if() check, or not?

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.h
File src/wave/model/ocb-wifi-mac.h (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.h#...
src/wave/model/ocb-wifi-mac.h:135: virtual void Receive (Ptr<Packet> packet,
const WifiMacHeader *hdr);
why is this not protected?  Won't this prevent a subclass from calling?

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
File src/wave/model/vendor-specific-action.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.cc:47: std::memcpy (m_oi, str, length);
run check-style.py to fix formatting issues

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.cc:116: *  so data parse here is
troublesome
I wonder whether there is a cleaner way to do this, however, than the raw
pointer method you are using.  Let's discuss further.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
File src/wave/model/vendor-specific-action.h (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.h:37: *     or IEEE802.11-2012 section
8.4.1.31
please add some description here for people who do not have easy access to these
standards

Doxygen on public methods is missing in this class

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.h:43: OrganizationIdentifier (const
uint8_t *str, uint32_t length);
we avoid passing raw pointers in our public APIs; can you refactor to avoid
this?

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.h:112: * the categoru field shall be
CATEGORY_OF_VSA
category

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.h:131: * \returns true if the callback
could handle the packet successfully;
this doxygen doesn't seem to match the callback

https://codereview.appspot.com/11685043/diff/1/src/wave/test/ocb-test-suite.cc
File src/wave/test/ocb-test-suite.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/test/ocb-test-suite.c...
src/wave/test/ocb-test-suite.cc:24: #include "ns3/config-store-module.h"
we do not use the "-module" header variants within source code under src/.  Only
list the headers that you need.

why is config-store needed?

https://codereview.appspot.com/11685043/diff/1/src/wave/test/ocb-test-suite.c...
src/wave/test/ocb-test-suite.cc:84: Ptr<MobilityModel> mobility =
node->GetObject<MobilityModel> ();
use ConstantPositionMobilityModel pointer here.

https://codereview.appspot.com/11685043/diff/1/src/wave/wscript
File src/wave/wscript (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/wscript#newcode35
src/wave/wscript:35: # bld.ns3_python_bindings()
we'll need to enable these before merging

https://codereview.appspot.com/11685043/diff/1/src/wifi/model/edca-txop-n.cc
File src/wifi/model/edca-txop-n.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:834: if (m_typeOfStation == STA || m_typeOfStation
== ADHOC_STA || m_typeOfStation == OCB)
I don't know whether it is worth the effort, but I wonder whether this could be
made extensible in a way that didn't require future edits to wifi module when
future types of station are defined.  If we don't anticipate future modules with
different behavior here, patching it seems OK.

Also, where is the handling of 'MESH' type of station?

[Daniel L., 2013-07-28 22:55:54]

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.cc
File src/wave/model/ocb-wifi-mac.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.cc...
src/wave/model/ocb-wifi-mac.cc:43: const static Mac48Address WILDCARD_BSSID =
Mac48Address ("ff:ff:ff:ff:ff:ff");
May be use Mac48Address::GetBroadcast instead of defining this.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
File src/wave/model/vendor-specific-action.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.cc:47: std::memcpy (m_oi, str, length);
Should we avoid using raw pointer/buffer? I think it's better to use ns3::Buffer
if possible.

[Nicola Baldo, 2013-07-29 10:49:25]

Just a couple detailed comments on this patchset.

https://codereview.appspot.com/11685043/diff/1/src/wifi/model/edca-txop-n.cc
File src/wifi/model/edca-txop-n.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:834: if (m_typeOfStation == STA || m_typeOfStation
== ADHOC_STA || m_typeOfStation == OCB)

In the event that Tom's idea does not get implemented, I would suggest changing
the if statement to a switch statement, listing explicitly all cases (with
fall-through) for the two expected behavior, and adding a final default: case
with a NS_FATAL_ERROR. This way, if someone adds a new type of station, he'll be
forced to think about which behavior to specify.

Ok, you could do the same with if...else, but I argue that a switch statement is
more readable for this purpose.

https://codereview.appspot.com/11685043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:850: if (m_typeOfStation == AP || m_typeOfStation
== ADHOC_STA || m_typeOfStation == OCB)
same as previous comment here

[Junling Bu, 2013-07-30 16:52:00]

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
File src/wave/examples/wave-simple-ocb.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
src/wave/examples/wave-simple-ocb.cc:46: *  WaveHelper.
On 2013/07/28 15:22:29, Tom Henderson wrote:
> 
> In the documentation (.rst file) it should probably be clarified what is the
> division of functionality between these helpers.

Done.

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
File src/wave/examples/wave-simple-vsa.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
src/wave/examples/wave-simple-vsa.cc:34: *  Normally the up layer can send
packets in different protocols
On 2013/07/28 15:22:29, Tom Henderson wrote:
> upper

can I use "the higher layer"? I find the "the higher layer" is used in the
standard.

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
src/wave/examples/wave-simple-vsa.cc:50: *  3. OcbWifiMac regists this
identifier and function
On 2013/07/28 15:22:29, Tom Henderson wrote:
> registers

Done.

https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-...
src/wave/examples/wave-simple-vsa.cc:82: m_16093oi = oi_16093;
On 2013/07/28 15:22:29, Tom Henderson wrote:
> need to run check-style.py on this file, to fix indentation and spacing

Done.

https://codereview.appspot.com/11685043/diff/1/src/wave/helper/wifi-80211p-he...
File src/wave/helper/wifi-80211p-helper.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/helper/wifi-80211p-he...
src/wave/helper/wifi-80211p-helper.cc:97: /* Need REDO */
On 2013/07/28 15:22:29, Tom Henderson wrote:
> can you clarify what is the open issue here?
No, the "REDO" is used when I copy code from WifiHelper to remind myself that
the code maybe need some modification. It just support 802.11a phy standard with
10MHz or 20MHz channel width. And I think now the code works out fine.

https://codereview.appspot.com/11685043/diff/1/src/wave/helper/wifi-80211p-he...
File src/wave/helper/wifi-80211p-helper.h (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/helper/wifi-80211p-he...
src/wave/helper/wifi-80211p-helper.h:43: * shall 802.11a
(WIFI_PHY_STANDARD_80211_10MHZ or WIFI_PHY_STANDARD_80211a).
On 2013/07/28 15:22:29, Tom Henderson wrote:
> can you clarify the last sentence further?
Besides that, the MAC and PHY layer shall be configured to 802.11a with 20MHz or
10MHz.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.cc
File src/wave/model/ocb-wifi-mac.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.cc...
src/wave/model/ocb-wifi-mac.cc:43: const static Mac48Address WILDCARD_BSSID =
Mac48Address ("ff:ff:ff:ff:ff:ff");
On 2013/07/28 22:55:55, Daniel L. wrote:
> May be use Mac48Address::GetBroadcast instead of defining this.

It is a good idea.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.cc...
src/wave/model/ocb-wifi-mac.cc:263: NS_LOG_LOGIC ("packet is not for us");
On 2013/07/28 15:22:29, Tom Henderson wrote:
> change "packet is not for us" to "management packet is not for us"

change to "the management frame is not for us"

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.cc...
src/wave/model/ocb-wifi-mac.cc:284: NS_LOG_DEBUG("vsc callback could not handle
the packet successfully");
On 2013/07/28 15:22:29, Tom Henderson wrote:
> statement needs braces, is the return associated with the if() check, or not?
not, the result is used to tell whether the vsc frames handled successfully by
high layer, if fail, we log a debug message to help users, if succeed, we are
silent to say nothing.
we need to return no matter succeed or fail, because we have handled the frame.
Now I add an empty line between NS_LOG_DEBUG and return.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.h
File src/wave/model/ocb-wifi-mac.h (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/model/ocb-wifi-mac.h#...
src/wave/model/ocb-wifi-mac.h:135: virtual void Receive (Ptr<Packet> packet,
const WifiMacHeader *hdr);
On 2013/07/28 15:22:29, Tom Henderson wrote:
> why is this not protected?  Won't this prevent a subclass from calling?
But in wifi module, the Receive method in AdhocWifiMac and ApWifiMac is private,
so I implement without protected.
Should I change to Receive with protected? Only OcbWifiMac or other high layer
mac in wifi module also?

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
File src/wave/model/vendor-specific-action.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.cc:47: std::memcpy (m_oi, str, length);
On 2013/07/28 22:55:55, Daniel L. wrote:
> Should we avoid using raw pointer/buffer? I think it's better to use
ns3::Buffer
> if possible.
Here I implement OrganizationIdentifier by taking reference of Mac48Adderss
which uses an array. Because the 802.11p standard declares that the length of
organizationally unique identifier is either 3 octets or 36 bits, so here I use
an array with 5 bytes.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.cc:47: std::memcpy (m_oi, str, length);
On 2013/07/28 15:22:29, Tom Henderson wrote:
> run check-style.py to fix formatting issues

Done.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.cc:116: *  so data parse here is
troublesome
On 2013/07/28 15:22:29, Tom Henderson wrote:
> I wonder whether there is a cleaner way to do this, however, than the raw
> pointer method you are using.  Let's discuss further.

maybe I need to remove this comment that makes user confused,
the length of OrganizationIdentifier is variable, but the length is now either 3
bytes or 36 bits that defined by the standard, so it is enough to only care
about two different lengths.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
File src/wave/model/vendor-specific-action.h (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.h:37: *     or IEEE802.11-2012 section
8.4.1.31
On 2013/07/28 15:22:29, Tom Henderson wrote:
> please add some description here for people who do not have easy access to
these
> standards
> 
> Doxygen on public methods is missing in this class

Done.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.h:43: OrganizationIdentifier (const
uint8_t *str, uint32_t length);
On 2013/07/28 15:22:29, Tom Henderson wrote:
> we avoid passing raw pointers in our public APIs; can you refactor to avoid
> this?
This class is implemented by taking reference of Mac48Address.
To construct a new OrganizationIdentifier, we should use some bytes. So I think
there is hard to find a better way. And if is safe, we have an internal array.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.h:112: * the categoru field shall be
CATEGORY_OF_VSA
On 2013/07/28 15:22:29, Tom Henderson wrote:
> category

Done.

https://codereview.appspot.com/11685043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.h:131: * \returns true if the callback
could handle the packet successfully;
On 2013/07/28 15:22:29, Tom Henderson wrote:
> this doxygen doesn't seem to match the callback

Done.

https://codereview.appspot.com/11685043/diff/1/src/wave/test/ocb-test-suite.cc
File src/wave/test/ocb-test-suite.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/test/ocb-test-suite.c...
src/wave/test/ocb-test-suite.cc:24: #include "ns3/config-store-module.h"
On 2013/07/28 15:22:29, Tom Henderson wrote:
> we do not use the "-module" header variants within source code under src/. 
Only
> list the headers that you need.
> 
> why is config-store needed?
Yes, I should use what real headers I need.

https://codereview.appspot.com/11685043/diff/1/src/wave/test/ocb-test-suite.c...
src/wave/test/ocb-test-suite.cc:84: Ptr<MobilityModel> mobility =
node->GetObject<MobilityModel> ();
On 2013/07/28 15:22:29, Tom Henderson wrote:
> use ConstantPositionMobilityModel pointer here.

Done.

https://codereview.appspot.com/11685043/diff/1/src/wave/wscript
File src/wave/wscript (right):

https://codereview.appspot.com/11685043/diff/1/src/wave/wscript#newcode35
src/wave/wscript:35: # bld.ns3_python_bindings()
On 2013/07/28 15:22:29, Tom Henderson wrote:
> we'll need to enable these before merging
I am now learning python now, I will try to bind before GSoC ends.

https://codereview.appspot.com/11685043/diff/1/src/wifi/model/edca-txop-n.cc
File src/wifi/model/edca-txop-n.cc (right):

https://codereview.appspot.com/11685043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:834: if (m_typeOfStation == STA || m_typeOfStation
== ADHOC_STA || m_typeOfStation == OCB)
On 2013/07/29 10:49:25, Nicola Baldo wrote:
> 
> In the event that Tom's idea does not get implemented, I would suggest
changing
> the if statement to a switch statement, listing explicitly all cases (with
> fall-through) for the two expected behavior, and adding a final default: case
> with a NS_FATAL_ERROR. This way, if someone adds a new type of station, he'll
be
> forced to think about which behavior to specify.
> 
> Ok, you could do the same with if...else, but I argue that a switch statement
is
> more readable for this purpose.
> 
OK, I will try this approach.

https://codereview.appspot.com/11685043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:834: if (m_typeOfStation == STA || m_typeOfStation
== ADHOC_STA || m_typeOfStation == OCB)
On 2013/07/28 15:22:29, Tom Henderson wrote:
> I don't know whether it is worth the effort, but I wonder whether this could
be
> made extensible in a way that didn't require future edits to wifi module when
> future types of station are defined.  If we don't anticipate future modules
with
> different behavior here, patching it seems OK.
> 
> Also, where is the handling of 'MESH' type of station?

Actually since the 802.11p standard said nothing about aggregation (support? not
support? modify?), I have no idea to do here, and OCB mode is similar to ADHOC,
so I use the way of ADHOC, maybe I can find answer from 802.11-2012 (2793
pages...)
And I agree with your problem, I also find the new 802.11n patch added two types
of station, maybe we need to find a more suitable way as you suggested.

https://codereview.appspot.com/11685043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:850: if (m_typeOfStation == AP || m_typeOfStation
== ADHOC_STA || m_typeOfStation == OCB)
On 2013/07/29 10:49:25, Nicola Baldo wrote:
> same as previous comment here
answer as previous answer