WAVE review Sept 6

Review of Junling's code as of Sept 6.  This is not a public (ns-developers) review yet.

wifi module
(1) remove deprecated 802.11p code
(2) fix a little bugs
(3) modify some codes to make wave module work, including: make some private method and variable protected access, make some method non-virtual to virtual,

wave module
(1) for 802.11p, ocb-wifi-mac.cc, vendor-specific.action.cc, higher-tx-tag.cc are implemented,
support VSA send/receive, specific EDCA arguments, OCB mac mode,  and support higher layer control tx arguments.
(2) for 1609.4, wave-net-device.cc, channel-coordinator.cc, channel-scheduler.cc, channel-manager.c, vsa-repeater.cc, expire-time-tag.cc, wave-edca-txop-n.cc',
support data route (WSMP, IP-based pacakets, management packets), timelife control of per-packe, four channel access assigments (Continuous, Extened, Immediate, and Alternating), channel coordination for CCHI, SCHI and GuardI,  when approaching the guard interval, avoiding transmission process will start, when in guard interval, mac activities will be suspend but mac queue will not flush.
(3) to show the impact of multiple channel operation, a meaningful simulation in wave-mutiple-channel.c can work with four different configurations under different arguments.
(4) the wave module is updated to based on the lates ns-3 release 3.18

however the remaining time still needs some work to do
(1) document is not completed yet with poor doxygen.
(2) the relation among classes is not very clear, some methods are depend on each, I hope to find a better way to  organize classes.
(3) the multiple-channel simulation need to deep develop to make user-friendly and verify correctness.

[Tom Henderson, 2013-09-10 17:52:24]

Some initial comments, mainly focusing on the src/wifi modifications and some of
the class/file organization of src/wave.

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

https://codereview.appspot.com/13606043/diff/1/src/wave/model/ocb-wifi-mac.cc...
src/wave/model/ocb-wifi-mac.cc:49: class WaveMacLow : public MacLow
The declarations of these publicly visible classes should be moved to a header. 
In this specific case, WaveMacLow should be split into separate header/cc files
from OcbWifiMac

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

https://codereview.appspot.com/13606043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.h:55: const uint8_t * PeekData (void)
const;
We typically do not expose raw pointers like this in ns-3 public APIs, can we
find another way?

https://codereview.appspot.com/13606043/diff/1/src/wifi/helper/wifi-helper.h
File src/wifi/helper/wifi-helper.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/helper/wifi-helper.h#...
src/wifi/helper/wifi-helper.h:190: protected:
in general, the WiFi helpers were not written to be subclassed; this probably
needs some brief discussion/confirmation from WiFi maintainers about making this
change (also for the qos/nqos helpers)

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/dcf-manager.cc
File src/wifi/model/dcf-manager.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/dcf-manager.cc#...
src/wifi/model/dcf-manager.cc:300: }
was this a separate memory leak found and fixed?  Should it just be committed as
a separate bug fix?

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

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:403: m_transmissionListener);
Can you summarize (in the reply to this comment) what implementation changes
that this (and the other changes below) are doing?  Are these related to Wave or
to bug fixing?

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

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.h#n...
src/wifi/model/edca-txop-n.h:163: * \return the number of stream indices
assigned by this model
the above is purely a whitespace change, right?  It is better to avoid including
unrelated whitespace changes in your patch.

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h
File src/wifi/model/mac-low.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h#newco...
src/wifi/model/mac-low.h:517: virtual WifiTxVector GetDataTxVector (Ptr<const
Packet> packet, const WifiMacHeader *hdr) const;
For your changes to mac-low.{cc,h}, can you explain what this is doing?  Is this
fixing a separate bug, or is it in support of your WaveNetDevice?

If it is a separate bug, please factor into a separate patch.

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue.cc
File src/wifi/model/wifi-mac-queue.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue....
src/wifi/model/wifi-mac-queue.cc:98: if (m_queue.size () == m_maxSize)
is this a bug fix?

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue.h
File src/wifi/model/wifi-mac-queue.h (left):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue....
src/wifi/model/wifi-mac-queue.h:123: private:
Do you need to make all of the below members protected?

Note that whoever wrote this class to begin with (Mathieu) didn't seem to intend
for it to be subclassed.  You are changing this assumption, is it necessary?

[Junling Bu, 2013-09-12 15:32:52]

Tom , I think here I should talk about my implementation approach again. If you
think this is not a good way, I will change my implementation way.

1609.4 and 802.11p will need some features in MAC layer,
how to achieve that? one way is to add code into wifi module, another way is to
use subclass. Here I prefer to the second way.

Although add feature about 802.11p in wifi module seems OK, the feature about
1609.4 may be not so good, which will make users confused why code is there. If
we use second way, we still have to do small modification in wifi module,
sometimes it will be strange: first create a internal class by Regualar class,
then destruct it and recreate a subclass by OcbWifiMac class. Another problem of
second way is that I have to change some private method or variable to
protected, so subclass can control and change behaviour.
But I still prefer to second, and wifi module will be safe (I almost change
nothing).

If you think it is not suitable, I can change to what you suggested.

Thanks.

Junling

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

https://codereview.appspot.com/13606043/diff/1/src/wave/model/ocb-wifi-mac.cc...
src/wave/model/ocb-wifi-mac.cc:49: class WaveMacLow : public MacLow
On 2013/09/10 17:52:24, Tom Henderson wrote:
> The declarations of these publicly visible classes should be moved to a
header. 
> In this specific case, WaveMacLow should be split into separate header/cc
files
> from OcbWifiMac

Here I think I only need overwirte private virtual method to add a feature, and
this new subclass will not provide new public method for user.

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

https://codereview.appspot.com/13606043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.h:55: const uint8_t * PeekData (void)
const;
On 2013/09/10 17:52:24, Tom Henderson wrote:
> We typically do not expose raw pointers like this in ns-3 public APIs, can we
> find another way?

