Bugzilla – Bug 2843
Incorrect channel width and center frequency provided for non-HT PPDUs when building SpectralDensity
Last modified: 2018-01-15 04:40:04 EST
Created attachment 2988 [details] Proposed patch (based on 35b461a12b26 of Dec 30 2017) 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). 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). Also note that this implies using the MultiModelSpectrumChannel (since it supports SpectrumModel conversions whereas SingleModelSpectrumChannel doesn't). The attached patch enables to generate a PSD of correct size when falling back to vanilla OFDM.
OK, I agree with the patch. After patching, should we keep open with the issue that the primary channel is not always correct? We should probably put some regression tests in as well.
Thanks Tom. I also think that it would be good to keep track of it. I'll try to write a couple of tests on this subject and update the patch.
I do not really like the idea to post process the tx vector in AdjustChannelWidthToMode. IMO it should be possible to directly return the correct channel width in the wifi manager. Or am I missing something?
You're right, better keep it within WifiRemoteStationManager. At first sight, I thought that it would be easier in MacLow since the former had a lot of DoGetXX methods. I'll include these modifications in the new patch.
Created attachment 2992 [details] Updated patch (still based on 35b461a12b26 of Dec 30 2017) Tom, Sébastien, Here's the updated patch. As requested, the AdjustChannelWidthToMode is now in the WifiRemoteStationManager and a regression test has been added. I have to admit that it was quite laborious to find the correct traces enabling to test all the modifications of the patch (especially the frequency correction). The only way that I've found went through the addition of a new trace source to MultiModelSpectrumChannel (since defining a child class of the latter within wifi-test wasn't possible due to calls to SpectrumPhy class). Rediet
Rediet, thanks for your updated patch. I was also not thinking to post-processing the tx vector in the manager, I would expect something like (similar to what we have for preamble selection, see GetPreambleForTransmission): uint8_t GetChannelWidthForTransmission(WifiMode mode, uint8_t maxSupportedChannelWidth); And then call it this way in the manager: v.SetChannelWidth (GetChannelWidthForTransmission (mode, m_wifiPhy->GetChannelWidth ()));
Created attachment 2993 [details] 2nd update of patch (still based on 35b461a12b26 of Dec 30 2017) Sébastien, you'll find a new version of the patch using the signature you've provided. Hope that it corresponds to what you were expecting. Btw, I didn't have to apply the method to all rate managers since they often assume a channel width of either 20 or 22 MHz.
Thanks, I think it is almost ready (I only focused on the wifi part, not on the spectrum). Why do we need traffic for the tests? I am more in favor or unit testing this. I know other tests are also broader like yours, but I would like to have wifi tests independent of non-wifi modules. But I can still work on improvements in the future if you do not have time to rework this.
Created attachment 2994 [details] 3rd update of patch (based on 35b461a12b26 of Dec 30 2017) It was just simple copy-paste from existing code. I've swapped all IP and above mechanics by simple packet burst generation.
Thanks. Can you also add a check that we do not use larger channel widths than 20 MHz for legacy OFDM?
In the previously submitted test case there is already a check (line 1337) on the fact that the first tuple (of 20 MHz channel width) only concerns legacy OFDM (beacon, ack, RTS, CTS, and BA, considering the scenario). Do you want me to add another series of verification on the TxVectors generated by each remote station manager type?
(In reply to Rediet from comment #11) > In the previously submitted test case there is already a check (line 1337) > on the fact that the first tuple (of 20 MHz channel width) only concerns > legacy OFDM (beacon, ack, RTS, CTS, and BA, considering the scenario). > Do you want me to add another series of verification on the TxVectors > generated by each remote station manager type? OK. Can't you then add a bandwidth check on that tuple?
Created attachment 2998 [details] New update (based on 35b461a12b26 of Dec 30 2017) Had already done it in line 1334 (relative check), but rephrased it in this update (passed to absolute check) since it wasn't so obvious, nor easily extensible.
OK thanks, I think we can push this patch.
Pushed in changeset 13250:62c458e9789b
(In reply to Tom Henderson from comment #1) > OK, I agree with the patch. After patching, should we keep open with the > issue that the primary channel is not always correct? > > We should probably put some regression tests in as well. @Tom Just submitted bug #2855 to track this issue. @Sébastien, thanks for having pushed the patch.