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

Issue 254900043: Minstrel HT Rate Control

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

Description

Minstrel HT Rate Control

Patch Set 1 #

Total comments: 49
Unified diffs Side-by-side diffs Delta from patch set Stats (+1280 lines, -2 lines) Patch
M examples/wireless/ht-wifi-network.cc View 1 chunk +1 line, -2 lines 3 comments Download
A src/wifi/model/minstrel-ht-wifi-manager.h View 1 chunk +191 lines, -0 lines 0 comments Download
A src/wifi/model/minstrel-ht-wifi-manager.cc View 1 chunk +1086 lines, -0 lines 46 comments Download
M src/wifi/wscript View 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 7
S. Deronne
Ghada, This first implementation looks almost ok, I added comments inline. I also have few ...
3 years, 2 months ago (2015-07-19 17:20:45 UTC) #1
S. Deronne
Another comment is that we should ensure that non-HT stations are not using Minstrel HT ...
3 years, 2 months ago (2015-07-20 11:18:38 UTC) #2
Matías Richart
The implementation looks good. I agree with Sebastien comments and I added some more comments. ...
3 years, 2 months ago (2015-07-31 22:22:52 UTC) #3
S. Deronne
On 2015/07/31 22:22:52, Matías Richart wrote: > The implementation looks good. > I agree with ...
3 years, 2 months ago (2015-08-01 09:16:40 UTC) #4
S. Deronne
https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wifi-manager.cc File src/wifi/model/minstrel-ht-wifi-manager.cc (right): https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wifi-manager.cc#newcode243 src/wifi/model/minstrel-ht-wifi-manager.cc:243: if (GetShortGuardInterval(station) && GetNumberOfReceiveAntennas(station)==2) You are right. I propose ...
3 years, 2 months ago (2015-08-01 09:22:39 UTC) #5
gbadawy
I have replied to some of your comments. I will start working on them. Thanks ...
3 years, 2 months ago (2015-08-05 18:25:57 UTC) #6
Tom Henderson
3 years, 1 month ago (2015-08-18 20:25:51 UTC) #7
This is urgently needed IMO in the ns-3 WiFi model.  Since it is an extension of
existing Minstrel, I wonder whether we need a new class?  I'd just extend the
class that we have.

This needs tests, example(s), and documentation to complete, aside from
addressing the review comments.

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
File src/wifi/model/minstrel-ht-wifi-manager.cc (right):

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:20: * Some Comments:
this code (and the comments) is adapted from original ns-3 minstrel
implementation; copyright and author should be included from those original
files.

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:112:
.AddConstructor<MinstrelHtWifiManager> ()
.SetGroupName ("Wifi")

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:122: MakeDoubleChecker<double> ())
MakeDoubleChecker<double> (0, 100)

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:132: MakeDoubleChecker <double> ())
should be Uinteger type.  Actually should be deleted because it has no usage in
the model.

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:137: MakeDoubleChecker <double> ())
should be Uinteger type

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:140: DoubleValue (65536),
On 2015/08/05 18:25:57, gbadawy wrote:
> On 2015/07/19 17:20:45, S. Deronne wrote:
> > Why not 1200 bytes?
> 
> Because I assume here A-MPDU will be used most of the time so I thought 64K
> would be a better representation
Linux appears to use 1200
http://lxr.free-electrons.com/source/net/mac80211/rc80211_minstrel_ht.c?v=3.19

Also, why is this not an Uinteger value (and force the arithmetic to double
type)?

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:142: MakeDoubleChecker <double> ())
You can enforce min/max values like this: MakeUintegerChecker<uint16_t> (min,
max)

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:149: m_nsupported = 0;
make into member initializer

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:160: uint32_t nModes = phy->GetNMcs
();
NS_LOG_FUNCTION() log statements missing throughout

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:175: preamble= WIFI_PREAMBLE_LONG;
On 2015/08/05 18:25:57, gbadawy wrote:
> On 2015/07/19 17:20:45, S. Deronne wrote:
> > HT rates should only use MF or GF preambles, so I assume there is something
> > wrong here...
> 
> Just in case a non HT node is using minstrelHT? should I add an error?
> I didn't consider GF and probably won't bother with it now since it is dropped
> in later standards

Agree to not support GF

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:199: m_calcTxTime.push_back
(std::make_pair (t, mode));
can a mode be accidentally added twice?  should it be instead a std::map with
checking that modes not being added multiple times?

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:226: station->m_txstreams = 1;
can phy be used to initialize these to better initial values?  (is
WifiRemoteStationManager::GetPhy() valid when this method is called?)

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:349:
station->m_mcsTable[groupid].m_minstrelTable[rateid].numRateAttempt++; // for
some reason kept track at FinalDataFail!!!
I don't understand the comment, is it still valid?

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:492:
//station->m_minstrelTable[station->m_txrate].numRateAttempt +=
station->m_retry;
delete comment

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:693: Ptr<UniformRandomVariable>
coinFlip = CreateObject<UniformRandomVariable> ();
we will not want to create random variables this way.  We need a random variable
as a member variable, and we need to be able to AssignStreams() to it.

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:768: NS_LOG_DEBUG ("FindRate " <<
"sample rate=" << idx);
suggest to move log above; either FindRate finds a new rate, or continue to use
the best rate (i.e. two separate log statements)

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:806: /// calculate the perfect tx
time for this rate
how is perfect tx time defined?

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:824: * assume probability scales from
0 to 18000
where does value of 18000 come from?  Should it be a defined constant?

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:1035: Ptr<UniformRandomVariable> uv =
CreateObject<UniformRandomVariable> ();
again, we ought to use a member uniform random variable and scale the samples
drawn from it as needed.

https://www.codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-h...
src/wifi/model/minstrel-ht-wifi-manager.cc:1053:
MinstrelHtWifiManager::PrintSampleTable (MinstrelHtWifiRemoteStation *station)
recommend to instead pass in a ostream that can be assigned to cout or to files
as needed.
Sign in to reply to this message.

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