Bug 2184

Created attachment 2150 [details]
quick fix for bug 2184

I could reproduce the compilation error on a setup.
I made an quick fix for this.

I wait till the end of the weekend before committing it, in case someone suggests a more elegant solution.

(In reply to sebastien.deronne from comment #1)
> Created attachment 2150 [details]
> quick fix for bug 2184
> 
> I could reproduce the compilation error on a setup.
> I made an quick fix for this.
> 
> I wait till the end of the weekend before committing it, in case someone
> suggests a more elegant solution.

This patch will cause the value to roll over just before it reaches the value of 0xffffffff; i.e. it will increment from (0xffffffff - 1) to 0, not ever taking the value 0xffffffff.  Is that intended (to avoid a reserved value)?  

Otherwise, if it just needs to roll over, why not let it?

I also had questions about this variable.  It seems to be never used in the model; is that correct or did I not spot its usage somewhere?

I looked it up online such as here:
http://www.radiotap.org/defined-fields/A-MPDU%20status

and it says that it should be set on the receiver side, but the ns-3 code sets it on the sending side, which might lead to a violation of the assumption that it could be guaranteed to be unique across the same channel.

(In reply to Tom Henderson from comment #2)
> (In reply to sebastien.deronne from comment #1)
> > Created attachment 2150 [details]
> > quick fix for bug 2184
> > 
> > I could reproduce the compilation error on a setup.
> > I made an quick fix for this.
> > 
> > I wait till the end of the weekend before committing it, in case someone
> > suggests a more elegant solution.
> 
> This patch will cause the value to roll over just before it reaches the
> value of 0xffffffff; i.e. it will increment from (0xffffffff - 1) to 0, not
> ever taking the value 0xffffffff.  Is that intended (to avoid a reserved
> value)?  

0xffffffff should be allowed, so it is indeed not ok.
Maybe something like "if value is 0xffffffff, reset to 0" is better here?

> 
> Otherwise, if it just needs to roll over, why not let it?
> 
> I also had questions about this variable.  It seems to be never used in the
> model; is that correct or did I not spot its usage somewhere?
> 
> I looked it up online such as here:
> http://www.radiotap.org/defined-fields/A-MPDU%20status
> 
> and it says that it should be set on the receiver side, but the ns-3 code
> sets it on the sending side, which might lead to a violation of the
> assumption that it could be guaranteed to be unique across the same channel.

Correct, I had missed the "generated by the capture device". I will rework this part as well.

(In reply to sebastien.deronne from comment #3)
> (In reply to Tom Henderson from comment #2)
> > (In reply to sebastien.deronne from comment #1)
> > > Created attachment 2150 [details]
> > > quick fix for bug 2184
> > > 
> > > I could reproduce the compilation error on a setup.
> > > I made an quick fix for this.
> > > 
> > > I wait till the end of the weekend before committing it, in case someone
> > > suggests a more elegant solution.
> > 
> > This patch will cause the value to roll over just before it reaches the
> > value of 0xffffffff; i.e. it will increment from (0xffffffff - 1) to 0, not
> > ever taking the value 0xffffffff.  Is that intended (to avoid a reserved
> > value)?  
> 
> 0xffffffff should be allowed, so it is indeed not ok.
> Maybe something like "if value is 0xffffffff, reset to 0" is better here?

if you just increment it, it will wrap to 0; this is well defined/legal behavior for unsigned integers.

Created attachment 2151 [details]
patch to remove overflow issue and to fulfil MPDU status field only at RX

I made a new patch based on Tom's comments.

A-MPDU status is now only fulfilled at RX (and there is no longer overflow issue).
However, it seems it has to be defined at TX as well in order to decode the MPDUs in the transmitter PCAPs. I filled it with null values, but please feel free to propose a better approach (maybe a padding can make it work?).

(In reply to sebastien.deronne from comment #5)
> Created attachment 2151 [details]
> patch to remove overflow issue and to fulfil MPDU status field only at RX
> 
> I made a new patch based on Tom's comments.

Do you mind if I just apply the fix to the increment for ns-3.24.1:

https://www.nsnam.org/bugzilla/attachment.cgi?id=2151&action=diff#a/src/wifi/model/yans-wifi-phy.cc_sec3

and save the rest of it for ns-3-dev (since it is more extensive and involves API changes)?

> 
> A-MPDU status is now only fulfilled at RX (and there is no longer overflow
> issue).
> However, it seems it has to be defined at TX as well in order to decode the
> MPDUs in the transmitter PCAPs. I filled it with null values, but please
> feel free to propose a better approach (maybe a padding can make it work?).

Does it need to have valid values on the sending side for wireshark to successfully associate frames?  I would say null is OK unless you find that wireshark is complaining about it, in which case I'd suggest to implement the unique counter also on the sending side but overwrite it on the receiving side.

(In reply to Tom Henderson from comment #6)
> (In reply to sebastien.deronne from comment #5)
> > Created attachment 2151 [details]
> > patch to remove overflow issue and to fulfil MPDU status field only at RX
> > 
> > I made a new patch based on Tom's comments.
> 
> Do you mind if I just apply the fix to the increment for ns-3.24.1:
> 
> https://www.nsnam.org/bugzilla/attachment.cgi?id=2151&action=diff#a/src/wifi/
> model/yans-wifi-phy.cc_sec3
> 
> and save the rest of it for ns-3-dev (since it is more extensive and
> involves API changes)?

Yes sure, you can do that.

> 
> > 
> > A-MPDU status is now only fulfilled at RX (and there is no longer overflow
> > issue).
> > However, it seems it has to be defined at TX as well in order to decode the
> > MPDUs in the transmitter PCAPs. I filled it with null values, but please
> > feel free to propose a better approach (maybe a padding can make it work?).
> 
> Does it need to have valid values on the sending side for wireshark to
> successfully associate frames?  I would say null is OK unless you find that
> wireshark is complaining about it, in which case I'd suggest to implement
> the unique counter also on the sending side but overwrite it on the
> receiving side.

Yes, I fill it with null values and  wireshark is then happy, no need to have valid values, but it makes me think padding should work too.

Created attachment 2157 [details]
patch to remove overflow issue and to fulfil MPDU status field only at RX

Updated previous patch to fix compilation errors when examples are enabled.

Created attachment 2158 [details]
final patch

I changed a bit the previous patch after my discussion with Hany.

The A-MPDU status field is not specific to Rx only, since Wireshark does not distinguish between Tx and Rx. 

The counter for Tx and Rx A-MPDU reference number should not be passed in the function(s), it should be calculated independently at Tx and Rx side. I this added m_txMpduReferenceNumber and m_rxMpduReferenceNumber in YansWifiPhy to keep track of this value at each side.

Unless someone has a remark, this should be the final fix for this bug.

(In reply to sebastien.deronne from comment #9)
> Created attachment 2158 [details]
> final patch
> 
> I changed a bit the previous patch after my discussion with Hany.
> 
> The A-MPDU status field is not specific to Rx only, since Wireshark does not
> distinguish between Tx and Rx. 
> 
> The counter for Tx and Rx A-MPDU reference number should not be passed in
> the function(s), it should be calculated independently at Tx and Rx side. I
> this added m_txMpduReferenceNumber and m_rxMpduReferenceNumber in
> YansWifiPhy to keep track of this value at each side.
> 
> Unless someone has a remark, this should be the final fix for this bug.

This looks good to me but for documentation purposes, I might suggest to change at least the name of parameter packetType to 'mpduType', and if you want to go further (it is up to you; would also change the struct), you could consider to make it an enum rather than uint8_t.

Tom,
Thanks for your comments.

I will do the proposed changes and commit them together with this patch.

Changeset 11683:dad754e84171