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

Issue 119130043: New power and rate control mechanisms. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 8 months ago by Matías Richart
Modified:
7 years, 11 months ago
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

New power and rate control mechanisms.

Patch Set 1 #

Patch Set 2 #

Total comments: 31

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+645 lines, -537 lines) Patch
M examples/wireless/power-adaptation-distance.cc View 1 2 15 chunks +106 lines, -97 lines 0 comments Download
M examples/wireless/power-adaptation-interf.cc View 1 2 18 chunks +196 lines, -176 lines 0 comments Download
M src/wifi/model/aparf-wifi-manager.h View 1 2 4 chunks +46 lines, -12 lines 0 comments Download
M src/wifi/model/aparf-wifi-manager.cc View 1 2 5 chunks +139 lines, -132 lines 0 comments Download
M src/wifi/model/parf-wifi-manager.h View 1 2 3 chunks +38 lines, -6 lines 0 comments Download
M src/wifi/model/parf-wifi-manager.cc View 1 2 9 chunks +120 lines, -114 lines 0 comments Download

Messages

Total messages: 4
Tom Henderson
Sorry for the delay in reviewing this. This code appears to be in good shape ...
9 years, 4 months ago (2014-11-12 20:44:42 UTC) #1
Daniel L.
I looked through the code and didn't see anything major. Most of the things I ...
9 years, 2 months ago (2015-01-13 05:12:10 UTC) #2
Peter Barnes
Only major comment is to echo Tom's request for complete doxygen. Most of my inline ...
9 years, 2 months ago (2015-01-13 18:34:27 UTC) #3
Matías Richart
9 years, 2 months ago (2015-01-14 21:06:37 UTC) #4
On 2015/01/13 18:34:27, Peter Barnes wrote:
> Only major comment is to echo Tom's request for complete doxygen.
> 
> Most of my inline comments are fairly minor.
> 
>
https://codereview.appspot.com/119130043/diff/20001/examples/wireless/power-a...
> File examples/wireless/power-adaptation-distance.cc (right):
> 
>
https://codereview.appspot.com/119130043/diff/20001/examples/wireless/power-a...
> examples/wireless/power-adaptation-distance.cc:31: * ./waf --run
> "power-adaptation-distance --manager=ns3::AparfWifiManager
> --outputFileName=aparf"
> Consider using code blocks to set off the example commands:
> 
> * \code{.sh}
> *   ./waf …
> * \endcode
> 
>
https://codereview.appspot.com/119130043/diff/20001/examples/wireless/power-a...
> examples/wireless/power-adaptation-distance.cc:61: class APStatics
> class APStatistics?
> class ApStaStatistics? (AP and STA statistics)
> 
>
https://codereview.appspot.com/119130043/diff/20001/examples/wireless/power-a...
> examples/wireless/power-adaptation-distance.cc:339: APStatics atpCounter =
> APStatics(wifiApDevices, wifiStaDevices);
> I'm clueless:  why is atpCounter a reasonable variable name?  what is atp?
> 
>
https://codereview.appspot.com/119130043/diff/20001/examples/wireless/power-a...
> File examples/wireless/power-adaptation-interf.cc (right):
> 
>
https://codereview.appspot.com/119130043/diff/20001/examples/wireless/power-a...
> examples/wireless/power-adaptation-interf.cc:35: * ./waf --run
> "power-adaptation-interf --manager=ns3::AparfWifiManager
--outputFileName=aparf"
> Consider \code blocks.
> 
>
https://codereview.appspot.com/119130043/diff/20001/examples/wireless/power-a...
> examples/wireless/power-adaptation-interf.cc:64: class APStatics
> class APStatistics?
> class ApStaStatistics?
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> File src/wifi/model/aparf-wifi-manager.cc (right):
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.cc:57: static TypeId tid =
> TypeId("ns3::AparfWifiManager") .SetParent<WifiRemoteStationManager> ()
> Please begin all chained calls on separate line:
> static TypeId tid = TypeId (…)
>   .SetParent (…)
>   ….
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.cc:101:
> MakeTraceSourceAccessor(&AparfWifiManager::m_powerChange))
> Please add the fully qualified callback function signature typedef name:
> "ns3::AparfWifiManager::…"
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.cc:104:
> MakeTraceSourceAccessor(&AparfWifiManager::m_rateChange))
> Ditto.
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.cc:139: NS_LOG_DEBUG ("create station=" <<
> station << ", rate=" << station->m_rate << ", power=" <<
(int)station->m_power);
> Please break long lines.
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.cc:151: station->m_power = m_nPower - 1;
> Odd indentation in codereview.  Are there tabs here?  Please replace with
> spaces, or run the entire code through utils/check-style.py
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.cc:162: /**
> Consider marking this section as internal implementation details:
> /**
>  * \internal
>  * ...
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.cc:174: AparfWifiRemoteStation *station =
> (AparfWifiRemoteStation *) st;
> Should this be DynamicCast<AparfWifiRemoteStation>(st) ?
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.cc:223: NS_LOG_FUNCTION (this << station <<
> ctsSnr << ctsMode << rtsSnr); NS_LOG_DEBUG ("station=" << station << " rts
ok");
> Each statement on a separate line, please.
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.cc:230: AparfWifiRemoteStation *station =
> (AparfWifiRemoteStation *) st;
> DynamicCast ?
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> File src/wifi/model/aparf-wifi-manager.h (right):
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.h:38: * Networks, Springer, 2005, 12,
123-145
> Suggest adding url to paper.
> I think this link is freely available:
> http://link.springer.com/content/pdf/10.1007%2Fs10776-005-0006-x.pdf
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.h:90: TracedCallback<uint8_t, Mac48Address>
> m_powerChange;
> Please add a typedef for the callback function signature, documenting the
> meaning and units of the two arguments.
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
> src/wifi/model/aparf-wifi-manager.h:94: TracedCallback<uint32_t, Mac48Address>
> m_rateChange;
> Ditto.
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
> File src/wifi/model/parf-wifi-manager.cc (right):
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
> src/wifi/model/parf-wifi-manager.cc:75: MakeTraceSourceAccessor
> (&ParfWifiManager::m_powerChange))
> Fully-qualified callback signature typedef.
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
> src/wifi/model/parf-wifi-manager.cc:78: MakeTraceSourceAccessor
> (&ParfWifiManager::m_rateChange))
> Ditto.
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
> src/wifi/model/parf-wifi-manager.cc:115: NS_LOG_DEBUG ("create station=" <<
> station << ", timer=" << station->m_timer << ", rate=" << station->m_rate <<
",
> power=" << (int)station->m_power);
> Please break long lines.
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
> src/wifi/model/parf-wifi-manager.cc:127: station->m_power = m_nPower - 1;
> Indentation weirdness.
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
> src/wifi/model/parf-wifi-manager.cc:139: /**
> Consider \internal
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
> File src/wifi/model/parf-wifi-manager.h (right):
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
> src/wifi/model/parf-wifi-manager.h:37: * Wireless Networks, Kluwer Academic
> Publishers, 2007, 13, 737-755
> Link to paper:
>
http://dl.acm.org/ft_gateway.cfm?id=1080849&ftid=326992&dwn=1&CFID=618017405&...
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
> src/wifi/model/parf-wifi-manager.h:76: TracedCallback<uint8_t, Mac48Address>
> m_powerChange;
> Typedef the callback signature.
> 
>
https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
> src/wifi/model/parf-wifi-manager.h:80: TracedCallback<uint32_t, Mac48Address>
> m_rateChange;
> Ditto.

Thank you Daniel and Peter for the reviews.
I uploaded a new patch set taking care of most of the comments.

I have also corrected some styling issues, corrected some variable's names and
add the necessary comments to complete doxygen.

I have one doubt about a comment from Peter. I don't know if it is necessary to
use dynamic cast when casting from WifiRemoteStation to the specific sub class
like Parf or AparfWifiManager.
Because of how WifiRemoteStation is used I think there is no problem in using
the "regular" cast.
Also this is done this way in all other WifiManagers.
Sign in to reply to this message.

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