Bugzilla – Bug 2739
HtCapabilities/VhtCapabilities/HeCapabilities subfield binary coding changes
Last modified: 2017-06-14 16:55:40 UTC
Created attachment 2843 [details] Proposed patch (diff with e31ae85) This patch covers the following aspects of HtCapabilities/VhtCapabilities/HeCapabilities fields and takes commit e31ae85 of ns-3-dev-git (2017-05-16) as reference: - ht-capabilities: * bug: SetTxMaxNSpatialStreams binary encoding should follow standard * evolution: add getter for m_minMpduStartSpace - vht-capabilities: * bug: m_rxMcsMap and m_txMcsMap should be filled with value 3 upon construction (to indicate that no spatial stream supported by default) - regular-wifi-mac: * evolution: no longer hard code max A-MSDU and max A-MPDU subfields for HtCapabilities and VhtCapabilities. Fill the latter subfield for HeCapabilities based on VHT specs for the time being Considering that Capabilities binary encoding is addressed by all changes, I reported them all in this bug. If need be, I can split it in 2 or 3 (one per class).
Thanks for this, I am fine with the changes. I just wonder why you need to do a max with 3 in SetMaxAmpduLength? Is the cast also needed?
I'm glad that you're OK with the proposed modifications. As for capping the max A-MPDU exponent in HT Capabilities, it is due to the fact that the "Maximum A-MPDU Length Exponent" subfield of the "A-MPDU Parameters" field is coded using 2 bits; there is an explicit indication that the "field is an integer in the range 0 to 3". It was thus to avoid the case where a VHT STA with >65kB would induce errors without such a capping. As for the cast, it may indeed be unnecessary; Eclipse had hinted an error without it...
Ok, I would then just remove the cast and the patch is good to be pushed for me.
(In reply to sebastien.deronne from comment #3) > Ok, I would then just remove the cast and the patch is good to be pushed for > me. I might try to change uint8_t (std::max (7.0, std::ceil (std::log (maxAmpduLength + 1.0) - 13.0))) to something like std::max<uint8_t> (7, static_cast<uint8_t> (std::ceil (std::log (maxAmpduLength + 1.0) - 13)))) and can you guarantee that the std::ceil produces a double value between 0 and 255
Rediet, can you please provide a final patch addressing Tom's comments?
Created attachment 2868 [details] Proposed patch (diff with e31ae85) -> modified as per Tom's comments Sorry for the delay. Here is the updated patch as per Tom's comments.
Thanks, I think this can be merged to ns-3-dev.
I see many tests failing with this assert so I cannot push: assert failed. cond="maxAmpduLengthExponent >= 0 && maxAmpduLengthExponent <= 255", file=../src/wifi/model/regular-wifi-mac.cc, line=163 Rediet, could you please investigate?
Created attachment 2869 [details] Proposed patch (diff with e31ae85) -> modified as per Tom's comments and tested Sorry I forgot to test it. With this modified version all tests pass (with commit e31ae85, excluding 10 skipped py tests). I had forgotten the following 2 points: - divide the log(max+1) by log(2) in order to retrieve the 2^x exponent - put a lower limit of 0 to the exponent (in case A-MPDU aggregation is forbidden or configured with sizes lower than 2^13)
Thanks, I will push your changes.
Patch is pushed in changeset 12931:c6e34fd04597. I also added Rediet to the author list.