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

Issue 1926043: ns-3 Bug 978: Consolidate Wi-Fi MAC high functionality (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 7 months ago by Dean
Modified:
13 years, 4 months ago
Reviewers:
Nicola Baldo, and.kirill, nbaldo, Gary Pei
CC:
ns-3-reviews_googlegroups.com, Dean
Visibility:
Public.

Description

Initial patch. See ns-3 bug 978 for description and discussion.

Patch Set 1 #

Total comments: 13

Patch Set 2 : Draft 2 addressing some of Nicola's comments #

Patch Set 3 : Draft 3 adding a layer of inheritance between WifiMac and the rest #

Total comments: 2

Patch Set 4 : Draft 4 making RegularWifiMac::SetupEdcaQueue() protected per Gary's request #

Total comments: 28

Patch Set 5 : Draft 5 addressing Nicola's detailed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1908 lines, -4497 lines) Patch
M examples/flowmon/wifi-olsr-flowmon.py View 1 chunk +3 lines, -1 line 0 comments Download
M examples/mpi/third-distributed.cc View 1 chunk +7 lines, -5 lines 0 comments Download
M examples/routing/aodv.cc View 1 chunk +2 lines, -1 line 0 comments Download
M examples/routing/olsr-hna.cc View 1 chunk +2 lines, -1 line 0 comments Download
M examples/stats/wifi-example-sim.cc View 1 chunk +2 lines, -1 line 0 comments Download
M examples/tap/tap-wifi-dumbbell.cc View 1 chunk +5 lines, -3 lines 0 comments Download
M examples/tap/tap-wifi-virtual-machine.cc View 1 chunk +2 lines, -1 line 0 comments Download
M examples/tap/tap-wifi-virtual-machine.py View 1 chunk +2 lines, -1 line 0 comments Download
M examples/tutorial/third.cc View 1 chunk +7 lines, -5 lines 0 comments Download
M examples/wireless/mixed-wireless.cc View 2 chunks +9 lines, -5 lines 0 comments Download
M examples/wireless/mixed-wireless.py View 2 chunks +11 lines, -7 lines 0 comments Download
M examples/wireless/multirate.cc View 1 chunk +3 lines, -1 line 0 comments Download
M examples/wireless/simple-wifi-frame-aggregation.cc View 1 chunk +7 lines, -5 lines 0 comments Download
M examples/wireless/wifi-adhoc.cc View 1 chunk +2 lines, -1 line 0 comments Download
M examples/wireless/wifi-ap.cc View 1 chunk +7 lines, -4 lines 0 comments Download
M examples/wireless/wifi-ap.py View 1 chunk +9 lines, -6 lines 0 comments Download
M examples/wireless/wifi-blockack.cc View 1 chunk +7 lines, -2 lines 0 comments Download
M examples/wireless/wifi-clear-channel-cmu.cc View 1 chunk +2 lines, -1 line 0 comments Download
M examples/wireless/wifi-hidden-terminal.cc View 1 chunk +2 lines, -1 line 0 comments Download
M examples/wireless/wifi-simple-adhoc.cc View 1 chunk +2 lines, -1 line 0 comments Download
M examples/wireless/wifi-simple-adhoc-grid.cc View 1 chunk +2 lines, -1 line 0 comments Download
M examples/wireless/wifi-simple-infra.cc View 1 chunk +5 lines, -2 lines 0 comments Download
M examples/wireless/wifi-simple-interference.cc View 1 chunk +2 lines, -1 line 0 comments Download
M examples/wireless/wifi-wired-bridging.cc View 2 chunks +7 lines, -5 lines 0 comments Download
M src/devices/mesh/dot11s/airtime-metric.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M src/devices/mesh/mesh-wifi-interface-mac.h View 1 2 8 chunks +2 lines, -69 lines 0 comments Download
M src/devices/mesh/mesh-wifi-interface-mac.cc View 1 2 3 4 12 chunks +25 lines, -267 lines 0 comments Download
M src/devices/wifi/adhoc-wifi-mac.h View 1 2 3 4 2 chunks +11 lines, -71 lines 0 comments Download
M src/devices/wifi/adhoc-wifi-mac.cc View 1 2 3 4 4 chunks +112 lines, -237 lines 0 comments Download
M src/devices/wifi/ap-wifi-mac.h View 1 2 3 4 2 chunks +121 lines, -97 lines 0 comments Download
M src/devices/wifi/ap-wifi-mac.cc View 1 2 3 4 15 chunks +207 lines, -591 lines 0 comments Download
M src/devices/wifi/edca-txop-n.h View 3 chunks +4 lines, -2 lines 0 comments Download
M src/devices/wifi/msdu-aggregator.h View 1 chunk +1 line, -0 lines 0 comments Download
R src/devices/wifi/nqap-wifi-mac.h View 1 chunk +0 lines, -139 lines 0 comments Download
R src/devices/wifi/nqap-wifi-mac.cc View 1 chunk +0 lines, -621 lines 0 comments Download
R src/devices/wifi/nqsta-wifi-mac.h View 1 chunk +0 lines, -173 lines 0 comments Download
R src/devices/wifi/nqsta-wifi-mac.cc View 1 chunk +0 lines, -695 lines 0 comments Download
R src/devices/wifi/qadhoc-wifi-mac.h View 1 chunk +0 lines, -123 lines 0 comments Download
R src/devices/wifi/qadhoc-wifi-mac.cc View 1 chunk +0 lines, -572 lines 0 comments Download
A src/devices/wifi/regular-wifi-mac.h View 3 4 1 chunk +264 lines, -0 lines 0 comments Download
A src/devices/wifi/regular-wifi-mac.cc View 3 4 1 chunk +749 lines, -0 lines 0 comments Download
M src/devices/wifi/sta-wifi-mac.h View 1 2 3 4 2 chunks +58 lines, -116 lines 0 comments Download
M src/devices/wifi/sta-wifi-mac.cc View 1 2 3 4 13 chunks +194 lines, -552 lines 0 comments Download
M src/devices/wifi/wifi-mac.h View 1 2 3 chunks +18 lines, -14 lines 0 comments Download
M src/devices/wifi/wifi-mac.cc View 1 2 2 chunks +0 lines, -67 lines 0 comments Download
M src/devices/wifi/wifi-test.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M src/devices/wifi/wscript View 1 2 5 chunks +7 lines, -11 lines 0 comments Download
M src/helper/qos-wifi-mac-helper.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/helper/wifi-helper.cc View 1 2 3 4 2 chunks +3 lines, -5 lines 0 comments Download
M src/routing/aodv/test/aodv-regression.cc View 1 chunk +2 lines, -1 line 0 comments Download
M src/routing/aodv/test/bug-772.cc View 1 chunk +2 lines, -1 line 0 comments Download
M src/routing/aodv/test/loopback.cc View 1 chunk +2 lines, -1 line 0 comments Download
src/routing/olsr/test/bug780-test.cc View 1 chunk +2 lines, -1 line 0 comments Download
M src/routing/olsr/test/tc-regression-test.cc View 2 chunks +3 lines, -1 line 0 comments Download
M src/test/ns3wifi/wifi-interference-test-suite.cc View 2 chunks +3 lines, -1 line 0 comments Download
M src/test/ns3wifi/wifi-msdu-aggregator-test-suite.cc View 2 chunks +4 lines, -2 lines 0 comments Download

Messages

Total messages: 10
Nicola Baldo
a few detailed comments here. More generic comments on bugzilla. http://codereview.appspot.com/1926043/diff/1/41 File src/devices/wifi/sta-wifi-mac.cc (right): http://codereview.appspot.com/1926043/diff/1/41#newcode179 ...
13 years, 7 months ago (2010-08-27 13:45:37 UTC) #1
Dean
Nicola, Thanks for your attention on this and your comments. As a result I've made ...
13 years, 7 months ago (2010-09-02 21:48:07 UTC) #2
Nicola Baldo
Dean, thanks for the updated patch and for the many clarifications. Several of my doubts ...
13 years, 6 months ago (2010-09-06 12:40:18 UTC) #3
Dean
Nicola, Thanks for your comments; I've responded below. I haven't updated the issue with the ...
13 years, 6 months ago (2010-09-06 13:31:31 UTC) #4
Dean
All, I've updated the code at this issue along the lines that Nicola suggested in ...
13 years, 6 months ago (2010-09-16 19:28:25 UTC) #5
Gary Pei
http://codereview.appspot.com/1926043/diff/17001/src/devices/wifi/regular-wifi-mac.h File src/devices/wifi/regular-wifi-mac.h (right): http://codereview.appspot.com/1926043/diff/17001/src/devices/wifi/regular-wifi-mac.h#newcode322 src/devices/wifi/regular-wifi-mac.h:322: void SetupEdcaQueue (enum AcIndex ac); Can you move this ...
13 years, 6 months ago (2010-09-17 23:05:42 UTC) #6
Dean
Gary, Thanks for your comment. I've updated the code here per your request. Kind regards, ...
13 years, 6 months ago (2010-09-30 20:13:46 UTC) #7
Nicola Baldo
I reviewed as carefully as I could all the code. The most critical issue that ...
13 years, 5 months ago (2010-10-04 16:54:25 UTC) #8
Dean
Nicola, Thanks for this new set of useful comments. I believe I have addressed them ...
13 years, 5 months ago (2010-10-10 20:10:52 UTC) #9
nbaldo_cttc.es
13 years, 4 months ago (2010-11-09 17:39:33 UTC) #10
Hi Dean,

On 10/10/2010 10:10 PM, deanarm@gmail.com wrote:
 > Thanks for this new set of useful comments. I believe I have addressed
 > them in the latest patch.

I reviewed your latest patch. I think it is now satisfactory, so you 
have my +1 to merge, provided that the following is done:

1) Kirill should approve the changes on the mesh device

2) fix history + check-style.py, as you proposed (see below comments)

3) add a meaningful description to RELEASE_NOTES and CHANGES.html
This is very important since this change is a major one for the wifi 
device.


> I'll also shortly push the corresponding change to
> http://code.nsnam.org/deanarm/ns-3-dev-wifi-devel. The history is still
> not pretty in that repository, but if you think we are getting close to
> approval to push, then I'll look at tidying it up.

I would be fine with having just one big changeset labeled "bug 978". 
It's going to break things for many people, but I don't see how a more 
fine-grained history would be better. It could actually be worse, 
because with many incremental changes then people won't know exactly 
what to blame when they find a change that breaks their code. With a 
single change, they can be redirected to the overall description of the 
change (which should be in CHANGES.html and should have the same title 
of the commit message).

What do you think?


> I'll note that check-style.py points out a few issues in the files I've
> touched. I haven't fixed these up yet, but will do this as part of the
> final tidy before push, if that is acceptable.

yes absolutely.


Thank you very much for your good work!

Regards,

Nicola



>
> http://codereview.appspot.com/1926043/
>
Sign in to reply to this message.

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