Could you please detail a bit more?
Also, traces and/or a simulation script would be useful here in order to reproduce and investigate the problem.

But first of all, is this not a duplicate of bug 1205???

Thanks for the quick answer. No, it's not a duplicate of bug 1205.

More details: assume two or more EDCA txops contend for the channel and internal collision happens. The winner continues with the transmission and the loser starts the backoff procedure. According to the standard the contention window must be increased and the retry counter must be incremented  before the backoff procedure starts. This is the same procedure as if an external collision would have happened.

I'll try to cook something to reproduce.

(In reply to levente.meszaros from comment #2)
> Thanks for the quick answer. No, it's not a duplicate of bug 1205.
> 
> More details: assume two or more EDCA txops contend for the channel and
> internal collision happens. The winner continues with the transmission and
> the loser starts the backoff procedure. According to the standard the
> contention window must be increased and the retry counter must be
> incremented  before the backoff procedure starts. This is the same procedure
> as if an external collision would have happened.
> 
> I'll try to cook something to reproduce.

Did you prepare a test case in the meantime?

No, I didn't. Unfortunately I don't have time for this, so it's very unlikely that I will, sorry.

Looking back at the description and the bug is quite clear. 
Current code looks indeed wrong: unlike what is mentioned in 9.19.2.5 EDCA backoff procedure, we do not increase the contention window once an internal collision has been detected.

It is stated the appropriate retry counter should be increased. But I have doubts on how to handle the case where we reach the maximum number of retries. 

Should we notify it toward the station manager? 
We currently should in order to increase the retry counter, but I am not sure whether the rate manager should also adapt its selected rate in case of internal collisions?

Also, a retry counter could be first incremented because of an internal collisions, and later on it might get increased because of an external collision. In that case, what should be notified to the rate manager?

These are questions I didn't think of and I don't really have a good answer.

Matias maybe has a better view on this?

I think we must act the same way as any other type of retransmissions. I mean, if retry limit is reached, the frame should be discarded and inform this to the manager.

This is what I understand from this paragraph:
"For internal collisions occurring with the EDCA access method, the appropriate retry counters (short retry counter for MSDU, A-MSDU, or MMPDU and QSRC[AC] or long retry counter for MSDU, A-MSDU, or MMPDU and QLRC[AC]) are incremented. For transmissions that use Block Ack, the rules in 9.21.3 also apply. STAs shall retry failed transmissions until the transmission is successful or until the relevant retry limit is reached."

Regarding the last two question of Sebastien:
Deciding when to change rates depending on the type of loss (collision, low SNR, interference, etc) is one of the problems of rate adaptation mechanisms. Currently most mechanisms just consider frame losses, independent of the type of loss.
So, I think we should not distinguish between if it is an internal or external collision. As we do not distinguish if loss occur because of a collision or low SNR.

Created attachment 2407 [details]
Proposed patch to fix

Here is a patch that might address this bug.
I have not tested it at all (just checked regression), I rely on the submitter to have a look.

I no longer work regularly with ns3, so I cannot find the time to check this patch. Sorry for not being helpful.

I believe what we have now is already better than what was previously handled in case of internal collision.

But how was this bug noticed? Don't you have any example we could include in our regression?

This bug was found while I was comparing the 802.11 behavior to another simulator. Unfortunately I don't have a test case.

Created attachment 2470 [details]
patch to handle virtual collisions

fixes issues with previous patch

Created attachment 2474 [details]
test case

add test case

(In reply to sebastien.deronne from comment #14)
> Created attachment 2470 [details]
> patch to handle virtual collisions
> 
> fixes issues with previous patch

Can you provide a few NS_LOG_DEBUG() statements in the heart of this code, to alert what is going on when logging is enabled?

https://www.nsnam.org/bugzilla/attachment.cgi?id=2470&action=diff#a/src/wifi/model/edca-txop-n.cc_sec4

Otherwise, patch looks good to me (haven't tested).

(In reply to sebastien.deronne from comment #15)
> Created attachment 2474 [details]
> test case
> 
> add test case

Three main comments:

1) The test patch introduces dependency on internet and applications module, which must be avoided.  It is unnecessary; a PacketSocket will suffice for this.

2) Although the need for the new 'Tx' callback will be obviated once 1) is addresssed, I suggest to avoid modifying packets (tagging them) through a Tx callback.  

I know there is discussion of this in Packet::AddByteTag that this tagging is OK, but in this specific case, it seems to violate the logical constness of the packet since the model code relies on the QoS tag.

3) This test causes two packets to collide, but only one TxDataFailed callback is invoked.  Shouldn't two be invoked?

Tom, what is then the best way to set QoS when using a PacketSocket?

Since this is internal collision, this means the packet with higher priority will be sent, and that internal collision is triggered for packets with lower priorities (so here, only one packet failed)

(In reply to sebastien.deronne from comment #18)
> Tom, what is then the best way to set QoS when using a PacketSocket?


With the existing codebase, the test case could subclass PacketSocketClient and create a new virtual StartApplication and (non-virtual) SendWithQos() method; the latter of which could be the same as Send() but also attach the QosTag.

With possible 802.1p (bug 2241) support, then that solution could be used.

> 
> Since this is internal collision, this means the packet with higher priority
> will be sent, and that internal collision is triggered for packets with
> lower priorities (so here, only one packet failed)

OK

Tom, I guess the same solution as for the TXOP example should be used?
In that case, I am waiting for Stefano's solution.

Created attachment 2506 [details]
test case

test case updated

This fix should be included in the upcoming release. 
Any comments/remarks before pushing?

pushed in changesets 12241:ddaf3c4a8e4e and 12242:8b197f75774b