ieee80211: establish Compressed BlockAck state foundations - #1272
mgonzalezlopezudc wants to merge 2 commits into
Conversation
Compressed BlockAck acknowledges several data frames with one bitmap. Its independent state foundation must validate agreements before radio integration can use them. Implement the approved steps 1 through 3. The user excluded rate publication, exchange admission, and radio integration from this change. The supported profile requires immediate HT operation and accepted peer capabilities. Receive status follows the negotiated window and survives packet delivery. One shared event follows the earliest absolute deadline. Expiry and DELBA remove only the selected agreement direction. Automatic agreement initiation remains disabled until radio integration. Both build modes pass. Three unit cases, one module case, three legacy protocol cases, and three aggregation fingerprint cases pass. The report records exact commands and existing global naming and interface defects. The worktree transfer preserves the validated source and test bytes. No recorded test expectation changes. Change: src.ieee80211 | behavior.add | test whatsnew migration | ieee80211-compressed-blockack
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 5 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if (dataHeader->getAckPolicy() == BLOCK_ACK) | ||
| agreement->blockAckPolicyFrameReceived(dataHeader); |
There was a problem hiding this comment.
🔴 Discarded data gets acknowledged
When distant data arrives, blockAckPolicyFrameReceived records receipt before ReceiveBuffer::insertFrame rejects it. The recipient acknowledges data it discarded.
Learn more
The receive record tracks acknowledgment status, while the reorder buffer retains packets for delivery. Their windows move independently: a distant sequence advances the record, but insertFrame rejects the packet outside its unchanged nextExpectedSequenceNumber window. Recording acknowledgment before that rejection tells the sender its discarded data arrived successfully.
Example: With both windows starting at 0 and size 64, sequence 100 advances the record to 37. The reorder buffer still starts at 0, rejects sequence 100, and deletes it; getAckState(100, 0) returns true.
Recommended fix: Keep record and reorder-buffer window movement consistent when accepting a distant sequence, or record acknowledgment only for packets that the recipient actually retains or delivers. Add a test for an arrival beyond the existing reorder buffer window.
Was this helpful? React with 👍 or 👎 to provide feedback.
| else { | ||
| throw cRuntimeError("Multi-Tid BlockAckReq is currently an unimplemented feature"); | ||
| } | ||
| agreement->getBlockAckRecord()->blockAckReqReceived(startingSequenceNumber); |
There was a problem hiding this comment.
🔴 First request desynchronizes receive windows
When a BAR arrives before data, blockAckReqReceived advances the record without creating a reorder buffer. Later data uses the original agreement start and gets discarded outside the stale reorder window.
Learn more
A recipient has an acknowledgment window in BlockAckRecord and a separate packet window in ReceiveBuffer. createReceiveBufferIfNecessary initializes the latter from the agreement's original starting sequence. When a BAR precedes all data, no buffer exists, so advancing only the record leaves future packet reception behind its current position.
Example: An agreement begins at sequence 0 with size 64. A BAR requests sequence 100 before data arrives, advancing the record to 100; the first sequence-100 packet creates a reorder buffer starting at 0 and is rejected.
Recommended fix: Synchronize the reorder buffer's starting/next-expected sequence with BAR window movement even when no buffer exists, or initialize newly created buffers from the current receive-record window. Test BAR-before-data with a sequence jump beyond the negotiated window.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (blockAckAgreementPolicy->isAddbaReqAccepted(addbaRequest) && | ||
| getAgreement(addbaRequest->getTid(), addbaRequest->getTransmitterAddress()) == nullptr) | ||
| addAgreement(addbaRequest, response); | ||
| else | ||
| response->setStatusCode(37); // REFUSED |
There was a problem hiding this comment.
🔴 Failed ADDBA response blocks negotiation
If an ADDBA response never transmits, getAgreement makes every retried request return REFUSED. Pending agreements never expire, so that peer and TID cannot negotiate again.
Learn more
An accepted ADDBA request installs an inactive recipient agreement immediately. The agreement becomes active only after processTransmittedAddbaResp, but getEarliestExpirationTime and blockAckAgreementExpired ignore inactive agreements. The existence check then refuses all subsequent requests, including a retry following a failed response transmission.
Example: Peer A requests TID 5; the recipient accepts but exhausts transmission retries on its ADDBA response. Peer A retries the request and receives REFUSED forever, while the inactive agreement remains installed.
Recommended fix: Define a failure/cancellation path for pending responses and allow valid retransmissions or replacement of pending agreements. Ensure a pending agreement cannot remain indefinitely after the response has been abandoned.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return fragmentNumber == 0 && | ||
| (sequenceNumber == startingSequenceNumber || startingSequenceNumber < sequenceNumber) && | ||
| sequenceNumber < startingSequenceNumber + windowSize && | ||
| containsKey(acknowledgmentState, SequenceControlField(sequenceNumber.get(), fragmentNumber)); |
There was a problem hiding this comment.
🟡 Delivered packets lose old-window acknowledgment
When a Basic BAR starts before startingSequenceNumber, getAckState returns false for previously delivered packets. The resulting bitmap asks the sender to retransmit them.
Learn more
The receive record retains acknowledgment status only for its current sequence window. buildBlockAck queries getAckState for all 64 sequence positions from the request's start. The previous implementation returned true for positions older than its retained status; the new window guard instead returns false and encodes missing data for already received packets.
Example: The record advances from sequence 10 to 20 after delivering sequence 10. A Basic BAR starting at 10 asks for sequence 10; the bitmap now marks it missing instead of acknowledged.
Recommended fix: Return acknowledged status for sequences known to precede the current receive window, while retaining the false result for missing positions inside the window. Cover an older BAR across sequence wrap in a focused test.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (header->getTransmitterAddress() != originatorAddress || header->getTid() != tid || | ||
| header->getType() != ST_DATA_WITH_QOS || header->getAckPolicy() != BLOCK_ACK || | ||
| header->getFragmentNumber() != 0 || header->getMoreFragments() || header->isIncorrect()) | ||
| return; |
There was a problem hiding this comment.
🟡 Fragmented data loses Basic BlockAck status
For fragmented BlockAck data, blockAckPolicyFrameReceived ignores every later fragment and every nonfinal first fragment. Basic BlockAck bitmaps then mark received fragments missing.
Learn more
The previous receive record stored acknowledgment by sequence and fragment number. The existing buildBlockAck still constructs Basic BlockAck bitmaps with 16 fragment bits per sequence. Rejecting all fragments prevents those bitmaps from reflecting received fragmented MPDUs, even though the new Compressed BlockAck profile is unfragmented.
Example: A Basic BlockAck agreement receives fragment 0 with moreFragments=true and fragment 1 with moreFragments=false. Neither fragment enters the record, and both bitmap bits remain zero.
Recommended fix: Preserve fragment-specific status for Basic BlockAck agreements, or explicitly restrict the receive-record API and callers to Compressed BlockAck while keeping the legacy Basic record behavior. Test fragmented Basic BlockAck reception.
Was this helpful? React with 👍 or 👎 to provide feedback.
The reorder buffer can discard data after the receipt record sets its BlockAck bit. The mixed-policy test receives Normal Ack sequence 100 and Block Ack sequence 10 from windows at 0. The old frame gains receipt status despite discard. Move the reorder window before admission. Update receipt state for each valid related Data frame. Keep delivery and inactivity refresh separate from receipt status. An ADDBA response can disappear before its first transmission callback. The inactive agreement then refuses every repeated request. Reuse a matching pending transaction or replace it for a new dialog token. Reject stale response tokens before activation of the replacement. Timeout expiry removes local state before its DELBA enters the queue. A new agreement can start before that DELBA transmits. Its late callback must preserve the replacement agreement and receive buffer. Cancel obsolete unreferenced timeout frames through the queue, ACK, and recovery owners. Preserve active direct and RTS frame references. The debug build passes. Three unit tests and one module test pass. Three legacy protocol tests and three aggregation fingerprints pass. No recorded expectation changes. Compressed BAR integration remains pending. Change: src.ieee80211 | behavior.change.fix | test whatsnew | ieee80211-compressed-blockack
This branch has not been deployed
1 participant
Compressed BlockAck needs a supported wire format and validated agreement state before the MAC can use it in a radio exchange. This change adds those foundations in one commit.
The change:
The complete Compressed BlockAck radio exchange and aggregate data session remain incomplete. Radio integration still requires rate publication and exchange admission. The tests establish wire format and state behavior; they do not establish a Compressed BlockAck radio exchange.
The default
isBlockAckSupported = falseretains normal acknowledgments. Enabled configurations require explicit agreement setup and no longer start the former automatic session. The user guide, developer guide, migration guide, andWHATSNEWdescribe these limits.The architectural surface covers the existing frame representation, serializer, agreement policies and handlers, receive record, recipient data service, and HCF. Both policies gain
mibModule = default("^.^.^.mib").External implementations of these contracts require the new pure virtual methods:
IOriginatorBlockAckAgreementHandler:getEarliestExpirationTime() const.IRecipientBlockAckAgreementHandler:getEarliestExpirationTime() constandprocessReceivedBlockAckReq().IBlockAckAgreementHandlerCallback:rescheduleInactivityTimer(),blockAckAgreementTerminated(cObject *), andrecipientAgreementTerminated(MacAddress, Tid).IRecipientQosMacDataService:clearBlockAckReceiveBuffer(MacAddress, Tid).The existing
scheduleInactivityTimer(simtime_t)method remains deprecated for one release. The change adds no production class, NED type, frame type, or feature descriptor. It changes no sealed source path or exception ledger.Validation used a separate checkout on the same upstream base. All 32 source and test files match the saved validation manifest. The four release and guide files contain later documentation edits.
Both compilation checks passed with exit status 0:
The focused tests loaded the fresh debug library. These commands passed with exit status 0: three unit cases, one module case, and three legacy protocol cases.
This command passed from
tests/fingerprintwith exit status 0:It selected
NoAggregation,Aggregation, andVoicePriorityAggregation, each with run 0 and the configured seed. All three cases matched their recorded expectations. No baseline changed.The scoped architecture check and checks of the four changed interfaces passed. Global checks reported 15 existing interface violations and 27 existing naming findings. The changed declarations introduced no naming finding. The existing frame-step bodies appear in
AV-CONTRACT-02; existing package problems appear inNV-02andNV-03.Before submission, these checks passed against the current upstream base:
The existing automatic-session protocol tests await the dependent integration step. The saved self-audit does not constitute an independent review.