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

Issue 5552055: Continuing Ashwin Narayans nsclick monitor-mode like interface integration

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

Description

Ashwin's description: This work involves full radiotap header support for ns-3's
Wifi model. It includes the provision of a MonitorWifiMac abstraction and
support for a monitor mode. A click-based script was also developed for the
purpose of testing the working of the monitor mode by means of setting TX
parameters in the radiotap header for the packet through the click script. Frame
injection is also supported and the parameters that can be controlled are TX
power, RTS retries and DATA retries.

===

Briefly summarized i add a correct implementation of per packet tx parameters,
feedback packets and some more examples on top of that (and some smaller
fixes).


Patchset 1
==========
Patchset 1 is Ashwin Narayan's work. It already had multiple reviews (latest see
http://codereview.appspot.com/4890044/ ).
My merge was based off Ashwin Narayan's latest commit in his public repository
(http://code.nsnam.org/ashwin/ns-3-click-mac-dev/rev/0011207456f5), merged into
ns3-dev as of 30th January 2012. 

However there were some really strange merge artefacts and changes outside of
Ashwins work scope, so although i have double checked, please, if possible, have
a look again that i did not miss anything or took something wrong .. 

Patchset 2
==========
Just a small fix to clear FRAME_FLAG_FCS_INCLUDED when the FCS is taken off,
else Click for example will cut off FCS with RadiotapDecap although it is not
there anymore (use demo-datafb in Patchset7 to test that).

Patchset 3
==========
Just a very small patchset correcting some debug output i used. This is slightly
out of this issue, sorry. And maybe can be handled somewhere else in NS_LOG code
directly?! Because i think the default case is that we want to have uint8 output
as numbers or hex, but not as chars.

Patchset 4
==========
Introducing tx feedback which is common for monitor devices. It is realized in
MonitorWifiMac via the TxOk and TxFailed callbacks of the txop. The callbacks
were extended to also carry the packet back up and the txops had to be extended
to also call the TxOk callback for broadcast packets (however the optimal
location would be to call that at the end of the transmission, not at the start
as it is here, is this easily possible?)
At the momend the tx feedback is passed with an empty RadiotapHeader because the
RadiotapHeader implementation is still lacking some things.

Patchset 5
==========
Another very small patch set to make Tags appendable to ostreams.

Patchset 6
==========
Patchset which implements  per packet tx parameters via radiotap headers
slightly different as Ashwin due to the problems mentioned on the mailing list (
it worked only with OnoeRateManager, the only one which has IsLowLatency=false,
and it worked only for destinations which have not the group bit set due to the
implementation in WifiRemoteStationManager::PrepareForQueue and
WifiRemoteStationManager::Get***TxVector).

A RadiotapTxTag is introduced which is essentially a copy of the tx parameters
of a RadiotapHeader only. It is added in MonitorWifiMac::Enqueue and executed in
WifiRemoteStationManager::GetDataTxVector.

I do not like the duplication of code in RadoptapTxTag (vs RadiotapHeader),
however i think it's only avoidable if we allow some kind of multiple
inheritance or mixin style here. A totally different route would be to pass the
RadiotapHeader itself a little bit more down the stack, avoiding the
RadiotapTxTag completely (because now we have 3 classes carrying that
information, RadiotapHeader first, then RadiotapTxTag and then WifiTxVector),
however this would mean passing a RadiotapHeader around in a lot of places,
although there is just one "user" currently (the MonitorWifiMac).

Patchset 7
==========
5 examples which test and demonstrate some of Click's functionality: 
basic scheduling, sending a packet from one Click instance to another, setting
tx rate and power.

Patchset 8
==========
- Ashwins radiotap header extension missed some things. I was afraid that having
more and more fields in there would make it harder with the "leave no
gaps"-rule, so i rewrote it to handle padding correctly in all cases. In the end
i realize it still will be hard to produce an example which requires padding,
but being correct in all cases for reading and writing is still nice. Updated
other parts then concerning tx feedbacks packets.

- Took back patchset 4/5 from http://codereview.appspot.com/4890044/ in
yans-wifi-phy. Firstly it introduced a bug because m_state was not properly
updated for control frames anymore. Secondly i think the idea was wrong at all,
control frames should be visible, they were visible before and they are even in
some pcap regression traces. 

Patchset 9
==========
 - yans-wifi-phy: wifimacheader stripped off for trace is wrong (and not needed
anymore anyway)
 - yans-wifi-phy-helper: pcap output needs to strip off the radiotapheader for
DLT_IEEE802_11
 - mesh-wifi-interface-mac had its signature not updated leading to not
overwriting RegularWifiMac::Receive anymore (where is C++11's override!)
 - wifi-net-device: m_monitor was uninitialized, lucky that this triggered a
regression sometimes
 - put back lost line in wifi-phy-test back in 


./test.py reports no crashes/fails with patchset 9

Still TODO
==========
• Updating doc, CHANGES etc..



Patch Set 1 #

Total comments: 7

Patch Set 2 : bugfix: FRAME_FLAG_FCS_INCLUDED is cleared now when FCS is taken off #

Patch Set 3 : bugfix: uint8_t unconverted is output as char (not desired) #

Patch Set 4 : tx feedback for monitor devices #

Total comments: 2

Patch Set 5 : making tags appendable to ostreams #

Total comments: 3

Patch Set 6 : added RadiotapTxTag, adopted MonitorWifiMac and WifiRemoteStationManager, setting rates now possible #

Patch Set 7 : added 5 examples for testing nsclick lowlevel / radiotap stuff and fixed a warning #

Patch Set 8 : rewriting radiotap and continueing tx feedback #

Patch Set 9 : fixing other several problems #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -9 lines) Patch
M src/mesh/model/mesh-wifi-interface-mac.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M src/mesh/model/mesh-wifi-interface-mac.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M src/wifi/examples/wifi-phy-test.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M src/wifi/helper/yans-wifi-helper.cc View 1 2 3 4 5 6 7 8 1 chunk +7 lines, -2 lines 0 comments Download
M src/wifi/model/wifi-net-device.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M src/wifi/model/yans-wifi-phy.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -3 lines 0 comments Download

Messages

Total messages: 7
blightblu
Hi, as stated in the previous post to ns-3-devel i'd like to get the code ...
2 years, 2 months ago #1
blightblu
http://codereview.appspot.com/5552055/diff/1/src/wifi/examples/wifi-phy-test.cc File src/wifi/examples/wifi-phy-test.cc (right): http://codereview.appspot.com/5552055/diff/1/src/wifi/examples/wifi-phy-test.cc#newcode119 src/wifi/examples/wifi-phy-test.cc:119: seems to be a merge accident
2 years, 2 months ago #2
Nicola Baldo
Patchset 1: Ashwin's patch is in general ok with me, though I have to say ...
2 years, 2 months ago #3
Nicola Baldo
On 2012/02/05 13:16:23, Nicola Baldo wrote: > Patchset 5: > Well... I don't like it. ...
2 years, 2 months ago #4
blightblu
Thanks Nicola. Short answers here, regarding the RadiotapTxTag change i replied to your ns-3-dev post. ...
2 years, 2 months ago #5
Mathieu Lacage
the change to tags looks good to me http://codereview.appspot.com/5552055/diff/60/src/network/model/tag.cc File src/network/model/tag.cc (right): http://codereview.appspot.com/5552055/diff/60/src/network/model/tag.cc#newcode38 src/network/model/tag.cc:38: return ...
2 years, 2 months ago #6
blightblu
2 years, 2 months ago #7
Hi,

added latest patches. I can reorder them or upload all as one patchset when we
agree mostly about the changes and final polishing has been done. But for the
moment: 

Patchset 8
==========
- Ashwins radiotap header extension missed some things. I was afraid that having
more and more fields in there would make it harder with the "leave no
gaps"-rule, so i rewrote it to handle padding correctly in all cases. In the end
i realize it still will be hard to produce an example which requires padding,
but being correct in all cases for reading and writing is still nice. Updated
other parts then concerning tx feedbacks packets.

- Took back patchset 4/5 from http://codereview.appspot.com/4890044/ in
yans-wifi-phy. Firstly it introduced a bug because m_state was not properly
updated for control frames anymore. Secondly i think the idea was wrong at all,
control frames should be visible, they were visible before and they are even in
some pcap regression traces. 

Patchset 9
==========
 - yans-wifi-phy: wifimacheader stripped off for trace is wrong (and not needed
anymore anyway)
 - yans-wifi-phy-helper: pcap output needs to strip off the radiotapheader for
DLT_IEEE802_11
 - mesh-wifi-interface-mac had its signature not updated leading to not
overwriting RegularWifiMac::Receive anymore (where is C++11's override!)
 - wifi-net-device: m_monitor was uninitialized, lucky that this triggered a
regression sometimes
 - put back lost line in wifi-phy-test


./test.py reports no crashes/fails with patchset 9
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1274:9b9295c7fd64