Issue 14549044: Adding MPDU aggregation

Daniel L.

My initial review for the "modified" files. Also, don't forget to credit yourself in the ...

10 years, 6 months ago (2013-10-11 15:25:05 UTC) #1

gbadawy

Hi Daniel, Thanks for your review please find my comments below. I updated the code ...

10 years, 6 months ago (2013-10-11 18:35:10 UTC) #2

Hi Daniel,
Thanks for your review please find my comments below. I updated the code to
include all of your comments but since they are all minor comments I will
not post a patch now I will wait until there are more updates and then
post. Is that OK?

On Fri, Oct 11, 2013 at 11:25 AM, <nikkipui@gmail.com> wrote:

> My initial review for the "modified" files.
>
> Also, don't forget to credit yourself in the files you modified/created!
>
>
> https://codereview.appspot.**com/14549044/diff/1/src/wifi/**
>
model/wifi-mac-queue.cc<https://codereview.appspot.com/14549044/diff/1/src/wifi/model/wifi-mac-queue.cc>
> File src/wifi/model/wifi-mac-queue.**cc (right):
>
> https://codereview.appspot.**com/14549044/diff/1/src/wifi/**
>
model/wifi-mac-queue.cc#**newcode313<https://codereview.appspot.com/14549044/diff/1/src/wifi/model/wifi-mac-queue.cc#newcode313>
> src/wifi/model/wifi-mac-queue.**cc:313: Ptr<WifiMacQueue>
> packetsToAggregate;
> Would it be better to create the Ptr<WifiMacQueue> only once and clear
> it here instead? I think that might be faster than creating a new
> instance every time.
>
> Done

> https://codereview.appspot.**com/14549044/diff/1/src/wifi/**
>
model/wifi-phy-state-helper.h<https://codereview.appspot.com/14549044/diff/1/src/wifi/model/wifi-phy-state-helper.h>
> File src/wifi/model/wifi-phy-state-**helper.h (right):
>
> https://codereview.appspot.**com/14549044/diff/1/src/wifi/**
>
model/wifi-phy-state-helper.h#**newcode76<https://codereview.appspot.com/14549044/diff/1/src/wifi/model/wifi-phy-state-helper.h#newcode76>
> src/wifi/model/wifi-phy-state-**helper.h:76: void NotifyRxEnd (void);
> Document that we merged the Ok/Error to RxEnd
>
Done

>
> https://codereview.appspot.**com/14549044/diff/1/src/wifi/**
>
model/wifi-phy.h<https://codereview.appspot.com/14549044/diff/1/src/wifi/model/wifi-phy.h>
> File src/wifi/model/wifi-phy.h (right):
>
> https://codereview.appspot.**com/14549044/diff/1/src/wifi/**
>
model/wifi-phy.h#newcode153<https://codereview.appspot.com/14549044/diff/1/src/wifi/model/wifi-phy.h#newcode153>
> src/wifi/model/wifi-phy.h:153: typedef Callback<void,Ptr<Packet>,
> double,double, WifiMode, enum WifiPreamble> RxOkCallback;
> Can you document on the argument list above to reflect the change?
>
 Done

>
> https://codereview.appspot.**com/14549044/diff/1/src/wifi/**
>
model/wifi-phy.h#newcode159<https://codereview.appspot.com/14549044/diff/1/src/wifi/model/wifi-phy.h#newcode159>
> src/wifi/model/wifi-phy.h:159: typedef Callback<void,Ptr<Packet>,
> double, double, WifiMode, WifiPreamble> RxCallback;
> Can you document on the argument list above to reflect the change?
>
> Done

