|
Bugzilla – Full Text Bug Listing |
| Summary: | HtCapabilities/VhtCapabilities/HeCapabilities subfield binary coding changes | ||
|---|---|---|---|
| Product: | ns-3 | Reporter: | Rediet <redieteab.orange> |
| Component: | wifi | Assignee: | sebastien.deronne |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | CC: | ns-bugs, tomh |
| Priority: | P3 | ||
| Version: | ns-3-dev | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: |
Proposed patch (diff with e31ae85)
Proposed patch (diff with e31ae85) -> modified as per Tom's comments Proposed patch (diff with e31ae85) -> modified as per Tom's comments and tested |
||
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. |
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).