Bugzilla – Bug 2379
BlockAckRequest should not be part of single-TID A-MPDUs
Last modified: 2016-06-17 08:08:11 UTC
Created attachment 2393 [details] proposed workaround I got a comment recently about whether or not BlockAckReq should belong to A-MPDUs. Checking more carefully the standard and some reference books, the current implementation is not correct and we should not carry BlockAckRequest in single-TID A-MPDUs. Table 8-284 in the standard: At most one BlockAckReq frame with a TID that corresponds to an HT-immediate Block Ack agreement. This is the last MPDU in the A-MPDU. It is not present if any QoS data frames for that TID are present. The last sentence indicate it is only ok to have BlockAckRequest in multi-TID A-MPDUs, which is not currently supported in ns-3 (a new thread will be opened later to support the feature). Since BlockAckRequest in multi-TID A-MPDUs will be later supported and will reuse part of the code that has been developed so far, I suggest the attached workaround which avoid to remove/change a lot in our current code.
Created attachment 2412 [details] proposed patch to fix Since Multi-TID A-MPDUs are only possible when PSMP (Power Save Multi-Poll) is enabled, a BlockAckReq transmission when PSMP is not supported should never be carried in an A-MPDU (because of previously mentioned section in the standard). This new patch introduces the setting of the PSMP capability flag (which will always be false for now since it is not yet supported). This is already a step forward compared to the previous patch. Furthermore, this patch suggests to return quicker once we detect that we won't perform aggregation.
(In reply to sebastien.deronne from comment #1) > Created attachment 2412 [details] > proposed patch to fix > > Since Multi-TID A-MPDUs are only possible when PSMP (Power Save Multi-Poll) > is enabled, a BlockAckReq transmission when PSMP is not supported should > never be carried in an A-MPDU (because of previously mentioned section in > the standard). > > This new patch introduces the setting of the PSMP capability flag (which > will always be false for now since it is not yet supported). This is already > a step forward compared to the previous patch. Furthermore, this patch > suggests to return quicker once we detect that we won't perform aggregation. I don't understand this proposal; it seems to me to be misleading to advertise PSMP capability in an attribute if it is not really supported.
I know it is a bit tricky, but we actually have this for few other parameters (but we do not have any assert saying it is not currently supported). The other solution is to simply return in AggregateToAmpdu when the packet is a BlockAckReq, and it will be there that we will later have to do some checks if once PSMP is supported.
(In reply to sebastien.deronne from comment #3) > I know it is a bit tricky, but we actually have this for few other > parameters (but we do not have any assert saying it is not currently > supported). > > The other solution is to simply return in AggregateToAmpdu when the packet > is a BlockAckReq, and it will be there that we will later have to do some > checks if once PSMP is supported. I suggest (In reply to sebastien.deronne from comment #3) > I know it is a bit tricky, but we actually have this for few other > parameters (but we do not have any assert saying it is not currently > supported). > > The other solution is to simply return in AggregateToAmpdu when the packet > is a BlockAckReq, and it will be there that we will later have to do some > checks if once PSMP is supported. If you want to leave it as a hint that it should be supported in a particular way in the future, you could do something like: #if 0 .AddAttribute ("PowerSaveMultiPollSupported", ... #endif or #ifdef NOTYETSUPPORTED but I'm not in favor of providing attributes that could be misleading about what they actually do at present, so I suggest to either do the above or delete for now.
Created attachment 2417 [details] proposed workaround I changed the patch to simply have a return if we detect the frame is a BlockAckReq. I added a comment to indicate that this will have to be adapted once we support multi-TID A-MPDUs.
*** Bug 2417 has been marked as a duplicate of this bug. ***
As I was suspecting, this patch fixes a lot of other incoming issues. I think we should really focus on this one.
Waiting few more days before pushing, but it definitely solves a lot of issues I can still see. Another solution is to cleanup every part that uses BlockAckReq in A-MPDU, but then lots of things will have to be re-implemented once we support multi-TID (but not sure we will one day, it is an optional feature). But this could still be handled later. Any last comments?
changeset 12166:78f607741271