> https://codereview.appspot.**com/14549044/diff/1/src/wifi/**
>
model/yans-wifi-phy.cc<https://codereview.appspot.com/14549044/diff/1/src/wifi/model/yans-wifi-phy.cc>
> File src/wifi/model/yans-wifi-phy.**cc (right):
>
> https://codereview.appspot.**com/14549044/diff/1/src/wifi/**
>
model/yans-wifi-phy.cc#**newcode843<https://codereview.appspot.com/14549044/diff/1/src/wifi/model/yans-wifi-phy.cc#newcode843>
> src/wifi/model/yans-wifi-phy.**cc:843: /*if (m_random->GetValue () >
> snrPer.per)
> I think it might be a good idea to write down some comment regarding the
> change below. At least we can keep track of the changes.
>
Done

>
>
https://codereview.appspot.**com/14549044/<https://codereview.appspot.com/145...
>

Sign in to reply to this message.

kusoof

Hi, I'm not sure whether this is the right list to post to, but if ...

10 years, 6 months ago (2013-10-15 15:36:36 UTC) #3

tomh_tomh.org

On 10/15/2013 08:36 AM, Lynne Salameh wrote: > Hi, > > I'm not sure whether ...

10 years, 6 months ago (2013-10-15 15:40:37 UTC) #4

kusoof

Hi Ghada, Thanks a lot for working on a much needed feature for ns-3. I'd ...

10 years, 6 months ago (2013-10-15 17:14:08 UTC) #5

gbadawy

Hi Lynne, Thanks for helping. Adding 11n to NS3 is not an easy job and ...

10 years, 6 months ago (2013-10-16 15:20:40 UTC) #6

Hi Lynne,
Thanks for helping. Adding 11n to NS3 is not an easy job and all comments
and reviews are highly appreciated. Please find my response below

On Tue, Oct 15, 2013 at 1:14 PM, <kusoof@gmail.com> wrote:

> Hi Ghada,
>
> Thanks a lot for working on a much needed feature for ns-3. I'd like to
> help out with commenting on your submission.
>
> I haven't compiled and run your code yet, but a quick glance at it left
> me with a couple of questions:
>
> 1. What's your motivation for implementing deaggregation/aggregation in
> mac-low.cc? In my implementation, I added that functionality to
> yans-wifi-phy.cc for several reasons. The most important one being that
> the pcap tracing facility, which is called form the phy using
> NotifyMonitorSniffRx, needs to see the independent packets for it to be
> of any use. In other words, I would like to see the individual packets
> from the pcap in wireshark, and I don't think wireshark can parse and
> display the aggregated packet anyway. Similarly, running tcpdump on real
> wifi traces shows the behavior I described: individual packets from the
> aggregate with subsequent timestamps. Ditto for the send chain, the
> tracing facilities need to see the individual packets.
>

I implemented aggregation/deaggregation in mac-low.cc because this is where
the standard mentions it should be done. I didn't implement any
PCAP functionalities yet.

>
> 2. How are you calculating rxPer for the individual packets? Granted, I
> need to look at your code in more detail but I thought I'd ask you. In
> my understanding, you can get two scenarios on receiving an A-MPDU.
> First, the preamble cannot be decoded, which means that the entire
> aggregated packet is lost, and the receiver cannot send any form of
> acknowledgement. Second, individual frames have errors independently
> from the rest of the frames. In order to get this functionality to work,
> I had to modify the interference-helper.cc to calculate the packet error
> rate for the preamble and each frame separately taking into
> consideration interference from other sources.
>

I calculate the rxPer normally using the interference helper and
yans-wifi-phy (didn't change anything in this part of the code except
ofcource adding 11n preambles) then send this rxPer to MacLow then MacLow
deaggregates the A-MPDU and decides which MPDU is received correctly and
which is not depending on a random number drawn for each MPDU from a random
number generator. Again I did this because I was trying to closely follow
the standard and in the standard de-aggregation happens at the Mac not at
the Phy.

>
> Just a couple of points to consider. Hopefully, I will have some more
> comments when I try running your code with my existing setup. Thanks!
>
>
>
>
https://codereview.appspot.**com/14549044/<https://codereview.appspot.com/145...

Thanks,
Ghada

Sign in to reply to this message.

Daniel L.

That is actually a very good point. I forgot to look into the pcap thing ...

10 years, 6 months ago (2013-10-16 16:01:37 UTC) #7

kusoof

Hi Ghada, Indeed, you are right. The 802.11 spec does indicate that the aggregation functionality ...

10 years, 6 months ago (2013-10-16 18:07:56 UTC) #8

Hi Ghada,

Indeed, you are right. The 802.11 spec does indicate that the aggregation
functionality should happen in the lower mac layer. Unfortunately, I've found
that it made my life much easier to add it to the phy layer due to the reasons
I've described (tracing and packet error calculation). Just thought I'd throw my
two cents in, but it's really up to you and the reviewers :). 

As for frame errors, using the interference-helper code without modification
does the following: Suppose you have an aggregate of 2 packets and an
interfering packet:

    |<---preamble-->| <-------- mpdu 1 ---------------> | <---- mpdu 2  --- >|

                       |<-----interfering packet-->|

The correct behavior is that mdpu 1 would have a higher per than mpdu 2 and
would most likely be lost, whereas mpdu 2 will be received correctly. Instead of
computing packet error rates independently for frames 1 and 2, you are computing
the per over the entire aggregate, and tossing a coin twice using that
probability to see whether frame 1 or frame 2 were received. 

I don't believe that is the correct behavior.

We also have to take into consideration that any errors in the preamble will
cause the entire A-MPDU to fail decoding.

If it's ok with you and the reviewers, I'd love to collaborate with you, and
contribute some of the code I've written to fix the per calculation problem. It
might still need some tweaking, but hopefully we can iterate on it so that it
works correctly.

Thanks,
Lynne

On 2013/10/16 16:01:37, Daniel L. wrote:
> That is actually a very good point. I forgot to look into the pcap thing
> earlier.
> 
> I think that with the current design, the whole A-MPDU is either received or
> lost since they are sent in one big packet. I don't think the scenario where a
> receiver receives only some packets is possible here? The PCAP file also
groups
> all packet together in one big packet too.
> 
> We might need to look into this issue more.
> 
> Thanks,
> Daniel

Sign in to reply to this message.

Daniel L.

I think it is better to keep things in the MAC layer rather than doing ...

10 years, 6 months ago (2013-10-16 18:18:43 UTC) #9

I think it is better to keep things in the MAC layer rather than doing it at
PHY.

May be we can have MAC send a train of MPDUs? That way PHY will still see
multiple packets without any modification.
The first MPDU will have include preamble, the remaining MPDUs will not have
preamble (modification will be needed to support
sending without preamble).

-Daniel

On 2013/10/16 18:07:56, kusoof wrote:
> Hi Ghada,
> 
> Indeed, you are right. The 802.11 spec does indicate that the aggregation
> functionality should happen in the lower mac layer. Unfortunately, I've found
> that it made my life much easier to add it to the phy layer due to the reasons
> I've described (tracing and packet error calculation). Just thought I'd throw
my
> two cents in, but it's really up to you and the reviewers :). 
> 
> As for frame errors, using the interference-helper code without modification
> does the following: Suppose you have an aggregate of 2 packets and an
> interfering packet:
> 
>     |<---preamble-->| <-------- mpdu 1 ---------------> | <---- mpdu 2  --- >|
>                           
> 
>                        |<-----interfering packet-->|
> 
> The correct behavior is that mdpu 1 would have a higher per than mpdu 2 and
> would most likely be lost, whereas mpdu 2 will be received correctly. Instead
of
> computing packet error rates independently for frames 1 and 2, you are
computing
> the per over the entire aggregate, and tossing a coin twice using that
> probability to see whether frame 1 or frame 2 were received. 
> 
> I don't believe that is the correct behavior.
> 
> We also have to take into consideration that any errors in the preamble will
> cause the entire A-MPDU to fail decoding.
>  
> If it's ok with you and the reviewers, I'd love to collaborate with you, and
> contribute some of the code I've written to fix the per calculation problem.
It
> might still need some tweaking, but hopefully we can iterate on it so that it
> works correctly.
> 
> Thanks,
> Lynne
> 
> On 2013/10/16 16:01:37, Daniel L. wrote:
> > That is actually a very good point. I forgot to look into the pcap thing
> > earlier.
> > 
> > I think that with the current design, the whole A-MPDU is either received or
> > lost since they are sent in one big packet. I don't think the scenario where
a
> > receiver receives only some packets is possible here? The PCAP file also
> groups
> > all packet together in one big packet too.
> > 
> > We might need to look into this issue more.
> > 
> > Thanks,
> > Daniel

Sign in to reply to this message.

kusoof

Hi Daniel, > May be we can have MAC send a train of MPDUs? That ...

10 years, 6 months ago (2013-10-16 19:05:33 UTC) #10

gbadawy

Hi Lynna and Daniel, On Wed, Oct 16, 2013 at 3:05 PM, <kusoof@gmail.com> wrote: > ...

10 years, 6 months ago (2013-10-16 20:20:16 UTC) #11

gbadawy

Hi Lynne, On Wed, Oct 16, 2013 at 2:07 PM, <kusoof@gmail.com> wrote: > Hi Ghada, ...

10 years, 6 months ago (2013-10-16 20:25:51 UTC) #12

Hi Lynne,


On Wed, Oct 16, 2013 at 2:07 PM, <kusoof@gmail.com> wrote:

> Hi Ghada,
>
> Indeed, you are right. The 802.11 spec does indicate that the
> aggregation functionality should happen in the lower mac layer.
> Unfortunately, I've found that it made my life much easier to add it to
> the phy layer due to the reasons I've described (tracing and packet
> error calculation). Just thought I'd throw my two cents in, but it's
> really up to you and the reviewers :).
>
> As for frame errors, using the interference-helper code without
> modification does the following: Suppose you have an aggregate of 2
> packets and an interfering packet:
>
>     |<---preamble-->| <-------- mpdu 1 ---------------> | <---- mpdu 2
> --- >|
>
>
>                        |<-----interfering packet-->|
>
> The correct behavior is that mdpu 1 would have a higher per than mpdu 2
> and would most likely be lost, whereas mpdu 2 will be received
> correctly. Instead of computing packet error rates independently for
> frames 1 and 2, you are computing the per over the entire aggregate, and
> tossing a coin twice using that probability to see whether frame 1 or
> frame 2 were received.
>
> I don't believe that is the correct behavior.
>
> We also have to take into consideration that any errors in the preamble
> will cause the entire A-MPDU to fail decoding.
>
> If it's ok with you and the reviewers, I'd love to collaborate with you,
> and contribute some of the code I've written to fix the per calculation
> problem. It might still need some tweaking, but hopefully we can iterate
> on it so that it works correctly.
>

I think what you are saying is correct. I am not sure in literature how
A-MPDU rxPer is calculated (I remember I read it is binomial somewhere). I
usually don't work with AWGN channel models so all the work I did there I
did just to provide a complete solution for ns3 so you can submit patches
and improvements to any part of the code that you like and your
contribution will be highly appreciated.

>
> Thanks,
> Lynne


Regards,
Ghada


>
>
> On 2013/10/16 16:01:37, Daniel L. wrote:
>
>> That is actually a very good point. I forgot to look into the pcap
>>
> thing
>
>> earlier.
>>
>
>  I think that with the current design, the whole A-MPDU is either
>>
> received or
>
>> lost since they are sent in one big packet. I don't think the scenario
>>
> where a
>
>> receiver receives only some packets is possible here? The PCAP file
>>
> also groups
>
>> all packet together in one big packet too.
>>
>
>  We might need to look into this issue more.
>>
>
>  Thanks,
>> Daniel
>>
>
>
>
>
https://codereview.appspot.**com/14549044/<https://codereview.appspot.com/145...
>

Sign in to reply to this message.

kusoof

Hi Ghada, > Will the receiving node see a train of packets or 1 big ...

10 years, 6 months ago (2013-10-16 20:48:55 UTC) #13

Daniel L.

On 2013/10/16 20:48:55, kusoof wrote: > Hi Ghada, > > > Will the receiving node ...

10 years, 6 months ago (2013-10-16 21:26:21 UTC) #14

kusoof

> I think that de-aggregation should not be done at PHY. I think we should ...

10 years, 6 months ago (2013-10-16 22:49:10 UTC) #15

Daniel L.

On 2013/10/16 22:49:10, kusoof wrote: > > I think that de-aggregation should not be done ...

10 years, 6 months ago (2013-10-17 13:57:52 UTC) #16

kusoof

> How about something like this: > > MacLow::StartTransmission receives a list of Packets instead ...

10 years, 6 months ago (2013-10-17 14:21:47 UTC) #17

gbadawy

On Thu, Oct 17, 2013 at 9:57 AM, <nikkipui@gmail.com> wrote: > On 2013/10/16 22:49:10, kusoof ...

10 years, 6 months ago (2013-10-17 14:22:40 UTC) #18

On Thu, Oct 17, 2013 at 9:57 AM, <nikkipui@gmail.com> wrote:

> On 2013/10/16 22:49:10, kusoof wrote:
>
>> > I think that de-aggregation should not be done at PHY. I think we
>>
> should stick
>
>> > with Ghada's implementation in de-aggregation aspect.
>> >
>> > What do you think?
>>
>
>  That could work, but I'd still need to know the mpdu frame boundaries
>>
> in the phy
>
>> when I'm trying to calculate the packet error rates independently. I
>>
> guess I
>
>> need to take a look at the present code in more detail and see what
>>
> can be done.
>
> How about something like this:
>
> MacLow::StartTransmission receives a list of Packets instead of a single
> packet and an indication that the packets should be sent as A-MPDU.
>
> MacLow::StartTransmission should only receive 1 packet from EdcaTxopN as
it already does. But in MacLow we can make MacLow::ForwardDown call
YansWifiPhy::SendPacket for every packet in m_aggregateQueue if m_mpdu is
true. I think this will be the path with the least changes. If we change
input to StartTransmission to receive multiple packets then we should move
aggregation to EdcaTxopN which will need a lot of work.

MacLow calls YansWifiPhy::SendPacket one by one for each Packet object.
> The first one has preamble, the remaining Packets do not have preamble.
>

We need to add a new preamble type called None for example (because
preamble is not actually added to the packet it is just used when
calculation TxDuration and in interference helper) and extend/add case
statements where preamble is used to include the new type.

>
> I think that this way, the packet error rate is taken care normally
> (still need to handle sending without preamble). WifiPhy sees multiple
> Packets as it should.
>
> Of course, we would have to be able to distinguish between
> correct/incorrect preamble in the first Packet. But to simplify things
> for now, we can start by assuming that preamble is correct if the first
> Packet is received, and the preamble is incorrect if the first Packet is
> not received (i.e. all MPDUs will not be received).
>
> Ghada and Lynne, what do you think about this?
>
>
https://codereview.appspot.**com/14549044/<https://codereview.appspot.com/145...


I think we should also add tags to the packets to flag that they are part
of an A-MPDU so that when YansWifiPhy forwards them up to MacLow. MacLow
can correctly deal with them. I think the last MPDU in the A-MPDU will also
need an extra tag to flag that it is the last one so that MacLow can send
the BlockAck after it.

What do you think?

Sign in to reply to this message.

kusoof

> > MacLow::StartTransmission should only receive 1 packet from EdcaTxopN as > it already does. ...

10 years, 6 months ago (2013-10-17 14:30:32 UTC) #19

Daniel L.

On 2013/10/17 14:21:47, kusoof wrote: > > How about something like this: > > > ...

10 years, 6 months ago (2013-10-17 14:36:58 UTC) #20

Daniel L.

On 2013/10/17 14:30:32, kusoof wrote: > > > MacLow::StartTransmission should only receive 1 packet from ...

10 years, 6 months ago (2013-10-17 14:43:59 UTC) #21

gbadawy

On Thu, Oct 17, 2013 at 10:21 AM, <kusoof@gmail.com> wrote: > How about something like ...

10 years, 6 months ago (2013-10-17 14:52:18 UTC) #22

On Thu, Oct 17, 2013 at 10:21 AM, <kusoof@gmail.com> wrote:

> How about something like this:
>>
>
>  MacLow::StartTransmission receives a list of Packets instead of a
>>
> single packet
>
>> and an indication that the packets should be sent as A-MPDU.
>>
>
>  MacLow calls YansWifiPhy::SendPacket one by one for each Packet
>>
> object. The
>
>> first one has preamble, the remaining Packets do not have preamble.
>>
>
> I see what you're thinking. Send the packets separately but back to back
> to mimic A-MPDU behavior. Unfortunately, I think this approach might add
> some unnecessary complexity to the design. There are extra delimiters
> and subframe headers in between the frames,


ForwardDown can call the MpduStandardAggregator::Aggregate function to add
subframe header and delimiters before sending the packet.

 and the receiving end needs
> to know what they are and strip them correctly.


That is why I said we need a tag so that MacLow can do the correct
stripping

I think it would be
> better if the phy glues the different frames it receives together into
> one packet, the A-MPDU, before sending.


>  I think that this way, the packet error rate is taken care normally
>>
> (still need
>
>> to handle sending without preamble). WifiPhy sees multiple Packets as
>>
> it should.
>
> The packet error calculation happens on the receive chain as I recall.
> If we send the A-MPDU frames back to back as you suggest above, indeed
> this will work correctly (with the preamble fix you suggested).

Otherwise, we also need to deaggregate in the phy. Deaggregation in real
> hardware occurs on the NIC anyway, so it's not a stretch to have the
> Yans-Wifi-Phy deaggregate, calculate the success rate of each packet,
> and then transmit only the successful ones to the upper layer.
>
What do you guys think?
>
> -Lynne
>
>
>
>
https://codereview.appspot.**com/14549044/<https://codereview.appspot.com/145...
>

Sign in to reply to this message.

kusoof

> The concern I have when gluing them together is that I think the receiving ...

10 years, 6 months ago (2013-10-17 15:02:22 UTC) #23

gbadawy

Hi, On Thu, Oct 17, 2013 at 11:02 AM, <kusoof@gmail.com> wrote: > > The concern ...

10 years, 6 months ago (2013-10-18 13:49:23 UTC) #24

Hi,

On Thu, Oct 17, 2013 at 11:02 AM, <kusoof@gmail.com> wrote:

>
>  The concern I have when gluing them together is that I think the
>>
> receiving PHY
>
>> will only see one big packet. I think we can de-aggregate them into
>>
> multiple
>
>> sub-frames but I'm not sure how to get timestamp for each subframe.
>>
> (For
>
>> example, timestamp in pcap file for each subframe.)
>>
>
> Ok I think I'm now convinced that your approach is correct. I had to do
> some extra hacks in my code to make sure the timestamps were logged
> correctly for each consecutive frame in an A-MPDU. Your suggestion makes
> timestamps work organically.
>
> I will update my code to include the changes that we talked about which are
1- Adding a new NONE preamble
2- MacLow::ForwardDown will call YansWifiPhy::SendPacket multiple time
3- Changes in YansWifiPhy and WifiPhy to allow reception of packets with
the preamble-NONE
4- Changes in WifiInteferenceHelper to calculate interference for packets
with preamble=NONE

and after I am done Lynne or Daniel can add the Pcap stuff
OK?

>
>
>  Do you know what the specification says about this? Does the receiver
>>
> deliver each sub-frame as soon
>
>> as the sub-frame is received? Or does it wait until all A-MPDU is
>>
> fully received
>
>> before delivering all of them at once? If we glue them together, I
>>
> think that
>
>> the receiver must received the whole A-MPDU before proceeding.
>>
>
> I don't think the spec indicates whether mac-low passes up subframes
> separately to the upper MAC. The only thing it specifies is that the MAC
> needs to pass subframes contiguously and in ascending order to the upper
> layers. Because of this and the block ack mechanism, frames are passed
> to upper layers in one batch at a specific timestamp.
>
>
>
https://codereview.appspot.**com/14549044/<https://codereview.appspot.com/145...
>

Sign in to reply to this message.

kusoof

> > I will update my code to include the changes that we talked about ...

10 years, 6 months ago (2013-10-18 13:53:27 UTC) #25

Daniel L.

On 2013/10/18 13:49:23, gbadawy wrote: > Hi, > I will update my code to include ...

10 years, 6 months ago (2013-10-18 13:54:47 UTC) #26

kusoof

Hi Daniel and Ghada, Just a minor bug I discovered when I tried to implement ...

10 years, 6 months ago (2013-10-21 16:10:04 UTC) #27

kusoof

I missed a spot. See the additional diffs. -Lynne https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/mac-low.cc File src/wifi/model/mac-low.cc (right): https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/mac-low.cc#newcode2287 src/wifi/model/mac-low.cc:2287: ...

10 years, 6 months ago (2013-10-21 16:20:22 UTC) #28

kusoof

Another minor bug related to the previous one. https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/block-ack-agreement.cc File src/wifi/model/block-ack-agreement.cc (right): https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/block-ack-agreement.cc#newcode125 src/wifi/model/block-ack-agreement.cc:125: uint16_t ...

10 years, 6 months ago (2013-10-21 16:22:24 UTC) #29

kusoof

https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/block-ack-manager.cc File src/wifi/model/block-ack-manager.cc (right): https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/block-ack-manager.cc#newcode566 src/wifi/model/block-ack-manager.cc:566: NS_ASSERT (it != m_agreements.end ()); Should insert: //Check the ...

10 years, 5 months ago (2013-11-05 19:17:21 UTC) #30

kusoof

Hi, I've managed to run the attached test case (see tcp_test.cc) with the code patches. ...

10 years, 5 months ago (2013-11-05 19:27:33 UTC) #31

Hi,

I've managed to run the attached test case (see tcp_test.cc) with the code 
patches. I've quickly hacked up some code to do pcap printing correctly for 
the aggregates so that I could produce the attached .pcap output file. The 
topology was the following:


                     x          x
                     |            |
o---------------o           o----------------o
server        AP    Wifi Station          Client

Where the server is sending TCP packets to the client through a 
point-to-point link to the AP.

From the attached pcap file, we can see that although the AP is receiving 
block acks, it still sends block ack requests. I've located the source of 
the problem, and added the modified code to the codereview website, 
repeated here for posterity:

https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/block-ack-ma...
<https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/block-ack-ma...>
File src/wifi/model/block-ack-manager.cc (right):
https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/block-ack-ma...
<https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/block-ack-ma...>
src/wifi/model/block-ack-manager.cc:566: NS_ASSERT (it != m_agreements.end ());
Should insert:

//Check the ack policy of the packets in the queue
  //If the ack policy is normal ack, then we have an implicit block ack request
  bool normalAck = (*it).second.second.front().hdr.IsQosAck();
  //We shouldn't send a block ack request
  if(normalAck){
    
    OriginatorBlockAckAgreement &agreement = (*it).second.first;
    agreement.CompleteExchange ();
    
    return 0;
  }



One other change needs to be done for the above to work. The ack policy 
should be set to normal ack policy for the aggregate frames. We require an 
immediate block ack, with an implicit block ack request, and I believe the 
802.11 spec indicates that for implicit block ack requests, the ack policy 
should be a normal ack. If I am wrong about this, a different check needs 
to be added to the block ack manager to check whether packets are 
aggregates, in order to disable scheduling block ack requests when they are 
unnecessary.

Sign in to reply to this message.

jinjing81

https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/mac-low.cc File src/wifi/model/mac-low.cc (right): https://codereview.appspot.com/14549044/diff/8001/src/wifi/model/mac-low.cc#newcode2506 src/wifi/model/mac-low.cc:2506: WifiTxVector dataTxVector = GetDataTxVector (m_currentPacket, &m_currentHdr); The dataTxVector is ...

10 years, 5 months ago (2013-11-18 19:48:43 UTC) #32

jinjing81

https://codereview.appspot.com/14549044/patch/8001/9028 there are several typos on "Msdu", should be "Mpdu"

10 years, 5 months ago (2013-11-19 00:11:29 UTC) #33

gbadawy

Yep you are correct I removed it On Mon, Nov 18, 2013 at 2:48 PM, ...

10 years, 5 months ago (2013-11-19 17:48:08 UTC) #34

gbadawy

These were corrected On Mon, Nov 18, 2013 at 7:11 PM, <jinjing81@gmail.com> wrote: > https://codereview.appspot.com/14549044/patch/8001/9028 ...

10 years, 5 months ago (2013-11-19 17:49:00 UTC) #35

jinjing81

Thanks for your hard work, gbadawy! One suggestion is to enable pcap in current patch. ...

10 years, 4 months ago (2013-12-17 02:23:19 UTC) #36

yannis

Hello, I 'm testing the A-mpdu and I have some queries. 1. The m_sentMpdus shouldn't ...

10 years ago (2014-03-25 16:18:51 UTC) #37

gbadawy

On Tue, Mar 25, 2014 at 12:18 PM, <selinis.g@gmail.com> wrote: > Hello, > > I ...

10 years ago (2014-03-25 18:15:41 UTC) #38

yannis

On 2014/03/25 18:15:41, gbadawy wrote: > On Tue, Mar 25, 2014 at 12:18 PM, <mailto:selinis.g@gmail.com> ...

10 years ago (2014-03-25 19:16:43 UTC) #39

gbadawy

On Tue, Mar 25, 2014 at 3:16 PM, <selinis.g@gmail.com> wrote: > On 2014/03/25 18:15:41, gbadawy ...

10 years ago (2014-03-25 19:26:50 UTC) #40

On Tue, Mar 25, 2014 at 3:16 PM, <selinis.g@gmail.com> wrote:

> On 2014/03/25 18:15:41, gbadawy wrote:
>
>> On Tue, Mar 25, 2014 at 12:18 PM, <mailto:selinis.g@gmail.com> wrote:
>>
>
>  > Hello,
>> >
>> > I 'm testing the A-mpdu and I have some queries.
>> > 1. The m_sentMpdus shouldn't be zero each time we send an
>>
> Ampdu?
>
>> > Because in other words, only for the first packet received from
>> > EdcaTxopN the A-Mpdu mechanism works right. After that, we don't
>>
> have
>
>> > access in the while loop (due to m_sentMpdus), until the blockAck
>> > received.
>> >
>> Hi I only have Ht-immdeiate block ack implemented Ht-Delayed blockAck
>>
> is
>
>> not implemented yet. So we don't want another packet to start
>>
> accessing the
>
>> medium before the first A-MPDU is acknowledged. Does that answer your
>> question?
>>
>
> Maybe I didn't explain it well. I know that we should get block Ack in
> each
> A-Mpdu, but instead of that I get only for the first A-Mpdu and after
> that
> the A-Mpdu size equals to the total size of two packets. We request
> Block Ack but
> it's dropped the ctl_BACKRESP and that's why the m_sentMpdus continues
> counting.



>

I'm sorry but I don't really understand what is wrong. When a BlockAck is
> not recieved the sender sends a BlockAckRequest and it can aggregated more
> MPDUs with it if there are any. The m_sentMpdus counts the number of sent
> MPDUs that have not been acknowledged yet to keep the number below the
> limit defined in the standard which is 64. Does that answer your question?
>



>
>  > 2. When I use RTS/CTS, I get cts timeout, and in each A-Mpdu the
>>
> first
>
>> > packet is send it again.
>> >
>> A-MPDU should not affect RTS/CTS. I will check it and get back to you
>>
>
>
>
>  > https://codereview.appspot.com/14549044/
>> >
>>
>
>
> https://codereview.appspot.com/14549044/
>

Sign in to reply to this message.

yannis

On 2014/03/25 19:26:50, gbadawy wrote: > On Tue, Mar 25, 2014 at 3:16 PM, <mailto:selinis.g@gmail.com> ...

10 years ago (2014-03-25 20:15:04 UTC) #41

On 2014/03/25 19:26:50, gbadawy wrote:
> On Tue, Mar 25, 2014 at 3:16 PM, <mailto:selinis.g@gmail.com> wrote:
> 
> > On 2014/03/25 18:15:41, gbadawy wrote:
> >
> >> On Tue, Mar 25, 2014 at 12:18 PM, <mailto:selinis.g@gmail.com> wrote:
> >>
> >
> >  > Hello,
> >> >
> >> > I 'm testing the A-mpdu and I have some queries.
> >> > 1. The m_sentMpdus shouldn't be zero each time we send an
> >>
> > Ampdu?
> >
> >> > Because in other words, only for the first packet received from
> >> > EdcaTxopN the A-Mpdu mechanism works right. After that, we don't
> >>
> > have
> >
> >> > access in the while loop (due to m_sentMpdus), until the blockAck
> >> > received.
> >> >
> >> Hi I only have Ht-immdeiate block ack implemented Ht-Delayed blockAck
> >>
> > is
> >
> >> not implemented yet. So we don't want another packet to start
> >>
> > accessing the
> >
> >> medium before the first A-MPDU is acknowledged. Does that answer your
> >> question?
> >>
> >
> > Maybe I didn't explain it well. I know that we should get block Ack in
> > each
> > A-Mpdu, but instead of that I get only for the first A-Mpdu and after
> > that
> > the A-Mpdu size equals to the total size of two packets. We request
> > Block Ack but
> > it's dropped the ctl_BACKRESP and that's why the m_sentMpdus continues
> > counting.
> 
> 
> 
> >
> 
> I'm sorry but I don't really understand what is wrong. When a BlockAck is
> > not recieved the sender sends a BlockAckRequest and it can aggregated more
> > MPDUs with it if there are any. The m_sentMpdus counts the number of sent
> > MPDUs that have not been acknowledged yet to keep the number below the
> > limit defined in the standard which is 64. Does that answer your question?
> >
> 
> 
> 
> >
> >  > 2. When I use RTS/CTS, I get cts timeout, and in each A-Mpdu the
> >>
> > first
> >
> >> > packet is send it again.
> >> >
> >> A-MPDU should not affect RTS/CTS. I will check it and get back to you
> >>
> >
> >
> >
> >  > https://codereview.appspot.com/14549044/
> >> >
> >>
> >
> >
> > https://codereview.appspot.com/14549044/
> >

The problem was in the MacLow::ForwardDown.
We have to add on the first condition

std::string h = hdr.GetTypeString();
if(!m_ampdu || h ==" CTL_RTS")
{..}

That fixes the problem with RTS/CTS and 
the problem with the blockAck in each A-mpdu.Without this
the blockAck is never rx and the m_sentMpdus equals to zero 
when it reaches the number of 64.

Sign in to reply to this message.

yannis

Is there any solution for the infinite loop on the BlockAckManager::GetSeqNumOfRetryPacket() ? I get inside ...

10 years ago (2014-04-01 17:24:12 UTC) #42

gbadawy

On Tue, Mar 25, 2014 at 4:15 PM, <selinis.g@gmail.com> wrote: > On 2014/03/25 19:26:50, gbadawy ...

10 years ago (2014-04-01 17:54:09 UTC) #43

On Tue, Mar 25, 2014 at 4:15 PM, <selinis.g@gmail.com> wrote:

> On 2014/03/25 19:26:50, gbadawy wrote:
>
>> On Tue, Mar 25, 2014 at 3:16 PM, <mailto:selinis.g@gmail.com> wrote:
>>
>
>  > On 2014/03/25 18:15:41, gbadawy wrote:
>> >
>> >> On Tue, Mar 25, 2014 at 12:18 PM, <mailto:selinis.g@gmail.com>
>>
> wrote:
>
>> >>
>> >
>> >  > Hello,
>> >> >
>> >> > I 'm testing the A-mpdu and I have some queries.
>> >> > 1. The m_sentMpdus shouldn't be zero each time we send an
>> >>
>> > Ampdu?
>> >
>> >> > Because in other words, only for the first packet received from
>> >> > EdcaTxopN the A-Mpdu mechanism works right. After that, we don't
>> >>
>> > have
>> >
>> >> > access in the while loop (due to m_sentMpdus), until the blockAck
>> >> > received.
>> >> >
>> >> Hi I only have Ht-immdeiate block ack implemented Ht-Delayed
>>
> blockAck
>
>> >>
>> > is
>> >
>> >> not implemented yet. So we don't want another packet to start
>> >>
>> > accessing the
>> >
>> >> medium before the first A-MPDU is acknowledged. Does that answer
>>
> your
>
>> >> question?
>> >>
>> >
>> > Maybe I didn't explain it well. I know that we should get block Ack
>>
> in
>
>> > each
>> > A-Mpdu, but instead of that I get only for the first A-Mpdu and
>>
> after
>
>> > that
>> > the A-Mpdu size equals to the total size of two packets. We request
>> > Block Ack but
>> > it's dropped the ctl_BACKRESP and that's why the m_sentMpdus
>>
> continues
>
>> > counting.
>>
>
>
>
>  >
>>
>
>  I'm sorry but I don't really understand what is wrong. When a BlockAck
>>
> is
>
>> > not recieved the sender sends a BlockAckRequest and it can
>>
> aggregated more
>
>> > MPDUs with it if there are any. The m_sentMpdus counts the number of
>>
> sent
>
>> > MPDUs that have not been acknowledged yet to keep the number below
>>
> the
>
>> > limit defined in the standard which is 64. Does that answer your
>>
> question?
>
>> >
>>
>
>
>
>  >
>> >  > 2. When I use RTS/CTS, I get cts timeout, and in each A-Mpdu the
>> >>
>> > first
>> >
>> >> > packet is send it again.
>> >> >
>> >> A-MPDU should not affect RTS/CTS. I will check it and get back to
>>
> you
>
>> >>
>> >
>> >
>> >
>> >  > https://codereview.appspot.com/14549044/
>> >> >
>> >>
>> >
>> >
>> > https://codereview.appspot.com/14549044/
>> >
>>
>
> The problem was in the MacLow::ForwardDown.
> We have to add on the first condition
>
> std::string h = hdr.GetTypeString();
> if(!m_ampdu || h ==" CTL_RTS")
> {..}


> That fixes the problem with RTS/CTS and
> the problem with the blockAck in each A-mpdu.Without this
> the blockAck is never rx and the m_sentMpdus equals to zero
> when it reaches the number of 64.
>

Hi I understand how this solves the issue of sending RTS/CTS before an
A-MPDU. Thanks for catching it
But I don't understand how does that solve your blockack issue.


>
> https://codereview.appspot.com/14549044/
>

Sign in to reply to this message.

gbadawy

On Tue, Apr 1, 2014 at 1:24 PM, <selinis.g@gmail.com> wrote: > Is there any solution ...

10 years ago (2014-04-01 18:01:15 UTC) #44

gbadawy

On Tue, Apr 1, 2014 at 1:53 PM, Ghada Badawy <gbadawy@gmail.com> wrote: > > > ...

10 years ago (2014-04-01 18:08:56 UTC) #45

On Tue, Apr 1, 2014 at 1:53 PM, Ghada Badawy <gbadawy@gmail.com> wrote:

>
>
>
> On Tue, Mar 25, 2014 at 4:15 PM, <selinis.g@gmail.com> wrote:
>
>> On 2014/03/25 19:26:50, gbadawy wrote:
>>
>>> On Tue, Mar 25, 2014 at 3:16 PM, <mailto:selinis.g@gmail.com> wrote:
>>>
>>
>>  > On 2014/03/25 18:15:41, gbadawy wrote:
>>> >
>>> >> On Tue, Mar 25, 2014 at 12:18 PM, <mailto:selinis.g@gmail.com>
>>>
>> wrote:
>>
>>> >>
>>> >
>>> >  > Hello,
>>> >> >
>>> >> > I 'm testing the A-mpdu and I have some queries.
>>> >> > 1. The m_sentMpdus shouldn't be zero each time we send an
>>> >>
>>> > Ampdu?
>>> >
>>> >> > Because in other words, only for the first packet received from
>>> >> > EdcaTxopN the A-Mpdu mechanism works right. After that, we don't
>>> >>
>>> > have
>>> >
>>> >> > access in the while loop (due to m_sentMpdus), until the blockAck
>>> >> > received.
>>> >> >
>>> >> Hi I only have Ht-immdeiate block ack implemented Ht-Delayed
>>>
>> blockAck
>>
>>> >>
>>> > is
>>> >
>>> >> not implemented yet. So we don't want another packet to start
>>> >>
>>> > accessing the
>>> >
>>> >> medium before the first A-MPDU is acknowledged. Does that answer
>>>
>> your
>>
>>> >> question?
>>> >>
>>> >
>>> > Maybe I didn't explain it well. I know that we should get block Ack
>>>
>> in
>>
>>> > each
>>> > A-Mpdu, but instead of that I get only for the first A-Mpdu and
>>>
>> after
>>
>>> > that
>>> > the A-Mpdu size equals to the total size of two packets. We request
>>> > Block Ack but
>>> > it's dropped the ctl_BACKRESP and that's why the m_sentMpdus
>>>
>> continues
>>
>>> > counting.
>>>
>>
>>
>>
>>  >
>>>
>>
>>  I'm sorry but I don't really understand what is wrong. When a BlockAck
>>>
>> is
>>
>>> > not recieved the sender sends a BlockAckRequest and it can
>>>
>> aggregated more
>>
>>> > MPDUs with it if there are any. The m_sentMpdus counts the number of
>>>
>> sent
>>
>>> > MPDUs that have not been acknowledged yet to keep the number below
>>>
>> the
>>
>>> > limit defined in the standard which is 64. Does that answer your
>>>
>> question?
>>
>>> >
>>>
>>
>>
>>
>>  >
>>> >  > 2. When I use RTS/CTS, I get cts timeout, and in each A-Mpdu the
>>> >>
>>> > first
>>> >
>>> >> > packet is send it again.
>>> >> >
>>> >> A-MPDU should not affect RTS/CTS. I will check it and get back to
>>>
>> you
>>
>>> >>
>>> >
>>> >
>>> >
>>> >  > https://codereview.appspot.com/14549044/
>>> >> >
>>> >>
>>> >
>>> >
>>> > https://codereview.appspot.com/14549044/
>>> >
>>>
>>
>> The problem was in the MacLow::ForwardDown.
>> We have to add on the first condition
>>
>> std::string h = hdr.GetTypeString();
>> if(!m_ampdu || h ==" CTL_RTS")
>> {..}
>
>
>> That fixes the problem with RTS/CTS and
>> the problem with the blockAck in each A-mpdu.Without this
>> the blockAck is never rx and the m_sentMpdus equals to zero
>> when it reaches the number of 64.
>>
>
> Hi I understand how this solves the issue of sending RTS/CTS before an
> A-MPDU. Thanks for catching it
> But I don't understand how does that solve your blockack issue.
>

I also set m_ampdu = false after receiving a blockack. This will help if
you send an A-MPDU and get its BlockAck then you receive an RTS( before
setting m_ampdu to false after receiving the BlockAck the CTS will not be
sent since m_ampdu is still true from the last transmission so it has to be
reset after A-MPDU transmission is complete). Thanks glad somebody is
testing before we go live :)

>
>
>>
>> https://codereview.appspot.com/14549044/
>>
>
>

Sign in to reply to this message.

yannis

On 2014/04/01 18:08:56, gbadawy wrote: > On Tue, Apr 1, 2014 at 1:53 PM, Ghada ...

10 years ago (2014-04-01 19:43:29 UTC) #46

On 2014/04/01 18:08:56, gbadawy wrote:
> On Tue, Apr 1, 2014 at 1:53 PM, Ghada Badawy <mailto:gbadawy@gmail.com> wrote:
> 
> >
> >
> >
> > On Tue, Mar 25, 2014 at 4:15 PM, <mailto:selinis.g@gmail.com> wrote:
> >
> >> On 2014/03/25 19:26:50, gbadawy wrote:
> >>
> >>> On Tue, Mar 25, 2014 at 3:16 PM, <mailto:selinis.g@gmail.com> wrote:
> >>>
> >>
> >>  > On 2014/03/25 18:15:41, gbadawy wrote:
> >>> >
> >>> >> On Tue, Mar 25, 2014 at 12:18 PM, <mailto:selinis.g@gmail.com>
> >>>
> >> wrote:
> >>
> >>> >>
> >>> >
> >>> >  > Hello,
> >>> >> >
> >>> >> > I 'm testing the A-mpdu and I have some queries.
> >>> >> > 1. The m_sentMpdus shouldn't be zero each time we send an
> >>> >>
> >>> > Ampdu?
> >>> >
> >>> >> > Because in other words, only for the first packet received from
> >>> >> > EdcaTxopN the A-Mpdu mechanism works right. After that, we don't
> >>> >>
> >>> > have
> >>> >
> >>> >> > access in the while loop (due to m_sentMpdus), until the blockAck
> >>> >> > received.
> >>> >> >
> >>> >> Hi I only have Ht-immdeiate block ack implemented Ht-Delayed
> >>>
> >> blockAck
> >>
> >>> >>
> >>> > is
> >>> >
> >>> >> not implemented yet. So we don't want another packet to start
> >>> >>
> >>> > accessing the
> >>> >
> >>> >> medium before the first A-MPDU is acknowledged. Does that answer
> >>>
> >> your
> >>
> >>> >> question?
> >>> >>
> >>> >
> >>> > Maybe I didn't explain it well. I know that we should get block Ack
> >>>
> >> in
> >>
> >>> > each
> >>> > A-Mpdu, but instead of that I get only for the first A-Mpdu and
> >>>
> >> after
> >>
> >>> > that
> >>> > the A-Mpdu size equals to the total size of two packets. We request
> >>> > Block Ack but
> >>> > it's dropped the ctl_BACKRESP and that's why the m_sentMpdus
> >>>
> >> continues
> >>
> >>> > counting.
> >>>
> >>
> >>
> >>
> >>  >
> >>>
> >>
> >>  I'm sorry but I don't really understand what is wrong. When a BlockAck
> >>>
> >> is
> >>
> >>> > not recieved the sender sends a BlockAckRequest and it can
> >>>
> >> aggregated more
> >>
> >>> > MPDUs with it if there are any. The m_sentMpdus counts the number of
> >>>
> >> sent
> >>
> >>> > MPDUs that have not been acknowledged yet to keep the number below
> >>>
> >> the
> >>
> >>> > limit defined in the standard which is 64. Does that answer your
> >>>
> >> question?
> >>
> >>> >
> >>>
> >>
> >>
> >>
> >>  >
> >>> >  > 2. When I use RTS/CTS, I get cts timeout, and in each A-Mpdu the
> >>> >>
> >>> > first
> >>> >
> >>> >> > packet is send it again.
> >>> >> >
> >>> >> A-MPDU should not affect RTS/CTS. I will check it and get back to
> >>>
> >> you
> >>
> >>> >>
> >>> >
> >>> >
> >>> >
> >>> >  > https://codereview.appspot.com/14549044/
> >>> >> >
> >>> >>
> >>> >
> >>> >
> >>> > https://codereview.appspot.com/14549044/
> >>> >
> >>>
> >>
> >> The problem was in the MacLow::ForwardDown.
> >> We have to add on the first condition
> >>
> >> std::string h = hdr.GetTypeString();
> >> if(!m_ampdu || h ==" CTL_RTS")
> >> {..}
> >
> >
> >> That fixes the problem with RTS/CTS and
> >> the problem with the blockAck in each A-mpdu.Without this
> >> the blockAck is never rx and the m_sentMpdus equals to zero
> >> when it reaches the number of 64.
> >>
> >
> > Hi I understand how this solves the issue of sending RTS/CTS before an
> > A-MPDU. Thanks for catching it
> > But I don't understand how does that solve your blockack issue.
> >
> 
> I also set m_ampdu = false after receiving a blockack. This will help if
> you send an A-MPDU and get its BlockAck then you receive an RTS( before
> setting m_ampdu to false after receiving the BlockAck the CTS will not be
> sent since m_ampdu is still true from the last transmission so it has to be
> reset after A-MPDU transmission is complete). Thanks glad somebody is
> testing before we go live :)
> 
> >
> >
> >>
> >> https://codereview.appspot.com/14549044/
> >>
> >
> >
It's not only the i++, I had to add and some other staff. Anyway,
I made and some others additions in some files, because of some errors.
As soon as I finish with these and the simulations, I ll post them. 
Hopefully, on time.

Sign in to reply to this message.

yannis

I'm testing under saturated condition, with multiple different applications installed and with various nodes on ...

10 years ago (2014-04-03 18:49:59 UTC) #47

gbadawy

On Thu, Apr 3, 2014 at 2:49 PM, <selinis.g@gmail.com> wrote: > I'm testing under saturated ...

10 years ago (2014-04-03 19:30:14 UTC) #48

On Thu, Apr 3, 2014 at 2:49 PM, <selinis.g@gmail.com> wrote:

> I'm testing under saturated condition, with multiple different
> applications installed and with various nodes on the network.
> One error that I get is on EdcaTxopN::NotifyAccessGranted when we are in
> the condition if (m_currentHdr.GetType () == WIFI_MAC_CTL_BACKREQ) :
> m_ctrType = 12 and m_ctrSubtype = 0. Of course, we get error NS_ASSERT
> (false) on WifiMacHeader::GetType.
>
When does that happen? These values are incorrect so I would look at the
part where the BAR is created check that if it is written to a queue with x
and y values when it is fetched it is fetched with the same x and y values
if not then something went wrong in the queue


> And another error SIGSEGV on MacLow::StartTransmission --
> m_currentPacket = packet->Copy ();
>
Is packet pointing to a valid packet? Maybe something inside copy is giving
the sigsegv


> Has anyone tried to simulate under saturated conditions and with
> multiple nodes transmitted?

Yes I did a simulation using 32 AP with 3 STA associated with each AP and
the datarates ranged from 1 Mbps till 35Mbps

Is it possible to happen these errors due to
> different Access Classes for the applications?

I never tried different applications with different ACs I usually use one
AC in my simulations. I can try and check multiple ACs and see if that
causes a problem


> Do we have to implement
> queues on this layer for that?
>
Multiple ACs are dealt with in the EdcaTxopN class where each AC has its
own queue. Check RegularWifi


> Just to mention, that I ve implemented code (MacLow::AggregateToAmpdu)
> for the case where multiple AMSDUs are included on the same AMPDU. (But
> these errors occur even if we only use the A-MPDU aggregation)

I didn't see your code so I can't comment on it.  Did you change something
in the old code to accommodate the new changes? I had to do that ti add
MPDU aggregation. Again the code is still under review I never tried it
with RTS/CTS or with different ACs in the same simulation scenario. Will do
that and see if I find anything that need fixing. Thanks again for the
testing :)

>
> https://codereview.appspot.com/14549044/
>

Sign in to reply to this message.

S. Deronne

I made a patch for all reviewers, including those features: - Add IsSupportedMcs in YansWifiPhy ...

10 years ago (2014-04-18 13:10:34 UTC) #49

Tom Henderson

On 2014/04/18 13:10:34, S. Deronne wrote: > I made a patch for all reviewers, including ...

10 years ago (2014-04-20 22:01:36 UTC) #50

On 2014/04/18 13:10:34, S. Deronne wrote:
> I made a patch for all reviewers, including those features:
> 
> -	Add IsSupportedMcs in YansWifiPhy to support 802.11n simulations
> -	Correct bug where the duration is not correctly computed with MPDU
aggregation
> -	Correct bug where no block ack is sent when the last subframe is erroneous,
> while a block ack has to be sent if at least one MPDU has been received in an
> aggregate.
> -	Correct bug where no DIFS is waited after block ack timeout
> -	Correct A-MPDU sequence number ordering when packets are retransmitted
> -	Correct sequence number span using modulo 4096 when it decides if a packet
can
> be aggregated in an A-MPDU
> -	Correct rate used to send block ack request
> -	Include patch for bug 1758 (Missing Yans and Nist error rate models for 5/6
> code rate of 802.11n HT)
> -	Avoid to store twice the same packet in BlockAckManager thanks to a minor
> modification in EdcaTxOpN
> -	Correct bug where a useless block ack request is sent after a block ack when
> the number of MPDU reaches 64, thanks to a minor modification in
> OriginatorBlockAckAgreement.
> 
> Known bug/limitations:
> 
> -	Simulation fails when a single packet needs to be retransmitted in an A-MPDU
> exchange.
> -	Wi-Fi module should add a method to check if PLCP header of an aggregate was
> correctly decoded. If not, all MPDU subframes within this aggregate should be
> marked as dropped.
> -	Simulation may fail in unsaturated conditions.
> -	Have only be tested in Basic access mode.
> 
> Link to download patch: http://www.sendspace.com/file/x0gyjb

This is a very large patch addressing multiple issues; can it be broken into
separate chunks?

In particular, are there any low-risk patches that are fixing existing
capabilities and can be factored out for applying separately?  such as the patch
for bug 1758?

Sign in to reply to this message.

S. Deronne

> This is a very large patch addressing multiple issues; can it be broken into ...

10 years ago (2014-04-23 08:28:39 UTC) #51

> This is a very large patch addressing multiple issues; can it be broken into
> separate chunks?
> 
> In particular, are there any low-risk patches that are fixing existing
> capabilities and can be factored out for applying separately?  such as the
patch
> for bug 1758?

Hi Tom,
Sorry for the little delay but I was away for some days, I'm now ready to focus
on the upcoming ns-3.20 update.

Indeed, patch for bug 1758 was already posted on
https://www.nsnam.org/bugzilla/show_bug.cgi?id=1758
But I included this feature in the patch since otherwise 802.11n was not working
correctly.

I propose to break my patch in different chunks as follows:
- Patch for bug 1758 (available on
https://www.nsnam.org/bugzilla/show_bug.cgi?id=1758).
- Patch to add IsSupportedMcs in YansWifiPhy: without those modifications,
802.11n simulations do not work at all in ns-3-dev.
- Patch to BlockAckManager: correct A-MPDU sequence number ordering when packets
are retransmitted.
- Patch to WifiPhy: correct bug where the duration is not correctly computed
with MPDU aggregation.
- Patch to EdcaTxopN: avoid to store twice the same packet when A-MPDU is used.
- Patch to Maclow: correct bug where no block ack is sent when the last subframe
is erroneous, while a block ack has to be sent if at least one MPDU has been
received in an aggregate.
- Patch to Maclow: correct bug where no DIFS is waited after block ack timeout.
- Patch to MacLow: correct sequence number span using modulo 4096 when it
decides if a packet can be aggregated in an A-MPDU.
- Patch to MacLow: correct rate used to send block ack request.
- Patch to OriginatorBlockAckAgreement: correct bug where a useless block ack
request is sent after a block ack when the number of MPDU reaches 64 (< instead
of <=).

Anyway, this results in many chunks with minor modifications (sometimes a single
line...), do you eventually see some features that could be merged?

Since my patch is based on ns-3-dev which doesn't include existing Ghada
implementation, I also wonder how I should do?
Because my previous patch includes Ghada modifications, and that's why it's
pretty huge while my modifications are minor.

Regards,
Sebastien.

Sign in to reply to this message.

Daniel L.

I think your suggested chunks you are good. I think it’s better to have one ...

9 years, 12 months ago (2014-04-24 17:58:52 UTC) #52

I think your suggested chunks you are good.  I think it’s better to have one
patch per bug/feature since it’s easier to keep track.

On Apr 23, 2014, at 4:28 AM, sebastien.deronne@gmail.com wrote:

>> This is a very large patch addressing multiple issues; can it be
> broken into
>> separate chunks?
> 
>> In particular, are there any low-risk patches that are fixing existing
>> capabilities and can be factored out for applying separately?  such as
> the patch
>> for bug 1758?
> 
> Hi Tom,
> Sorry for the little delay but I was away for some days, I'm now ready
> to focus on the upcoming ns-3.20 update.
> 
> Indeed, patch for bug 1758 was already posted on
> https://www.nsnam.org/bugzilla/show_bug.cgi?id=1758
> But I included this feature in the patch since otherwise 802.11n was not
> working correctly.
> 
> I propose to break my patch in different chunks as follows:
> - Patch for bug 1758 (available on
> https://www.nsnam.org/bugzilla/show_bug.cgi?id=1758).
> - Patch to add IsSupportedMcs in YansWifiPhy: without those
> modifications, 802.11n simulations do not work at all in ns-3-dev.
> - Patch to BlockAckManager: correct A-MPDU sequence number ordering when
> packets are retransmitted.
> - Patch to WifiPhy: correct bug where the duration is not correctly
> computed with MPDU aggregation.
> - Patch to EdcaTxopN: avoid to store twice the same packet when A-MPDU
> is used.
> - Patch to Maclow: correct bug where no block ack is sent when the last
> subframe is erroneous, while a block ack has to be sent if at least one
> MPDU has been received in an aggregate.
> - Patch to Maclow: correct bug where no DIFS is waited after block ack
> timeout.
> - Patch to MacLow: correct sequence number span using modulo 4096 when
> it decides if a packet can be aggregated in an A-MPDU.
> - Patch to MacLow: correct rate used to send block ack request.
> - Patch to OriginatorBlockAckAgreement: correct bug where a useless
> block ack request is sent after a block ack when the number of MPDU
> reaches 64 (< instead of <=).
> 
> Anyway, this results in many chunks with minor modifications (sometimes
> a single line...), do you eventually see some features that could be
> merged?
> 
> Since my patch is based on ns-3-dev which doesn't include existing Ghada
> implementation, I also wonder how I should do?
> Because my previous patch includes Ghada modifications, and that's why
> it's pretty huge while my modifications are minor.
> 
> Regards,
> Sebastien.
> 
> https://codereview.appspot.com/14549044/

Sign in to reply to this message.

S. Deronne

Thanks Daniel, I will proceed this way then. But on which code should I apply ...

9 years, 12 months ago (2014-04-24 18:24:21 UTC) #53

Daniel L.

I think from ns-3-dev sounds better. On Apr 24, 2014, at 2:24 PM, sebastien.deronne@gmail.com wrote: ...

9 years, 12 months ago (2014-04-24 18:48:09 UTC) #54

S. Deronne

On 2014/04/24 18:48:09, Daniel L. wrote: > I think from ns-3-dev sounds better. > > ...

9 years, 12 months ago (2014-04-24 19:05:34 UTC) #55

jinjing81

Hi, Sebastien, Thanks for your patch. I looked through your patches and have a question ...

9 years, 12 months ago (2014-04-25 02:06:08 UTC) #56

jinjing81

On 2014/04/24 19:05:34, S. Deronne wrote: > On 2014/04/24 18:48:09, Daniel L. wrote: > > ...

9 years, 12 months ago (2014-04-25 02:16:29 UTC) #57

S. Deronne

On 2014/04/25 02:06:08, jinjing81 wrote: > Hi, Sebastien, > > Thanks for your patch. I ...

9 years, 12 months ago (2014-04-25 07:22:53 UTC) #58

S. Deronne

On 2014/04/25 02:16:29, jinjing81 wrote: > On 2014/04/24 19:05:34, S. Deronne wrote: > > On ...

9 years, 12 months ago (2014-04-25 07:28:43 UTC) #59

jinjing81

Sorry I mean patch 3 from Ghada. Thought patch 2 is the latest... On Friday, ...

9 years, 12 months ago (2014-04-25 14:37:25 UTC) #60

Sorry I mean patch 3 from Ghada. Thought patch 2 is the latest...

On Friday, April 25, 2014, <sebastien.deronne@gmail.com> wrote:

> On 2014/04/25 02:16:29, jinjing81 wrote:
>
>> On 2014/04/24 19:05:34, S. Deronne wrote:
>> > On 2014/04/24 18:48:09, Daniel L. wrote:
>> > > I think from ns-3-dev sounds better.
>> > >
>> > > On Apr 24, 2014, at 2:24 PM, mailto:sebastien.deronne@gmail.com
>>
> wrote:
>
>> > >
>> > > > Thanks Daniel, I will proceed this way then.
>> > > >
>> > > > But on which code should I apply a diff file?
>> > > > Because Ghada implementation is not yet integrated in ns-3-dev.
>> > > >
>> > > > https://codereview.appspot.com/14549044/
>> > >
>> >
>> > Ok. But how I should include Ghada code then?
>> > Because each chunk is a correction of his code
>>
>
>  I do not know what is the best practice...
>> But I guess you can use mercurial to reverse to Ghada's revision and
>>
> patch
>
>> Ghada's patch 2 first and commit once, then replace the committed
>>
> version with
>
>> your latest and get a new patch 3 then people know the difference from
>>
> patch 2
>
>> and can review it accordingly..So in this way, we know the difference
>>
> you had
>
>> from Ghada's.
>>
>
>  BTW, it seems that Ghada's patch 2 is based upon some not-very-new
>>
> changeset
>
>> that includes some old 802.11 configuration (not seen in new
>>
> changesets), it
>
>> needs some work to make it work with current ns-3-dev if it is planned
>>
> to
>
>> release in future ns-3.2x...
>>
>
> OK. So if I understand well, I first apply patch 2 from Ghada on
> ns-3-dev. Then I apply my modifications and I provide a patch which will
> only contain my own modifications?
> And second, I apply patch 3 from Ghada on ns-3-dev and I again apply my
> modification, and finally  get another patch with my own modifications?
> Is that right?
>
> Why I should use patch 2 from Ghada?
> Because patch 3 is more recent and my work is based on that one.
>
> Thanks for your help!
>
> https://codereview.appspot.com/14549044/
>

Sign in to reply to this message.

jinjing81

On 2014/04/25 07:22:53, S. Deronne wrote: > On 2014/04/25 02:06:08, jinjing81 wrote: > > Hi, ...

9 years, 12 months ago (2014-04-25 16:48:48 UTC) #61

On 2014/04/25 07:22:53, S. Deronne wrote:
> On 2014/04/25 02:06:08, jinjing81 wrote:
> > Hi, Sebastien,
> > 
> > Thanks for your patch. I looked through your patches and have a question
> > for you on one of Patches to Maclow: "correct bug where no block ack is
> > sent when the last subframe is erroneous, while a block ack has to be sent
> > if at least one MPDU has been received in an aggregate."
> > 
> > I saw you added a few lines in ReceiveError(), but it seems buggy in the
> > sense that at the very beginning, when A-MPDU is transmitted but the
> > BlockAck agreement has not been established yet (lost/error AddBaRequest).
> > Also it is possible that the last packet in A-MPDU is BAR, then
> > hdr.GetQosTid() is problematic...I felt this seems a problem with
> > no guarantee in current BlockAck implementation that BlockAck is
> > established...
> > 
> > Any comments from you would be helpful!
> > 
> > Thanks and best regards
> > 
> > 
> > On Thu, Apr 24, 2014 at 12:05 PM, <mailto:sebastien.deronne@gmail.com>
wrote:
> > 
> > > On 2014/04/24 18:48:09, Daniel L. wrote:
> > >
> > >> I think from ns-3-dev sounds better.
> > >>
> > >
> > >  On Apr 24, 2014, at 2:24 PM, mailto:sebastien.deronne@gmail.com wrote:
> > >>
> > >
> > >  > Thanks Daniel, I will proceed this way then.
> > >> >
> > >> > But on which code should I apply a diff file?
> > >> > Because Ghada implementation is not yet integrated in ns-3-dev.
> > >> >
> > >> > https://codereview.appspot.com/14549044/
> > >>
> > >
> > >
> > > Ok. But how I should include Ghada code then?
> > > Because each chunk is a correction of his code.
> > >
> > > https://codereview.appspot.com/14549044/
> > >
> 
> Do you have a simulation script to reproduce the bug?
> 
> Maybe we should add a condition to check if BlockAck agreement has been
> established. What do you think about it?
> Indeed, BAR as last subframe may be a problem in this case. Maybe we should
> check it in the condition as well.

I do not have a working script now.
But it seems that this is a common issue when there are multiple BSSs attached
to the same channel 
and the number of STAs are high...These control messages can easily be
lost...even the association 
can be hard to finish...

Sign in to reply to this message.

gbadawy

Hi All, Sebastien I have a question why do you need IsMcsSupported in the Phy ...

9 years, 12 months ago (2014-04-25 17:14:58 UTC) #62

Hi All,
Sebastien I have a question why do you need IsMcsSupported in the Phy layer
that should be handled in WifiRemoteStationManager, right?
I didn't have a look at your patches. But this is something that I thought
of reading the emails.
Looking forward to reviewing your patches

Regards,
Ghada


On Fri, Apr 25, 2014 at 12:48 PM, <jinjing81@gmail.com> wrote:

> On 2014/04/25 07:22:53, S. Deronne wrote:
>
>> On 2014/04/25 02:06:08, jinjing81 wrote:
>> > Hi, Sebastien,
>> >
>> > Thanks for your patch. I looked through your patches and have a
>>
> question
>
>> > for you on one of Patches to Maclow: "correct bug where no block ack
>>
> is
>
>> > sent when the last subframe is erroneous, while a block ack has to
>>
> be sent
>
>> > if at least one MPDU has been received in an aggregate."
>> >
>> > I saw you added a few lines in ReceiveError(), but it seems buggy in
>>
> the
>
>> > sense that at the very beginning, when A-MPDU is transmitted but the
>> > BlockAck agreement has not been established yet (lost/error
>>
> AddBaRequest).
>
>> > Also it is possible that the last packet in A-MPDU is BAR, then
>> > hdr.GetQosTid() is problematic...I felt this seems a problem with
>> > no guarantee in current BlockAck implementation that BlockAck is
>> > established...
>> >
>> > Any comments from you would be helpful!
>> >
>> > Thanks and best regards
>> >
>> >
>> > On Thu, Apr 24, 2014 at 12:05 PM,
>>
> <mailto:sebastien.deronne@gmail.com> wrote:
>
>> >
>> > > On 2014/04/24 18:48:09, Daniel L. wrote:
>> > >
>> > >> I think from ns-3-dev sounds better.
>> > >>
>> > >
>> > >  On Apr 24, 2014, at 2:24 PM, mailto:sebastien.deronne@gmail.com
>>
> wrote:
>
>> > >>
>> > >
>> > >  > Thanks Daniel, I will proceed this way then.
>> > >> >
>> > >> > But on which code should I apply a diff file?
>> > >> > Because Ghada implementation is not yet integrated in ns-3-dev.
>> > >> >
>> > >> > https://codereview.appspot.com/14549044/
>> > >>
>> > >
>> > >
>> > > Ok. But how I should include Ghada code then?
>> > > Because each chunk is a correction of his code.
>> > >
>> > > https://codereview.appspot.com/14549044/
>> > >
>>
>
>  Do you have a simulation script to reproduce the bug?
>>
>
>  Maybe we should add a condition to check if BlockAck agreement has
>>
> been
>
>> established. What do you think about it?
>> Indeed, BAR as last subframe may be a problem in this case. Maybe we
>>
> should
>
>> check it in the condition as well.
>>
>
> I do not have a working script now.
> But it seems that this is a common issue when there are multiple BSSs
> attached to the same channel
> and the number of STAs are high...These control messages can easily be
> lost...even the association
> can be hard to finish...
>
>
> https://codereview.appspot.com/14549044/
>

Sign in to reply to this message.

S. Deronne

Hi Ghada, In fact this chunk is not dependent on A-MPDU aggregation. Since the addition ...

9 years, 12 months ago (2014-04-25 20:18:49 UTC) #63

Hi Ghada,

In fact this chunk is not dependent on A-MPDU aggregation.

Since the addition of "if (IsModeSupported (txMode))" in
YansWifiPhy::StartReceive as well as the move of the IsModeSupported method to
YansWifiPhy, IEEE 802.11n transmissions result in a null throughput in ns-3-dev.
The addition of IsMcsSupported and the modification of the condition in
StartReceive recover the support of 802.11 MCS rates.

I created a special bug for this part, you can find it on
https://www.nsnam.org/bugzilla/show_bug.cgi?id=1907

I hope it helps.

Regards,
Sebastien


On 2014/04/25 17:14:58, gbadawy wrote:
> Hi All,
> Sebastien I have a question why do you need IsMcsSupported in the Phy layer
> that should be handled in WifiRemoteStationManager, right?
> I didn't have a look at your patches. But this is something that I thought
> of reading the emails.
> Looking forward to reviewing your patches
> 
> Regards,
> Ghada
> 
> 
> On Fri, Apr 25, 2014 at 12:48 PM, <mailto:jinjing81@gmail.com> wrote:
> 
> > On 2014/04/25 07:22:53, S. Deronne wrote:
> >
> >> On 2014/04/25 02:06:08, jinjing81 wrote:
> >> > Hi, Sebastien,
> >> >
> >> > Thanks for your patch. I looked through your patches and have a
> >>
> > question
> >
> >> > for you on one of Patches to Maclow: "correct bug where no block ack
> >>
> > is
> >
> >> > sent when the last subframe is erroneous, while a block ack has to
> >>
> > be sent
> >
> >> > if at least one MPDU has been received in an aggregate."
> >> >
> >> > I saw you added a few lines in ReceiveError(), but it seems buggy in
> >>
> > the
> >
> >> > sense that at the very beginning, when A-MPDU is transmitted but the
> >> > BlockAck agreement has not been established yet (lost/error
> >>
> > AddBaRequest).
> >
> >> > Also it is possible that the last packet in A-MPDU is BAR, then
> >> > hdr.GetQosTid() is problematic...I felt this seems a problem with
> >> > no guarantee in current BlockAck implementation that BlockAck is
> >> > established...
> >> >
> >> > Any comments from you would be helpful!
> >> >
> >> > Thanks and best regards
> >> >
> >> >
> >> > On Thu, Apr 24, 2014 at 12:05 PM,
> >>
> > <mailto:sebastien.deronne@gmail.com> wrote:
> >
> >> >
> >> > > On 2014/04/24 18:48:09, Daniel L. wrote:
> >> > >
> >> > >> I think from ns-3-dev sounds better.
> >> > >>
> >> > >
> >> > >  On Apr 24, 2014, at 2:24 PM, mailto:sebastien.deronne@gmail.com
> >>
> > wrote:
> >
> >> > >>
> >> > >
> >> > >  > Thanks Daniel, I will proceed this way then.
> >> > >> >
> >> > >> > But on which code should I apply a diff file?
> >> > >> > Because Ghada implementation is not yet integrated in ns-3-dev.
> >> > >> >
> >> > >> > https://codereview.appspot.com/14549044/
> >> > >>
> >> > >
> >> > >
> >> > > Ok. But how I should include Ghada code then?
> >> > > Because each chunk is a correction of his code.
> >> > >
> >> > > https://codereview.appspot.com/14549044/
> >> > >
> >>
> >
> >  Do you have a simulation script to reproduce the bug?
> >>
> >
> >  Maybe we should add a condition to check if BlockAck agreement has
> >>
> > been
> >
> >> established. What do you think about it?
> >> Indeed, BAR as last subframe may be a problem in this case. Maybe we
> >>
> > should
> >
> >> check it in the condition as well.
> >>
> >
> > I do not have a working script now.
> > But it seems that this is a common issue when there are multiple BSSs
> > attached to the same channel
> > and the number of STAs are high...These control messages can easily be
> > lost...even the association
> > can be hard to finish...
> >
> >
> > https://codereview.appspot.com/14549044/
> >

Sign in to reply to this message.

gbadawy

On Fri, Apr 25, 2014 at 4:18 PM, <sebastien.deronne@gmail.com> wrote: > Hi Ghada, > > ...

9 years, 11 months ago (2014-04-28 18:24:59 UTC) #64

On Fri, Apr 25, 2014 at 4:18 PM, <sebastien.deronne@gmail.com> wrote:

> Hi Ghada,
>
> In fact this chunk is not dependent on A-MPDU aggregation.
>
> Since the addition of "if (IsModeSupported (txMode))" in
> YansWifiPhy::StartReceive as well as the move of the IsModeSupported
> method to YansWifiPhy, IEEE 802.11n transmissions result in a null
> throughput in ns-3-dev.
>
Who added IsModeSupported to YansWifiPhy? I have looked at my ns3 versions
and I can't find it. Any idea why was it added there? what purpose does it
serve?

Regards,
Ghada


> The addition of IsMcsSupported and the modification of the condition in
> StartReceive recover the support of 802.11 MCS rates.
>
> I created a special bug for this part, you can find it on
> https://www.nsnam.org/bugzilla/show_bug.cgi?id=1907
>
> I hope it helps.
>
> Regards,
> Sebastien
>
>
>
> On 2014/04/25 17:14:58, gbadawy wrote:
>
>> Hi All,
>> Sebastien I have a question why do you need IsMcsSupported in the Phy
>>
> layer
>
>> that should be handled in WifiRemoteStationManager, right?
>> I didn't have a look at your patches. But this is something that I
>>
> thought
>
>> of reading the emails.
>> Looking forward to reviewing your patches
>>
>
>  Regards,
>> Ghada
>>
>
>
>  On Fri, Apr 25, 2014 at 12:48 PM, <mailto:jinjing81@gmail.com> wrote:
>>
>
>  > On 2014/04/25 07:22:53, S. Deronne wrote:
>> >
>> >> On 2014/04/25 02:06:08, jinjing81 wrote:
>> >> > Hi, Sebastien,
>> >> >
>> >> > Thanks for your patch. I looked through your patches and have a
>> >>
>> > question
>> >
>> >> > for you on one of Patches to Maclow: "correct bug where no block
>>
> ack
>
>> >>
>> > is
>> >
>> >> > sent when the last subframe is erroneous, while a block ack has
>>
> to
>
>> >>
>> > be sent
>> >
>> >> > if at least one MPDU has been received in an aggregate."
>> >> >
>> >> > I saw you added a few lines in ReceiveError(), but it seems buggy
>>
> in
>
>> >>
>> > the
>> >
>> >> > sense that at the very beginning, when A-MPDU is transmitted but
>>
> the
>
>> >> > BlockAck agreement has not been established yet (lost/error
>> >>
>> > AddBaRequest).
>> >
>> >> > Also it is possible that the last packet in A-MPDU is BAR, then
>> >> > hdr.GetQosTid() is problematic...I felt this seems a problem with
>> >> > no guarantee in current BlockAck implementation that BlockAck is
>> >> > established...
>> >> >
>> >> > Any comments from you would be helpful!
>> >> >
>> >> > Thanks and best regards
>> >> >
>> >> >
>> >> > On Thu, Apr 24, 2014 at 12:05 PM,
>> >>
>> > <mailto:sebastien.deronne@gmail.com> wrote:
>> >
>> >> >
>> >> > > On 2014/04/24 18:48:09, Daniel L. wrote:
>> >> > >
>> >> > >> I think from ns-3-dev sounds better.
>> >> > >>
>> >> > >
>> >> > >  On Apr 24, 2014, at 2:24 PM,
>>
> mailto:sebastien.deronne@gmail.com
>
>> >>
>> > wrote:
>> >
>> >> > >>
>> >> > >
>> >> > >  > Thanks Daniel, I will proceed this way then.
>> >> > >> >
>> >> > >> > But on which code should I apply a diff file?
>> >> > >> > Because Ghada implementation is not yet integrated in
>>
> ns-3-dev.
>
>> >> > >> >
>> >> > >> > https://codereview.appspot.com/14549044/
>> >> > >>
>> >> > >
>> >> > >
>>
>> >> > > Ok. But how I should include Ghada code then?
>> >> > > Because each chunk is a correction of his code.
>> >> > >
>> >> > > https://codereview.appspot.com/14549044/
>> >> > >
>> >>
>> >
>> >  Do you have a simulation script to reproduce the bug?
>> >>
>> >
>> >  Maybe we should add a condition to check if BlockAck agreement has
>> >>
>> > been
>> >
>> >> established. What do you think about it?
>> >> Indeed, BAR as last subframe may be a problem in this case. Maybe
>>
> we
>
>> >>
>> > should
>> >
>> >> check it in the condition as well.
>> >>
>> >
>> > I do not have a working script now.
>> > But it seems that this is a common issue when there are multiple
>>
> BSSs
>
>> > attached to the same channel
>> > and the number of STAs are high...These control messages can easily
>>
> be
>
>> > lost...even the association
>> > can be hard to finish...
>> >
>> >
>> > https://codereview.appspot.com/14549044/
>> >
>>
>
>
> https://codereview.appspot.com/14549044/
>

Sign in to reply to this message.

S. Deronne

On 2014/04/28 18:24:59, gbadawy wrote: > On Fri, Apr 25, 2014 at 4:18 PM, <mailto:sebastien.deronne@gmail.com> ...

9 years, 11 months ago (2014-04-29 08:09:08 UTC) #65

On 2014/04/28 18:24:59, gbadawy wrote:
> On Fri, Apr 25, 2014 at 4:18 PM, <mailto:sebastien.deronne@gmail.com> wrote:
> 
> > Hi Ghada,
> >
> > In fact this chunk is not dependent on A-MPDU aggregation.
> >
> > Since the addition of "if (IsModeSupported (txMode))" in
> > YansWifiPhy::StartReceive as well as the move of the IsModeSupported
> > method to YansWifiPhy, IEEE 802.11n transmissions result in a null
> > throughput in ns-3-dev.
> >
> Who added IsModeSupported to YansWifiPhy? I have looked at my ns3 versions
> and I can't find it. Any idea why was it added there? what purpose does it
> serve?
> 
> Regards,
> Ghada
> 
> 
> > The addition of IsMcsSupported and the modification of the condition in
> > StartReceive recover the support of 802.11 MCS rates.
> >
> > I created a special bug for this part, you can find it on
> > https://www.nsnam.org/bugzilla/show_bug.cgi?id=1907
> >
> > I hope it helps.
> >
> > Regards,
> > Sebastien
> >
> >
> >
> > On 2014/04/25 17:14:58, gbadawy wrote:
> >
> >> Hi All,
> >> Sebastien I have a question why do you need IsMcsSupported in the Phy
> >>
> > layer
> >
> >> that should be handled in WifiRemoteStationManager, right?
> >> I didn't have a look at your patches. But this is something that I
> >>
> > thought
> >
> >> of reading the emails.
> >> Looking forward to reviewing your patches
> >>
> >
> >  Regards,
> >> Ghada
> >>
> >
> >
> >  On Fri, Apr 25, 2014 at 12:48 PM, <mailto:jinjing81@gmail.com> wrote:
> >>
> >
> >  > On 2014/04/25 07:22:53, S. Deronne wrote:
> >> >
> >> >> On 2014/04/25 02:06:08, jinjing81 wrote:
> >> >> > Hi, Sebastien,
> >> >> >
> >> >> > Thanks for your patch. I looked through your patches and have a
> >> >>
> >> > question
> >> >
> >> >> > for you on one of Patches to Maclow: "correct bug where no block
> >>
> > ack
> >
> >> >>
> >> > is
> >> >
> >> >> > sent when the last subframe is erroneous, while a block ack has
> >>
> > to
> >
> >> >>
> >> > be sent
> >> >
> >> >> > if at least one MPDU has been received in an aggregate."
> >> >> >
> >> >> > I saw you added a few lines in ReceiveError(), but it seems buggy
> >>
> > in
> >
> >> >>
> >> > the
> >> >
> >> >> > sense that at the very beginning, when A-MPDU is transmitted but
> >>
> > the
> >
> >> >> > BlockAck agreement has not been established yet (lost/error
> >> >>
> >> > AddBaRequest).
> >> >
> >> >> > Also it is possible that the last packet in A-MPDU is BAR, then
> >> >> > hdr.GetQosTid() is problematic...I felt this seems a problem with
> >> >> > no guarantee in current BlockAck implementation that BlockAck is
> >> >> > established...
> >> >> >
> >> >> > Any comments from you would be helpful!
> >> >> >
> >> >> > Thanks and best regards
> >> >> >
> >> >> >
> >> >> > On Thu, Apr 24, 2014 at 12:05 PM,
> >> >>
> >> > <mailto:sebastien.deronne@gmail.com> wrote:
> >> >
> >> >> >
> >> >> > > On 2014/04/24 18:48:09, Daniel L. wrote:
> >> >> > >
> >> >> > >> I think from ns-3-dev sounds better.
> >> >> > >>
> >> >> > >
> >> >> > >  On Apr 24, 2014, at 2:24 PM,
> >>
> > mailto:sebastien.deronne@gmail.com
> >
> >> >>
> >> > wrote:
> >> >
> >> >> > >>
> >> >> > >
> >> >> > >  > Thanks Daniel, I will proceed this way then.
> >> >> > >> >
> >> >> > >> > But on which code should I apply a diff file?
> >> >> > >> > Because Ghada implementation is not yet integrated in
> >>
> > ns-3-dev.
> >
> >> >> > >> >
> >> >> > >> > https://codereview.appspot.com/14549044/
> >> >> > >>
> >> >> > >
> >> >> > >
> >>
> >> >> > > Ok. But how I should include Ghada code then?
> >> >> > > Because each chunk is a correction of his code.
> >> >> > >
> >> >> > > https://codereview.appspot.com/14549044/
> >> >> > >
> >> >>
> >> >
> >> >  Do you have a simulation script to reproduce the bug?
> >> >>
> >> >
> >> >  Maybe we should add a condition to check if BlockAck agreement has
> >> >>
> >> > been
> >> >
> >> >> established. What do you think about it?
> >> >> Indeed, BAR as last subframe may be a problem in this case. Maybe
> >>
> > we
> >
> >> >>
> >> > should
> >> >
> >> >> check it in the condition as well.
> >> >>
> >> >
> >> > I do not have a working script now.
> >> > But it seems that this is a common issue when there are multiple
> >>
> > BSSs
> >
> >> > attached to the same channel
> >> > and the number of STAs are high...These control messages can easily
> >>
> > be
> >
> >> > lost...even the association
> >> > can be hard to finish...
> >> >
> >> >
> >> > https://codereview.appspot.com/14549044/
> >> >
> >>
> >
> >
> > https://codereview.appspot.com/14549044/
> >

Maybe you should ask Daniel for more information, I saw the changeset comes from
him.

Ghada, my patch should include your modifications as well? Because when I do a
diff on ns-3-dev, it includes your part since it is not in ns-3-dev.
I don't really know how I should handle with that.

Sign in to reply to this message.

gbadawy

On Tue, Apr 29, 2014 at 4:09 AM, <sebastien.deronne@gmail.com> wrote: > On 2014/04/28 18:24:59, gbadawy ...

9 years, 11 months ago (2014-04-29 12:58:18 UTC) #66

On Tue, Apr 29, 2014 at 4:09 AM, <sebastien.deronne@gmail.com> wrote:

> On 2014/04/28 18:24:59, gbadawy wrote:
>
>> On Fri, Apr 25, 2014 at 4:18 PM, <mailto:sebastien.deronne@gmail.com>
>>
> wrote:
>
>  > Hi Ghada,
>> >
>> > In fact this chunk is not dependent on A-MPDU aggregation.
>> >
>> > Since the addition of "if (IsModeSupported (txMode))" in
>> > YansWifiPhy::StartReceive as well as the move of the IsModeSupported
>> > method to YansWifiPhy, IEEE 802.11n transmissions result in a null
>> > throughput in ns-3-dev.
>> >
>> Who added IsModeSupported to YansWifiPhy? I have looked at my ns3
>>
> versions
>
>> and I can't find it. Any idea why was it added there? what purpose
>>
> does it
>
>> serve?
>>
>
>  Regards,
>> Ghada
>>
>
>
>  > The addition of IsMcsSupported and the modification of the condition
>>
> in
>
>> > StartReceive recover the support of 802.11 MCS rates.
>> >
>> > I created a special bug for this part, you can find it on
>> > https://www.nsnam.org/bugzilla/show_bug.cgi?id=1907
>> >
>> > I hope it helps.
>> >
>> > Regards,
>> > Sebastien
>> >
>> >
>> >
>> > On 2014/04/25 17:14:58, gbadawy wrote:
>> >
>> >> Hi All,
>> >> Sebastien I have a question why do you need IsMcsSupported in the
>>
> Phy
>
>> >>
>> > layer
>> >
>> >> that should be handled in WifiRemoteStationManager, right?
>> >> I didn't have a look at your patches. But this is something that I
>> >>
>> > thought
>> >
>> >> of reading the emails.
>> >> Looking forward to reviewing your patches
>> >>
>> >
>> >  Regards,
>> >> Ghada
>> >>
>> >
>> >
>> >  On Fri, Apr 25, 2014 at 12:48 PM, <mailto:jinjing81@gmail.com>
>>
> wrote:
>
>> >>
>> >
>> >  > On 2014/04/25 07:22:53, S. Deronne wrote:
>> >> >
>> >> >> On 2014/04/25 02:06:08, jinjing81 wrote:
>> >> >> > Hi, Sebastien,
>> >> >> >
>> >> >> > Thanks for your patch. I looked through your patches and have
>>
> a
>
>> >> >>
>> >> > question
>> >> >
>> >> >> > for you on one of Patches to Maclow: "correct bug where no
>>
> block
>
>> >>
>> > ack
>> >
>> >> >>
>> >> > is
>> >> >
>> >> >> > sent when the last subframe is erroneous, while a block ack
>>
> has
>
>> >>
>> > to
>> >
>> >> >>
>> >> > be sent
>> >> >
>> >> >> > if at least one MPDU has been received in an aggregate."
>> >> >> >
>> >> >> > I saw you added a few lines in ReceiveError(), but it seems
>>
> buggy
>
>> >>
>> > in
>> >
>> >> >>
>> >> > the
>> >> >
>> >> >> > sense that at the very beginning, when A-MPDU is transmitted
>>
> but
>
>> >>
>> > the
>> >
>> >> >> > BlockAck agreement has not been established yet (lost/error
>> >> >>
>> >> > AddBaRequest).
>> >> >
>> >> >> > Also it is possible that the last packet in A-MPDU is BAR,
>>
> then
>
>> >> >> > hdr.GetQosTid() is problematic...I felt this seems a problem
>>
> with
>
>> >> >> > no guarantee in current BlockAck implementation that BlockAck
>>
> is
>
>> >> >> > established...
>> >> >> >
>> >> >> > Any comments from you would be helpful!
>> >> >> >
>> >> >> > Thanks and best regards
>> >> >> >
>> >> >> >
>> >> >> > On Thu, Apr 24, 2014 at 12:05 PM,
>> >> >>
>> >> > <mailto:sebastien.deronne@gmail.com> wrote:
>> >> >
>> >> >> >
>> >> >> > > On 2014/04/24 18:48:09, Daniel L. wrote:
>> >> >> > >
>> >> >> > >> I think from ns-3-dev sounds better.
>> >> >> > >>
>> >> >> > >
>> >> >> > >  On Apr 24, 2014, at 2:24 PM,
>> >>
>> > mailto:sebastien.deronne@gmail.com
>> >
>> >> >>
>> >> > wrote:
>> >> >
>> >> >> > >>
>> >> >> > >
>> >> >> > >  > Thanks Daniel, I will proceed this way then.
>> >> >> > >> >
>> >> >> > >> > But on which code should I apply a diff file?
>> >> >> > >> > Because Ghada implementation is not yet integrated in
>> >>
>> > ns-3-dev.
>> >
>> >> >> > >> >
>> >> >> > >> > https://codereview.appspot.com/14549044/
>> >> >> > >>
>> >> >> > >
>> >> >> > >
>> >>
>> >> >> > > Ok. But how I should include Ghada code then?
>> >> >> > > Because each chunk is a correction of his code.
>> >> >> > >
>> >> >> > > https://codereview.appspot.com/14549044/
>> >> >> > >
>> >> >>
>> >> >
>> >> >  Do you have a simulation script to reproduce the bug?
>> >> >>
>> >> >
>> >> >  Maybe we should add a condition to check if BlockAck agreement
>>
> has
>
>> >> >>
>> >> > been
>> >> >
>> >> >> established. What do you think about it?
>> >> >> Indeed, BAR as last subframe may be a problem in this case.
>>
> Maybe
>
>> >>
>> > we
>> >
>> >> >>
>> >> > should
>> >> >
>> >> >> check it in the condition as well.
>> >> >>
>> >> >
>> >> > I do not have a working script now.
>> >> > But it seems that this is a common issue when there are multiple
>> >>
>> > BSSs
>> >
>> >> > attached to the same channel
>> >> > and the number of STAs are high...These control messages can
>>
> easily
>
>> >>
>> > be
>> >
>> >> > lost...even the association
>> >> > can be hard to finish...
>> >> >
>> >> >
>> >> > https://codereview.appspot.com/14549044/
>> >> >
>> >>
>> >
>> >
>> > https://codereview.appspot.com/14549044/
>> >
>>
>
> Maybe you should ask Daniel for more information, I saw the changeset
> comes from him.
>
Yes Daniel said he added the changes my ns3-dev copy is old so it didn't
have these changes in it

>
> Ghada, my patch should include your modifications as well? Because when
> I do a diff on ns-3-dev, it includes your part since it is not in
> ns-3-dev.
> I don't really know how I should handle with that.
>
> I don't really know but maybe you can tell me the errors and I can fix
them in my ns3 version and update my patch with the new fixes or if non of
the issues are blocking maybe we can merge my patch and then apply your
changes later on. I think this is something Tom or Daniel are better suited
to answer.

>
> https://codereview.appspot.com/14549044/
>

Sign in to reply to this message.

Daniel L.

I personally think it might be the best if we can get everything working based ...

9 years, 11 months ago (2014-04-29 22:46:22 UTC) #67

I personally think it might be the best if we can get everything working based
on whatever version you originally cloned from.  I will make it into a
ready-to-merge version and let both of you review it.  How’s that sound?

On Apr 29, 2014, at 8:57 AM, Ghada Badawy <gbadawy@gmail.com> wrote:

> 
> 
> On Tue, Apr 29, 2014 at 4:09 AM, <sebastien.deronne@gmail.com> wrote:
> On 2014/04/28 18:24:59, gbadawy wrote:
> On Fri, Apr 25, 2014 at 4:18 PM, <mailto:sebastien.deronne@gmail.com>
> wrote:
> 
> > Hi Ghada,
> >
> > In fact this chunk is not dependent on A-MPDU aggregation.
> >
> > Since the addition of "if (IsModeSupported (txMode))" in
> > YansWifiPhy::StartReceive as well as the move of the IsModeSupported
> > method to YansWifiPhy, IEEE 802.11n transmissions result in a null
> > throughput in ns-3-dev.
> >
> Who added IsModeSupported to YansWifiPhy? I have looked at my ns3
> versions
> and I can't find it. Any idea why was it added there? what purpose
> does it
> serve?
> 
> Regards,
> Ghada
> 
> 
> > The addition of IsMcsSupported and the modification of the condition
> in
> > StartReceive recover the support of 802.11 MCS rates.
> >
> > I created a special bug for this part, you can find it on
> > https://www.nsnam.org/bugzilla/show_bug.cgi?id=1907
> >
> > I hope it helps.
> >
> > Regards,
> > Sebastien
> >
> >
> >
> > On 2014/04/25 17:14:58, gbadawy wrote:
> >
> >> Hi All,
> >> Sebastien I have a question why do you need IsMcsSupported in the
> Phy
> >>
> > layer
> >
> >> that should be handled in WifiRemoteStationManager, right?
> >> I didn't have a look at your patches. But this is something that I
> >>
> > thought
> >
> >> of reading the emails.
> >> Looking forward to reviewing your patches
> >>
> >
> >  Regards,
> >> Ghada
> >>
> >
> >
> >  On Fri, Apr 25, 2014 at 12:48 PM, <mailto:jinjing81@gmail.com>
> wrote:
> >>
> >
> >  > On 2014/04/25 07:22:53, S. Deronne wrote:
> >> >
> >> >> On 2014/04/25 02:06:08, jinjing81 wrote:
> >> >> > Hi, Sebastien,
> >> >> >
> >> >> > Thanks for your patch. I looked through your patches and have
> a
> >> >>
> >> > question
> >> >
> >> >> > for you on one of Patches to Maclow: "correct bug where no
> block
> >>
> > ack
> >
> >> >>
> >> > is
> >> >
> >> >> > sent when the last subframe is erroneous, while a block ack
> has
> >>
> > to
> >
> >> >>
> >> > be sent
> >> >
> >> >> > if at least one MPDU has been received in an aggregate."
> >> >> >
> >> >> > I saw you added a few lines in ReceiveError(), but it seems
> buggy
> >>
> > in
> >
> >> >>
> >> > the
> >> >
> >> >> > sense that at the very beginning, when A-MPDU is transmitted
> but
> >>
> > the
> >
> >> >> > BlockAck agreement has not been established yet (lost/error
> >> >>
> >> > AddBaRequest).
> >> >
> >> >> > Also it is possible that the last packet in A-MPDU is BAR,
> then
> >> >> > hdr.GetQosTid() is problematic...I felt this seems a problem
> with
> >> >> > no guarantee in current BlockAck implementation that BlockAck
> is
> >> >> > established...
> >> >> >
> >> >> > Any comments from you would be helpful!
> >> >> >
> >> >> > Thanks and best regards
> >> >> >
> >> >> >
> >> >> > On Thu, Apr 24, 2014 at 12:05 PM,
> >> >>
> >> > <mailto:sebastien.deronne@gmail.com> wrote:
> >> >
> >> >> >
> >> >> > > On 2014/04/24 18:48:09, Daniel L. wrote:
> >> >> > >
> >> >> > >> I think from ns-3-dev sounds better.
> >> >> > >>
> >> >> > >
> >> >> > >  On Apr 24, 2014, at 2:24 PM,
> >>
> > mailto:sebastien.deronne@gmail.com
> >
> >> >>
> >> > wrote:
> >> >
> >> >> > >>
> >> >> > >
> >> >> > >  > Thanks Daniel, I will proceed this way then.
> >> >> > >> >
> >> >> > >> > But on which code should I apply a diff file?
> >> >> > >> > Because Ghada implementation is not yet integrated in
> >>
> > ns-3-dev.
> >
> >> >> > >> >
> >> >> > >> > https://codereview.appspot.com/14549044/
> >> >> > >>
> >> >> > >
> >> >> > >
> >>
> >> >> > > Ok. But how I should include Ghada code then?
> >> >> > > Because each chunk is a correction of his code.
> >> >> > >
> >> >> > > https://codereview.appspot.com/14549044/
> >> >> > >
> >> >>
> >> >
> >> >  Do you have a simulation script to reproduce the bug?
> >> >>
> >> >
> >> >  Maybe we should add a condition to check if BlockAck agreement
> has
> >> >>
> >> > been
> >> >
> >> >> established. What do you think about it?
> >> >> Indeed, BAR as last subframe may be a problem in this case.
> Maybe
> >>
> > we
> >
> >> >>
> >> > should
> >> >
> >> >> check it in the condition as well.
> >> >>
> >> >
> >> > I do not have a working script now.
> >> > But it seems that this is a common issue when there are multiple
> >>
> > BSSs
> >
> >> > attached to the same channel
> >> > and the number of STAs are high...These control messages can
> easily
> >>
> > be
> >
> >> > lost...even the association
> >> > can be hard to finish...
> >> >
> >> >
> >> > https://codereview.appspot.com/14549044/
> >> >
> >>
> >
> >
> > https://codereview.appspot.com/14549044/
> >
> 
> Maybe you should ask Daniel for more information, I saw the changeset
> comes from him.
> Yes Daniel said he added the changes my ns3-dev copy is old so it didn't have
these changes in it 
> 
> Ghada, my patch should include your modifications as well? Because when
> I do a diff on ns-3-dev, it includes your part since it is not in
> ns-3-dev.
> I don't really know how I should handle with that.
> 
> I don't really know but maybe you can tell me the errors and I can fix them in
my ns3 version and update my patch with the new fixes or if non of the issues
are blocking maybe we can merge my patch and then apply your changes later on. I
think this is something Tom or Daniel are better suited to answer.
> 
> https://codereview.appspot.com/14549044/
>

Sign in to reply to this message.

yannis

Hi All, - Daniel I have the same question as Ghada,for the implementation of IsModeSupported. ...

9 years, 11 months ago (2014-05-15 17:24:53 UTC) #68

yannis

Hi All, I made a patch, based on Daniel's modifications on Ghada's 3rd patch. -BlockAckManager::PeekNextPacket, ...

9 years, 11 months ago (2014-05-26 22:47:16 UTC) #69

yannis

Something I forgot to mention. While the number of aggregated packets is increasing, the probability ...

9 years, 11 months ago (2014-05-27 17:57:28 UTC) #70

Yongsen

Hi all, I was trying to patch the A-MPDU on NS3.19, but I got some ...

9 years, 10 months ago (2014-06-05 07:01:57 UTC) #71

S. Deronne

Thanks Yannis for your patch, I will try to have a look at this shortly. ...

9 years, 10 months ago (2014-06-12 20:34:55 UTC) #72

Yongsen

On 2014/05/27 17:57:28, yannis wrote: > Something I forgot to mention. While the number of ...

9 years, 10 months ago (2014-06-24 10:48:54 UTC) #73

Tom Henderson

On 2014/06/24 10:48:54, Yongsen wrote: > On 2014/05/27 17:57:28, yannis wrote: > > Something I ...

9 years, 10 months ago (2014-06-24 13:56:34 UTC) #74

Tom Henderson

Hi all, Can we make a final push to finish MPDU aggregation off in the ...

9 years, 6 months ago (2014-09-25 20:04:52 UTC) #75

gbadawy

On 2014/09/25 20:04:52, Tom Henderson wrote: > Hi all, > > Can we make a ...

9 years, 6 months ago (2014-09-26 12:35:01 UTC) #76

Tom Henderson

> Since the original patch was based on ns3-dev 10 months ago I will have ...

9 years, 5 months ago (2014-11-01 14:46:27 UTC) #77

s.fakhrali

On 2014/11/01 14:46:27, Tom Henderson wrote: > > Since the original patch was based on ...

9 years, 5 months ago (2014-11-21 14:34:39 UTC) #78

Gigi

On 2014/11/01 14:46:27, Tom Henderson wrote: > > Since the original patch was based on ...

9 years, 4 months ago (2014-12-06 01:38:03 UTC) #79

Gigi

On 2014/12/06 01:38:03, GigiIggytech wrote: > On 2014/11/01 14:46:27, Tom Henderson wrote: > > > ...

9 years, 4 months ago (2014-12-06 02:13:46 UTC) #80

Gigi

Has anyone tried enabling RTS/CTS? I found that if RTS/CST is enabled then the aggregation ...

9 years, 1 month ago (2015-02-26 19:58:08 UTC) #81

S. Deronne

9 years, 1 month ago (2015-02-26 22:28:04 UTC) #82

The final code has been pushed in ns-3.22, so please do no longer use this
rietveld.

If you encounter some bugs, please either post a message on ns-3-users google
group or open a bug in Bugzilla.

In the latest version, RTS/CTS together with A-MPDU aggregation is fully
supported.
Anyway, some bugs can still happen, so feel free to indicate when issues are
found.

Sign in to reply to this message.

Issue 14549044: Adding MPDU aggregation

Description

Patch Set 1 #

Patch Set 2 : Correcting some compilation errors #

Patch Set 3 : changed A-MPDU implementation to simplify PCAP generation #

Messages