Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(22)

Issue 11685043: IEEE 802.11p MAC

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 2 months ago by Tom Henderson
Modified:
8 years, 1 month ago
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 46

Patch Set 2 : updates after midterm review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2253 lines, -139 lines) Patch
A src/wave/examples/wave-simple-ocb.cc View 1 1 chunk +192 lines, -0 lines 0 comments Download
A src/wave/examples/wave-simple-vsa.cc View 1 1 chunk +174 lines, -0 lines 0 comments Download
A src/wave/examples/wscript View 1 chunk +11 lines, -0 lines 0 comments Download
A src/wave/helper/wave-mac-helper.h View 1 1 chunk +139 lines, -0 lines 0 comments Download
A src/wave/helper/wave-mac-helper.cc View 1 1 chunk +119 lines, -0 lines 0 comments Download
A src/wave/helper/wifi-80211p-helper.h View 1 1 chunk +80 lines, -0 lines 0 comments Download
A src/wave/helper/wifi-80211p-helper.cc View 1 1 chunk +111 lines, -0 lines 0 comments Download
A src/wave/model/ocb-wifi-mac.h View 1 1 chunk +141 lines, -0 lines 0 comments Download
A src/wave/model/ocb-wifi-mac.cc View 1 1 chunk +354 lines, -0 lines 0 comments Download
A src/wave/model/vendor-specific-action.h View 1 1 chunk +177 lines, -0 lines 0 comments Download
A src/wave/model/vendor-specific-action.cc View 1 1 chunk +323 lines, -0 lines 0 comments Download
A src/wave/test/ocb-test-suite.cc View 1 1 chunk +343 lines, -0 lines 0 comments Download
A src/wave/wscript View 1 chunk +36 lines, -0 lines 0 comments Download
M src/wifi/helper/nqos-wifi-mac-helper.h View 1 3 chunks +3 lines, -3 lines 0 comments Download
M src/wifi/helper/qos-wifi-mac-helper.h View 1 3 chunks +5 lines, -3 lines 0 comments Download
M src/wifi/helper/wifi-helper.h View 1 6 chunks +7 lines, -7 lines 0 comments Download
M src/wifi/helper/yans-wifi-helper.h View 1 1 chunk +1 line, -1 line 0 comments Download
M src/wifi/helper/yans-wifi-helper.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M src/wifi/model/edca-txop-n.h View 1 chunk +2 lines, -1 line 0 comments Download
M src/wifi/model/edca-txop-n.cc View 1 5 chunks +33 lines, -13 lines 0 comments Download
M src/wifi/model/regular-wifi-mac.cc View 2 chunks +1 line, -15 lines 0 comments Download
M src/wifi/model/wifi-mac.h View 2 chunks +0 lines, -3 lines 0 comments Download
M src/wifi/model/wifi-mac.cc View 3 chunks +0 lines, -54 lines 0 comments Download
M src/wifi/model/yans-wifi-phy.cc View 2 chunks +0 lines, -38 lines 0 comments Download

Messages

Total messages: 4
Tom Henderson
https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-ocb.cc File src/wave/examples/wave-simple-ocb.cc (right): https://codereview.appspot.com/11685043/diff/1/src/wave/examples/wave-simple-ocb.cc#newcode46 src/wave/examples/wave-simple-ocb.cc:46: * WaveHelper. In the documentation (.rst file) it should ...
8 years, 2 months ago (2013-07-28 15:22:29 UTC) #1
Daniel L.
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#newcode43 src/wave/model/ocb-wifi-mac.cc:43: const static Mac48Address WILDCARD_BSSID = Mac48Address ("ff:ff:ff:ff:ff:ff"); May be ...
8 years, 2 months ago (2013-07-28 22:55:54 UTC) #2
Nicola Baldo
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#newcode834 src/wifi/model/edca-txop-n.cc:834: if ...
8 years, 2 months ago (2013-07-29 10:49:25 UTC) #3
Junling Bu
8 years, 2 months ago (2013-07-30 16:52:00 UTC) #4
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b