Tom, as I said in email, 802.11p uses a 3 bytes or 5 bytes organization
identifier, but 5 bytes will only use 36 bits, the last 4 bits will be used by
higher layer or user. Off course we can use subclass, and provide a method to
delivery last 4 bits,
but I think it is meaningless.I will consider carefully again.

https://codereview.appspot.com/13606043/diff/1/src/wifi/helper/wifi-helper.h
File src/wifi/helper/wifi-helper.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/helper/wifi-helper.h#...
src/wifi/helper/wifi-helper.h:190: protected:
On 2013/09/10 17:52:24, Tom Henderson wrote:
> in general, the WiFi helpers were not written to be subclassed; this probably
> needs some brief discussion/confirmation from WiFi maintainers about making
this
> change (also for the qos/nqos helpers)

Yes, I understand what you mean. 
But in my wave-helper
"
NetDeviceContainer devices;
  for (NodeContainer::Iterator i = c.Begin (); i != c.End (); ++i)
    {
      Ptr<Node> node = *i;
      Ptr<WaveNetDevice> device = CreateObject<WaveNetDevice> ();
      Ptr<WifiRemoteStationManager> manager =
m_stationManager.Create<WifiRemoteStationManager> ();
      Ptr<WifiPhy> phy = phyHelper.Create (node, device);
      "
Here I need create a WaveNetDevice first, then configure, which will need
private m_stationManager.
If you do not like protected, I will have to add a public method
GetRemoteStationManager in wifi-helper to get this object factory.

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/dcf-manager.cc
File src/wifi/model/dcf-manager.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/dcf-manager.cc#...
src/wifi/model/dcf-manager.cc:300: }
On 2013/09/10 17:52:24, Tom Henderson wrote:
> was this a separate memory leak found and fixed?  Should it just be committed
as
> a separate bug fix?

At first, it is not a bug, since this method is normally only be called.
But if this method is called more than one, it will cause memory problem.
And I declare a subclass WaveMacLow, and call this method again to set a new
MacLow object into wifi class. So I add this.

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

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:403: m_transmissionListener);
On 2013/09/10 17:52:24, Tom Henderson wrote:
> Can you summarize (in the reply to this comment) what implementation changes
> that this (and the other changes below) are doing?  Are these related to Wave
or
> to bug fixing?

They are related to Wave. 1609.4 requires avoiding transmission at scheduled
guard intervals in Annex, when sending time of a packet exceeds the remaining
time to channel guard interval, the packet will be queued again, not send.
So define StartTransmisson method here, and do check operation in subclass
StartTransmisson method.

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

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.h#n...
src/wifi/model/edca-txop-n.h:163: * \return the number of stream indices
assigned by this model
On 2013/09/10 17:52:24, Tom Henderson wrote:
> the above is purely a whitespace change, right?  It is better to avoid
including
> unrelated whitespace changes in your patch.

fixed, my fault

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h
File src/wifi/model/mac-low.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h#newco...
src/wifi/model/mac-low.h:517: virtual WifiTxVector GetDataTxVector (Ptr<const
Packet> packet, const WifiMacHeader *hdr) const;
On 2013/09/10 17:52:24, Tom Henderson wrote:
> For your changes to mac-low.{cc,h}, can you explain what this is doing?  Is
this
> fixing a separate bug, or is it in support of your WaveNetDevice?
> 
> If it is a separate bug, please factor into a separate patch.

Changing GetDataTxVector to protected virtual method is used for support of
WaveNetDevice. Since 802.11p declares that higher layer can control data rate
and tx power level, and 1609.4 actually needs this feature. However current data
rate control mechanism is based on RemoteStationManager which is MAC layer with
different policies. So I want to overwrite this pure method to support that
feature. And I think it could have minimum impact on wifi module.
And I use this approach to support some features in 802.11p and 1609.4, and try
to avoid modifying source code directly.

Below methods like NowUs() are not implemented in wifi, so I think they are
useless and can been treated as bug.

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue.cc
File src/wifi/model/wifi-mac-queue.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue....
src/wifi/model/wifi-mac-queue.cc:98: if (m_queue.size () == m_maxSize)
On 2013/09/10 17:52:24, Tom Henderson wrote:
> is this a bug fix?

At first, I think m_size is useless to record current queued packets, because
std::list has size () member method.
This time, I google and find that some websites say size method is O(n) in gcc
implementation, so I think that's why here is m_size in WifiMacQueue.
So question is which way is better.

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue.h
File src/wifi/model/wifi-mac-queue.h (left):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue....
src/wifi/model/wifi-mac-queue.h:123: private:
On 2013/09/10 17:52:24, Tom Henderson wrote:
> Do you need to make all of the below members protected?
> 
> Note that whoever wrote this class to begin with (Mathieu) didn't seem to
intend
> for it to be subclassed.  You are changing this assumption, is it necessary?

Tom, as I said above, I want to provide small features required by the standard,
so I use my approach that will use some subclassess and tags, but it seems that
you do not like this way, so should I add some small modification directly into
wifi module?
These codes are not big but will add an extended feature for a class which I
think will increase the complexity for understanding the class.

[Daniel L., 2013-09-13 16:29:44]

Here's my review of the latest patch. Most of the comments are related to coding
styles.

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-coordin...
File src/wave/model/channel-coordinator.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-coordin...
src/wave/model/channel-coordinator.h:115: inline bool IsSchiNow ()
I think "inline" in header files is discouraged unless it provides significant
improvement. My view is that these should be in the .cc files.

