Re: Full Compressed Block Ack support (issue144050)

[mathieu...@gmail.com]

Mirko,

Sorry for this long delay in getting back to you. The overall patch
looks great and I believe that you could merge as-is after our next
release: we are in feature freeze now.

I have only two comments:
- you or someone else needs to update the mesh code to use the new
action header you introduced. This can be done after the merge.

- I think that before you start working on further features, it would
make sense to change the way the ba manager/tx queue are working: I am
not going to _request_ that you change things that way but I believe
that doing so would simplify your further work. More details below.

Right now, enqueued packets are removed one by one for their first
transmission within a ba transaction after you receive a successfull
ADDBA response. I think that the logic of the EdcaTxop code as well as
the overall data/code flow would be simplified if, after receiving the
successfull ADDBA response, you removed all the packets you intend to
transmit at once from the tx queue and moved them to a separate queue in
the ba manager. Then, you would get them one by one from the ba manager
for transmission in the edca txop code and would ignore the WifiMacQueue
until you are done with these packets. This would make the transmission
and retransmission code the same which would make it easier to make sure
that retransmissions behave correctly.

I know that this would change slightly the behavior of the code, most
notably with regard to the tx queue inactivity timeout but I feel that
this would simplify a bit the ba manager code and would make the
transmission logic simpler.

Anyway, thanks again for your continued work on this code.

http://codereview.appspot.com/144050

[mk.b...@gmail.com]

On 2009/12/14 12:58:40, Mathieu Lacage wrote:
> Mirko,

> Sorry for this long delay in getting back to you.

No problem.

> I have only two comments:
> - you or someone else needs to update the mesh code to use the >new
action
> header you introduced. This can be done after the merge.

Kirill have already moved implementation of action headers in
src/devices/wifi/mgt-headers.{cc,h}. I have already aligned my
implementation with them.

Yes, you are right. In my opinion this semplifies logic transmission but
doesn't permit to add packets easily (packets that are enqueued in the
queue after that the n packets under block ack are moved in the ba
manager) to
current block ack exchange. I think this problem exists because standard
is not clear about the correct beahviour. However i'll try to semplify
the logic.

Thank you very much for these advices.

Mirko


http://codereview.appspot.com/144050

[Mathieu Lacage]

On Mon, Dec 14, 2009 at 3:15 PM, <mk.b...@gmail.com> wrote:

Kirill have already moved implementation of action headers in
src/devices/wifi/mgt-headers.{cc,h}. I have already aligned my
implementation with them.

Ok: I did not carefully monitor the patches going in ns-3-dev: thanks for the information.
 

Yes, you are right. In my opinion this semplifies logic transmission but
doesn't permit to add packets easily (packets that are enqueued in the
queue after that the n packets under block ack are moved in the ba
manager) to

Hrm, that is a good point: it is clearly un-desirable. If you can figure out what real physical systems do, it will probably help.

Mathieu
--
Mathieu Lacage <mathieu...@gmail.com>