Issue 329500043: Wi-Fi adjacent channel modeling through transmit spectrum mask (OFDM) (Closed)
|
Created:
6 years, 6 months ago by Rediet Modified:
6 years, 1 month ago CC:
ns-3-reviews_googlegroups.com Visibility:
Public. |
DescriptionThis patch models the transmit spectrum mask defined for OFDM-based Wi-Fi standards (within the WifiSpectrumValueHelper) so as to be able to model adjacent channel interference.
It takes ns-3-dev revision 13114:ba42ffbfbe1e (Oct 12 2017) as reference.
Any suggestions / reviews would be much appreciated.
Patch Set 1 : Diff with ns-3-dev rev:13114 #
Total comments: 13
Patch Set 2 : Handle review comments and add unit tests #
Total comments: 58
Patch Set 3 : Patch Set 3: Handle Sebastien's comments on 2nd patch #
Total comments: 2
Patch Set 4 : Handle Sebastien's and S.Kalpalatha's comments concerning previous patchset #Patch Set 5 : Correct channel width and frequency for non-HT OFDM PSD #Patch Set 6 : Adapting channel width following Sebastien's recommendations in Bug 2483 #
Total comments: 35
Patch Set 7 : Diff with ns-3-dev rev:13336 (Feb 25th) #
Total comments: 16
Patch Set 8 : Diff with ns-3-dev rev:13352 (March 1st) -> First step (with missing changes) [obsolete] #Patch Set 9 : Diff with ns-3-dev rev:13352 (March 1st) -> First step: guard band not limited to 20 MHz #Patch Set 10 : Diff with ns-3-dev rev:13352 (March 1st) -> Second step: guard band limited to 20 MHz #
Total comments: 1
MessagesTotal messages: 36
Hello Sebastien, Michael, Skalpa, everyone, This patch models the transmit spectrum mask defined for OFDM-based Wi-Fi standards (within the WifiSpectrumValueHelper) so as to be able to model adjacent channel interference. Any suggestions / reviews would be much appreciated. BR, Rediet
Sign in to reply to this message.
Rediet, thanks for your nice job. I am wondering: could you add unit tests? This is the kind of changes that are easily unit testable and which IMO is the best way to verify it works as expected.
Sign in to reply to this message.
https://codereview.appspot.com/329500043/diff/1/src/wifi/model/spectrum-wifi-... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/1/src/wifi/model/spectrum-wifi-... src/wifi/model/spectrum-wifi-phy.cc:381: guardBandwidth = uint32_t (GetChannelWidth ()); why this? In case the channel width is for example 160, this looks way too much! Also, please avoid C style casting.
Sign in to reply to this message.
https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-... File src/wifi/examples/wifi-trans-gnuplot.plg (right): https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-... src/wifi/examples/wifi-trans-gnuplot.plg:42: #pause -1 Why do you need a separate file? I saw examples where all is contained in the simulation script itself.
Sign in to reply to this message.
Dear Rediet, Thanks a lot for the effort. Sadly, I am unable to get any meaningful data out of the provided files and examples. Gnuplot generates a 3d-plot with a plane located close to zero when using the wifi-trans-example.sh script. I uploaded one example plot here: https://imgur.com/a/GyCL4 In addition, I do not think that it is gnuplot issues since also the .tr files indicate zero values when running wifi-trans-example.cc from the scratch folder. Am I doing something wrong? Could you test this upload in new ns3-13114 clone and see if you forgot to commit some changes? Let me know if you need more information. With kind regards, Michael
Sign in to reply to this message.
Some c++ compile erros. https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spect... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spect... src/spectrum/model/wifi-spectrum-value-helper.cc:249: Values::iterator vit = c->ValuesBegin (); unused-but-set-variable (looks like it is save to delete this iterator) https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spect... src/spectrum/model/wifi-spectrum-value-helper.cc:250: Bands::const_iterator bit = c->ConstBandsBegin (); unused-but-set-variable https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spect... src/spectrum/model/wifi-spectrum-value-helper.cc:373: Values::iterator vit = c->ValuesBegin (); unused-but-set-variable https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spect... src/spectrum/model/wifi-spectrum-value-helper.cc:374: Bands::const_iterator bit = c->ConstBandsBegin (); unused-but-set-variable
Sign in to reply to this message.
On 2017/10/24 19:26:05, S. Deronne wrote: > Rediet, thanks for your nice job. > I am wondering: could you add unit tests? This is the kind of changes that are > easily unit testable and which IMO is the best way to verify it works as > expected. Hi Sébastien, I will try to add them in the next patch. Actually I tested all combinations in a "visual" manner by using the script. It is indeed better to have it automated.
Sign in to reply to this message.
Thanks Sébastien, thanks Michael. https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spect... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spect... src/spectrum/model/wifi-spectrum-value-helper.cc:249: Values::iterator vit = c->ValuesBegin (); On 2017/10/25 13:16:47, micha.rademacher wrote: > unused-but-set-variable > > (looks like it is save to delete this iterator) Acknowledged. https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spect... src/spectrum/model/wifi-spectrum-value-helper.cc:250: Bands::const_iterator bit = c->ConstBandsBegin (); On 2017/10/25 13:16:47, micha.rademacher wrote: > unused-but-set-variable Acknowledged. https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spect... src/spectrum/model/wifi-spectrum-value-helper.cc:373: Values::iterator vit = c->ValuesBegin (); On 2017/10/25 13:16:47, micha.rademacher wrote: > unused-but-set-variable Acknowledged. https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spect... src/spectrum/model/wifi-spectrum-value-helper.cc:374: Bands::const_iterator bit = c->ConstBandsBegin (); On 2017/10/25 13:16:47, micha.rademacher wrote: > unused-but-set-variable Acknowledged. https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-... File src/wifi/examples/wifi-trans-gnuplot.plg (right): https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-... src/wifi/examples/wifi-trans-gnuplot.plg:42: #pause -1 On 2017/10/24 19:27:25, S. Deronne wrote: > Why do you need a separate file? I saw examples where all is contained in the > simulation script itself. Will use a single file then. However, would it be interesting to leave the .plg file so that single runs of the example may be easily plotted? https://codereview.appspot.com/329500043/diff/1/src/wifi/model/spectrum-wifi-... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/1/src/wifi/model/spectrum-wifi-... src/wifi/model/spectrum-wifi-phy.cc:381: guardBandwidth = uint32_t (GetChannelWidth ()); On 2017/10/24 19:26:16, S. Deronne wrote: > why this? In case the channel width is for example 160, this looks way too much! I wanted to follow the spectrum mask as defined by the standard. In it, the left and right bands are of the same size as the used band. I agree with you that it will occupy a lot of virtual bandwidth (and thus increase processing load in SpectrumChannel) but it's the best way to stick with the standard and fully model the impact of the interference on two neighboring channels. > Also, please avoid C style casting. Ack, for next patch.
Sign in to reply to this message.
On 2017/10/25 13:16:19, micha.rademacher wrote: > Dear Rediet, > > Thanks a lot for the effort. Sadly, I am unable to get any meaningful data out > of the provided files and examples. > > Gnuplot generates a 3d-plot with a plane located close to zero when using the > wifi-trans-example.sh script. I uploaded one example plot here: > https://imgur.com/a/GyCL4 > In addition, I do not think that it is gnuplot issues since also the .tr files > indicate zero values when running wifi-trans-example.cc from the scratch folder. > > > Am I doing something wrong? Could you test this upload in new ns3-13114 clone > and see if you forgot to commit some changes? > > Let me know if you need more information. > > With kind regards, > Michael Hi Michael, The script is supposed to be run from the src/wifi/examples directory. I haven't tried running it with the .cc in the scratch folder. It should work though if you modify line 37 and add .plg in scratch folder. Apart from that I don't think that I have forgotten any changes (since followed upload.py guidelines). I'll double check while preparing second patch, taking into account your comments. Rediet
Sign in to reply to this message.
https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-... File src/wifi/examples/wifi-trans-gnuplot.plg (right): https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-... src/wifi/examples/wifi-trans-gnuplot.plg:42: #pause -1 On 2017/10/30 15:07:57, Rediet wrote: > On 2017/10/24 19:27:25, S. Deronne wrote: > > Why do you need a separate file? I saw examples where all is contained in the > > simulation script itself. > > Will use a single file then. > However, would it be interesting to leave the .plg file so that single runs of > the example may be easily plotted? Sorry, I hadn't understood what you meant until I saw ofdm-XX-validation.cc examples. Will modify simulation script (.cc) in order to directly configure Gnuplot in it. Will also modify .sh script accordingly. Thanks for the tip.
Sign in to reply to this message.
On 2017/10/30 15:13:34, Rediet wrote: > On 2017/10/25 13:16:19, micha.rademacher wrote: > > Dear Rediet, > > > > Thanks a lot for the effort. Sadly, I am unable to get any meaningful data out > > of the provided files and examples. > > > > Gnuplot generates a 3d-plot with a plane located close to zero when using the > > wifi-trans-example.sh script. I uploaded one example plot here: > > https://imgur.com/a/GyCL4 > > In addition, I do not think that it is gnuplot issues since also the .tr files > > indicate zero values when running wifi-trans-example.cc from the scratch > folder. > > > > > > Am I doing something wrong? Could you test this upload in new ns3-13114 clone > > and see if you forgot to commit some changes? > > > > Let me know if you need more information. > > > > With kind regards, > > Michael > > Hi Michael, > > The script is supposed to be run from the src/wifi/examples directory. I haven't > tried running it with the .cc in the scratch folder. It should work though if > you modify line 37 and add .plg in scratch folder. > Apart from that I don't think that I have forgotten any changes (since followed > upload.py guidelines). I'll double check while preparing second patch, taking > into account your comments. > > Rediet Hello Michael, Just pin pointed the problem. Beacon jitter is now enabled by default (see bug 2779, which had been reported by...myself...silly me). Simply deactivating the corresponding EnableBeaconJitter attribute while defining the ApWifiMac (line 212) should do the trick. Will be taken into account in the next patch. Rediet
Sign in to reply to this message.
Dear Sébastien, Michael, Here is a second patch taking into account your comments/corrections, namely: - addition of unit tests (note that bug #2813 has been submitted in the mean time thus the commented out test cases) - modification of the example (and .sh) so as to directly generate image Looking forward to your comments, BR, Rediet https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:401: start1 = (nGuardBands / 2) + 6; The 11ax SFD states that there are 6 edge tones for 20MHz https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:412: // skip the guard band and 12 subbands, then place power in 242 subbands, then 12 rather than 11
Sign in to reply to this message.
Thanks, I still need to find some time to review the helper class itself. Can you please double check your utests? I actually do not see any real checks in there. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... File src/wifi/test/wifi-test-spectrum-value-h.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:30: NS_LOG_COMPONENT_DEFINE ("WifiTestSpectrumValueHelper"); I find the name strange as well as the name of the file https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:85: * \param tol tolerance (in dB) is this tolerance needed? I do not really see a need for this. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:718: } What are you testing? I do not see EXPECT_EQ like functions, so I have the impression nothing is really tested.
Sign in to reply to this message.
Thanks Sébastien. My replies below. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... File src/wifi/test/wifi-test-spectrum-value-h.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:30: NS_LOG_COMPONENT_DEFINE ("WifiTestSpectrumValueHelper"); On 2017/11/07 19:08:22, S. Deronne wrote: > I find the name strange as well as the name of the file I took the lte-test-spectrum-value-helper as a basis for writing these tests (since roughly the same kind of verifications were to be done). The file name was a bit too long so I shortened the last part. I can change it to whatever you prefer. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:85: * \param tol tolerance (in dB) On 2017/11/07 19:08:22, S. Deronne wrote: > is this tolerance needed? I do not really see a need for this. I tried at first to do without it, but the assert kept failing since the parameters provided for the current test case (within the test suite) had a given resolution (3 decimals sounded reasonable enough considering the slopes). That's why I had to add the resolution as a parameter of this static method (to have it centralized). https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:718: } On 2017/11/07 19:08:22, S. Deronne wrote: > What are you testing? I do not see EXPECT_EQ like functions, so I have the > impression nothing is really tested. I am testing the consistency between the pre-computed slopes (Excel sheet based) and the ones obtained through the methods. The testing part is done in the DoRun of the test case. As stated above, I used equivalent lte tests as a guide line for this.
Sign in to reply to this message.
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:175: double lowestPointDbr = -40.0; this is not needed in this function, so this can be defined in CreateSpectrumMaskForOfdm https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:190: innerSlopeWidth = static_cast<uint32_t>((2e6 / bandBandwidth) + 0.5); // [-11;-9] & [9;11] why do you use the bandBandwidth here? https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:191: lowestPointDbm = -53.0; //dBm/MHz where does this come from? https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:228: //To handle cases where legacy OFDM is used for HT/VHT/HE STAs using bandwidth of 40 MHz and above Why? There are other functions for generating HT/VHT/HE OFDM masks. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:523: uint32_t innerSlopeWidth, double lowestPointDbr, double lowestPointDbm) I think lowestPointDbm is useless https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:533: double txPowerPerBandDbm = (10.0 * std::log10 (txPowerPerBandW * 1000.0)); there is already a function doing the conversion somewhere in the wifi code https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:536: double txPowerLowestPoint = txPowerPerBandDbm + lowestPointDbr; //Doing std::max (txPowerPerBandDbm + lowestPointDbr, lowestPointDbm) messed up everything for low signals (dBm/MHz to current band granularity conversion needed)... here simply use txPowerPerBandDbm - 40 https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:539: uint32_t outerSlopeWidth = nGuardBands / 4; why? https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-w... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-w... src/wifi/model/spectrum-wifi-phy.cc:366: uint32_t guardBandwidth = 0; are 32 bits necessary? https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-w... src/wifi/model/spectrum-wifi-phy.cc:381: guardBandwidth = static_cast<uint32_t> (GetChannelWidth ()); why a cast? I assume channel width and guard band width should have the same type in the end. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... File src/wifi/test/wifi-test-spectrum-value-h.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:30: NS_LOG_COMPONENT_DEFINE ("WifiTestSpectrumValueHelper"); Yes, a good name is better I think. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:85: * \param tol tolerance (in dB) Do you mean because you were comparing double values? There is a way with the google test framework to compare double values without the need to use a tolerance. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:718: } Do you mean the assert at the end of DoRun? It is much better to use the functions provided by the google tests framework to perform your checks.
Sign in to reply to this message.
Dear Sébastien, Thanks for your comments (and sorry for the delay, just managed to find some time to reply). I'll look into the google tests framework as suggested. Do you want me to change the whole TestSuite vs TestCase>DoRun logic or just the assertion? Best regards, Rediet https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:175: double lowestPointDbr = -40.0; On 2017/11/09 07:15:16, S. Deronne wrote: > this is not needed in this function, so this can be defined in > CreateSpectrumMaskForOfdm It is indeed used by CreateSpectrumMaskForOfdm but depending on the calling method it can take 2 values: -40 dBr or -45 dBr. The first value is used for OFDM (a/g) and HT/HE in 2.4GHz band, and the second one for VHT and HT/HE in 5GHz band. That's why I prefered specifying the value in the calling method rather than in CreateSpectrumMaskForOfdm. If it's not the best way, your suggestions will be welcome. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:190: innerSlopeWidth = static_cast<uint32_t>((2e6 / bandBandwidth) + 0.5); // [-11;-9] & [9;11] On 2017/11/09 07:15:17, S. Deronne wrote: > why do you use the bandBandwidth here? It was in order to determine, on the fly, the number of tones that are in the "inner slope" (see diagram in .h), rather than hard coding this number. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:191: lowestPointDbm = -53.0; //dBm/MHz On 2017/11/09 07:15:16, S. Deronne wrote: > where does this come from? I took this info from the 802.11 standard (e.g. 2016 version) and 11ax SFD. The minimal value is provided in dBr and dBm/MHz. As you underlined in another comment, I haven't used it (it was too complex to exploit it, so I commented it out). I'll suppress it if you want. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:228: //To handle cases where legacy OFDM is used for HT/VHT/HE STAs using bandwidth of 40 MHz and above On 2017/11/09 07:15:16, S. Deronne wrote: > Why? There are other functions for generating HT/VHT/HE OFDM masks. I stampled upon this case when testing >20MHz HT/VHT/HE traffic. Management and control messages are still generated with 20 MHz OFDM. I had to handle this exception. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:523: uint32_t innerSlopeWidth, double lowestPointDbr, double lowestPointDbm) On 2017/11/09 07:15:16, S. Deronne wrote: > I think lowestPointDbm is useless Acknowledged. I'll remove it. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:533: double txPowerPerBandDbm = (10.0 * std::log10 (txPowerPerBandW * 1000.0)); On 2017/11/09 07:15:16, S. Deronne wrote: > there is already a function doing the conversion somewhere in the wifi code Acknowledged. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:536: double txPowerLowestPoint = txPowerPerBandDbm + lowestPointDbr; //Doing std::max (txPowerPerBandDbm + lowestPointDbr, lowestPointDbm) messed up everything for low signals (dBm/MHz to current band granularity conversion needed)... On 2017/11/09 07:15:16, S. Deronne wrote: > here simply use txPowerPerBandDbm - 40 See previous comment https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:539: uint32_t outerSlopeWidth = nGuardBands / 4; On 2017/11/09 07:15:17, S. Deronne wrote: > why? Because it's size is half the useful band, thus a fourth of the total guard band (left+right). E.g. see Figure 19-17 of 802.11-2016 standard for OFDM. https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-w... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-w... src/wifi/model/spectrum-wifi-phy.cc:366: uint32_t guardBandwidth = 0; On 2017/11/09 07:15:17, S. Deronne wrote: > are 32 bits necessary? Indeed. I just wanted to keep the same signature. https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-w... src/wifi/model/spectrum-wifi-phy.cc:381: guardBandwidth = static_cast<uint32_t> (GetChannelWidth ()); On 2017/11/09 07:15:17, S. Deronne wrote: > why a cast? I assume channel width and guard band width should have the same > type in the end. Ack. I'll change it (I just tried to keep the same signature as stated above). https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... File src/wifi/test/wifi-test-spectrum-value-h.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:30: NS_LOG_COMPONENT_DEFINE ("WifiTestSpectrumValueHelper"); On 2017/11/09 07:15:17, S. Deronne wrote: > Yes, a good name is better I think. Acknowledged. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:85: * \param tol tolerance (in dB) On 2017/11/09 07:15:17, S. Deronne wrote: > Do you mean because you were comparing double values? There is a way with the > google test framework to compare double values without the need to use a > tolerance. I'll try to look into that. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:718: } On 2017/11/09 07:15:17, S. Deronne wrote: > Do you mean the assert at the end of DoRun? It is much better to use the > functions provided by the google tests framework to perform your checks. Yes I used the NS_TEST_ASSERT to verify it (took it from LTE spectrum value helper test suite). I'll look into the google tests framework and change it. Do you want me to change the whole TestSuite vs TestCase>DoRun logic or just the assertion?
Sign in to reply to this message.
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:175: double lowestPointDbr = -40.0; I agree with those values, but they are only used in the called function, so that can be defined there instead of here. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:190: innerSlopeWidth = static_cast<uint32_t>((2e6 / bandBandwidth) + 0.5); // [-11;-9] & [9;11] On 2017/11/22 16:22:38, Rediet wrote: > On 2017/11/09 07:15:17, S. Deronne wrote: > > why do you use the bandBandwidth here? > > It was in order to determine, on the fly, the number of tones that are in the > "inner slope" (see diagram in .h), rather than hard coding this number. OK clearer, I had not taken enough attention to the diagram. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:191: lowestPointDbm = -53.0; //dBm/MHz OK please clean it. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:228: //To handle cases where legacy OFDM is used for HT/VHT/HE STAs using bandwidth of 40 MHz and above I don't think this is correct, if those frames use larger spectrum, they are using a MCS, and thus Ht or Vht Ofdm function should be called. Otherwise you spotted a bug. Do you have a script to reproduce this? https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:539: uint32_t outerSlopeWidth = nGuardBands / 4; Which size is half of the useful band? https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... File src/wifi/test/wifi-test-spectrum-value-h.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:718: } I would expect to see some kind of NS_TEST_EXPECT_MSG_EQ macros when comparing expected and obtained results. I guess the difference is that with an assert the test would crash if comparison fails, whereas with the EQ the test would fail (which is the expected behavior), but I am not sure.
Sign in to reply to this message.
Dear Sébastien, Thank you for your review of the second patch and sorry for the delay. I've tried my best to take all of your comments into accout. See my replies below for some aspects that were too complex for me to quickly solve (will highly appreciate your insight on these) or that required more explanation. Best regards, Rediet https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:175: double lowestPointDbr = -40.0; On 2017/11/22 16:53:19, S. Deronne wrote: > I agree with those values, but they are only used in the called function, so > that can be defined there instead of here. Done. I've added a default value of -40.0 for the argument, so as not to have anything here. I have done it this way b/c it was too complex to determine whether it was -40 or -45 without using premable type info (that is in wifi module), since frequency only is not sufficient (11a is the exception). https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:191: lowestPointDbm = -53.0; //dBm/MHz On 2017/11/22 16:53:19, S. Deronne wrote: > OK please clean it. Done in Patch 3. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:228: //To handle cases where legacy OFDM is used for HT/VHT/HE STAs using bandwidth of 40 MHz and above On 2017/11/22 16:53:19, S. Deronne wrote: > I don't think this is correct, if those frames use larger spectrum, they are > using a MCS, and thus Ht or Vht Ofdm function should be called. Otherwise you > spotted a bug. Do you have a script to reproduce this? I'll work on that and will file a bug with the script. But just to be sure we're talking about the same thing: the beacon (e.g.) is generated using 20 MHz band, but still the underlying Spectrum instance is still 80 MHz (e.g.) wide and thus if this special case is not handled the code will fail due to some inconsistencies. It is thus a "normal" use case for me, but still I'll dig into it. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:523: uint32_t innerSlopeWidth, double lowestPointDbr, double lowestPointDbm) On 2017/11/09 07:15:16, S. Deronne wrote: > I think lowestPointDbm is useless Done. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:533: double txPowerPerBandDbm = (10.0 * std::log10 (txPowerPerBandW * 1000.0)); On 2017/11/09 07:15:16, S. Deronne wrote: > there is already a function doing the conversion somewhere in the wifi code I've tried using the wifi-utils file but the problem is that we end up having cyclic referencing (i.e. wifi module already depends upon spectrum module). I'm forced to leave it like this unless you have a workaround. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:539: uint32_t outerSlopeWidth = nGuardBands / 4; On 2017/11/22 16:53:19, S. Deronne wrote: > Which size is half of the useful band? The outer slope width on either side (as illustrated in method's doxygen header). As for the quarter (instead of a simple half) it was b/c I had to stick with the existing logic of defining the total guard band (left+right). https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-w... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-w... src/wifi/model/spectrum-wifi-phy.cc:366: uint32_t guardBandwidth = 0; On 2017/11/22 16:22:39, Rediet wrote: > On 2017/11/09 07:15:17, S. Deronne wrote: > > are 32 bits necessary? > > Indeed. I just wanted to keep the same signature. Done. https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-w... src/wifi/model/spectrum-wifi-phy.cc:381: guardBandwidth = static_cast<uint32_t> (GetChannelWidth ()); On 2017/11/22 16:22:39, Rediet wrote: > On 2017/11/09 07:15:17, S. Deronne wrote: > > why a cast? I assume channel width and guard band width should have the same > > type in the end. > > Ack. I'll change it (I just tried to keep the same signature as stated above). Done. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... File src/wifi/test/wifi-test-spectrum-value-h.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:30: NS_LOG_COMPONENT_DEFINE ("WifiTestSpectrumValueHelper"); On 2017/11/09 07:15:17, S. Deronne wrote: > Yes, a good name is better I think. Done. https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:85: * \param tol tolerance (in dB) On 2017/11/09 07:15:17, S. Deronne wrote: > Do you mean because you were comparing double values? There is a way with the > google test framework to compare double values without the need to use a > tolerance. I'm sorry but I couldn't find a way to do it (I'm now using the TestDoubleIsEqual method of test though) without using a certain tolerance level, since there were a lot of rounding operations done when building the expected mask (restrained myself to 3 point decimals). https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-s... src/wifi/test/wifi-test-spectrum-value-h.cc:718: } On 2017/11/22 16:53:20, S. Deronne wrote: > I would expect to see some kind of NS_TEST_EXPECT_MSG_EQ macros when comparing > expected and obtained results. I guess the difference is that with an assert the > test would crash if comparison fails, whereas with the EQ the test would fail > (which is the expected behavior), but I am not sure. Done. Changed to EXPECT.
Sign in to reply to this message.
Rediet, thanks for your new patchset, I have some small remarks, but I did not have time to go through a complete review of your new version. I think it would be important to add the maintainer of the spectrum module in this review (I think it is Nicola Baldo). https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:228: //To handle cases where legacy OFDM is used for HT/VHT/HE STAs using bandwidth of 40 MHz and above Is this not filling txVector with channelwidth set to 20 when sending beacons, even if 40 MHz is supported? https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:533: double txPowerPerBandDbm = (10.0 * std::log10 (txPowerPerBandW * 1000.0)); OK for now, even though there must be for sure a solution to avoid such copy-paste. https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:539: uint32_t outerSlopeWidth = nGuardBands / 4; Sorry, still not clear for me where this magic 4 comes from. https://codereview.appspot.com/329500043/diff/40001/src/spectrum/model/wifi-s... File src/spectrum/model/wifi-spectrum-value-helper.h (right): https://codereview.appspot.com/329500043/diff/40001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.h:183: uint32_t innerSlopeWidth, double lowestPointDbr = -40.0); we should avoid such way of doing I think, it should be passed explicitly to avoid confusions.
Sign in to reply to this message.
Dear Rediet Thanks for patch and all the recent updates and modifications. Here are some of our comments on the latest patch set 3. 1) The left and right flat junction in the spectrum mask is neither identified in the figure in wifi-spectrum-value-helper.h file nor available in the IEEE 802.11 standard (2016). Kindly provide a reference for the spectrum mask. 2) In the file, wifi-spectrum-value-helper.cc some variables like txPowerW clearly identify the unit of measurement, while a variable like TxPowerPerBand does not. 3) In the function CreateOfdmTxPowerSpectralDensity() when the channelwidth > 20MHz, the values for the variables start1, stop1 are initialized only to 0. 4) In the file wifi-spectrum-value-helper.cc, the variable name txPowerLowestPoint is not consistent with other variable names txPowerMinus20dBr and txPowerMinus28dBr. 5) The left innerSlopeWidth (and similarly right innerslopewidth) is identified between bands 64-69 (approximately [-10;-8] MHz) and not between [-11;-9] MHz (as mentioned in the wifi-spectrum-value-helper.cc, file). Thanks and Regards s.Kalpalatha On 2017/12/07 15:07:47, Rediet wrote: > Dear Sébastien, > > Thank you for your review of the second patch and sorry for the delay. I've > tried my best to take all of your comments into accout. See my replies below for > some aspects that were too complex for me to quickly solve (will highly > appreciate your insight on these) or that required more explanation. > > Best regards, > > Rediet > >
Sign in to reply to this message.
Dear Sébastien, S.Kalpalatha, Nicola, Thanks for your comments Sébastien and S.Kalpalatha. Nicola could you please have a quick look if you have some time to spare? I've uploaded a new patchset including most of them. As for the ones that weren't directly addressed within, you'll find my replies inline (for S.Kalpalatha) or as comment replies (for Sébastien). > 1) The left and right flat junction in the spectrum mask is neither identified > in the figure in wifi-spectrum-value-helper.h file nor available in the IEEE > 802.11 standard (2016). Kindly provide a reference for the spectrum mask. When working on the subject with the standard defined maximum spectrum mask I stumbled upon the fact that guard tones (and DC) are within the allocated band while not being allocated power. In order to satisfy this constraint while keeping the global shape of the spectrum mask, I had to shift the inner band inwards and thus add the flat junction. I am aware that we no longer stick 100% to the standard defined maximum mask, but that's what they precisely are, max limitations. I've enriched the doxygen with a short explanation. > 2) In the file, wifi-spectrum-value-helper.cc some variables like txPowerW > clearly identify the unit of measurement, while a variable like TxPowerPerBand > does not. Updated, thanks. > 3) In the function CreateOfdmTxPowerSpectralDensity() when the channelwidth > > 20MHz, the values for the variables start1, stop1 are initialized only to 0. The default case redirects to the 20 MHz case within the switch, so they are correctly initialized. > 4) In the file wifi-spectrum-value-helper.cc, the variable name > txPowerLowestPoint is not consistent with other variable names txPowerMinus20dBr > and txPowerMinus28dBr. Also updated. > 5) The left innerSlopeWidth (and similarly right innerslopewidth) is identified > between bands 64-69 (approximately [-10;-8] MHz) and not between [-11;-9] MHz > (as mentioned in the wifi-spectrum-value-helper.cc, file). See reply to 1). Still, I prefer keeping standard defined values in comments for the CreateXXXTxPowerSpectralDensity methods, while letting implementation tricks inside CreateSpectrumMaskForOfdm. Best regards, Rediet https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:228: //To handle cases where legacy OFDM is used for HT/VHT/HE STAs using bandwidth of 40 MHz and above On 2017/12/09 16:24:11, S. Deronne wrote: > Is this not filling txVector with channelwidth set to 20 when sending beacons, > even if 40 MHz is supported? TxVector is correctly set with 20 MHz channelWidth. However, SpectrumWifiPhy::StartTx calls GetTxPowerSpectralDensity using GetChannelWidth (even for non-HT/VHT/HE OFDM), which returns the total width supported by the WifiPhy. The same SpectrumModel is reused for HT/VHT and non-HT/VHT OFDM transmissions (HE is apart since it doesn't use the same intertone spacing). However, the primary channel ends up being the lowest 20 MHz channel (which is not always true by the way... we have a small bug here... nice insight Sébastien! I'll file it asap). If we substitute txVector.GetChannelWidth (current) for GetChannelWidth (supported) when calling GetTxPowerSpectralDensity, a problem arises since the provided center frequency is no longer correct; it still corresponds to the supported channel width. It should be adapted to the reduced 20 MHz band, and specifically to the primary channel's (ideally according to the standard's sophisticated primary/secondary logic). This was relatively transparent before spectrum mask considerations (except for the primary channel positioning issue), since the guard band was set to 10 MHz irrespective of the channel width. I'll try to find a quick solution for primary channel positioning, submit a patch for the bug, and will update this code. In the time being, roughly handling this exception within the WifiSpectrumValueHelper class will do the trick (except for the above-mentioned limitation). https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:533: double txPowerPerBandDbm = (10.0 * std::log10 (txPowerPerBandW * 1000.0)); On 2017/12/09 16:24:11, S. Deronne wrote: > OK for now, even though there must be for sure a solution to avoid such > copy-paste. Ack. I'll define a DbmToW method to do the job, by reusing what's already done in wifi-utils (since nothing similar found in subsequent propagation and antenna modules). https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:539: uint32_t outerSlopeWidth = nGuardBands / 4; On 2017/12/09 16:24:11, S. Deronne wrote: > Sorry, still not clear for me where this magic 4 comes from. > nGuardBands is the total left+right guard band. The spectrum mask defined in the standard covers a band of 3B, B being the useful band, thus a total guard band of 2B. Each part of the guard band (i.e. left or right) can be decomposed into 3 slopes, with the outer-most slope occupying a band of B/2 and thus nGuardBand/4. Would it be preferable to introduce an intermediate variable to model the total band (useful+guard)? https://codereview.appspot.com/329500043/diff/40001/src/spectrum/model/wifi-s... File src/spectrum/model/wifi-spectrum-value-helper.h (right): https://codereview.appspot.com/329500043/diff/40001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.h:183: uint32_t innerSlopeWidth, double lowestPointDbr = -40.0); On 2017/12/09 16:24:11, S. Deronne wrote: > we should avoid such way of doing I think, it should be passed explicitly to > avoid confusions. Acknowledged.
Sign in to reply to this message.
Dear Sébastien, I've updated the patch with bug's 2843's corrections. I think I've covered all previous comments (yours, Michael's and S.Kalpalatha's). Does anyone have more comments to share? BR, Rediet https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-s... src/spectrum/model/wifi-spectrum-value-helper.cc:228: //To handle cases where legacy OFDM is used for HT/VHT/HE STAs using bandwidth of 40 MHz and above On 2017/12/15 15:05:44, Rediet wrote: > On 2017/12/09 16:24:11, S. Deronne wrote: > > Is this not filling txVector with channelwidth set to 20 when sending beacons, > > even if 40 MHz is supported? > > TxVector is correctly set with 20 MHz channelWidth. However, > SpectrumWifiPhy::StartTx calls GetTxPowerSpectralDensity using GetChannelWidth > (even for non-HT/VHT/HE OFDM), which returns the total width supported by the > WifiPhy. The same SpectrumModel is reused for HT/VHT and non-HT/VHT OFDM > transmissions (HE is apart since it doesn't use the same intertone spacing). > However, the primary channel ends up being the lowest 20 MHz channel (which is > not always true by the way... we have a small bug here... nice insight > Sébastien! I'll file it asap). > > If we substitute txVector.GetChannelWidth (current) for GetChannelWidth > (supported) when calling GetTxPowerSpectralDensity, a problem arises since the > provided center frequency is no longer correct; it still corresponds to the > supported channel width. It should be adapted to the reduced 20 MHz band, and > specifically to the primary channel's (ideally according to the standard's > sophisticated primary/secondary logic). > > This was relatively transparent before spectrum mask considerations (except for > the primary channel positioning issue), since the guard band was set to 10 MHz > irrespective of the channel width. > > I'll try to find a quick solution for primary channel positioning, submit a > patch for the bug, and will update this code. In the time being, roughly > handling this exception within the WifiSpectrumValueHelper class will do the > trick (except for the above-mentioned limitation). Done in patch 5 (also filed bug 2843 in parallel).
Sign in to reply to this message.
Updated version following Sebastien's recommendation in bug 2483.
Sign in to reply to this message.
Rediet, could you please merge with ns-3-dev? I see already a lot of wifi changes pushed or conflicting. I am OK to push most of the wifi code changes (I will already push the change in the example), but I feel like passing 2 times the same parameter to CreateXXXOfdmTxPowerSpectralDensity is NOK. We have some days remaining before the release, it would be nice to have this included. https://codereview.appspot.com/329500043/diff/100001/examples/wireless/wifi-s... File examples/wireless/wifi-spectrum-saturation-example.cc (right): https://codereview.appspot.com/329500043/diff/100001/examples/wireless/wifi-s... examples/wireless/wifi-spectrum-saturation-example.cc:202: = CreateObject<MultiModelSpectrumChannel> (); OK to push this https://codereview.appspot.com/329500043/diff/100001/src/wifi/examples/wifi-t... File src/wifi/examples/wifi-trans-example.sh (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/examples/wifi-t... src/wifi/examples/wifi-trans-example.sh:145: echo "Finished" To be checked with Tom if we can put bash script here. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/constant-... File src/wifi/model/constant-rate-wifi-manager.cc (left): https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/constant-... src/wifi/model/constant-rate-wifi-manager.cc:128: } Those changes are already present in ns-3-dev, please merge again. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:295: v = WifiSpectrumValueHelper::CreateOfdmTxPowerSpectralDensity (centerFrequency, channelWidth, txPowerW, channelWidth); why channelWidth is provided 2 times? Passing the guard bandwidth seems more logical... https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:303: v = WifiSpectrumValueHelper::CreateHtOfdmTxPowerSpectralDensity (centerFrequency, channelWidth, txPowerW, channelWidth); same comment https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:306: v = WifiSpectrumValueHelper::CreateHeOfdmTxPowerSpectralDensity (centerFrequency, channelWidth, txPowerW, channelWidth); same comment https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:318: NS_LOG_FUNCTION (txVector); this << txVector https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:320: uint32_t supportedWidth = static_cast<uint32_t> (GetChannelWidth ()); why uint32_t? Width should be limited to uint8_t https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:327: return centerFrequencyForSupportedWidth; I am even thinking this should be pushed appart from this implementation (open new bug in the tracker?) https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:335: Ptr<SpectrumValue> txPowerSpectrum = GetTxPowerSpectralDensity (GetCenterFrequencyForChannelWidth (txVector), txVector.GetChannelWidth (), txPowerWatts, txVector.GetMode ().GetModulationClass ()); same comment here https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:378: uint8_t currently double in ns-3-dev... https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:396: guardBandwidth = GetChannelWidth (); why? https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... File src/wifi/model/spectrum-wifi-phy.h (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.h:159: uint8_t GetGuardBandwidth (void) const; changed to double by Tom in the latest ns-3-dev, please check and merge. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remo... File src/wifi/model/wifi-remote-station-manager.cc (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remo... src/wifi/model/wifi-remote-station-manager.cc:840: return maxSupportedChannelWidth; already pushed https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remo... File src/wifi/model/wifi-remote-station-manager.h (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remo... src/wifi/model/wifi-remote-station-manager.h:1088: static uint8_t GetChannelWidthForTransmission (WifiMode mode, uint8_t maxSupportedChannelWidth); already pushed to ns-3-dev https://codereview.appspot.com/329500043/diff/100001/src/wifi/test/spectrum-w... File src/wifi/test/spectrum-wifi-phy-test.cc (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/test/spectrum-w... src/wifi/test/spectrum-wifi-phy-test.cc:34: static const uint8_t GUARD_WIDTH = CHANNEL_WIDTH; // MHz (expanded to channel width to model spectrum mask) why? https://codereview.appspot.com/329500043/diff/100001/src/wifi/test/wifi-test.cc File src/wifi/test/wifi-test.cc (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/test/wifi-test.... src/wifi/test/wifi-test.cc:1367: AddTestCase (new Bug2483TestCase, TestCase::QUICK); //Bug 2483 already pushed
Sign in to reply to this message.
https://codereview.appspot.com/329500043/diff/100001/examples/wireless/wifi-s... File examples/wireless/wifi-spectrum-saturation-example.cc (right): https://codereview.appspot.com/329500043/diff/100001/examples/wireless/wifi-s... examples/wireless/wifi-spectrum-saturation-example.cc:202: = CreateObject<MultiModelSpectrumChannel> (); On 2018/02/24 09:57:28, S. Deronne wrote: > OK to push this This is actually already pushed.
Sign in to reply to this message.
Hi Sébastien, This new patchset takes the latest changeset (13336 of Feb 25th) as reference and takes into account your comments (except for the presence of shell script). Hope it's OK now. BR, Rediet https://codereview.appspot.com/329500043/diff/100001/examples/wireless/wifi-s... File examples/wireless/wifi-spectrum-saturation-example.cc (right): https://codereview.appspot.com/329500043/diff/100001/examples/wireless/wifi-s... examples/wireless/wifi-spectrum-saturation-example.cc:202: = CreateObject<MultiModelSpectrumChannel> (); On 2018/02/24 09:59:39, S. Deronne wrote: > On 2018/02/24 09:57:28, S. Deronne wrote: > > OK to push this > > This is actually already pushed. Acknowledged. Changeset 13336 (Feb 25th) taken as base for new patchset (#7). https://codereview.appspot.com/329500043/diff/100001/src/wifi/examples/wifi-t... File src/wifi/examples/wifi-trans-example.sh (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/examples/wifi-t... src/wifi/examples/wifi-trans-example.sh:145: echo "Finished" On 2018/02/24 09:57:28, S. Deronne wrote: > To be checked with Tom if we can put bash script here. OK. Will send a mail to Tom (with you in cc). But this part can be left out of the first patch if necessary and be added later on, if necessary. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/constant-... File src/wifi/model/constant-rate-wifi-manager.cc (left): https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/constant-... src/wifi/model/constant-rate-wifi-manager.cc:128: } On 2018/02/24 09:57:28, S. Deronne wrote: > Those changes are already present in ns-3-dev, please merge again. Acknowledged. Changeset 13336 (Feb 25th) taken as base for new patchset (#7). https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:295: v = WifiSpectrumValueHelper::CreateOfdmTxPowerSpectralDensity (centerFrequency, channelWidth, txPowerW, channelWidth); On 2018/02/24 09:57:29, S. Deronne wrote: > why channelWidth is provided 2 times? Passing the guard bandwidth seems more > logical... Acknowledged. Changed logic. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:303: v = WifiSpectrumValueHelper::CreateHtOfdmTxPowerSpectralDensity (centerFrequency, channelWidth, txPowerW, channelWidth); On 2018/02/24 09:57:29, S. Deronne wrote: > same comment Acknowledged. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:306: v = WifiSpectrumValueHelper::CreateHeOfdmTxPowerSpectralDensity (centerFrequency, channelWidth, txPowerW, channelWidth); On 2018/02/24 09:57:29, S. Deronne wrote: > same comment Acknowledged. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:318: NS_LOG_FUNCTION (txVector); On 2018/02/24 09:57:29, S. Deronne wrote: > this << txVector Acknowledged. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:320: uint32_t supportedWidth = static_cast<uint32_t> (GetChannelWidth ()); On 2018/02/24 09:57:29, S. Deronne wrote: > why uint32_t? Width should be limited to uint8_t Acknowledged. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:327: return centerFrequencyForSupportedWidth; On 2018/02/24 09:57:29, S. Deronne wrote: > I am even thinking this should be pushed appart from this implementation (open > new bug in the tracker?) Already done in bug 2843 https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:335: Ptr<SpectrumValue> txPowerSpectrum = GetTxPowerSpectralDensity (GetCenterFrequencyForChannelWidth (txVector), txVector.GetChannelWidth (), txPowerWatts, txVector.GetMode ().GetModulationClass ()); On 2018/02/24 09:57:28, S. Deronne wrote: > same comment here Acknowledged. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:378: uint8_t On 2018/02/24 09:57:29, S. Deronne wrote: > currently double in ns-3-dev... Already replied above. Also note that because of primary and secondary channel issues, the currently selected channel bandwidth has to be used. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:396: guardBandwidth = GetChannelWidth (); On 2018/02/24 09:57:29, S. Deronne wrote: > why? Explained in the code (newly added patchset 7). https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... File src/wifi/model/spectrum-wifi-phy.h (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.h:159: uint8_t GetGuardBandwidth (void) const; On 2018/02/24 09:57:29, S. Deronne wrote: > changed to double by Tom in the latest ns-3-dev, please check and merge. Had merged and put it to double at first. But considering that the guard bandwidth, when modeling OOB transmissions, should be large enough to encompass the frequencies depicted in the standard, reverted back to uint8_t since it should be equal to channelWidth. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remo... File src/wifi/model/wifi-remote-station-manager.cc (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remo... src/wifi/model/wifi-remote-station-manager.cc:840: return maxSupportedChannelWidth; On 2018/02/24 09:57:29, S. Deronne wrote: > already pushed Acknowledged. https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remo... File src/wifi/model/wifi-remote-station-manager.h (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remo... src/wifi/model/wifi-remote-station-manager.h:1088: static uint8_t GetChannelWidthForTransmission (WifiMode mode, uint8_t maxSupportedChannelWidth); On 2018/02/24 09:57:29, S. Deronne wrote: > already pushed to ns-3-dev Acknowledged. https://codereview.appspot.com/329500043/diff/100001/src/wifi/test/spectrum-w... File src/wifi/test/spectrum-wifi-phy-test.cc (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/test/spectrum-w... src/wifi/test/spectrum-wifi-phy-test.cc:34: static const uint8_t GUARD_WIDTH = CHANNEL_WIDTH; // MHz (expanded to channel width to model spectrum mask) On 2018/02/24 09:57:29, S. Deronne wrote: > why? Partly replied above, but actually the globally considered band (i.e. GUARD_WIDTH + CHANNEL_WIDTH + GUARD_WIDTH) has to encompass the whole spectrum illustrated in "Transmit spectrum mask" sections of the standard. https://codereview.appspot.com/329500043/diff/100001/src/wifi/test/wifi-test.cc File src/wifi/test/wifi-test.cc (right): https://codereview.appspot.com/329500043/diff/100001/src/wifi/test/wifi-test.... src/wifi/test/wifi-test.cc:1367: AddTestCase (new Bug2483TestCase, TestCase::QUICK); //Bug 2483 On 2018/02/24 09:57:29, S. Deronne wrote: > already pushed Acknowledged.
Sign in to reply to this message.
https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:395: case WIFI_PHY_STANDARD_80211b: I do not think spectrum does model DSSS https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:414: guardBandwidth = currentChannelWidth; This will break recent changes done by Tom to fix 5 MHz and 10 MHz channel widths. https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/wifi-utils.h File src/wifi/model/wifi-utils.h (right): https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/wifi-util... src/wifi/model/wifi-utils.h:41: double Log2 (double val); Why? I also do not see any implementation
Sign in to reply to this message.
https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:395: case WIFI_PHY_STANDARD_80211b: On 2018/02/26 22:37:57, S. Deronne wrote: > I do not think spectrum does model DSSS I think so, since method WifiSpectrumValueHelper::CreateDsssTxPowerSpectralDensity provides a PSD as well. However, I haven't modified the generated transmit mask, thus it's still the ideal one that is used, thus the unchanged guardBandwith value. https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:414: guardBandwidth = currentChannelWidth; On 2018/02/26 22:37:57, S. Deronne wrote: > This will break recent changes done by Tom to fix 5 MHz and 10 MHz channel > widths. I know that it'll impact this section but the rest (i.e. especially the bandBandwidth part) will remain unchanged. Considering the modeling choice that was done to cover the whole transmit mask (as per standard) it was necessary to extend the guard band (which was not so significant when dealing with ideal masks). I should have warned Tom about these awaiting paradigm changes but I forgot to do so when reviewing the patch for the bug... https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/wifi-utils.h File src/wifi/model/wifi-utils.h (right): https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/wifi-util... src/wifi/model/wifi-utils.h:41: double Log2 (double val); On 2018/02/26 22:37:57, S. Deronne wrote: > Why? I also do not see any implementation I don't understand why this method declaration has been duplicated. I must have made a mistake during merge. Should I provide another patch for this?
Sign in to reply to this message.
Thank you for this thorough patch; my only main comment/concern is setting guard BW to channel width even for widths > 20 MHz (please correct me if I am mistaken). https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-... src/spectrum/model/wifi-spectrum-value-helper.cc:429: Ptr<SpectrumModel> model = GetSpectrumModel (centerFrequency, channelWidth, bandBandwidth, static_cast<uint16_t> (guardBandwidth)); why is a cast to uint16_t needed here? https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-... src/spectrum/model/wifi-spectrum-value-helper.cc:454: NS_LOG_FUNCTION (centerFrequency << static_cast<uint16_t> (channelWidth) << bandGranularity << static_cast<uint16_t> (guardBandwidth)); same comment as above https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-... File src/spectrum/model/wifi-spectrum-value-helper.h (right): https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-... src/spectrum/model/wifi-spectrum-value-helper.h:195: * Normalize the transmit spectrum mask generated by CreateSepctrumMaskForOfdm CreateSpectrumMaskForOfdm https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:414: guardBandwidth = currentChannelWidth; On 2018/02/27 06:56:54, Rediet wrote: > On 2018/02/26 22:37:57, S. Deronne wrote: > > This will break recent changes done by Tom to fix 5 MHz and 10 MHz channel > > widths. > > I know that it'll impact this section but the rest (i.e. especially the > bandBandwidth part) will remain unchanged. Considering the modeling choice that > was done to cover the whole transmit mask (as per standard) it was necessary to > extend the guard band (which was not so significant when dealing with ideal > masks). > I should have warned Tom about these awaiting paradigm changes but I forgot to > do so when reviewing the patch for the bug... My concern is more with 40/80/160 channel widths; it seems to me that we should not have such large guard bands (which will just waste computation cycles covering hundreds of MHz). https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/wifi-utils.h File src/wifi/model/wifi-utils.h (right): https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/wifi-util... src/wifi/model/wifi-utils.h:41: double Log2 (double val); On 2018/02/27 06:56:55, Rediet wrote: > On 2018/02/26 22:37:57, S. Deronne wrote: > > Why? I also do not see any implementation > > I don't understand why this method declaration has been duplicated. I must have > made a mistake during merge. > Should I provide another patch for this? Just scrub from the next iteration.
Sign in to reply to this message.
Hi Tom, Thanks for your review. Taking into account the neighboring bands (even when >20 MHz) is quite heavy but also more precise than just modeling impact on immediately neighboring 20 MHz channels. Will upload a first patch taking into account all other comments, and then will add another one to limit the guard width to 20 MHz max (and document this limitation). I will try to get this done today. Rediet https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-... File src/spectrum/model/wifi-spectrum-value-helper.cc (right): https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-... src/spectrum/model/wifi-spectrum-value-helper.cc:429: Ptr<SpectrumModel> model = GetSpectrumModel (centerFrequency, channelWidth, bandBandwidth, static_cast<uint16_t> (guardBandwidth)); On 2018/03/01 21:43:05, Tom Henderson wrote: > why is a cast to uint16_t needed here? Sorry had mistaken it for a logging function. Will remove it. https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-... src/spectrum/model/wifi-spectrum-value-helper.cc:454: NS_LOG_FUNCTION (centerFrequency << static_cast<uint16_t> (channelWidth) << bandGranularity << static_cast<uint16_t> (guardBandwidth)); On 2018/03/01 21:43:05, Tom Henderson wrote: > same comment as above Here it's a normal logging function. By the way, will take this opportunity to take into account Sébastien latest changes concerning the output of uint8_t types (unary plus operator instead of static_cast). https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-... File src/spectrum/model/wifi-spectrum-value-helper.h (right): https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-... src/spectrum/model/wifi-spectrum-value-helper.h:195: * Normalize the transmit spectrum mask generated by CreateSepctrumMaskForOfdm On 2018/03/01 21:43:05, Tom Henderson wrote: > CreateSpectrumMaskForOfdm Acknowledged. https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-... File src/wifi/model/spectrum-wifi-phy.cc (right): https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-... src/wifi/model/spectrum-wifi-phy.cc:414: guardBandwidth = currentChannelWidth; On 2018/03/01 21:43:05, Tom Henderson wrote: > My concern is more with 40/80/160 channel widths; it seems to me that we should > not have such large guard bands (which will just waste computation cycles > covering hundreds of MHz). It'll indeed be quite heavy but also more precise than just modeling impact on immediately neighboring 20 MHz channels. Will upload a first patch taking into account all other comments, and then will add another one to limit the guard width to 20MHz max (and document this limitation). I will try to get this done today. https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/wifi-utils.h File src/wifi/model/wifi-utils.h (right): https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/wifi-util... src/wifi/model/wifi-utils.h:41: double Log2 (double val); On 2018/03/01 21:43:06, Tom Henderson wrote: > On 2018/02/27 06:56:55, Rediet wrote: > > On 2018/02/26 22:37:57, S. Deronne wrote: > > > Why? I also do not see any implementation > > > > I don't understand why this method declaration has been duplicated. I must > have > > made a mistake during merge. > > Should I provide another patch for this? > > Just scrub from the next iteration. Acknowledged.
Sign in to reply to this message.
Here's the first version (taking into account all comments except limitation of guard band) wrt to latest changset (Zoraze's commit n°13352).
Sign in to reply to this message.
On 2018/03/02 10:25:42, Rediet wrote: > Hi Tom, > > Thanks for your review. > Taking into account the neighboring bands (even when >20 MHz) is quite heavy but > also more precise than just modeling impact on immediately neighboring 20 MHz > channels. Will upload a first patch taking into account all other comments, and > then will add another one to limit the guard width to 20 MHz max (and document > this limitation). I will try to get this done today. > Rediet, my comment was based on the assumption that this was due to a code simplification, because it seemed to me unlikely that the ACI would be so large, but it appears that it may be; e.g. https://static.squarespace.com/static/54860cc3e4b020d690f237ba/548f5703e4b00e... So perhaps we should just leave it as you have it (guard band = channel width)? I would be fine with it, because I believe that SpectrumWifiPhy is for people who want this level of detail, and we have YansWifiPhy for those who want more abstraction.
Sign in to reply to this message.
On 2018/03/02 14:30:24, Tom Henderson wrote: > Rediet, my comment was based on the assumption that this was due to a code > simplification, because it seemed to me unlikely that the ACI would be so large, > but it appears that it may be; e.g. > > https://static.squarespace.com/static/54860cc3e4b020d690f237ba/548f5703e4b00e... > > So perhaps we should just leave it as you have it (guard band = channel width)? > I would be fine with it, because I believe that SpectrumWifiPhy is for people > who want this level of detail, and we have YansWifiPhy for those who want more > abstraction. OK, noted. I hadn't seen your mail until I posted the second phase (with limitation) so anyways we will have both if necessary. By the way, when working on this last patchset, I noticed a bug that I had introduced when correcting bug #2843 (not sure why I only noticed it now though, maybe because an assert was added for DSSS...). That's the main difference between patchsets 8 and 9 (patchset 10 is fine though); non-ERP DSSS was not handled when setting bandwidth. Hope patchset 9 (or 10) is good to be included in the coming release.
Sign in to reply to this message.
What is the current status of the latest patchset? We should decide to push this on Sunday so that Tom can prepare a RC on Monday. https://codereview.appspot.com/329500043/diff/180001/src/wifi/model/wifi-remo... File src/wifi/model/wifi-remote-station-manager.cc (right): https://codereview.appspot.com/329500043/diff/180001/src/wifi/model/wifi-remo... src/wifi/model/wifi-remote-station-manager.cc:830: if (modulationClass == WifiModulationClass::WIFI_MOD_CLASS_DSSS) // at 2.4 GHz basic rate can be non-ERP DSSS You forgot WIFI_MOD_CLASS_HR_DSSS, I will push directly this change.
Sign in to reply to this message.
On 2018/03/03 20:12:26, S. Deronne wrote: > What is the current status of the latest patchset? We should decide to push this > on Sunday so that Tom can prepare a RC on Monday. I'm fine with patchset 9; we can consider the optimization of patchset 10 if patchset 9 proves to be a performance problem at some point, I suppose.
Sign in to reply to this message.
On 2018/03/04 01:16:20, Tom Henderson wrote: > On 2018/03/03 20:12:26, S. Deronne wrote: > > What is the current status of the latest patchset? We should decide to push > this > > on Sunday so that Tom can prepare a RC on Monday. > > I'm fine with patchset 9; we can consider the optimization of patchset 10 if > patchset 9 proves to be a performance problem at some point, I suppose. OK, I will push changeset 9 to ns-3-dev
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