Also, I think it's better to spell out Interval instead of using "i".

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager.cc
File src/wave/model/channel-manager.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
src/wave/model/channel-manager.cc:42: for (uint32_t channel_index = 0;
channel_index != CHANNELS_OF_WAVE; ++channel_index)
channelIndex (CamelCase)

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
src/wave/model/channel-manager.cc:44: WaveChannel *channel = new WaveChannel;
WaveChannel is not a smart pointer?

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
src/wave/model/channel-manager.cc:75: return (channelNumber != CCH);
May be you can call IsCch since it's doing just that? Do you think it will be
much slower with the additional function call? If so, I think it's ok to leave
it like this.

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager.h
File src/wave/model/channel-manager.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
src/wave/model/channel-manager.h:110: ChannelDead,
All upper case for enums.

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-schedul...
File src/wave/model/channel-scheduler.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-schedul...
src/wave/model/channel-scheduler.cc:167: case ExtendedAccess:
Intend to fall through? It's better to provide comment that this is intentional.

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-schedul...
src/wave/model/channel-scheduler.cc:355: case ContinuousAccess:
Fall through

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-schedul...
src/wave/model/channel-scheduler.cc:377: case NoAccess:
Fall through

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

https://codereview.appspot.com/13606043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.cc:48: std::memcpy (m_oi, str, length);
Is it possible to use (De)Serialize provided by ns-3? Raw memory access is
discouraged.

https://codereview.appspot.com/13606043/diff/1/src/wave/model/vsa-repeater.cc
File src/wave/model/vsa-repeater.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/vsa-repeater.cc...
src/wave/model/vsa-repeater.cc:193: NS_FATAL_ERROR ("cannot be here");
Provide more explanation in the error message. In case it happens.

https://codereview.appspot.com/13606043/diff/1/src/wave/model/wave-net-device.h
File src/wave/model/wave-net-device.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/wave-net-device...
src/wave/model/wave-net-device.h:51: VsaSentInCchi  = 1,
uppercase enum

https://codereview.appspot.com/13606043/diff/1/src/wave/model/wave-net-device...
src/wave/model/wave-net-device.h:89: VsaInfo (Mac48Address p,
OrganizationIdentifier o, uint8_t m, Ptr<Packet> v,
Better to spell out p, o, m, ...

https://codereview.appspot.com/13606043/diff/1/src/wave/model/wave-net-device...
src/wave/model/wave-net-device.h:130: SchInfo (uint32_t c, bool i, uint32_t e)
spell out c, i, and e

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

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:177: : m_currentPacket (0),
Any specific reason to switch the order?

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:403: m_transmissionListener);
I agree with Tom. May need to provide additional comments in the code.

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.cc#newc...
src/wifi/model/mac-low.cc:548: MacLow::GetPromisc (void) const
IsPromisc sounds better

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h
File src/wifi/model/mac-low.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h#newco...
src/wifi/model/mac-low.h:414: bool GetPromisc (void) const;
IsPromisc

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue.cc
File src/wifi/model/wifi-mac-queue.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue....
src/wifi/model/wifi-mac-queue.cc:98: if (m_queue.size () == m_maxSize)
I think it might be better to use m_size as previously used. What do you think?

[Junling Bu, 2013-09-17 14:59:54]

Hi, Daniel

Thanks for reviewing my code,
if you have any other suggestions, please tell me, I will modify and update my
implementation.

Junling

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-coordin...
File src/wave/model/channel-coordinator.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-coordin...
src/wave/model/channel-coordinator.h:115: inline bool IsSchiNow ()
On 2013/09/13 16:29:44, Daniel L. wrote:
> I think "inline" in header files is discouraged unless it provides significant
> improvement. My view is that these should be in the .cc files.
> 
> Also, I think it's better to spell out Interval instead of using "i".

fixed.
Yes, I agree with you that here it is better to use Interval. I have changed
some methods like IsGuardIntervalNow.
But some other methods like NeedTimeToSchiNow may be better to use "i", since
the name of method is already long.
How do you think? should I change them with Interval rather than i ?

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager.cc
File src/wave/model/channel-manager.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
src/wave/model/channel-manager.cc:42: for (uint32_t channel_index = 0;
channel_index != CHANNELS_OF_WAVE; ++channel_index)
On 2013/09/13 16:29:44, Daniel L. wrote:
> channelIndex (CamelCase)
fixed

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
src/wave/model/channel-manager.cc:44: WaveChannel *channel = new WaveChannel;
On 2013/09/13 16:29:44, Daniel L. wrote:
> WaveChannel is not a smart pointer?
Yes, WaveChannel is an internal class, we can get channel information by public
methods.

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
src/wave/model/channel-manager.cc:75: return (channelNumber != CCH);
On 2013/09/13 16:29:44, Daniel L. wrote:
> May be you can call IsCch since it's doing just that? Do you think it will be
> much slower with the additional function call? If so, I think it's ok to leave
> it like this.
fixed

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager.h
File src/wave/model/channel-manager.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
src/wave/model/channel-manager.h:110: ChannelDead,
On 2013/09/13 16:29:44, Daniel L. wrote:
> All upper case for enums.

Fixed

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-schedul...
File src/wave/model/channel-scheduler.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-schedul...
src/wave/model/channel-scheduler.cc:167: case ExtendedAccess:
On 2013/09/13 16:29:44, Daniel L. wrote:
> Intend to fall through? It's better to provide comment that this is
intentional.
fixed

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-schedul...
src/wave/model/channel-scheduler.cc:355: case ContinuousAccess:
On 2013/09/13 16:29:44, Daniel L. wrote:
> Fall through

fixed

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-schedul...
src/wave/model/channel-scheduler.cc:377: case NoAccess:
On 2013/09/13 16:29:44, Daniel L. wrote:
> Fall through

fixed

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

https://codereview.appspot.com/13606043/diff/1/src/wave/model/vendor-specific...
src/wave/model/vendor-specific-action.cc:48: std::memcpy (m_oi, str, length);
On 2013/09/13 16:29:44, Daniel L. wrote:
> Is it possible to use (De)Serialize provided by ns-3? Raw memory access is
> discouraged.

