Issue 254900043: Minstrel HT Rate Control
|
Created:
9 years, 10 months ago by gbadawy Modified:
9 years, 8 months ago CC:
ns-3-reviews_googlegroups.com Visibility:
Public. |
DescriptionMinstrel HT Rate Control
Patch Set 1 #
Total comments: 49
MessagesTotal messages: 7
Ghada, This first implementation looks almost ok, I added comments inline. I also have few general comments: - you should run a check-style. - implementation fails when short guard enabled is used! - I do not see where the PHY will turn on/off the short guard interval based on what HT Minstrel decides. The current implementation is that, if the user selects short guard interval, only short guard interval rates will be picked. Same comment could be raised for channel width selection. https://codereview.appspot.com/254900043/diff/1/examples/wireless/ht-wifi-net... File examples/wireless/ht-wifi-network.cc (right): https://codereview.appspot.com/254900043/diff/1/examples/wireless/ht-wifi-net... examples/wireless/ht-wifi-network.cc:269: wifi.SetRemoteStationManager ("ns3::MinstrelHtWifiManager"); This is not ok, purpose of this script is to measure thoughput for each rate => bitrate should be fixed here! I suggest to write a new example script which illustrates HT Minstrel. https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... File src/wifi/model/minstrel-ht-wifi-manager.cc (right): https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:28: * 3) 40Mhz can't fall back to 20MHz What are the reasons for this assumption? https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:140: DoubleValue (65536), Why not 1200 bytes? https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:163: //WifiMode mode = phy->GetMode (i); Please remove instead of commenting https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:175: preamble= WIFI_PREAMBLE_LONG; HT rates should only use MF or GF preambles, so I assume there is something wrong here... https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:556: station->m_txrate = 0; Should it be added to normal Minstrel as well? https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:566: //if station->txrate == something then this is a 2x2 stream This comment is not clear https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:638: MinstrelHtWifiManager::GetStreams (uint32_t groupId, MinstrelHtWifiRemoteStation *station) Why up to 2 streams are supported? https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:660: sampleindex = station->m_sampleTable[station->m_mcsTable[station->m_sampleGroup].m_index][station->m_mcsTable[station->m_sampleGroup].m_col]; This is not really readable https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:777: rate = (groupid*8)+ index; This is not ok -> return MCS index higher than 7 when SGI is enabled! https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:983: //RateInit (station); to be removed?
Sign in to reply to this message.
Another comment is that we should ensure that non-HT stations are not using Minstrel HT (there is no check in the current implementation) and it should also be avoided that HT stations choose non-HT RAA.
Sign in to reply to this message.
The implementation looks good. I agree with Sebastien comments and I added some more comments. As a side comment, when trying to test it I notice that multiple streams are not supported in yans-wifi-phy, only 8 mcs indexes are supported. And also in wifi-remote-station-manager, the number of tx and rx antennas are harcoded to 1. Sebastien, is someone working on this? https://codereview.appspot.com/254900043/diff/1/examples/wireless/ht-wifi-net... File examples/wireless/ht-wifi-network.cc (right): https://codereview.appspot.com/254900043/diff/1/examples/wireless/ht-wifi-net... examples/wireless/ht-wifi-network.cc:269: wifi.SetRemoteStationManager ("ns3::MinstrelHtWifiManager"); On 2015/07/19 17:20:45, S. Deronne wrote: > This is not ok, purpose of this script is to measure thoughput for each rate => > bitrate should be fixed here! > I suggest to write a new example script which illustrates HT Minstrel. I agree with Sebastien comment. Perhaps you can use the example rate-adaptation-distance as inspiration. https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... File src/wifi/model/minstrel-ht-wifi-manager.cc (right): https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:161: for (uint32_t i = 0; i < nModes; i++) Also all the supported groups should be considered to generate all the posible modes. https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:243: if (GetShortGuardInterval(station) && GetNumberOfReceiveAntennas(station)==2) Why the limit of 2? If I'm correct 802.11n allows up to 4 antennas. https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:521: station->m_mcsTable[groupid].m_minstrelTable[rateid].numRateAttempt++; Already counted in ReportDataFail? https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:567: return WifiTxVector (GetPhy()->McsToWifiMode(GetMcsSupported (station, station->m_txrate)), GetDefaultTxPowerLevel (), st->m_slrc, st->m_state->m_shortGuardInterval,station->m_txstreams, st->m_state->m_tx, st->m_state->m_stbc); Here we should consider the GI selected by Minstrel or change it in the phy, if not, McsToWifiMode will not give the correct mode. https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:1010: station->m_mcsTable[j].m_minstrelTable[i].adjustedRetryCount = 1; Why not using the same algorithm as in Minstrel for getting the correct retries?
Sign in to reply to this message.
On 2015/07/31 22:22:52, Matías Richart wrote: > The implementation looks good. > I agree with Sebastien comments and I added some more comments. > > As a side comment, when trying to test it I notice that multiple streams are not > supported in yans-wifi-phy, only 8 mcs indexes are supported. > And also in wifi-remote-station-manager, the number of tx and rx antennas are > harcoded to 1. Sebastien, is someone working on this? MIMO is not yet supported in ns-3-dev. A first implementation has been started at UW (Tom's team). You can check in the LAA repository. Basically, they did an approximation based on TGn curves and translating them to tables, for channel model D (supports up to 2 streams for the moment)
Sign in to reply to this message.
https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... File src/wifi/model/minstrel-ht-wifi-manager.cc (right): https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:243: if (GetShortGuardInterval(station) && GetNumberOfReceiveAntennas(station)==2) You are right. I propose we extend up to 4 streams, so that it could be used once MIMO is "fully" implemented.
Sign in to reply to this message.
I have replied to some of your comments. I will start working on them. Thanks for the review :) https://codereview.appspot.com/254900043/diff/1/examples/wireless/ht-wifi-net... File examples/wireless/ht-wifi-network.cc (right): https://codereview.appspot.com/254900043/diff/1/examples/wireless/ht-wifi-net... examples/wireless/ht-wifi-network.cc:269: wifi.SetRemoteStationManager ("ns3::MinstrelHtWifiManager"); On 2015/07/31 22:22:52, Matías Richart wrote: > On 2015/07/19 17:20:45, S. Deronne wrote: > > This is not ok, purpose of this script is to measure thoughput for each rate > => > > bitrate should be fixed here! > > I suggest to write a new example script which illustrates HT Minstrel. > > I agree with Sebastien comment. Perhaps you can use the example > rate-adaptation-distance as inspiration. I agree with both of you I just wanted a quick check to make sure that no major issues are there. I will write a new test https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... File src/wifi/model/minstrel-ht-wifi-manager.cc (right): https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:140: DoubleValue (65536), 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 https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:161: for (uint32_t i = 0; i < nModes; i++) On 2015/07/31 22:22:53, Matías Richart wrote: > Also all the supported groups should be considered to generate all the posible > modes. Here McsToWifiMode will return the mode according to the Phy parameters but you are right a node that uses short guard interval should generate modes for short guard and long guard intervals. Sebastien should I wait for you to finish updating WifiMode and see what changes you did there or should I add the logic here assuming the normal WifiMode representation? https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:163: //WifiMode mode = phy->GetMode (i); On 2015/07/19 17:20:45, S. Deronne wrote: > Please remove instead of commenting Done. https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:175: preamble= WIFI_PREAMBLE_LONG; 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 https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:243: if (GetShortGuardInterval(station) && GetNumberOfReceiveAntennas(station)==2) On 2015/08/01 09:22:39, S. Deronne wrote: > You are right. > I propose we extend up to 4 streams, so that it could be used once MIMO is > "fully" implemented. I don't remember why the limit of 2 will change it to accommodate for higher numbers https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:556: station->m_txrate = 0; On 2015/07/19 17:20:45, S. Deronne wrote: > Should it be added to normal Minstrel as well? Probably some stations wouldn't be initialized and I needed the check to avoid an error https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:566: //if station->txrate == something then this is a 2x2 stream On 2015/07/19 17:20:45, S. Deronne wrote: > This comment is not clear I need to add an if statement here to update the txvector to 2 streams depending on the data rate. Note that txvector carry m_txstreams for number of streams which is a function of the node not the tx mode. This implementation doesn't truly support 2x2 streams since MIMO isn't implemented in NS3. https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:567: return WifiTxVector (GetPhy()->McsToWifiMode(GetMcsSupported (station, station->m_txrate)), GetDefaultTxPowerLevel (), st->m_slrc, st->m_state->m_shortGuardInterval,station->m_txstreams, st->m_state->m_tx, st->m_state->m_stbc); On 2015/07/31 22:22:53, Matías Richart wrote: > Here we should consider the GI selected by Minstrel or change it in the phy, if > not, McsToWifiMode will not give the correct mode. True I will work on that. Same goes for the number of streams as per the comment above https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:638: MinstrelHtWifiManager::GetStreams (uint32_t groupId, MinstrelHtWifiRemoteStation *station) On 2015/07/19 17:20:45, S. Deronne wrote: > Why up to 2 streams are supported? It only has to do with the simulation I was interested in. Again multiple streams aren't fully implemented and tested since there is no MIMO yet in ns3. I just have the framework to make it easy to expand https://codereview.appspot.com/254900043/diff/1/src/wifi/model/minstrel-ht-wi... src/wifi/model/minstrel-ht-wifi-manager.cc:1010: station->m_mcsTable[j].m_minstrelTable[i].adjustedRetryCount = 1; On 2015/07/31 22:22:53, Matías Richart wrote: > Why not using the same algorithm as in Minstrel for getting the correct retries? That wasn't implemented in the Minstrel file I based my HT version on. Will look at it and see how can it be implemented here
Sign in to reply to this message.
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.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
