Bug 2313

Created attachment 2309 [details]
proposed fix to solve this bug

The problem was that we did not remove the Ampdu subframe header in ReceiveError, resulting in an incorrect address field value returned by GetAddr1.
The patch is removing the Ampdu subframe header before peeking the WifiMacHeader.

Important fix to be delivered, so setting to LAST CALL

(In reply to sebastien.deronne from comment #1)
> Created attachment 2309 [details]
> proposed fix to solve this bug

I also have scenario affected by this bug and the patch fixes it in my case.

Why not commit it without waiting for release?

(In reply to Alexander Krotov from comment #3)
> (In reply to sebastien.deronne from comment #1)
> > Created attachment 2309 [details]
> > proposed fix to solve this bug
> 
> I also have scenario affected by this bug and the patch fixes it in my case.
> 
> Why not commit it without waiting for release?

We do not wait for release, we will push it normally today.
I wait a little bit since Tom told me yesterday he saw some issues with the patches that are planned to be delivered for the release, so I will first check what is causing the issue.

+1
Bear in mind that fix triggers bug 2315 in some cases.
Works for me if both fixes are applied.

pushed in changeset 11945:96b721d35158

I reopen this bug because I'm getting an assert fail when a CTS is missed.
msg="Current packet is not Qos Data", file=../src/wifi/model/edca-txop-n.cc, line=670

The problem happens when the method MissedCts is invoked.

I assume your scenario considers hidden nodes?
I wanna try first to reproduce.

Matias, could you please attach a test case?
I could not reproduce the bug so far.

(In reply to sebastien.deronne from comment #9)
> Matias, could you please attach a test case?
> I could not reproduce the bug so far.

Sorry I was trying to find an easy test case but I couldn't.

It appears when testing new minstrel-ht code with rate-adaptation-distance from here: https://codereview.appspot.com/283960043/

Just run ./waf --run "rate-adaptation-distance --STA1_x=100"

I found that the asserts fails because it is comparing against a BACK Request header.
-A BACK Req is sent as part of an A-MPDU
-The A-MPDU needs RTS/CTS 
-The CTS is never received.
-EdcaTxopN::m_currentHdr is the header of the BACK Request and QosData is expected.

Created attachment 2336 [details]
patch to fix

fixed in changeset 12002:e6f2231c1a96

Created attachment 2371 [details]
Patch to fix similar bug for the case without RTS/CTS

The same bugfix should be applied to MissedAck case. I run some experiments with different RTS threshold and NS_FATAL_ERROR is triggered sometimes. Attached is a patch that fixes the issue.

By the way, functions EdcaTxopN::MissedAck and EdcaTxopN::MissedCts are almost identical, but have small differences. For example, MissedCts calls m_low->FlushAggregateQueue (), but MissedAck does not. Is it correct? The common part should probably be factored out.

Created attachment 2372 [details]
Refactoring patch

This patch fixes the case without RTS/CTS and also fixes the same bug for BlockAck *response*. Common code is moved to the function EdcaTxopN::GetCurrentTid ().

Also, with this 
(In reply to Alexander Krotov from comment #15)
> Created attachment 2372 [details]
> Refactoring patch

With this patch applied I also sometimes get NS_FATAL_ERROR for this type of packets:
ns3::WifiActionHeader (category=BlockAck, value=PeerLinkOpen) ns3::MgtAddBaResponseHeader (status code=success)

Probably need to extract Tid for it too.

Created attachment 2373 [details]
Extract TID from BlockAck action frames

This patch should be applied on top of attachment 2372 [details].

Could you include a test case showing that your patch indeed fixes the issue?

It looks quite ok though, but I suggest to use peekheader iso removeheader, and to sign the new GetCurrentTid fct with const.

Created attachment 2387 [details]
Mark GetCurrentTid const

3rd patch in a patch series

Created attachment 2388 [details]
Use PeekHeader instead of RemoveHeader

4th patch

It looks good now, just also packet->RemoveHeader (actionHdr) should be replaced with packet->PeekHeader (actionHdr).

(In reply to sebastien.deronne from comment #21)
> It looks good now, just also packet->RemoveHeader (actionHdr) should be
> replaced with packet->PeekHeader (actionHdr).

It would be wrong, I need to look at MgtAddBaResponseHeader that is below WifiActionHeader, so I need to remove WifiActionHeader first.

Ok, got it, but we thus need to be sure the header is not expected to be retrieved elsewhere.

(In reply to sebastien.deronne from comment #23)
> Ok, got it, but we thus need to be sure the header is not expected to be
> retrieved elsewhere.

I copy m_currentPacket before removing header, so m_currentPacket is not modified.

Oh indeed, I had missed it, as you copy it you can safely call the remove function iso the peek function, so please ignore my previous comment.

BTW I received a failing test case (different scenario than the one tracked by this bug) and your patch does solve the problem (but other issues pop up still). 

IMO this bug should not have been reopened, your patch does address a different root cause even though the triggered assert is the same. Your patch fixes issues when management frames such as ADDBA or DELBA frames are exchanged, which might collide in very specific conditions (thus it should be treated as a normal or minor issue) and the TID is then not properly retrieved. I agree this was not clear from the beginning, but investigation tell us more now :-) Anyway, I agree to deliver the proposed patch in ns-3-dev, but thus maybe we should track it in another thread.

Opened new bug 2380 for the issue reported by Alexander.