Monitor mode support in ns-3

[Nicola Baldo, 2011-07-15 15:33:27]

I reviewed the code. I think the major issues are:

1) the "monitor" flag in mac-low bypasses a lot of code, this has a big impact
so we should be careful. Can you explain in words which functionality of MacLow
you need to keep, and which you need to remove/bypass for monitor mode? 

2) the patch was not generated correctly, as evident from the presence of some
unneeded deletions of code. Maybe due to not rebasing your patch before
generating the diff, maybe due to an incorrect merge. Please fix this an post a
revised patch.

Below some detailed comments.

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wifi-monitor-m...
File examples/wireless/wifi-monitor-mac-high.cc (right):

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wifi-monitor-m...
examples/wireless/wifi-monitor-mac-high.cc:17: *
Author?

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

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/mac-low.cc#newcod...
src/wifi/model/mac-low.cc:686: if (m_monitorMode)
So you are bypassing the whole MacLow reception logic when in monitor mode? I
don't like this very much. Are you sure the behavior with m_monitorMode==true is
always correct? Even with block ack? Even with fragmentation?

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

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/mac-low.h#newcode375
src/wifi/model/mac-low.h:375: typedef Callback<void, Ptr<Packet>, const
WifiMacHeader*, RadiotapHeader> MacLowRxCallback;
Any special reason why you pass RadiotapHeader by copy? If not, I would suggest
to pass it by const* as done for WifiMacHeader, to save the copy. Not sure if we
can use a Ptr<> instead.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high.cc
File src/wifi/model/monitor-mac-high.cc (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:18: * Author: Mathieu Lacage
<mathieu.lacage@sophia.inria.fr>
I think you should mention from which file you copied this code. Also, given the
amount of modifications, you should add yourself to the list of authors. It
would also be good to summarize very briefly what are your modifications.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:139: MonitorMacHigh::SetQosSupported (bool
enable)
this should trigger an error because you can't support QoS, you don't have the
necessary queues.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:142: m_qosSupported = enable;
just return false here.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:325: // We expect RegularWifiMac subclasses
which do support forwarding (e.g.,
RegularWifiMac??? please revise this part and make sure the code makes sense.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:364:
MonitorMacHigh::DeaggregateAmsduAndForward (Ptr<Packet> aggregatedPacket,
do you really need this method?

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high.h
File src/wifi/model/monitor-mac-high.h (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.h:18: * Author: Mathieu Lacage
<mathieu.lacage@sophia.inria.fr>
same comments as for monitor-mac-high.cc

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.h:42: class MonitorMacHigh : public WifiMac
This name is inappropriate, should be for example MonitorWifiMac

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac.cc
File src/wifi/model/regular-wifi-mac.cc (left):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac....
src/wifi/model/regular-wifi-mac.cc:437: // hurt to have it here too as a
backstop.
This patch presents too many unreasonable deletions of code. I guess you just
did not rebase your patch to the latest ns-3-dev before uploading to code
review. And you did not review your patch by yourself to make sure it was as
intended. Please do all this the next time you submit a patch for review.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac.h
File src/wifi/model/regular-wifi-mac.h (left):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac....
src/wifi/model/regular-wifi-mac.h:43: * \ingroup wifi
why did you remove this?

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac....
src/wifi/model/regular-wifi-mac.h:325: RegularWifiMac (const RegularWifiMac &);
why did you delete these?

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.cc
File src/wifi/model/wifi-net-device.cc (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.c...
src/wifi/model/wifi-net-device.cc:297: packet->Copy ()->RemoveHeader (llc);
Can you explain this?

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.h
File src/wifi/model/wifi-net-device.h (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.h...
src/wifi/model/wifi-net-device.h:84: * \param monitor sets mode as monitor mode
you should not use "\param" here

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-phy.h
File src/wifi/model/wifi-phy.h (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-phy.h#newcod...
src/wifi/model/wifi-phy.h:152: * arg4: type of preamble used for packet.
you need to update the documentation. It would appreciate if you could take this
opportunity to make it doxygen-compliant

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-phy.h#newcod...
src/wifi/model/wifi-phy.h:154: typedef Callback<void,Ptr<Packet>, double,
WifiMode, enum WifiPreamble, RadiotapHeader> RxOkCallback;
the SNR, the mode and the preamble are now redundant because the same data is in
RadiotapHeader. Can you investigate if it makes sense to remove these redundant
parameters?

[Tom Henderson, 2011-07-15 16:48:00]

Most of my comments are inline; here are a few remaining comments.

Support for this requires that the Receive() methods all take an extra argument
"RadiotapHeader" but this argument has not been added to the doxygen.  Since
this header is being plumbed up to all MacHigh classes but there is only one
(MonitorMac) that makes use of it, another alternative would be to add the
information to a packet Tag and just create the RadiotapHeader on the spot in
MacMonitorHigh::Receive(), thereby avoiding other API touches. I am not
necessarily recommending this over what you have done but just pointing it out
in case it hasn't been considered.

Since you are changing public API with the additional plumbing of
RadiotapHeader, you should update the CHANGES.html file with the details. 
Please also update RELEASE_NOTES.  Doing these updates before code review helps
the reviewer understand the intention and side effects of the patch before
getting into the review.

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wifi-monitor-m...
File examples/wireless/wifi-monitor-mac-high.cc (right):

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wifi-monitor-m...
examples/wireless/wifi-monitor-mac-high.cc:36: // while the other nodes talk to
each other.
I do not really understand the point of this example.  It doesn't make use of
the new monitor mode features that you added; when you mention below to use
tcpdump, it is just using the existing trace facilities.  Otherwise, this
example just adds a single node to wifi-simple-adhoc-grid example.  

I would suggest to create an example that makes use of monitor mode by consuming
the received packets by a process that sits on top of the WifiNetDevice and
doing something with them (even if just printing out the packet headers
received).

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wscript
File examples/wireless/wscript (right):

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wscript#newcode11
examples/wireless/wscript:11: obj = bld.create_ns3_program('wifi-adhoc',
['core', 'mobility', 'wifi', 'applications', 'tools'])
Changes in this patch are unrelated to monitor mode.  You are stripping out
dependencies on the visualizer module; if you want to propose it, please put
into a separate patch.

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

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/mac-low.h#newcode393
src/wifi/model/mac-low.h:393: void SetMonitorMode (void);
This is missing doxygen.

Are there any use cases when this would need to be unset?

Does it make sense to make this setter into an ns-3 attribute?

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high.cc
File src/wifi/model/monitor-mac-high.cc (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:139: MonitorMacHigh::SetQosSupported (bool
enable)
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> this should trigger an error because you can't support QoS, you don't have the
> necessary queues.

Agree; NS_ABORT_MSG type of macro may be appropriate.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:142: m_qosSupported = enable;
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> just return false here.

function returns void

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:325: // We expect RegularWifiMac subclasses
which do support forwarding (e.g.,
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> RegularWifiMac??? please revise this part and make sure the code makes sense.

agree, the methods in this class seem to have been copy pasted from WifiMac
class without cleanup.  This relates also to above comment on returning error
for setting QoS.  Each one of these methods should be reviewed and decided what
is the appropriate action for this special mode.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:336: {
this is another example; monitor mode does not send packets.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:383: WifiMacHeader hdr;
Likewise, my understanding is that MonitorMode does not ever send frames, acks,
or even associate with any access point.  These do not seem applicable.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high.h
File src/wifi/model/monitor-mac-high.h (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.h:41: 
Why does this mac derive from WifiMac while every other one now derives from
RegularWifiMac?  I'm guessing that there is a reason and it could be documented
in the class doxygen such as "This class derives from WifiMac instead of
RegularWifiMac because..."
If no reason, you might consider to subclass from RegularWifiMac instead.

The doxygen of this class is missing altogether.  It really should describe what
it functionally does (such as
pushing a RadiotapHeader before forwarding up, and not letting the device be
used for transmission...).

Why not simplify the name MonitorMacHigh to MonitorMac (for symmetry with the
other WifiMac derivatives that do not postfix "High").

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac.cc
File src/wifi/model/regular-wifi-mac.cc (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac....
src/wifi/model/regular-wifi-mac.cc:156: m_edca.insert (std::make_pair(ac,
edca));
Please remove whitespace changes from the patch.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.h
File src/wifi/model/wifi-net-device.h (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.h...
src/wifi/model/wifi-net-device.h:85: */
This doxygen could be improved by functionally describing what happens when
MonitorMode is set (here, this means that LLC header is not stripped from
packets that are forwarded up).

What happens if user sets this on a WifiNetDevice that is
not using MacMonitorHigh?  You will not get all the frames.  Should this raise
an error?  At the very least, I think you need to document somewhere that to
make this work, your WifiNetDevice has to have this flag set and has to have a
MacMonitorHigh mac underneath.

[ashwin.narayan89, 2011-07-15 17:34:24]

Hello,

Thank you for your comments. I'm sorry about the issues with the merge. I'll
work on the problems mentioned and will get back to you with the necessary
changes.

- Ashwin.

[Lalith Suresh, 2011-07-18 05:20:00]

On Fri, Jul 15, 2011 at 10:18 PM, <tomh.org@gmail.com> wrote:

> Most of my comments are inline; here are a few remaining comments.
>
> Support for this requires that the Receive() methods all take an extra
> argument "RadiotapHeader" but this argument has not been added to the
> doxygen.  Since this header is being plumbed up to all MacHigh classes
> but there is only one (MonitorMac) that makes use of it, another
> alternative would be to add the information to a packet Tag and just
> create the RadiotapHeader on the spot in MacMonitorHigh::Receive(),
> thereby avoiding other API touches. I am not necessarily recommending
> this over what you have done but just pointing it out in case it hasn't
> been considered.
>
>
Regarding your suggestion, the use of tags for this was considered but
dropped because of duplication of info in a tag and a header.

Here's the discussion:
http://mailman.isi.edu/pipermail/ns-developers/2011-April/008838.html


> [snip]
>
> http://codereview.appspot.com/**4685048/diff/1/src/wifi/model/**
>
monitor-mac-high.cc#newcode336<http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high.cc#newcode336>
> src/wifi/model/monitor-mac-**high.cc:336: {
> this is another example; monitor mode does not send packets.
>

True. We'll correct it for this review because it addresses a "listening
only monitor mode", but we'll need it later because there are cards that can
transmit in monitor mode, and that's what we'd finally like to have for
Click.



-- 
Lalith Suresh
*Department of Computer Science and Engineering*
*Instituto Superior Técnico*
www.lalith.in

[Lalith Suresh, 2011-07-18 05:28:19]

On Fri, Jul 15, 2011 at 9:03 PM, <nicobaldo@gmail.com> wrote:

> I reviewed the code. I think the major issues are:
>
> 1) the "monitor" flag in mac-low bypasses a lot of code, this has a big
> impact so we should be careful. Can you explain in words which
> functionality of MacLow you need to keep, and which you need to
> remove/bypass for monitor mode?
>

We're addressing everything except this for the next patchset because I'm
waiting for some feedback from some Click and Madwifi experts on the above.
Ruben isn't available this week for me to clarify with either. Click seems
to have elements to handle Wifi fragmentation and acknowledgements for
unicast data, but I'll still the experts before making any conclusions
myself.


-- 
Lalith Suresh
*Department of Computer Science and Engineering*
*Instituto Superior Técnico*
www.lalith.in

[Lalith Suresh, 2011-07-18 07:49:59]

On Fri, Jul 15, 2011 at 9:03 PM, <nicobaldo@gmail.com> wrote:

> [snip]
>



>
http://codereview.appspot.com/**4685048/diff/1/src/wifi/model/**mac-low.cc<ht...
> File src/wifi/model/mac-low.cc (right):
>
> http://codereview.appspot.com/**4685048/diff/1/src/wifi/model/**
>
mac-low.cc#newcode686<http://codereview.appspot.com/4685048/diff/1/src/wifi/model/mac-low.cc#newcode686>
> src/wifi/model/mac-low.cc:686: if (m_monitorMode)
> So you are bypassing the whole MacLow reception logic when in monitor
> mode? I don't like this very much. Are you sure the behavior with
> m_monitorMode==true is always correct? Even with block ack? Even with
> fragmentation?
>
>
Here's what I was told by one of the original developers of ns-2's Madwifi
extensions for Click:

- The Madwifi driver does not implement 802.11n yet, and Click seems to be
able to handle fragmentation but not Block ACK. I think it's better to go
for a more conservative approach and let ns-3's MacLow handle this than
dividing 802.11n functionality between ns-3 and Click.
- A monitor card will perform the MacLow functions like responding to ACKs,
but will bypass MacHigh stuff completely, and allow it to be controlled by
madwifi/radiotap/prism headers (of which we've opted to use radiotap).

So it seems to me like we just need to enable the m_promisc flag in MacLow.
What do you think Nicola, Tom?


-- 
Lalith Suresh
*Department of Computer Science and Engineering*
*Instituto Superior Técnico*
www.lalith.in

[ashwin.narayan89, 2011-07-18 08:33:30]

Thank you for your comments again. I have managed to make the necessary
corrections with the exception of a couple of issues. Will work on those over
the course of the week. 

I've uploaded a new patch set containing fixes and the modifications suggested:
http://codereview.appspot.com/4685048/

Thank you.

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wifi-monitor-m...
File examples/wireless/wifi-monitor-mac-high.cc (right):

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wifi-monitor-m...
examples/wireless/wifi-monitor-mac-high.cc:17: *
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> Author?
Added my name to the author's list

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wifi-monitor-m...
examples/wireless/wifi-monitor-mac-high.cc:36: // while the other nodes talk to
each other.
Added a click-based example to print info about received packets from inside
click

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wscript
File examples/wireless/wscript (right):

http://codereview.appspot.com/4685048/diff/1/examples/wireless/wscript#newcode11
examples/wireless/wscript:11: obj = bld.create_ns3_program('wifi-adhoc',
['core', 'mobility', 'wifi', 'applications', 'tools'])
On 2011/07/15 16:48:00, Tom Henderson wrote:
> Changes in this patch are unrelated to monitor mode.  You are stripping out
> dependencies on the visualizer module; if you want to propose it, please put
> into a separate patch.
Sorry - this was an issue with the merging. Corrected.

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

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/mac-low.cc#newcod...
src/wifi/model/mac-low.cc:686: if (m_monitorMode)
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> So you are bypassing the whole MacLow reception logic when in monitor mode? I
> don't like this very much. Are you sure the behavior with m_monitorMode==true
is
> always correct? Even with block ack? Even with fragmentation?
It appears that there is no need to do any bypassing at all. Will work on
correcting this and will repost.

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

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/mac-low.h#newcode375
src/wifi/model/mac-low.h:375: typedef Callback<void, Ptr<Packet>, const
WifiMacHeader*, RadiotapHeader> MacLowRxCallback;
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> Any special reason why you pass RadiotapHeader by copy? If not, I would
suggest
> to pass it by const* as done for WifiMacHeader, to save the copy. Not sure if
we
> can use a Ptr<> instead.
Modified.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/mac-low.h#newcode393
src/wifi/model/mac-low.h:393: void SetMonitorMode (void);
Added comments

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high.cc
File src/wifi/model/monitor-mac-high.cc (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:18: * Author: Mathieu Lacage
<mathieu.lacage@sophia.inria.fr>
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> I think you should mention from which file you copied this code. Also, given
the
> amount of modifications, you should add yourself to the list of authors. It
> would also be good to summarize very briefly what are your modifications.
Renamed the class. Made the necessary changes there.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:139: MonitorMacHigh::SetQosSupported (bool
enable)
On 2011/07/15 16:48:00, Tom Henderson wrote:
> On 2011/07/15 15:33:28, Nicola Baldo wrote:
> > this should trigger an error because you can't support QoS, you don't have
the
> > necessary queues.
> 
> Agree; NS_ABORT_MSG type of macro may be appropriate.
Changed.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:142: m_qosSupported = enable;
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> just return false here.
Changed.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:325: // We expect RegularWifiMac subclasses
which do support forwarding (e.g.,
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> RegularWifiMac??? please revise this part and make sure the code makes sense.
Changed

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:336: {
On 2011/07/15 16:48:00, Tom Henderson wrote:
> this is another example; monitor mode does not send packets.
Changed.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:364:
MonitorMacHigh::DeaggregateAmsduAndForward (Ptr<Packet> aggregatedPacket,
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> do you really need this method?
Modified.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.cc:383: WifiMacHeader hdr;
On 2011/07/15 16:48:00, Tom Henderson wrote:
> Likewise, my understanding is that MonitorMode does not ever send frames,
acks,
> or even associate with any access point.  These do not seem applicable.
Made appropriate changes.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high.h
File src/wifi/model/monitor-mac-high.h (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.h:18: * Author: Mathieu Lacage
<mathieu.lacage@sophia.inria.fr>
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> same comments as for monitor-mac-high.cc
Changed.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.h:41: 
Changed to MonitorWifiMac

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/monitor-mac-high....
src/wifi/model/monitor-mac-high.h:42: class MonitorMacHigh : public WifiMac
Changed.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac.cc
File src/wifi/model/regular-wifi-mac.cc (left):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac....
src/wifi/model/regular-wifi-mac.cc:437: // hurt to have it here too as a
backstop.
Sorry about the merge issues. Corrected.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac.cc
File src/wifi/model/regular-wifi-mac.cc (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac....
src/wifi/model/regular-wifi-mac.cc:156: m_edca.insert (std::make_pair(ac,
edca));
On 2011/07/15 16:48:00, Tom Henderson wrote:
> Please remove whitespace changes from the patch.
Corrected

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac.h
File src/wifi/model/regular-wifi-mac.h (left):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac....
src/wifi/model/regular-wifi-mac.h:43: * \ingroup wifi
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> why did you remove this?
Corrected.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/regular-wifi-mac....
src/wifi/model/regular-wifi-mac.h:325: RegularWifiMac (const RegularWifiMac &);
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> why did you delete these?
Corrected.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.cc
File src/wifi/model/wifi-net-device.cc (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.c...
src/wifi/model/wifi-net-device.cc:297: packet->Copy ()->RemoveHeader (llc);
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> Can you explain this?
Code has been modified. If not in monitor mode, the LLC, RadiotapHeader and
WifiMac are preserved because it confuses Ipv4Header removal. If in monitor
mode, those are removed and sent up the stack through WifiNetDevice.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.h
File src/wifi/model/wifi-net-device.h (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.h...
src/wifi/model/wifi-net-device.h:84: * \param monitor sets mode as monitor mode
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> you should not use "\param" here
Changed.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-net-device.h...
src/wifi/model/wifi-net-device.h:85: */
On 2011/07/15 16:48:00, Tom Henderson wrote:
> This doxygen could be improved by functionally describing what happens when
> MonitorMode is set (here, this means that LLC header is not stripped from
> packets that are forwarded up).
> 
> What happens if user sets this on a WifiNetDevice that is
> not using MacMonitorHigh?  You will not get all the frames.  Should this raise
> an error?  At the very least, I think you need to document somewhere that to
> make this work, your WifiNetDevice has to have this flag set and has to have a
> MacMonitorHigh mac underneath.
Modified.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-phy.h
File src/wifi/model/wifi-phy.h (right):

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-phy.h#newcod...
src/wifi/model/wifi-phy.h:152: * arg4: type of preamble used for packet.
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> you need to update the documentation. It would appreciate if you could take
this
> opportunity to make it doxygen-compliant
Done.

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-phy.h#newcod...
src/wifi/model/wifi-phy.h:154: typedef Callback<void,Ptr<Packet>, double,
WifiMode, enum WifiPreamble, RadiotapHeader> RxOkCallback;
On 2011/07/15 15:33:28, Nicola Baldo wrote:
> the SNR, the mode and the preamble are now redundant because the same data is
in
> RadiotapHeader. Can you investigate if it makes sense to remove these
redundant
> parameters?
I haven't worked on this yet. Will investigate and get back to you.

[ashwin.narayan89, 2011-07-19 09:51:51]

Hello,


> http://codereview.appspot.com/**4685048/diff/1/src/wifi/model/**
>
wifi-phy.h#newcode154<http://codereview.appspot.com/4685048/diff/1/src/wifi/model/wifi-phy.h#newcode154>
> src/wifi/model/wifi-phy.h:154: typedef Callback<void,Ptr<Packet>,
> double, WifiMode, enum WifiPreamble, RadiotapHeader> RxOkCallback;
> On 2011/07/15 15:33:28, Nicola Baldo wrote:
>
>> the SNR, the mode and the preamble are now redundant because the same
>>
> data is in
>
>> RadiotapHeader. Can you investigate if it makes sense to remove these
>>
> redundant
>
>> parameters?
>
> I managed to figure out a way to remove the SNR and the WifiPreamble from
here. I would be grateful if you could provide some directions on how I must
go about removing WifiMode as well.

I've uploaded a new patch set on http://codereview.appspot.com/4685048

WifiMode requires:
1) Bandwidth
 I'm not very sure about this, but I believed I could get the channel
frequency from the radiotap information and divide by 500,000. Would this
give the necessary bandwidth?

2) PhyRate, DataRate
I'm assuming one can get this using GetRate () and GetChannelFrequency ()
respectively.

3) I wish to know if it's possible to find out the following from the
Radiotap information:
 
CodeRate<http://www.nsnam.org/docs/release/3.11/doxygen/classns3_1_1_wifi_mode.html#a3da23b8bda23feda9e5ef2e43c6361ca>
  
ConstellationSize<http://www.nsnam.org/docs/release/3.11/doxygen/classns3_1_1_wifi_mode.html#ad65976005ffb7c08c9e6b5896d375a62>
 
UniqueName<http://www.nsnam.org/docs/release/3.11/doxygen/classns3_1_1_wifi_mode.html#a18e0a33034f484d3ca8c0497d3ea1dc7>
   
IsMandatory<http://www.nsnam.org/docs/release/3.11/doxygen/classns3_1_1_wifi_mode.html#ad7db9c41e193eb656c8dc66ec68cf0d0>
  
Uid<http://www.nsnam.org/docs/release/3.11/doxygen/classns3_1_1_wifi_mode.html#a430e2531c5ed9a955bf8511f9ad61df5>
ModulationClass<http://www.nsnam.org/docs/release/3.11/doxygen/classns3_1_1_wifi_mode.html#a252b5496b0719b445f50aadf75d0530d>
Thank you.

- Ashwin Narayan.

[Tom Henderson, 2011-07-19 14:32:40]

> 
> So it seems to me like we just need to enable the m_promisc flag in MacLow.
> What do you think Nicola, Tom?
> 
I don't know the architecture or monitor mode well enough to offer an informed
opinion.

[ashwin.narayan89, 2011-07-21 18:09:19]

Following the previous discussion, I've removed the m_monitorMode flag in MacLow

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

http://codereview.appspot.com/4685048/diff/1/src/wifi/model/mac-low.cc#newcod...
src/wifi/model/mac-low.cc:686: if (m_monitorMode)
The bypassing using the m_monitorMode flag has been removed, and MonitorWifiMac
invokes m_low->SetPromisc() as was agreed in the previous discussion.

[Nicola Baldo, 2011-07-22 09:07:00]

Please find some detailed comments on the code below. 

As a more generic comment, I think you should also update the wifi documentation
by adding a high-level description of what monitor mode is, what are its
intended use cases, and explaining how it can be used by ns-3 users in their
simulation programs.

http://codereview.appspot.com/4685048/diff/19001/RELEASE_NOTES
File RELEASE_NOTES (left):

http://codereview.appspot.com/4685048/diff/19001/RELEASE_NOTES#oldcode28
RELEASE_NOTES:28: SpectrumPropagationLossModel class already supported by
again, please be careful with the merging

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (left):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/mac-low.cc#ol...
src/wifi/model/mac-low.cc:665: MacLow::ReceiveOk (Ptr<Packet> packet, double
rxSnr, WifiMode txMode, WifiPreamble preamble)
Can you please re-introduce rxSnr and preamble as per our discussion on the
mailing list?

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/mac-low.cc#ne...
src/wifi/model/mac-low.cc:1857: double
after re-introducing the rxSnr param in MacLow::ReceiveOk, this should not be
needed any more

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/monitor-wifi-...
File src/wifi/model/monitor-wifi-mac.cc (right):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/monitor-wifi-...
src/wifi/model/monitor-wifi-mac.cc:309: return m_low->GetBssid ();
if it's not applicable, why do you return some value?

In general, can you please clarify whether you expect that BSSID and SSID will
ever be used by Monitor Mode or not? Not even when radiotap injection is
implemented?

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/monitor-wifi-...
File src/wifi/model/monitor-wifi-mac.h (right):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/monitor-wifi-...
src/wifi/model/monitor-wifi-mac.h:81: * The following functions have been
inherited from RegularWifiMac.
you mean WifiMac?

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/monitor-wifi-...
src/wifi/model/monitor-wifi-mac.h:102: * The following functions have been
inherited from RegularMacWifi
WifiMac?

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/wifi-net-devi...
File src/wifi/model/wifi-net-device.cc (right):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/wifi-net-devi...
src/wifi/model/wifi-net-device.cc:298: Ptr<Packet> pCopy = packet->Copy ();
I still don't get the point of this. You are copying the packet, then you remove
headers from the copy, then... the copy is deleted because pCopy is destroyed at
the end of this block?

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/wifi-net-devi...
src/wifi/model/wifi-net-device.cc:303: pCopy->RemoveHeader (llc);
I still don't get the point of this. You are copying the packet, then you remove
headers from the copy, then... the copy is deleted because pCopy is destroyed at
the end of this block?

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/wifi-net-devi...
File src/wifi/model/wifi-net-device.h (right):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/wifi-net-devi...
src/wifi/model/wifi-net-device.h:90: * \returns monitor mode state of device
please be clearer, i.e.: "returns true if..."

by the way:
- the naming of the method is not really compliant with our coding style 
- who is using this method? do you really need a getter?

[ashwin.narayan89, 2011-07-23 16:20:50]

Made corrections based on the comments. Reintroduced the redundant parameters
(SNR, WifiPreamble). Changed GetMonitorMode to IsMonitorMode.

http://codereview.appspot.com/4685048/diff/19001/RELEASE_NOTES
File RELEASE_NOTES (left):

http://codereview.appspot.com/4685048/diff/19001/RELEASE_NOTES#oldcode28
RELEASE_NOTES:28: SpectrumPropagationLossModel class already supported by
On 2011/07/22 09:07:00, Nicola Baldo wrote:
> again, please be careful with the merging
Corrected

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (left):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/mac-low.cc#ol...
src/wifi/model/mac-low.cc:665: MacLow::ReceiveOk (Ptr<Packet> packet, double
rxSnr, WifiMode txMode, WifiPreamble preamble)
Done

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/mac-low.cc#ne...
src/wifi/model/mac-low.cc:1857: double
Corrected

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/monitor-wifi-...
File src/wifi/model/monitor-wifi-mac.cc (right):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/monitor-wifi-...
src/wifi/model/monitor-wifi-mac.cc:309: return m_low->GetBssid ();
Yes, BSSID to be set in MacLow as it used by MacLow::NotifyNav (), which is used
in Monitor Mode. I don't SSID would be used though. Would it be okay to use
NS_ABORT_MSGs for these methods where I return values?

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/monitor-wifi-...
File src/wifi/model/monitor-wifi-mac.h (right):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/monitor-wifi-...
src/wifi/model/monitor-wifi-mac.h:81: * The following functions have been
inherited from RegularWifiMac.
Corrected

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/monitor-wifi-...
src/wifi/model/monitor-wifi-mac.h:102: * The following functions have been
inherited from RegularMacWifi
Corrected

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/wifi-net-devi...
File src/wifi/model/wifi-net-device.cc (right):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/wifi-net-devi...
src/wifi/model/wifi-net-device.cc:298: Ptr<Packet> pCopy = packet->Copy ();
The packet is essentially copied to remove the LLC header from it (which should
be preserved in the original packet). The LLC header is needed for getting some
information from it in the code to follow. One can't remove the LLC header
without stripping off the other headers. Thus, I had to use RemoveHeader for
rtHeader and wmHeader.

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/wifi-net-devi...
File src/wifi/model/wifi-net-device.h (right):

http://codereview.appspot.com/4685048/diff/19001/src/wifi/model/wifi-net-devi...
src/wifi/model/wifi-net-device.h:90: * \returns monitor mode state of device
Corrected. Changed method to IsMonitorMode. Hope that's okay.