I agree with you that raw memory access is not a good approach. The standard is
a little strange here: it uses 36 bits of 5 bytes to indicate the type of
packet, and remaining 4 bits will be deliver to higher layer.
Below words are copied from the standard which I want to model, can you help me
or give me a better choice? As I have talked with Tom, I can not find a suitable
implementation way.

7.3.1.31 Organization Identifier field
The Organization Identifier field shall contain a public organizationally unique
identifier assigned by the IEEE. The order of the Organization Identifier field
shall follow the ordering convention for MAC addresses from 7.1.1. The IEEE has
assigned public organizationally unique identifiers both of 24-bit length (OUI)
and longer length. In the latter case specific OUI values are shared over
multiple organizations, e.g., using 36-bit length identifiers (OUI-36 and IAB)
(see IEEE Registration Authority [Bc]). The length of the Organization
Identifier field (j) shall be the minimum number of octets required to contain
the entire organizationally unique identifier (see Figure 7-75), and the first 3
octets shall contain the OUI portion of the identifier. Thus, the Organization
Identifier field is 3 octets in length if the organizationally unique identifier
is an OUI, or 5 octets in length if the organizationally unique identifier is 36
bits in length. The IEEE assigns 36-bit organizationally unique identifiers such
that the OUI portion indicates that total length of the identifier is 36 bits.
If the length of the organizationally unique identifier is not an integral
number of octets, the least significant bits of the last octet are specified by
the organization identified.
NOTE—For example, for the organizationally unique identifier 0x0050C24A4, the
Organization Identifier field would
contain 0x0050C24A4y where y represents the four least significant bits of the
fifth octet of the field. The value of y is
specified by the organization whose identifier is 0x0050C24A4.

https://codereview.appspot.com/13606043/diff/1/src/wave/model/vsa-repeater.cc
File src/wave/model/vsa-repeater.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/vsa-repeater.cc...
src/wave/model/vsa-repeater.cc:193: NS_FATAL_ERROR ("cannot be here");
On 2013/09/13 16:29:44, Daniel L. wrote:
> Provide more explanation in the error message. In case it happens.

fixed

https://codereview.appspot.com/13606043/diff/1/src/wave/model/wave-net-device.h
File src/wave/model/wave-net-device.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/wave-net-device...
src/wave/model/wave-net-device.h:51: VsaSentInCchi  = 1,
On 2013/09/13 16:29:44, Daniel L. wrote:
> uppercase enum
fixed

https://codereview.appspot.com/13606043/diff/1/src/wave/model/wave-net-device...
src/wave/model/wave-net-device.h:89: VsaInfo (Mac48Address p,
OrganizationIdentifier o, uint8_t m, Ptr<Packet> v,
On 2013/09/13 16:29:44, Daniel L. wrote:
> Better to spell out p, o, m, ...

fixed

https://codereview.appspot.com/13606043/diff/1/src/wave/model/wave-net-device...
src/wave/model/wave-net-device.h:130: SchInfo (uint32_t c, bool i, uint32_t e)
On 2013/09/13 16:29:44, Daniel L. wrote:
> spell out c, i, and e
fixed

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

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:177: : m_currentPacket (0),
On 2013/09/13 16:29:44, Daniel L. wrote:
> Any specific reason to switch the order?

Because I need to control m_currentPacket in EdcaTxopN subclass, so I move
m_currentPacket to protected part which will cause m_currentPacket is in front
of m_manager

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.cc#...
src/wifi/model/edca-txop-n.cc:403: m_transmissionListener);
On 2013/09/13 16:29:44, Daniel L. wrote:
> I agree with Tom. May need to provide additional comments in the code.

Ok, I have added additional comments

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.cc#newc...
src/wifi/model/mac-low.cc:548: MacLow::GetPromisc (void) const
On 2013/09/13 16:29:44, Daniel L. wrote:
> IsPromisc sounds better

fixed

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h
File src/wifi/model/mac-low.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h#newco...
src/wifi/model/mac-low.h:414: bool GetPromisc (void) const;
On 2013/09/13 16:29:44, Daniel L. wrote:
> IsPromisc

OK

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue.cc
File src/wifi/model/wifi-mac-queue.cc (right):

https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue....
src/wifi/model/wifi-mac-queue.cc:98: if (m_queue.size () == m_maxSize)
On 2013/09/13 16:29:44, Daniel L. wrote:
> I think it might be better to use m_size as previously used. What do you
think?

Ok, I will change it.

[Tom Henderson, 2013-09-18 03:27:22]

> > 
> > Also, I think it's better to spell out Interval instead of using "i".
> 
> fixed.
> Yes, I agree with you that here it is better to use Interval. I have changed
> some methods like IsGuardIntervalNow.
> But some other methods like NeedTimeToSchiNow may be better to use "i", since
> the name of method is already long.
> How do you think? should I change them with Interval rather than i ?

Interval is preferred here.

I don't know what all of these "Is..Now()", "Is..After()" methods are supposed
to be doing; can you document their meaning?   Also, this type of method: 

 inline bool IsCchiNow ()
  {
    return IsCchiAfter (Seconds (0));
  }

is an example of something that probably ought to either be in a helper function
(if really needed for convenience) or just removed altogether, in the interest
of a more minimal API.  

I would suggest that if you find that you are using IsCcchiAfter(Seconds (0)) a
lot in your example/test code, and you want to provide a convenience function
for it, then perhaps retain it (as part of a helper), but if you are just
writing convenience functions without much current usage, suggest to delete
them.

> 
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager.cc
> File src/wave/model/channel-manager.cc (right):
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
> src/wave/model/channel-manager.cc:42: for (uint32_t channel_index = 0;
> channel_index != CHANNELS_OF_WAVE; ++channel_index)
> On 2013/09/13 16:29:44, Daniel L. wrote:
> > channelIndex (CamelCase)
> fixed
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
> src/wave/model/channel-manager.cc:44: WaveChannel *channel = new WaveChannel;
> On 2013/09/13 16:29:44, Daniel L. wrote:
> > WaveChannel is not a smart pointer?
> Yes, WaveChannel is an internal class, we can get channel information by
public
> methods.

