New power and rate control mechanisms.

New power and rate control mechanisms.

[Tom Henderson, 2014-11-12 20:44:42]

Sorry for the delay in reviewing this.

This code appears to be in good shape and a candidate for our next release, so I
would like to suggest the following steps (since I'm the release manager for
ns-3.22):

- have our wifi maintainer review it and submit any comments
- I will then create a repository on code.nsnam.org to stage this code
- I will then create a checklist of items for Matias to work through, to finish
this up.  I expect these will include:
1) complete the doxygen so that there are no warnings
2) create a test suite based on the provided examples, for future regression
testing
3) create some description of these new rate controls for the WiFi documentation
(.rst file in the model-library)

[Daniel L., 2015-01-13 05:12:10]

I looked through the code and didn't see anything major.  Most of the things I
commented are just styles so you may disregard them if you don't agree with the
comments.  I generally like the variable names to be clear even if they are
longer.

For example, I think m_success may be better named m_nSuccess to emphasize that
it is a counter (at first glance I thought it is a bool).

Other than the styles, I think the code is ready to go.

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:36: uint32_t m_failed;
Are these counters? If so, m_nSuccess/m_nFailed may be better as
m_success/m_failed may look like bool.

https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
src/wifi/model/aparf-wifi-manager.cc:37: uint32_t m_pCount;
May be a good idea to reference to the corresponding value in the paper?

https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/aparf-wifi...
src/wifi/model/aparf-wifi-manager.cc:46: uint32_t m_nsupported;
m_nSupported

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:40: uint32_t m_failed;
m_nSuccess/m_nFailed

https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
src/wifi/model/parf-wifi-manager.cc:42: bool m_recoveryPower;
m_usingRecoveryRate/m_usingRecoveryPower might be better

https://codereview.appspot.com/119130043/diff/20001/src/wifi/model/parf-wifi-...
src/wifi/model/parf-wifi-manager.cc:43: uint32_t m_retry;
m_nRetry?

[Peter Barnes, 2015-01-13 18:34:27]

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.

[Matías Richart, 2015-01-14 21:06:37]

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.