Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5161)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 254440043: Add RRPAA, a new power and rate control mechanism

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

Description

Add RRPAA, a new power and rate control mechanism

Patch Set 1 #

Total comments: 36

Patch Set 2 : Fix all comments from reviewers #

Patch Set 3 : Fix doxygen warnings #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1527 lines, -182 lines) Patch
M examples/wireless/examples-to-run.py View 1 1 chunk +1 line, -0 lines 0 comments Download
M examples/wireless/power-adaptation-distance.cc View 1 2 12 chunks +38 lines, -43 lines 0 comments Download
M examples/wireless/power-adaptation-interference.cc View 1 2 13 chunks +39 lines, -47 lines 0 comments Download
M src/test/traced/traced-callback-typedef-test-suite.cc View 1 1 chunk +4 lines, -4 lines 0 comments Download
M src/wifi/model/aparf-wifi-manager.h View 1 2 1 chunk +4 lines, -4 lines 0 comments Download
M src/wifi/model/aparf-wifi-manager.cc View 1 8 chunks +53 lines, -38 lines 0 comments Download
M src/wifi/model/parf-wifi-manager.h View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M src/wifi/model/parf-wifi-manager.cc View 1 2 9 chunks +45 lines, -29 lines 0 comments Download
A src/wifi/model/rrpaa-wifi-manager.h View 1 2 1 chunk +239 lines, -0 lines 0 comments Download
A src/wifi/model/rrpaa-wifi-manager.cc View 1 1 chunk +644 lines, -0 lines 0 comments Download
M src/wifi/model/wifi-phy.h View 1 2 2 chunks +10 lines, -10 lines 0 comments Download
M src/wifi/model/wifi-remote-station-manager.h View 1 2 2 chunks +7 lines, -4 lines 0 comments Download
M src/wifi/test/power-rate-adaptation-test.cc View 1 2 3 chunks +438 lines, -0 lines 0 comments Download
M src/wifi/wscript View 1 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 7
S. Deronne
I did not go through it yet, I just had a very quick look. https://codereview.appspot.com/254440043/diff/1/src/wifi/model/rrpaa-wifi-manager.cc ...
8 years ago (2016-03-31 20:39:46 UTC) #1
Matías Richart
https://codereview.appspot.com/254440043/diff/1/src/wifi/model/rrpaa-wifi-manager.cc File src/wifi/model/rrpaa-wifi-manager.cc (right): https://codereview.appspot.com/254440043/diff/1/src/wifi/model/rrpaa-wifi-manager.cc#newcode49 src/wifi/model/rrpaa-wifi-manager.cc:49: uint32_t m_rtsWnd; //!< Window size for the ARts mechanism. ...
8 years ago (2016-04-01 09:41:48 UTC) #2
Tom Henderson
In general, two main things to improve: 1) random variable implementation needs to be aligned ...
8 years ago (2016-04-01 14:56:41 UTC) #3
Matías Richart
Hi. Thanks for the review. I fixed most issues. In the comments I put a ...
8 years ago (2016-04-05 11:22:22 UTC) #4
Tom Henderson
> > https://codereview.appspot.com/254440043/diff/1/src/wifi/model/rrpaa-wifi-manager.cc#newcode243 > src/wifi/model/rrpaa-wifi-manager.cc:243: station->m_pdTable[i][j] = 1; > On 2016/04/01 14:56:40, Tom Henderson wrote: ...
7 years, 9 months ago (2016-07-25 12:20:00 UTC) #5
Matías Richart
On 2016/07/25 12:20:00, Tom Henderson wrote: > > > > > https://codereview.appspot.com/254440043/diff/1/src/wifi/model/rrpaa-wifi-manager.cc#newcode243 > > src/wifi/model/rrpaa-wifi-manager.cc:243: ...
7 years, 8 months ago (2016-07-27 17:01:45 UTC) #6
Matías Richart
7 years, 2 months ago (2017-01-31 22:03:51 UTC) #7 
On 2016/07/27 17:01:45, Matías Richart wrote:
> On 2016/07/25 12:20:00, Tom Henderson wrote:
> > > 
> > >
> >
>
https://codereview.appspot.com/254440043/diff/1/src/wifi/model/rrpaa-wifi-man...
> > > src/wifi/model/rrpaa-wifi-manager.cc:243: station->m_pdTable[i][j] = 1;
> > > On 2016/04/01 14:56:40, Tom Henderson wrote:
> > > > can you add NS_LOG_DEBUG() statements to output values to which the
table
> is
> > > > initialized?
> > > The table is initialized with all 1s.
> > > Why do you think is necessary to output this?
> > 
> > OK, perhaps there is not necessary.  If you have, further down in this
method,
> > added NS_LOG_DEBUG() around table state changes, that is probably enough.
> 
> Ok. I added NS_LOG_DEBUG() in table changes.
> 
> > 
> > > 
> > >
> >
>
https://codereview.appspot.com/254440043/diff/1/src/wifi/model/rrpaa-wifi-man...
> > > src/wifi/model/rrpaa-wifi-manager.cc:368: return WifiTxVector
(GetSupported
> > > (station, station->m_rate), station->m_power, GetLongRetryCount (station),
> > > GetShortGuardInterval (station), Min (GetNumberOfReceiveAntennas
> > > (station),GetNumberOfTransmitAntennas ()), GetNumberOfTransmitAntennas
> > > (station), GetAggregation (station), GetStbc (station));
> > > On 2016/04/01 14:56:40, Tom Henderson wrote:
> > > > WifiPhy::GetSupportedTxSpatialStreams() can be used instead of the
"Min()"
> > > > above.
> > > 
> > > I align this to what is done in the others rate adaptation algorithms
> > > implemented in NS3 that do not support 802.11n/ac
> > > That is to always return 1 tx stream.
> > > 
> > 
> > OK
> > 
> > >
> >
>
https://codereview.appspot.com/254440043/diff/1/src/wifi/model/rrpaa-wifi-man...
> > > src/wifi/model/rrpaa-wifi-manager.h:93: typedef void
> > > (*RateChangeTracedCallback)(const uint32_t rate, const Mac48Address
> > > remoteAddress);
> > > On 2016/04/01 14:56:41, Tom Henderson wrote:
> > > > why not make this a single callback that is triggered whenever either
rate
> > > > and/or power changes?
> > > 
> > > And have both parameters in the same function? Something like this? 
> > > typedef void (*RatePowerChangeTracedCallback)(const uint32_t rate, const
> > uint8_t
> > > power, const Mac48Addre ss remoteAddress);
> > > 
> > > The only disadvantage I see is that the user of this callback would need
to
> > keep
> > > track of previous rates and powers to see which parameter actually
changed.
> > > Do you think this is better?
> > > 
> > 
> > Looking at it again today, I agree that it is probably easier on the user to
> > separate it as you have.  However, what do you think about passing both the
> old
> > rate and the new rate (or old power and new power) values like is done for
> > TracedValue?
> 
> I think it's a good idea. I will also need to change the same
> callbacks in ParfWifiManager and AparfWifiManager so we can keep using
> the same simulation scripts easily for all the power managers.
> 
> > 
> > Please also document in the doxygen what are the units for the power and
rate
> > values.  Also in the RrpaaWifiRemoteStation struct doxygen.
> > 
> > Also, finally, did you consider to use the ns-3 DataRate class instead of a
> > uint32_t?
> 
> Now the callbacks report the rate index and power level (no units).
> Then, in the simulation script this is used to obtain the dBm and the 
> WifiMode.
> 
> For the power, I will improve this to report the dBm directly. I think it is 
> more useful for the user.
> 
> For the rate, I'm doubtful of which is the better option.
> In Ideal and MinstelHt we report it as bit rate using a uint64_t.
> We could also use the DataRate class easily.
> However, we could also use WifiMode which gives more information and could
> be more useful. In the example script I propose I use the WifiMode to
calculate
> some
> data.
> What do you think?

The new patch set fixes all previous comments and includes updates to match the
last changes of ns-3-dev.

I updated the trace callbacks for rate and power as suggested by Tom in the last
comment.
To maintain coherence and be able to use the same examples I needed to change
also the tracing in parf and aparf.
I also needed to change the method GetPowerDbm in wifi-phy to public so as to
reuse the code.
Sign in to reply to this message.

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