another option here (may be more readable) is to put a constructor in the struct
and just initialize when new() is called

> 
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/vendor-specific...
> src/wave/model/vendor-specific-action.cc:48: std::memcpy (m_oi, str, length);
> On 2013/09/13 16:29:44, Daniel L. wrote:
> > Is it possible to use (De)Serialize provided by ns-3? Raw memory access is
> > discouraged.
> 
> I agree with you that raw memory access is not a good approach. The standard
is
> a little strange here: it uses 36 bits of 5 bytes to indicate the type of
> packet, and remaining 4 bits will be deliver to higher layer.
> Below words are copied from the standard which I want to model, can you help
me
> or give me a better choice? As I have talked with Tom, I can not find a
suitable
> implementation way.
> 
>

It seems that you are using PeekData() only to get at the last byte, in class
WaveNetDevice:

  uint32_t managementId = oi.PeekData ()[4] | 0x0f;

Why don't you just provide a uint32_t GetManagementId () method here instead of
PeekData()?

  (could instead return uint16_t or uint8_t)

[Tom Henderson, 2013-09-18 03:55:15]

On 2013/09/12 15:32:52, Junling Bu wrote:
> Tom , I think here I should talk about my implementation approach again. If
you
> think this is not a good way, I will change my implementation way.
> 
> 1609.4 and 802.11p will need some features in MAC layer,
> how to achieve that? one way is to add code into wifi module, another way is
to
> use subclass. Here I prefer to the second way.
> 
> Although add feature about 802.11p in wifi module seems OK, the feature about
> 1609.4 may be not so good, which will make users confused why code is there.
If
> we use second way, we still have to do small modification in wifi module,
> sometimes it will be strange: first create a internal class by Regualar class,
> then destruct it and recreate a subclass by OcbWifiMac class. Another problem
of
> second way is that I have to change some private method or variable to
> protected, so subclass can control and change behaviour.
> But I still prefer to second, and wifi module will be safe (I almost change
> nothing).

Could you please try to clarify the above further?  I didn't follow it.

> 
> If you think it is not suitable, I can change to what you suggested.

My main reaction was to the making of all private member variables protected. 
It is easy to do so, but breaks encapsulation. Perhaps it may be better to add
protected accessor methods to get access to the underlying private data (which
is kept private).

Let's leave it as is for now, it may not be worth any changes


> 
> https://codereview.appspot.com/13606043/diff/1/src/wave/model/ocb-wifi-mac.cc
> File src/wave/model/ocb-wifi-mac.cc (right):
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/ocb-wifi-mac.cc...
> src/wave/model/ocb-wifi-mac.cc:49: class WaveMacLow : public MacLow
> On 2013/09/10 17:52:24, Tom Henderson wrote:
> > The declarations of these publicly visible classes should be moved to a
> header. 
> > In this specific case, WaveMacLow should be split into separate header/cc
> files
> > from OcbWifiMac
> 
> Here I think I only need overwirte private virtual method to add a feature,
and
> this new subclass will not provide new public method for user.

It is not totally private; you are registering a TypeId for this class.  I think
that any such classes should be split out to header/implementation.

> 
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/vendor-specific...
> File src/wave/model/vendor-specific-action.h (right):
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/vendor-specific...
> src/wave/model/vendor-specific-action.h:55: const uint8_t * PeekData (void)
> const;
> On 2013/09/10 17:52:24, Tom Henderson wrote:
> > We typically do not expose raw pointers like this in ns-3 public APIs, can
we
> > find another way?
> 
> Tom, as I said in email, 802.11p uses a 3 bytes or 5 bytes organization
> identifier, but 5 bytes will only use 36 bits, the last 4 bits will be used by
> higher layer or user. Off course we can use subclass, and provide a method to
> delivery last 4 bits,
> but I think it is meaningless.I will consider carefully again.

see my other comment (I suggest to remove PeekData())

> 
> https://codereview.appspot.com/13606043/diff/1/src/wifi/helper/wifi-helper.h
> File src/wifi/helper/wifi-helper.h (right):
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/helper/wifi-helper.h#...
> src/wifi/helper/wifi-helper.h:190: protected:
> On 2013/09/10 17:52:24, Tom Henderson wrote:
> > in general, the WiFi helpers were not written to be subclassed; this
probably
> > needs some brief discussion/confirmation from WiFi maintainers about making
> this
> > change (also for the qos/nqos helpers)
> 
> Yes, I understand what you mean. 
> But in my wave-helper
> "
> NetDeviceContainer devices;
>   for (NodeContainer::Iterator i = c.Begin (); i != c.End (); ++i)
>     {
>       Ptr<Node> node = *i;
>       Ptr<WaveNetDevice> device = CreateObject<WaveNetDevice> ();
>       Ptr<WifiRemoteStationManager> manager =
> m_stationManager.Create<WifiRemoteStationManager> ();
>       Ptr<WifiPhy> phy = phyHelper.Create (node, device);
>       "
> Here I need create a WaveNetDevice first, then configure, which will need
> private m_stationManager.
> If you do not like protected, I will have to add a public method
> GetRemoteStationManager in wifi-helper to get this object factory.

I am just mentioning that, in general, practice has been to not subclass
helpers, but to rewrite them if similar functions are needed.

I think this decision has bee controversial and debated in years past.  I don't
think it means that you cannot do this, but it might require some list
discussion or some discussion in your final GSoC review.

> 
> https://codereview.appspot.com/13606043/diff/1/src/wifi/model/dcf-manager.cc
> File src/wifi/model/dcf-manager.cc (right):
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/dcf-manager.cc#...
> src/wifi/model/dcf-manager.cc:300: }
> On 2013/09/10 17:52:24, Tom Henderson wrote:
> > was this a separate memory leak found and fixed?  Should it just be
committed
> as
> > a separate bug fix?
> 
> At first, it is not a bug, since this method is normally only be called.
> But if this method is called more than one, it will cause memory problem.
> And I declare a subclass WaveMacLow, and call this method again to set a new
> MacLow object into wifi class. So I add this.

