Code review - Issue 329500043: Wi-Fi adjacent channel modeling through transmit spectrum mask (OFDM)https://codereview.appspot.com/2018-03-04T09:07:24+00:00rietveld
Message from unknown
2017-10-17T13:47:11+00:00Redieturn:md5:093643475ddacb2651d99f38f05a6ce9
Message from redieteab.orange@gmail.com
2017-10-18T08:35:32+00:00Redieturn:md5:074571044ac60daec530a86d77840f34
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
Message from sebastien.deronne@gmail.com
2017-10-24T19:26:05+00:00S. Deronneurn:md5:6a655e1bca79c0a2204cb9a1f8cb7b62
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.
Message from sebastien.deronne@gmail.com
2017-10-24T19:26:16+00:00S. Deronneurn:md5:4542f6ef0220db351351ee97efceda48
https://codereview.appspot.com/329500043/diff/1/src/wifi/model/spectrum-wifi-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/1/src/wifi/model/spectrum-wifi-phy.cc#newcode381
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.
Message from sebastien.deronne@gmail.com
2017-10-24T19:27:25+00:00S. Deronneurn:md5:135a83d647281bb824e2fe3cdbd741e7
https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-gnuplot.plg
File src/wifi/examples/wifi-trans-gnuplot.plg (right):
https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-gnuplot.plg#newcode42
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.
Message from micha.rademacher@gmail.com
2017-10-25T13:16:19+00:00micha.rademacherurn:md5:52ce316668ec82336466069f56ca5102
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
Message from micha.rademacher@gmail.com
2017-10-25T13:16:47+00:00micha.rademacherurn:md5:f8fb35a67e5a9f5bf7882492ff7be7e4
Some c++ compile erros.
https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode249
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-spectrum-value-helper.cc#newcode250
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-spectrum-value-helper.cc#newcode373
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-spectrum-value-helper.cc#newcode374
src/spectrum/model/wifi-spectrum-value-helper.cc:374: Bands::const_iterator bit = c->ConstBandsBegin ();
unused-but-set-variable
Message from redieteab.orange@gmail.com
2017-10-30T14:54:32+00:00Redieturn:md5:5079d460c4824cbddd6a292986fdf45c
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.
Message from redieteab.orange@gmail.com
2017-10-30T15:07:57+00:00Redieturn:md5:0b54782d539a347f04042466ba7aa12b
Thanks Sébastien, thanks Michael.
https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/1/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode249
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-spectrum-value-helper.cc#newcode250
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-spectrum-value-helper.cc#newcode373
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-spectrum-value-helper.cc#newcode374
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-gnuplot.plg
File src/wifi/examples/wifi-trans-gnuplot.plg (right):
https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-gnuplot.plg#newcode42
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-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/1/src/wifi/model/spectrum-wifi-phy.cc#newcode381
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.
Message from redieteab.orange@gmail.com
2017-10-30T15:13:34+00:00Redieturn:md5:1c0c4a1eeecff53494b522a47a616186
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
Message from redieteab.orange@gmail.com
2017-10-30T16:21:42+00:00Redieturn:md5:97e101959e28c95bc2931be7ac483c14
https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-gnuplot.plg
File src/wifi/examples/wifi-trans-gnuplot.plg (right):
https://codereview.appspot.com/329500043/diff/1/src/wifi/examples/wifi-trans-gnuplot.plg#newcode42
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.
Message from redieteab.orange@gmail.com
2017-10-31T12:52:09+00:00Redieturn:md5:11c1cc75f037dd8b3d174b14a470a736
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
Message from unknown
2017-11-07T12:52:28+00:00Redieturn:md5:288dfd2620f1d79589d6f28f6a9f7b13
Message from redieteab.orange@gmail.com
2017-11-07T13:05:52+00:00Redieturn:md5:592af3a117c9c0caab319123c2580fd9
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-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode401
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-spectrum-value-helper.cc#newcode412
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
Message from sebastien.deronne@gmail.com
2017-11-07T19:08:22+00:00S. Deronneurn:md5:ef0fa59ee04182af75f206f22e970a8b
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-spectrum-value-h.cc
File src/wifi/test/wifi-test-spectrum-value-h.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-spectrum-value-h.cc#newcode30
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-spectrum-value-h.cc#newcode85
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-spectrum-value-h.cc#newcode718
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.
Message from redieteab.orange@gmail.com
2017-11-08T10:51:41+00:00Redieturn:md5:2febce5b6f35196f57ea634a1842ee04
Thanks Sébastien.
My replies below.
https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-spectrum-value-h.cc
File src/wifi/test/wifi-test-spectrum-value-h.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-spectrum-value-h.cc#newcode30
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-spectrum-value-h.cc#newcode85
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-spectrum-value-h.cc#newcode718
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.
Message from sebastien.deronne@gmail.com
2017-11-09T07:15:17+00:00S. Deronneurn:md5:a522581259cce0291f2e041d97ac4b18
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode175
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-spectrum-value-helper.cc#newcode190
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-spectrum-value-helper.cc#newcode191
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-spectrum-value-helper.cc#newcode228
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-spectrum-value-helper.cc#newcode523
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-spectrum-value-helper.cc#newcode533
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-spectrum-value-helper.cc#newcode536
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-spectrum-value-helper.cc#newcode539
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-wifi-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-wifi-phy.cc#newcode366
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-wifi-phy.cc#newcode381
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-spectrum-value-h.cc
File src/wifi/test/wifi-test-spectrum-value-h.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-spectrum-value-h.cc#newcode30
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-spectrum-value-h.cc#newcode85
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-spectrum-value-h.cc#newcode718
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.
Message from redieteab.orange@gmail.com
2017-11-22T16:22:39+00:00Redieturn:md5:0822e289bb2a052a058feb81add33b7d
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-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode175
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-spectrum-value-helper.cc#newcode190
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-spectrum-value-helper.cc#newcode191
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-spectrum-value-helper.cc#newcode228
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-spectrum-value-helper.cc#newcode523
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-spectrum-value-helper.cc#newcode533
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-spectrum-value-helper.cc#newcode536
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-spectrum-value-helper.cc#newcode539
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-wifi-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-wifi-phy.cc#newcode366
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-wifi-phy.cc#newcode381
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-spectrum-value-h.cc
File src/wifi/test/wifi-test-spectrum-value-h.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-spectrum-value-h.cc#newcode30
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-spectrum-value-h.cc#newcode85
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-spectrum-value-h.cc#newcode718
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?
Message from sebastien.deronne@gmail.com
2017-11-22T16:53:20+00:00S. Deronneurn:md5:85334362782c8995dc0d329489851b37
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode175
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-spectrum-value-helper.cc#newcode190
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-spectrum-value-helper.cc#newcode191
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-spectrum-value-helper.cc#newcode228
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-spectrum-value-helper.cc#newcode539
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-spectrum-value-h.cc
File src/wifi/test/wifi-test-spectrum-value-h.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-spectrum-value-h.cc#newcode718
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.
Message from unknown
2017-12-07T14:52:54+00:00Redieturn:md5:4619dded852477c8c8e3681fc55b3c6a
Message from redieteab.orange@gmail.com
2017-12-07T15:07:47+00:00Redieturn:md5:8b65f2d85746835ecef0ed38c9e363e7
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-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode175
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-spectrum-value-helper.cc#newcode191
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-spectrum-value-helper.cc#newcode228
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-spectrum-value-helper.cc#newcode523
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-spectrum-value-helper.cc#newcode533
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-spectrum-value-helper.cc#newcode539
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-wifi-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/wifi/model/spectrum-wifi-phy.cc#newcode366
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-wifi-phy.cc#newcode381
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-spectrum-value-h.cc
File src/wifi/test/wifi-test-spectrum-value-h.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/wifi/test/wifi-test-spectrum-value-h.cc#newcode30
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-spectrum-value-h.cc#newcode85
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-spectrum-value-h.cc#newcode718
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.
Message from sebastien.deronne@gmail.com
2017-12-09T16:24:11+00:00S. Deronneurn:md5:d0c3a7f64ccf5135d8d68fd51e24ffaf
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-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode228
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-spectrum-value-helper.cc#newcode533
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-spectrum-value-helper.cc#newcode539
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-spectrum-value-helper.h
File src/spectrum/model/wifi-spectrum-value-helper.h (right):
https://codereview.appspot.com/329500043/diff/40001/src/spectrum/model/wifi-spectrum-value-helper.h#newcode183
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.
Message from skalpaiit@gmail.com
2017-12-15T08:36:11+00:00skalpaiiturn:md5:d756023e6efa25a0bf6d767a075503d9
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
>
>
Message from unknown
2017-12-15T15:05:00+00:00Redieturn:md5:2fdcb2243ef44b9f3d3a46f99571878c
Message from redieteab.orange@gmail.com
2017-12-15T15:05:44+00:00Redieturn:md5:a581a26a70225eabf87e5c60c3c04c20
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-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode228
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-spectrum-value-helper.cc#newcode533
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-spectrum-value-helper.cc#newcode539
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-spectrum-value-helper.h
File src/spectrum/model/wifi-spectrum-value-helper.h (right):
https://codereview.appspot.com/329500043/diff/40001/src/spectrum/model/wifi-spectrum-value-helper.h#newcode183
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.
Message from unknown
2018-01-05T10:59:47+00:00Redieturn:md5:b8b1d38e7b7ac2159b645a5a9256cb09
Message from redieteab.orange@gmail.com
2018-01-05T11:10:06+00:00Redieturn:md5:86a93f650489bbe769d64e42c28f4fa6
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-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/20001/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode228
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).
Message from unknown
2018-01-12T14:29:06+00:00Redieturn:md5:70bc9566a57ababfb7c07547dba023f5
Message from redieteab.orange@gmail.com
2018-01-12T14:31:35+00:00Redieturn:md5:69780beb1eedb3ba2509be613ac24ec7
Updated version following Sebastien's recommendation in bug 2483.
Message from sebastien.deronne@gmail.com
2018-02-24T09:57:29+00:00S. Deronneurn:md5:2af85eb1437a43dbdfe563ca9d4b7cf9
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-spectrum-saturation-example.cc
File examples/wireless/wifi-spectrum-saturation-example.cc (right):
https://codereview.appspot.com/329500043/diff/100001/examples/wireless/wifi-spectrum-saturation-example.cc#newcode202
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-trans-example.sh
File src/wifi/examples/wifi-trans-example.sh (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/examples/wifi-trans-example.sh#newcode145
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-rate-wifi-manager.cc
File src/wifi/model/constant-rate-wifi-manager.cc (left):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/constant-rate-wifi-manager.cc#oldcode128
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-wifi-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-wifi-phy.cc#newcode295
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-wifi-phy.cc#newcode303
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-wifi-phy.cc#newcode306
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-wifi-phy.cc#newcode318
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-wifi-phy.cc#newcode320
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-wifi-phy.cc#newcode327
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-wifi-phy.cc#newcode335
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-wifi-phy.cc#newcode378
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-wifi-phy.cc#newcode396
src/wifi/model/spectrum-wifi-phy.cc:396: guardBandwidth = GetChannelWidth ();
why?
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-wifi-phy.h
File src/wifi/model/spectrum-wifi-phy.h (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-wifi-phy.h#newcode159
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-remote-station-manager.cc
File src/wifi/model/wifi-remote-station-manager.cc (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remote-station-manager.cc#newcode840
src/wifi/model/wifi-remote-station-manager.cc:840: return maxSupportedChannelWidth;
already pushed
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remote-station-manager.h
File src/wifi/model/wifi-remote-station-manager.h (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remote-station-manager.h#newcode1088
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-wifi-phy-test.cc
File src/wifi/test/spectrum-wifi-phy-test.cc (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/test/spectrum-wifi-phy-test.cc#newcode34
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.cc#newcode1367
src/wifi/test/wifi-test.cc:1367: AddTestCase (new Bug2483TestCase, TestCase::QUICK); //Bug 2483
already pushed
Message from sebastien.deronne@gmail.com
2018-02-24T09:59:39+00:00S. Deronneurn:md5:c9b243079640b68f1820386e4e19755f
https://codereview.appspot.com/329500043/diff/100001/examples/wireless/wifi-spectrum-saturation-example.cc
File examples/wireless/wifi-spectrum-saturation-example.cc (right):
https://codereview.appspot.com/329500043/diff/100001/examples/wireless/wifi-spectrum-saturation-example.cc#newcode202
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.
Message from unknown
2018-02-26T16:09:02+00:00Redieturn:md5:8485e68f7560f1313a24df844e05d702
Message from redieteab.orange@gmail.com
2018-02-26T16:22:35+00:00Redieturn:md5:d2ba81d135f1f96de69d9663d775a1dd
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-spectrum-saturation-example.cc
File examples/wireless/wifi-spectrum-saturation-example.cc (right):
https://codereview.appspot.com/329500043/diff/100001/examples/wireless/wifi-spectrum-saturation-example.cc#newcode202
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-trans-example.sh
File src/wifi/examples/wifi-trans-example.sh (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/examples/wifi-trans-example.sh#newcode145
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-rate-wifi-manager.cc
File src/wifi/model/constant-rate-wifi-manager.cc (left):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/constant-rate-wifi-manager.cc#oldcode128
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-wifi-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-wifi-phy.cc#newcode295
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-wifi-phy.cc#newcode303
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-wifi-phy.cc#newcode306
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-wifi-phy.cc#newcode318
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-wifi-phy.cc#newcode320
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-wifi-phy.cc#newcode327
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-wifi-phy.cc#newcode335
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-wifi-phy.cc#newcode378
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-wifi-phy.cc#newcode396
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-wifi-phy.h
File src/wifi/model/spectrum-wifi-phy.h (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/spectrum-wifi-phy.h#newcode159
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-remote-station-manager.cc
File src/wifi/model/wifi-remote-station-manager.cc (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remote-station-manager.cc#newcode840
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-remote-station-manager.h
File src/wifi/model/wifi-remote-station-manager.h (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/model/wifi-remote-station-manager.h#newcode1088
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-wifi-phy-test.cc
File src/wifi/test/spectrum-wifi-phy-test.cc (right):
https://codereview.appspot.com/329500043/diff/100001/src/wifi/test/spectrum-wifi-phy-test.cc#newcode34
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.cc#newcode1367
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.
Message from sebastien.deronne@gmail.com
2018-02-26T22:37:58+00:00S. Deronneurn:md5:0c3a3d07a3404ee56fac78fca3aee221
https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-wifi-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-wifi-phy.cc#newcode395
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-wifi-phy.cc#newcode414
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-utils.h#newcode41
src/wifi/model/wifi-utils.h:41: double Log2 (double val);
Why? I also do not see any implementation
Message from redieteab.orange@gmail.com
2018-02-27T06:56:55+00:00Redieturn:md5:497f48f810d1bc11b1218927862567b4
https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-wifi-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-wifi-phy.cc#newcode395
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-wifi-phy.cc#newcode414
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-utils.h#newcode41
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?
Message from tomh.org@gmail.com
2018-03-01T21:43:06+00:00Tom Hendersonurn:md5:f32307d3288ba35f3606d1e391d81b8c
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-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode429
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-spectrum-value-helper.cc#newcode454
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-spectrum-value-helper.h
File src/spectrum/model/wifi-spectrum-value-helper.h (right):
https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-spectrum-value-helper.h#newcode195
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-wifi-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-wifi-phy.cc#newcode414
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-utils.h#newcode41
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.
Message from redieteab.orange@gmail.com
2018-03-02T10:25:42+00:00Redieturn:md5:526117e5964868778d760bb1f4b80e5f
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-spectrum-value-helper.cc
File src/spectrum/model/wifi-spectrum-value-helper.cc (right):
https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-spectrum-value-helper.cc#newcode429
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-spectrum-value-helper.cc#newcode454
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-spectrum-value-helper.h
File src/spectrum/model/wifi-spectrum-value-helper.h (right):
https://codereview.appspot.com/329500043/diff/120001/src/spectrum/model/wifi-spectrum-value-helper.h#newcode195
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-wifi-phy.cc
File src/wifi/model/spectrum-wifi-phy.cc (right):
https://codereview.appspot.com/329500043/diff/120001/src/wifi/model/spectrum-wifi-phy.cc#newcode414
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-utils.h#newcode41
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.
Message from unknown
2018-03-02T11:06:37+00:00Redieturn:md5:525d6ade2989346fd64f7711626c7cbe
Message from redieteab.orange@gmail.com
2018-03-02T11:09:00+00:00Redieturn:md5:fc320a6c237a7ba27ebbbe92a96c37ea
Here's the first version (taking into account all comments except limitation of guard band) wrt to latest changset (Zoraze's commit n°13352).
Message from tomh.org@gmail.com
2018-03-02T14:30:24+00:00Tom Hendersonurn:md5:ea6f8c5598ca5e3ef862b4b5012a4967
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/548f5703e4b00e8ec160e2fb/548f5704e4b00e8ec160e344/1418680068694/1000w/
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.
Message from unknown
2018-03-02T15:29:54+00:00Redieturn:md5:5cc85459692252e7af324eca7ec70ca1
Message from unknown
2018-03-02T15:38:41+00:00Redieturn:md5:268479f2434101eea6b23a94307c2110
Message from redieteab.orange@gmail.com
2018-03-02T15:45:58+00:00Redieturn:md5:f43327207f3ee3bf3c0554c031af23f9
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/548f5703e4b00e8ec160e2fb/548f5704e4b00e8ec160e344/1418680068694/1000w/
>
> 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.
Message from sebastien.deronne@gmail.com
2018-03-03T20:12:26+00:00S. Deronneurn:md5:0cc32fcb0bb2efc741a5fea39d9897ca
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-remote-station-manager.cc
File src/wifi/model/wifi-remote-station-manager.cc (right):
https://codereview.appspot.com/329500043/diff/180001/src/wifi/model/wifi-remote-station-manager.cc#newcode830
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.
Message from tomh.org@gmail.com
2018-03-04T01:16:20+00:00Tom Hendersonurn:md5:c836f28ddea71976f3bfaf9d2795f375
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.
Message from sebastien.deronne@gmail.com
2018-03-04T09:07:24+00:00S. Deronneurn:md5:7bfbd831ae2dd26addf6c6009f4da293
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