I would lean towards calling it a bug fix, since the API doesn't forbid a user
from calling this method twice (even though it is not commonly done).

> 
> https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.cc
> File src/wifi/model/edca-txop-n.cc (right):
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.cc#...
> src/wifi/model/edca-txop-n.cc:403: m_transmissionListener);
> On 2013/09/10 17:52:24, Tom Henderson wrote:
> > Can you summarize (in the reply to this comment) what implementation changes
> > that this (and the other changes below) are doing?  Are these related to
Wave
> or
> > to bug fixing?
> 
> They are related to Wave. 1609.4 requires avoiding transmission at scheduled
> guard intervals in Annex, when sending time of a packet exceeds the remaining
> time to channel guard interval, the packet will be queued again, not send.
> So define StartTransmisson method here, and do check operation in subclass
> StartTransmisson method.
> 
> https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.h
> File src/wifi/model/edca-txop-n.h (right):
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.h#n...
> src/wifi/model/edca-txop-n.h:163: * \return the number of stream indices
> assigned by this model
> On 2013/09/10 17:52:24, Tom Henderson wrote:
> > the above is purely a whitespace change, right?  It is better to avoid
> including
> > unrelated whitespace changes in your patch.
> 
> fixed, my fault
> 
> https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h
> File src/wifi/model/mac-low.h (right):
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h#newco...
> src/wifi/model/mac-low.h:517: virtual WifiTxVector GetDataTxVector (Ptr<const
> Packet> packet, const WifiMacHeader *hdr) const;
> On 2013/09/10 17:52:24, Tom Henderson wrote:
> > For your changes to mac-low.{cc,h}, can you explain what this is doing?  Is
> this
> > fixing a separate bug, or is it in support of your WaveNetDevice?
> > 
> > If it is a separate bug, please factor into a separate patch.
> 
> Changing GetDataTxVector to protected virtual method is used for support of
> WaveNetDevice. Since 802.11p declares that higher layer can control data rate
> and tx power level, and 1609.4 actually needs this feature. However current
data
> rate control mechanism is based on RemoteStationManager which is MAC layer
with
> different policies. So I want to overwrite this pure method to support that
> feature. And I think it could have minimum impact on wifi module.
> And I use this approach to support some features in 802.11p and 1609.4, and
try
> to avoid modifying source code directly.

OK, there has been some past discussion in the wifi module about per-packet
power control, and I'm not sure where things stand on this issue.  

> 
> Below methods like NowUs() are not implemented in wifi, so I think they are
> useless and can been treated as bug.
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue.cc
> File src/wifi/model/wifi-mac-queue.cc (right):
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue....
> src/wifi/model/wifi-mac-queue.cc:98: if (m_queue.size () == m_maxSize)
> On 2013/09/10 17:52:24, Tom Henderson wrote:
> > is this a bug fix?
> 
> At first, I think m_size is useless to record current queued packets, because
> std::list has size () member method.
> This time, I google and find that some websites say size method is O(n) in gcc
> implementation, so I think that's why here is m_size in WifiMacQueue.
> So question is which way is better.

Mathieu or Nicola may know whether they performance optimized this in the past.

[Junling Bu, 2013-09-18 12:06:56]

On 2013/09/18 03:27:22, Tom Henderson wrote:
> > > 
> > > Also, I think it's better to spell out Interval instead of using "i".
> > 
> > fixed.
> > Yes, I agree with you that here it is better to use Interval. I have changed
> > some methods like IsGuardIntervalNow.
> > But some other methods like NeedTimeToSchiNow may be better to use "i",
since
> > the name of method is already long.
> > How do you think? should I change them with Interval rather than i ?
> 
> Interval is preferred here.
> 
> I don't know what all of these "Is..Now()", "Is..After()" methods are supposed
> to be doing; can you document their meaning?   Also, this type of method: 
> 
>  inline bool IsCchiNow ()
>   {
>     return IsCchiAfter (Seconds (0));
>   }
> 
> is an example of something that probably ought to either be in a helper
function
> (if really needed for convenience) or just removed altogether, in the interest
> of a more minimal API.  
> 
> I would suggest that if you find that you are using IsCcchiAfter(Seconds (0))
a
> lot in your example/test code, and you want to provide a convenience function
> for it, then perhaps retain it (as part of a helper), but if you are just
> writing convenience functions without much current usage, suggest to delete
> them.
> 
I will write doxygen to show meaning of methods, and I remove "Now" and "After",
but
still use and think we could use two types of methods: one can get current
interval state, and the other can get interval state of future time. I am not
sure how to use a helper, can you give an example? I think making it in the
class is not very bad.
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager.cc
> > File src/wave/model/channel-manager.cc (right):
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
> > src/wave/model/channel-manager.cc:42: for (uint32_t channel_index = 0;
> > channel_index != CHANNELS_OF_WAVE; ++channel_index)
> > On 2013/09/13 16:29:44, Daniel L. wrote:
> > > channelIndex (CamelCase)
> > fixed
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-manager...
> > src/wave/model/channel-manager.cc:44: WaveChannel *channel = new
WaveChannel;
> > On 2013/09/13 16:29:44, Daniel L. wrote:
> > > WaveChannel is not a smart pointer?
> > Yes, WaveChannel is an internal class, we can get channel information by
> public
> > methods.
> 
> another option here (may be more readable) is to put a constructor in the
struct
> and just initialize when new() is called
> 
OK
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/vendor-specific...
> > src/wave/model/vendor-specific-action.cc:48: std::memcpy (m_oi, str,
length);
> > On 2013/09/13 16:29:44, Daniel L. wrote:
> > > Is it possible to use (De)Serialize provided by ns-3? Raw memory access is
> > > discouraged.
> > 
> > I agree with you that raw memory access is not a good approach. The standard
> is
> > a little strange here: it uses 36 bits of 5 bytes to indicate the type of
> > packet, and remaining 4 bits will be deliver to higher layer.
> > Below words are copied from the standard which I want to model, can you help
> me
> > or give me a better choice? As I have talked with Tom, I can not find a
> suitable
> > implementation way.
> > 
> >
> 
> It seems that you are using PeekData() only to get at the last byte, in class
> WaveNetDevice:
> 
>   uint32_t managementId = oi.PeekData ()[4] | 0x0f;
> 
> Why don't you just provide a uint32_t GetManagementId () method here instead
of
> PeekData()?
> 
>   (could instead return uint16_t or uint8_t)
OK, fixed.

[Junling Bu, 2013-09-18 12:34:46]

> > Although add feature about 802.11p in wifi module seems OK, the feature
about
> > 1609.4 may be not so good, which will make users confused why code is there.
> If
> > we use second way, we still have to do small modification in wifi module,
> > sometimes it will be strange: first create a internal class by Regualar
class,
> > then destruct it and recreate a subclass by OcbWifiMac class. Another
problem
> of
> > second way is that I have to change some private method or variable to
> > protected, so subclass can control and change behaviour.
> > But I still prefer to second, and wifi module will be safe (I almost change
> > nothing).
> 
> Could you please try to clarify the above further?  I didn't follow it.
> 
My consideration here is that some features declared by 1609.4 in MAC layer are
needed to provide by wifi classes, and I think using subclass of wifi module is
better than modifying code directly in wifi module. But using subclass still
need to do small modification in wifi module to use these new subclass objects.
> > 
> > If you think it is not suitable, I can change to what you suggested.
> 
> My main reaction was to the making of all private member variables protected. 
> It is easy to do so, but breaks encapsulation. Perhaps it may be better to add
> protected accessor methods to get access to the underlying private data (which
> is kept private).
> 
> Let's leave it as is for now, it may not be worth any changes
> 
OK
> 
> > 
> >
https://codereview.appspot.com/13606043/diff/1/src/wave/model/ocb-wifi-mac.cc
> > File src/wave/model/ocb-wifi-mac.cc (right):
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/ocb-wifi-mac.cc...
> > src/wave/model/ocb-wifi-mac.cc:49: class WaveMacLow : public MacLow
> > On 2013/09/10 17:52:24, Tom Henderson wrote:
> > > The declarations of these publicly visible classes should be moved to a
> > header. 
> > > In this specific case, WaveMacLow should be split into separate header/cc
> > files
> > > from OcbWifiMac
> > 
> > Here I think I only need overwirte private virtual method to add a feature,
> and
> > this new subclass will not provide new public method for user.
> 
> It is not totally private; you are registering a TypeId for this class.  I
think
> that any such classes should be split out to header/implementation.
> 
OK,fixed
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/vendor-specific...
> > File src/wave/model/vendor-specific-action.h (right):
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/vendor-specific...
> > src/wave/model/vendor-specific-action.h:55: const uint8_t * PeekData (void)
> > const;
> > On 2013/09/10 17:52:24, Tom Henderson wrote:
> > > We typically do not expose raw pointers like this in ns-3 public APIs, can
> we
> > > find another way?
> > 
> > Tom, as I said in email, 802.11p uses a 3 bytes or 5 bytes organization
> > identifier, but 5 bytes will only use 36 bits, the last 4 bits will be used
by
> > higher layer or user. Off course we can use subclass, and provide a method
to
> > delivery last 4 bits,
> > but I think it is meaningless.I will consider carefully again.
> 
> see my other comment (I suggest to remove PeekData())
fixed
> 
> > 
> > https://codereview.appspot.com/13606043/diff/1/src/wifi/helper/wifi-helper.h
> > File src/wifi/helper/wifi-helper.h (right):
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/helper/wifi-helper.h#...
> > src/wifi/helper/wifi-helper.h:190: protected:
> > On 2013/09/10 17:52:24, Tom Henderson wrote:
> > > in general, the WiFi helpers were not written to be subclassed; this
> probably
> > > needs some brief discussion/confirmation from WiFi maintainers about
making
> > this
> > > change (also for the qos/nqos helpers)
> > 
> > Yes, I understand what you mean. 
> > But in my wave-helper
> > "
> > NetDeviceContainer devices;
> >   for (NodeContainer::Iterator i = c.Begin (); i != c.End (); ++i)
> >     {
> >       Ptr<Node> node = *i;
> >       Ptr<WaveNetDevice> device = CreateObject<WaveNetDevice> ();
> >       Ptr<WifiRemoteStationManager> manager =
> > m_stationManager.Create<WifiRemoteStationManager> ();
> >       Ptr<WifiPhy> phy = phyHelper.Create (node, device);
> >       "
> > Here I need create a WaveNetDevice first, then configure, which will need
> > private m_stationManager.
> > If you do not like protected, I will have to add a public method
> > GetRemoteStationManager in wifi-helper to get this object factory.
> 
> I am just mentioning that, in general, practice has been to not subclass
> helpers, but to rewrite them if similar functions are needed.
> 
> I think this decision has bee controversial and debated in years past.  I
don't
> think it means that you cannot do this, but it might require some list
> discussion or some discussion in your final GSoC review.
> 
OK, I will change according to final GSoC review
> > 
> > https://codereview.appspot.com/13606043/diff/1/src/wifi/model/dcf-manager.cc
> > File src/wifi/model/dcf-manager.cc (right):
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/dcf-manager.cc#...
> > src/wifi/model/dcf-manager.cc:300: }
> > On 2013/09/10 17:52:24, Tom Henderson wrote:
> > > was this a separate memory leak found and fixed?  Should it just be
> committed
> > as
> > > a separate bug fix?
> > 
> > At first, it is not a bug, since this method is normally only be called.
> > But if this method is called more than one, it will cause memory problem.
> > And I declare a subclass WaveMacLow, and call this method again to set a new
> > MacLow object into wifi class. So I add this.
> 
> I would lean towards calling it a bug fix, since the API doesn't forbid a user
> from calling this method twice (even though it is not commonly done).
> 
OK
> > 
> > https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.cc
> > File src/wifi/model/edca-txop-n.cc (right):
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.cc#...
> > src/wifi/model/edca-txop-n.cc:403: m_transmissionListener);
> > On 2013/09/10 17:52:24, Tom Henderson wrote:
> > > Can you summarize (in the reply to this comment) what implementation
changes
> > > that this (and the other changes below) are doing?  Are these related to
> Wave
> > or
> > > to bug fixing?
> > 
> > They are related to Wave. 1609.4 requires avoiding transmission at scheduled
> > guard intervals in Annex, when sending time of a packet exceeds the
remaining
> > time to channel guard interval, the packet will be queued again, not send.
> > So define StartTransmisson method here, and do check operation in subclass
> > StartTransmisson method.
> > 
> > https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.h
> > File src/wifi/model/edca-txop-n.h (right):
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/edca-txop-n.h#n...
> > src/wifi/model/edca-txop-n.h:163: * \return the number of stream indices
> > assigned by this model
> > On 2013/09/10 17:52:24, Tom Henderson wrote:
> > > the above is purely a whitespace change, right?  It is better to avoid
> > including
> > > unrelated whitespace changes in your patch.
> > 
> > fixed, my fault
> > 
> > https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h
> > File src/wifi/model/mac-low.h (right):
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/mac-low.h#newco...
> > src/wifi/model/mac-low.h:517: virtual WifiTxVector GetDataTxVector
(Ptr<const
> > Packet> packet, const WifiMacHeader *hdr) const;
> > On 2013/09/10 17:52:24, Tom Henderson wrote:
> > > For your changes to mac-low.{cc,h}, can you explain what this is doing? 
Is
> > this
> > > fixing a separate bug, or is it in support of your WaveNetDevice?
> > > 
> > > If it is a separate bug, please factor into a separate patch.
> > 
> > Changing GetDataTxVector to protected virtual method is used for support of
> > WaveNetDevice. Since 802.11p declares that higher layer can control data
rate
> > and tx power level, and 1609.4 actually needs this feature. However current
> data
> > rate control mechanism is based on RemoteStationManager which is MAC layer
> with
> > different policies. So I want to overwrite this pure method to support that
> > feature. And I think it could have minimum impact on wifi module.
> > And I use this approach to support some features in 802.11p and 1609.4, and
> try
> > to avoid modifying source code directly.
> 
> OK, there has been some past discussion in the wifi module about per-packet
> power control, and I'm not sure where things stand on this issue.  
> 
> > 
> > Below methods like NowUs() are not implemented in wifi, so I think they are
> > useless and can been treated as bug.
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue.cc
> > File src/wifi/model/wifi-mac-queue.cc (right):
> > 
> >
>
https://codereview.appspot.com/13606043/diff/1/src/wifi/model/wifi-mac-queue....
> > src/wifi/model/wifi-mac-queue.cc:98: if (m_queue.size () == m_maxSize)
> > On 2013/09/10 17:52:24, Tom Henderson wrote:
> > > is this a bug fix?
> > 
> > At first, I think m_size is useless to record current queued packets,
because
> > std::list has size () member method.
> > This time, I google and find that some websites say size method is O(n) in
gcc
> > implementation, so I think that's why here is m_size in WifiMacQueue.
> > So question is which way is better.
> 
> Mathieu or Nicola may know whether they performance optimized this in the
past.

As suggested by Daniel, I will keep m_size as before.

[Daniel L., 2013-09-18 12:47:27]

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-coordin...
File src/wave/model/channel-coordinator.h (right):

https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-coordin...
src/wave/model/channel-coordinator.h:115: inline bool IsSchiNow ()
On 2013/09/17 14:59:54, Junling Bu wrote:
> On 2013/09/13 16:29:44, Daniel L. wrote:
> > I think "inline" in header files is discouraged unless it provides
significant
> > improvement. My view is that these should be in the .cc files.
> > 
> > Also, I think it's better to spell out Interval instead of using "i".
> 
> fixed.
> Yes, I agree with you that here it is better to use Interval. I have changed
> some methods like IsGuardIntervalNow.
> But some other methods like NeedTimeToSchiNow may be better to use "i", since
> the name of method is already long.
> How do you think? should I change them with Interval rather than i ?

I think changing every "i" to Interval is better.

[Junling Bu, 2013-09-18 15:13:34]

On 2013/09/18 12:47:27, Daniel L. wrote:
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-coordin...
> File src/wave/model/channel-coordinator.h (right):
> 
>
https://codereview.appspot.com/13606043/diff/1/src/wave/model/channel-coordin...
> src/wave/model/channel-coordinator.h:115: inline bool IsSchiNow ()
> On 2013/09/17 14:59:54, Junling Bu wrote:
> > On 2013/09/13 16:29:44, Daniel L. wrote:
> > > I think "inline" in header files is discouraged unless it provides
> significant
> > > improvement. My view is that these should be in the .cc files.
> > > 
> > > Also, I think it's better to spell out Interval instead of using "i".
> > 
> > fixed.
> > Yes, I agree with you that here it is better to use Interval. I have changed
> > some methods like IsGuardIntervalNow.
> > But some other methods like NeedTimeToSchiNow may be better to use "i",
since
> > the name of method is already long.
> > How do you think? should I change them with Interval rather than i ?
> 
> I think changing every "i" to Interval is better.
OK, I have changed these methods according to your and Tom's comments.