Full Compressed Block Ack support

[faker.moatamri, 2009-11-03 14:10:49]

Reviews

http://codereview.appspot.com/144050/diff/1/2
File examples/wireless/wifi-blockack.cc (right):

http://codereview.appspot.com/144050/diff/1/2#newcode21
Line 21: //This is a simple example in order to show how 802.11n compressed
block ack mechanism could be used.
please use doxygen style comments

http://codereview.appspot.com/144050/diff/1/2#newcode55
Line 55: LogComponentEnable ("EdcaTxopN", LOG_LEVEL_DEBUG);
I think that debug level logs should not be activated all the time.

http://codereview.appspot.com/144050/diff/1/3
File src/devices/wifi/block-ack-agreement.cc (right):

http://codereview.appspot.com/144050/diff/1/3#newcode86
Line 86: BlockAckAgreement::SetAmsduSupported (void)
why not SetAmsduSupported (true or false) instead of two methods?

http://codereview.appspot.com/144050/diff/1/3#newcode95
Line 95: void
Again why not SetState(PENDING or ESTABLISHED or INACTIVE or UNSUCCESSFUL)
instead of having 4 methods?

http://codereview.appspot.com/144050/diff/1/3#newcode163
Line 163: return (m_amsduSupported == 1)?true:false;
this variable should simply be a boolean, also in C you can use 0 as false and
one as true, so return m_amsduSupported would normally work.

http://codereview.appspot.com/144050/diff/1/4
File src/devices/wifi/block-ack-agreement.h (right):

http://codereview.appspot.com/144050/diff/1/4#newcode27
Line 27: /**
No doxygen here! can you please add doxygen?

http://codereview.appspot.com/144050/diff/1/4#newcode80
Line 80: /* represents the state for this agreement.
I think that doxygen style is 
/**
 *
**/

http://codereview.appspot.com/144050/diff/1/5
File src/devices/wifi/block-ack-manager.cc (right):

http://codereview.appspot.com/144050/diff/1/5#newcode102
Line 102: return ((*it).second.first.IsUnsuccessful () == true);
your don't need to use '== true' here

http://codereview.appspot.com/144050/diff/1/5#newcode121
Line 121: agreement.SetAmsduSupported ();
this will become:
agreement.SetAmsduSupported (reqHdr->IsAmsduSupported ())
instead of the multiple if and the multiple methods

http://codereview.appspot.com/144050/diff/1/5#newcode176
Line 176: if (respHdr->IsAmsduSupported ())
same as above...

http://codereview.appspot.com/144050/diff/1/5#newcode243
Line 243: /* From section 9.10.3 in IEEE802.11e standard:
doxygen style

http://codereview.appspot.com/144050/diff/1/5#newcode308
Line 308: BlockAckManager::GetNRetryPacketsForAgreement (Mac48Address recipient,
uint8_t tid) const
those two functions are really very similar and I guess you used copy and paste
at certain time here, can you please make only one method accepting a variable
saying if it will compute Nretypackets or Npackets?

http://codereview.appspot.com/144050/diff/1/5#newcode428
Line 428: NS_ASSERT (false);
NS_ERROR with message

http://codereview.appspot.com/144050/diff/1/5#newcode441
Line 441: /* This method checks if a BlockAckRequest frame should be send to the
recipient station.
doxygen style

http://codereview.appspot.com/144050/diff/1/5#newcode484
Line 484: NS_ASSERT (false);
NS_ERROR, please replace all NS_ASSERT (false) by NS_ERROR

http://codereview.appspot.com/144050/diff/1/6
File src/devices/wifi/block-ack-manager.h (right):

http://codereview.appspot.com/144050/diff/1/6#newcode49
Line 49: ~BlockAckManager ();
constructor?

http://codereview.appspot.com/144050/diff/1/7
File src/devices/wifi/ctrl-headers.cc (right):

http://codereview.appspot.com/144050/diff/1/7#newcode59
Line 59: {}
No code?

http://codereview.appspot.com/144050/diff/1/7#newcode77
Line 77: NS_ASSERT (false);
Please replace NS_ASSERT (false) by NS_ERROR with message...

http://codereview.appspot.com/144050/diff/1/7#newcode80
Line 80: break;
also it is good practice to add default behavior and to generate an error if the
default behavior should never be reached.

http://codereview.appspot.com/144050/diff/1/7#newcode109
Line 109: NS_ASSERT (false);
same remarks as above

http://codereview.appspot.com/144050/diff/1/7#newcode154
Line 154: //res |= (m_reserved << 3) & (0x1ff<<3);
please remove all commented code

http://codereview.appspot.com/144050/diff/1/7#newcode180
Line 180: void
please use SetAck (u_int32 ack)
{
  m_barAckPolicy = ack;
}
instead of writing 2 methods, or justify why you did that?

http://codereview.appspot.com/144050/diff/1/7#newcode228
Line 228: return (m_barAckPolicy == 1)?true:false;
I think that in C++ return m_barAckPolicy would work.

http://codereview.appspot.com/144050/diff/1/7#newcode253
Line 253: return (m_multiTid == 0 && m_compressed == 1)?true:false;
return (!m_multiTid && m_compressed) would do the job

http://codereview.appspot.com/144050/diff/1/7#newcode318
Line 318: break;
same as before

http://codereview.appspot.com/144050/diff/1/7#newcode340
Line 340: case 0:
also coding style for switch case statements

http://codereview.appspot.com/144050/diff/1/7#newcode402
Line 402: CtrlBAckResponseHeader::SetAck (void)
same as before

http://codereview.appspot.com/144050/diff/1/7#newcode724
Line 724: //uint8_t index = (seq - m_startingSeq)%4096;
remove

http://codereview.appspot.com/144050/diff/1/9
File src/devices/wifi/edca-txop-n.cc (right):

http://codereview.appspot.com/144050/diff/1/9#newcode376
Line 376: !m_currentHdr.IsQosAmsdu ()) ||
weird indentation

http://codereview.appspot.com/144050/diff/1/9#newcode418
Line 418: WifiMacHeader::ADDR1,
please avoid white space changes

http://codereview.appspot.com/144050/diff/1/9#newcode817
Line 817: MY_DEBUG ("received ADDBA response from "<<recipient);
What about NS_LOG_DEBUG?

http://codereview.appspot.com/144050/diff/1/10
File src/devices/wifi/edca-txop-n.h (right):

http://codereview.appspot.com/144050/diff/1/10#newcode118
Line 118: void GotBlockAck (const CtrlBAckResponseHeader *blockAck, Mac48Address
recipient);
the file is missing doxygen here and in the following block of methods

http://codereview.appspot.com/144050/diff/1/10#newcode176
Line 176: /* For now is typically invoked to complete transmission of a packets
sent with ack policy
please use doxygen style

http://codereview.appspot.com/144050/diff/1/11
File src/devices/wifi/mac-low.cc (right):

http://codereview.appspot.com/144050/diff/1/11#newcode161
Line 161: void
again you can replace these 3 methods by only one taking into argument the
BlockAck style

http://codereview.appspot.com/144050/diff/1/11#newcode770
Line 770: NS_ASSERT (false);
NS_FATAL_ERROR

http://codereview.appspot.com/144050/diff/1/11#newcode1523
Line 1523: if (respHdr->IsAmsduSupported ())
can be agreement.SetAmsduSupported (respHdr->IsAmsduSupported ())

http://codereview.appspot.com/144050/diff/1/11#newcode1672
Line 1672: NS_ASSERT (false);
NS_FATAL_ERROR replaces all NS_ASSERT (false)

http://codereview.appspot.com/144050/diff/1/15
File src/devices/wifi/mgt-headers.cc (right):

http://codereview.appspot.com/144050/diff/1/15#newcode428
Line 428: //This header MUST be serialized with Action Frames.
Is it normal that this function is empty?

http://codereview.appspot.com/144050/diff/1/15#newcode554
Line 554: MgtAddBaRequestHeader::SetDelayedBlockAck ()
can be only one method

http://codereview.appspot.com/144050/diff/1/16
File src/devices/wifi/mgt-headers.h (right):

http://codereview.appspot.com/144050/diff/1/16#newcode162
Line 162: public:
Doxygen?

http://codereview.appspot.com/144050/diff/1/19
File src/devices/wifi/qap-wifi-mac.cc (right):

http://codereview.appspot.com/144050/diff/1/19#newcode835
Line 835: void
this is a copy of the function in the previous .cc file, is that normal? can you
do otherwise to avoid copy and paste of the same code?

http://codereview.appspot.com/144050/diff/1/23
File src/devices/wifi/qsta-wifi-mac.cc (right):

http://codereview.appspot.com/144050/diff/1/23#newcode826
Line 826: 
same copy and paste function again!

http://codereview.appspot.com/144050/diff/1/29
File src/devices/wifi/wifi-mac.cc (right):

http://codereview.appspot.com/144050/diff/1/29#newcode75
Line 75: // This value must be rivisited
when should it be revisited?

http://codereview.appspot.com/144050/diff/1/29#newcode119
Line 119: //this methos must be implemented by QoS WifiMacs
any plans?

[Andrey Mazo, 2009-11-03 23:19:58]

Valgrind seems to be unhappy with this code and shows memory leaks:

==30779== LEAK SUMMARY:
==30779==    definitely lost: 1,008 bytes in 6 blocks.
==30779==    indirectly lost: 9,156 bytes in 103 blocks.
==30779==      possibly lost: 0 bytes in 0 blocks.
==30779==    still reachable: 0 bytes in 0 blocks.
==30779==         suppressed: 0 bytes in 0 blocks.

http://codereview.appspot.com/144050/diff/1/2
File examples/wireless/wifi-blockack.cc (right):

http://codereview.appspot.com/144050/diff/1/2#newcode1
Line 1: /* -*-  Mode: C++; c-file-style: "gnu"; indent-tabs-mode:nil; -*- */
Have you forgotten to add wscript entry for this example?

http://codereview.appspot.com/144050/diff/1/5
File src/devices/wifi/block-ack-manager.cc (right):

http://codereview.appspot.com/144050/diff/1/5#newcode154
Line 154: m_retryPackets.erase (toRemove);
Isn't this block the same as
i = m_retryPackets.erase (i);
?

http://codereview.appspot.com/144050/diff/1/5#newcode375
Line 375: it->second.second.erase (toRemove);
Again, isn't this block the same as
queueIt = it->second.second.erase (queueIt);
?

http://codereview.appspot.com/144050/diff/1/6
File src/devices/wifi/block-ack-manager.h (right):

http://codereview.appspot.com/144050/diff/1/6#newcode34
Line 34: using namespace std;
Seems to be useless here.
There are a couple of containers in block-ack-manager.cc but there much more
std::containers, so better convert them all to std::container form.

http://codereview.appspot.com/144050/diff/1/7
File src/devices/wifi/ctrl-headers.cc (right):

http://codereview.appspot.com/144050/diff/1/7#newcode99
Line 99: }
What's the reason for this switch?
Behaviors in both cases are identical.

http://codereview.appspot.com/144050/diff/1/7#newcode130
Line 130: }
same as above.

http://codereview.appspot.com/144050/diff/1/7#newcode155
Line 155: res |= (m_tidInfo << 12) & (0xf<<12);
This way looks simplier:
 res |= m_barAckPolicy & 0x1;
 res |= (m_multiTid & 0x1) << 1;
 res |= (m_compressed & 0x1) << 2;
 res |= (m_tidInfo & 0xf) << 12;

http://codereview.appspot.com/144050/diff/1/7#newcode277
Line 277: bitmap.m_compressedBitmap = 0;
memset (&bitmap, 0, sizeof(bitmap));
will do the job better.

http://codereview.appspot.com/144050/diff/1/7#newcode491
Line 491: res |= (m_tidInfo << 12) & (0xf<<12);
same as above

http://codereview.appspot.com/144050/diff/1/8
File src/devices/wifi/ctrl-headers.h (right):

http://codereview.appspot.com/144050/diff/1/8#newcode81
Line 81: uint8_t m_compressed;
Isn't boolean enough here instead of uint8_t?
This will simplify several conditional statements.

http://codereview.appspot.com/144050/diff/1/11
File src/devices/wifi/mac-low.cc (right):

http://codereview.appspot.com/144050/diff/1/11#newcode1529
Line 1529: agreement.SetAmsduNotSupported ();
Excess white space in indent.

http://codereview.appspot.com/144050/diff/1/25
File src/devices/wifi/wifi-mac-header.cc (right):

http://codereview.appspot.com/144050/diff/1/25#newcode849
Line 849: break;
It's safe to omit these 2 lines.

http://codereview.appspot.com/144050/diff/1/25#newcode1060
Line 1060: break;
Again, safe to omit these 2 lines.

http://codereview.appspot.com/144050/diff/1/25#newcode1111
Line 1111: break;
same as above.

http://codereview.appspot.com/144050/diff/1/27
File src/devices/wifi/wifi-mac-queue.cc (right):

http://codereview.appspot.com/144050/diff/1/27#newcode127
Line 127: m_queue.erase (toRemove);
Again, isn't it equal to
i = m_queue.erase (i);
?

[Andrey Mazo, 2009-11-04 00:14:03]

I've tried to set some more Ptr's to zero in several DoDispose ()'s, but it
hasn't helped.

http://codereview.appspot.com/144050/diff/1/23
File src/devices/wifi/qsta-wifi-mac.cc (right):

http://codereview.appspot.com/144050/diff/1/23#newcode793
Line 793: m_low->SetQueueForAc (ac, edca);
You're introducing a circular references here.
edca->SetLow (m_low);
m_low->SetQueueForAc (ac, edca);
Probably this makes valgrind unhappy.

[Andrey Mazo, 2009-11-04 11:10:15]

On 2009/11/03 23:19:58, Andrey Mazo wrote:
> Valgrind seems to be unhappy with this code and shows memory leaks:
> 
> ==30779== LEAK SUMMARY:
> ==30779==    definitely lost: 1,008 bytes in 6 blocks.
> ==30779==    indirectly lost: 9,156 bytes in 103 blocks.
> ==30779==      possibly lost: 0 bytes in 0 blocks.
> ==30779==    still reachable: 0 bytes in 0 blocks.
> ==30779==         suppressed: 0 bytes in 0 blocks.

Mirko noticed, that the leak already exists in simple-wifi-frame-aggregation
example in ns-3-dev, so the current patch set doesn't introduce any new leaks.
Sorry for false alarm here.

[Andrey Mazo, 2009-11-04 11:16:29]

On 2009/11/04 13:39, Mirko Banchi wrote:
>On 2009/11/04 00:14:03, Andrey Mazo wrote:
>> I've tried to set some more Ptr's to zero in several DoDispose ()'s, but it
>> hasn't helped.
>There is no need because Ptr denstructor is called
I think, zeroing Ptr's in DoDispose()'s may break some circular references and
can help to get everything freed correctly.

[Andrey Mazo, 2009-11-04 11:40:57]

On 2009/11/04 13:35, Mirko Banchi wrote:
>On 2009/11/03 23:19:58, Andrey Mazo wrote:
>> http://codereview.appspot.com/144050/diff/1/7#newcode277
>> Line 277: bitmap.m_compressedBitmap = 0;
>> memset (&bitmap, 0, sizeof(bitmap));
>> will do the job better.
>Why do you advice a function call instead of a simple for?
1) To avoid zeroing the same memory twice
2) Function call is faster here (gcc convert it so several movl instructions
instead of conditional jump plus movl of for-loop.

>> http://codereview.appspot.com/144050/diff/1/8#newcode81
>> Line 81: uint8_t m_compressed;
>> Isn't boolean enough here instead of uint8_t?
>> This will simplify several conditional statements.
>Which?
I meant m_baAckPolicy, m_multiTid and m_compressed to be boolean.
They all seems to have only 2 values, so maybe it's worth replacing all
corresponding switch statements with if-else.

[Nicola Baldo, 2009-11-04 17:27:41]

In general the patch seems good to me, though I have to say that due to the many
modifications proposed I have not been able to go through all the code in
detail. 
I am suggesting a few minor modifications to address mainly  naming issues. A
few of the proposed changes does not seem to be relevant for the BlockAck
support, if this is confirmed they should go in separate patches.

http://codereview.appspot.com/144050/diff/1/7
File src/devices/wifi/ctrl-headers.cc (right):

http://codereview.appspot.com/144050/diff/1/7#newcode66
Line 66: switch (m_multiTid) {
since the argument of this switch statement should actually be a boolean
variable, the switch statement itself should be changed to an if statement.
Note that in the following there are several statements like this which should
receive the same treatment.

http://codereview.appspot.com/144050/diff/1/7#newcode152
Line 152: res |= (m_multiTid << 1) & (0x1<<1);
change this to:
if (m_barAckPolicy)
  {
    res |= 0x01;
  }
and similarly for the other statements

http://codereview.appspot.com/144050/diff/1/8
File src/devices/wifi/ctrl-headers.h (right):

http://codereview.appspot.com/144050/diff/1/8#newcode43
Line 43: class CtrlBAckRequestHeader : public Header {
Can we use the same naming adopted in the standard here, i.e., BlockAckReq
instead of BAckRequest? Thus it would be:
class CtrlBlockAckReqHeader : public Header

http://codereview.appspot.com/144050/diff/1/8#newcode54
Line 54: void SetAck (void);
the name of the method is not representative of what the method does, what about
something like this:
enum BarAckPolicy {NormalAck, NoAck};
void SetAckPolicy(enum BarAckPolicy p);

http://codereview.appspot.com/144050/diff/1/8#newcode56
Line 56: void SetMultiTid (void);
As above, the name of the method is not very clear, what about merging all these
methods into a single one:
enum BarFrameVariant {BasicAck, CompressedAck, MultiTidAck};
void SetFrameVariant(enum BarFrameVariant v);

http://codereview.appspot.com/144050/diff/1/8#newcode81
Line 81: uint8_t m_compressed;
On 2009/11/03 23:19:58, Andrey Mazo wrote:
> Isn't boolean enough here instead of uint8_t?
> This will simplify several conditional statements.

I agree: m_barAckPolicy, m_multiTid and m_compressed could all be boolean, since
they all represent 1-bit subfields in the Bar Control Field.

http://codereview.appspot.com/144050/diff/1/8#newcode86
Line 86: class CtrlBAckResponseHeader : public Header {
again, can we use the same naming used in the standard (BlockAck instead of
BAckResponse)?

http://codereview.appspot.com/144050/diff/1/11
File src/devices/wifi/mac-low.cc (right):

http://codereview.appspot.com/144050/diff/1/11#newcode365
Line 365: it->second = 0;
is this relevant to the BlockAck feature? If not, it should go as a separate
patch.

http://codereview.appspot.com/144050/diff/1/12
File src/devices/wifi/mac-low.h (right):

http://codereview.appspot.com/144050/diff/1/12#newcode478
Line 478: void SetQueueForAc (AccessClass ac, Ptr<EdcaTxopN> queue);
is this new method part of the BlockAck patch?

http://codereview.appspot.com/144050/diff/1/22
File src/devices/wifi/qos-utils.h (right):

http://codereview.appspot.com/144050/diff/1/22#newcode50
Line 50: * station completed "old" (see section in IEEE802.11e) packets must be
forwarded up before "new" packets.
which section?

[Andrey Mazo, 2009-11-04 22:04:46]

On 2009/11/04 11:10:15, Andrey Mazo wrote:
> On 2009/11/03 23:19:58, Andrey Mazo wrote:
> > Valgrind seems to be unhappy with this code and shows memory leaks:
> > 
> > ==30779== LEAK SUMMARY:
> > ==30779==    definitely lost: 1,008 bytes in 6 blocks.
> > ==30779==    indirectly lost: 9,156 bytes in 103 blocks.
> > ==30779==      possibly lost: 0 bytes in 0 blocks.
> > ==30779==    still reachable: 0 bytes in 0 blocks.
> > ==30779==         suppressed: 0 bytes in 0 blocks.
> 
> Mirko noticed, that the leak already exists in simple-wifi-frame-aggregation
> example in ns-3-dev, so the current patch set doesn't introduce any new leaks.
> Sorry for false alarm here.
Well, alarm wasn't false actually. :(
In ns-3-dev@3827a2a06b38 no wireless examples show memory leaks (mesh example
shows only uninitialised values).
After applying this patch set against ns-3-dev@3827a2a06b38 valgrind complains
about leaks in simple-wifi-frame-aggregation and wifi-blockack.
So either this patch set adds memory leaks, or reveals already existing ones.

[and.kirill, 2009-11-10 17:15:26]

The code looks very nice, and a really huge job is done, but the main thing that
one must remember is that after mering this code all duplicates must be removed
(like ActionHeader and DcaTxop class).

http://codereview.appspot.com/144050/diff/5001/6006
File src/devices/wifi/block-ack-manager.h (right):

http://codereview.appspot.com/144050/diff/5001/6006#newcode264
src/devices/wifi/block-ack-manager.h:264: */
Please, fix all comments. Doxygen requires /** ... */ instead of /* ... */

http://codereview.appspot.com/144050/diff/5001/6010
File src/devices/wifi/edca-txop-n.h (right):

http://codereview.appspot.com/144050/diff/5001/6010#newcode34
src/devices/wifi/edca-txop-n.h:34: #include "ctrl-headers.h"
Before this patchset, an only difference between DcaTxop and EdcaTxop was in
n-aggregation. If you (after merging this code, as a future work) add an
attribute that enables or disables N-aggregation (because block ack may be
disabled), DcaTxop class may be killed. This will help to avoid duplicate code
and will help in bugfix process.

http://codereview.appspot.com/144050/diff/5001/6016
File src/devices/wifi/mgt-headers.h (right):

http://codereview.appspot.com/144050/diff/5001/6016#newcode153
src/devices/wifi/mgt-headers.h:153: */
Doxygen fix (/**...*/ instead of /*...*/)

http://codereview.appspot.com/144050/diff/5001/6016#newcode171
src/devices/wifi/mgt-headers.h:171: /* For now only actions for BLOCK_ACK
category are implemented */
Action header is also needed by 802.11s code. The same header is implemented in
src/devices/mesh/dot11s and class is called WifiMeshActionHeader. So, after your
code will be merged, it is needed to merge WifiMeshActionHeader and your action
header to avoid duplicated code. In my class action value is a union of enums,
and each enum is responsible for each category value. This approach avoids a lot
of methods like IsSomething. What do you thing about this in the future?

[Nicola Baldo, 2009-11-13 12:58:49]

Good work. The comments below refer to a number of minor issues found here and
there.

http://codereview.appspot.com/144050/diff/7001/7007
File src/devices/wifi/block-ack-manager.h (right):

http://codereview.appspot.com/144050/diff/7001/7007#newcode68
src/devices/wifi/block-ack-manager.h:68: *
Wasn't this enum already documented in block-ack-agreement.h?

http://codereview.appspot.com/144050/diff/7001/7009
File src/devices/wifi/ctrl-headers.h (right):

http://codereview.appspot.com/144050/diff/7001/7009#newcode55
src/devices/wifi/ctrl-headers.h:55: void SetVariant (enum BlockAckType type);
can you choose a coherent naming here? 
Like:
void SetVariant (enum BlockAckVariant variant)
or
void SetType (enum BlockAckType type)

http://codereview.appspot.com/144050/diff/7001/7009#newcode74
src/devices/wifi/ctrl-headers.h:74: /* barAckPolicy field is used only for
HT-delayed block ack,
not clear what this comment means

http://codereview.appspot.com/144050/diff/7001/7009#newcode95
src/devices/wifi/ctrl-headers.h:95: void SetVariant (enum BlockAckType type);
same as above about naming

http://codereview.appspot.com/144050/diff/7001/7012
File src/devices/wifi/mac-low.cc (right):

http://codereview.appspot.com/144050/diff/7001/7012#newcode759
src/devices/wifi/mac-low.cc:759: //Delayed block ack...
nothing to be done in this case? what about NS_LOG_DEBUG at least?

http://codereview.appspot.com/144050/diff/7001/7012#newcode764
src/devices/wifi/mac-low.cc:764: //There's not a valid agreement for this block
ack request
same as a few lines earlier

http://codereview.appspot.com/144050/diff/7001/7012#newcode770
src/devices/wifi/mac-low.cc:770: NS_ASSERT (false);
NS_FATAL_ERROR ("Multi-tid block ack is not supported.");

http://codereview.appspot.com/144050/diff/7001/7012#newcode913
src/devices/wifi/mac-low.cc:913: * the BlockAckReq frame if it is sent in
response to a BlockAckReq frame.
I don't see why this comment is relevant here, since the WifiMode is passed as a
function parameter. I guess this comment should be moved elsewhere.

[Mathieu Lacage, 2009-11-17 11:59:13]

Hi,

First, I apologize for taking so long to properly review this code. With that
being said, there are lots of comments below so, I will try to give a high-level
perspective on them first.

1) I feel that the current patchset introduces too many inter-dependencies
between objects: low-level objects start having direct pointers back to
higher-level objects which, so far, has been avoided. 

1.1) Specifically, MacLow should _not_ depend on EdcaTxop and I believe that it
should not be too hard to fix this for MacLow.

1.2) The same goes for the queue: it has a pointer to the ba manager but this is
pretty evil: I don't see why the code could not be reworked to avoid this issue
by moving out of the WifiMacQueue all the packets which have been already
transmitted on the medium.

1.3) The same goes for the ba manager: it is pointed to by EdcaTxop but it
points back to EdcaTxop. These classes now have a circular dependency which
makes it really hard to follow call stacks. There are two options here: you
could simply merge much of the ba manager code _into_ EdcaTxopN and move out in
simple helper classes the remaining code or you could try to reorganize the ba
manager code to avoid having a back pointer to EdcaTxop by moving out of the ba
manager select pieces of code such as the block ack req packet queuing

2) the ba manager code looks really scary: it's really hard to understand the
logic of the code. One of the most problematic method is
SwitchToBlockAckIfNeeded which does too many things: it would really help if you
could first make the state tests explicit (not test for (!state) but for
(state)) and then move the state change code out of the state test code. If you
start to re-organize that code, I believe that some of my comments on the ba
manager (for example, my comment on GetNextBlockAckRequest below) will become
stale and irrelevant.

3) it would be really nice to unify/align the action frame handling with the
mesh code

4) tests, tests, tests: I could not see any tests for this new code. We need
some because I can't believe _anyone_ will be able to figure out how this block
ack code works so, we need a safety net and tests are that safety net.

To summarize, I feel rather un-inclined to merge this in ns-3.7: I feel that the
current codebase needs some serious re-organization as well as tests before
merging.

http://codereview.appspot.com/144050/diff/7098/8005
File src/devices/wifi/block-ack-manager.cc (right):

http://codereview.appspot.com/144050/diff/7098/8005#newcode200
src/devices/wifi/block-ack-manager.cc:200: SwitchToBlockAckIfNeeded (recipient,
tid, hdr.GetSequenceNumber ()))
This code would be much more clear if you separated clearly the switch to
ESTABLISHED from the rest:

if (ExistsInState (..., INACTIVE))
  {
    SwitchToEstablishedState ();
  }
if (ExistsInState (..., ESTABLISHED))
  {
    ... do your stuff;
  }

http://codereview.appspot.com/144050/diff/7098/8005#newcode377
src/devices/wifi/block-ack-manager.cc:377: (!foundFirstLost &&
!SwitchToBlockAckIfNeeded (recipient, tid)))
If you change the signature of this function (GotBlockAck) to include the
sequence number returned by GetNextSeqNumberByTidAndAddress, you don't need to
call this overloaded SwitchToBlockAckIfNeeded method, and, if you do this, you
can remove the 2-argument SwitchToBlockAckIfNeeded method

But, more importantly, I can't make any sense of this logic. You seem to be
doing: "if cannot become established, mark inactive" which does not make much
sense: inactive should be moved to _only_ if you are already established, right
?

http://codereview.appspot.com/144050/diff/7098/8005#newcode409
src/devices/wifi/block-ack-manager.cc:409: (GetNRetryPacketsForAgreement
(recipient, tid) == 0 &&
why do you schedule a block ack request if there are no queued packets ?

http://codereview.appspot.com/144050/diff/7098/8005#newcode500
src/devices/wifi/block-ack-manager.cc:500:
BlockAckManager::NotifyMpduTransmission (Mac48Address recipient, uint8_t tid)
change the signature to return a Ptr<Packet> which is the block ack request if
you need one and make the caller store it in its outgoing queue.

http://codereview.appspot.com/144050/diff/7098/8005#newcode547
src/devices/wifi/block-ack-manager.cc:547: uint32_t packets =
m_queue->GetNPacketsByTidAndAddress (tid, WifiMacHeader::ADDR1, recipient);
don't you miss a + GetNPacketsForAgreement (recipient,tid) here ?

http://codereview.appspot.com/144050/diff/7098/8006
File src/devices/wifi/block-ack-manager.h (right):

http://codereview.appspot.com/144050/diff/7098/8006#newcode91
src/devices/wifi/block-ack-manager.h:91: void UpdateAgreement (const
MgtAddBaResponseHeader *respHdr, Mac48Address recipient);
NotifySuccessfulAgreement

http://codereview.appspot.com/144050/diff/7098/8006#newcode116
src/devices/wifi/block-ack-manager.h:116: void GetNextBlockAckRequest (Ptr<const
Packet> &req, WifiMacHeader &hdr, bool &immediate);
the pass-by-reference semantics here make the caller code harder to read that
needed. 

struct BlockAckRequest {
  Ptr<const Packet> p;
  WifiMacHeader header;
  bool immediate;
};

struct BlockAckRequest GetNextBlockAckRequest (void);

Yes, it's not efficient but we can optimized this later if needed.

http://codereview.appspot.com/144050/diff/7098/8006#newcode131
src/devices/wifi/block-ack-manager.h:131: void GotBlockAck (const
CtrlBAckResponseHeader *blockAck, Mac48Address recipient);
NotifyGotBlockAck

http://codereview.appspot.com/144050/diff/7098/8006#newcode139
src/devices/wifi/block-ack-manager.h:139: uint32_t GetNPacketsForAgreement
(Mac48Address recipient, uint8_t tid) const;
GetNBufferedPackets ?

http://codereview.appspot.com/144050/diff/7098/8006#newcode147
src/devices/wifi/block-ack-manager.h:147: uint32_t GetNRetryPacketsForAgreement
(Mac48Address recipient, uint8_t tid) const;
GetNRetryNeededPackets

http://codereview.appspot.com/144050/diff/7098/8006#newcode155
src/devices/wifi/block-ack-manager.h:155: void NotifyEstablishedAgreement
(Mac48Address recipient, uint8_t tid, uint16_t startingSeq);
NotifyAgreementEstablished

http://codereview.appspot.com/144050/diff/7098/8006#newcode164
src/devices/wifi/block-ack-manager.h:164: void NotifyUnsuccessfulAgreement
(Mac48Address recipient, uint8_t tid);
NotifyAgreementUnsuccessful

http://codereview.appspot.com/144050/diff/7098/8006#newcode174
src/devices/wifi/block-ack-manager.h:174: void NotifyMpduTransmission
(Mac48Address recipient, uint8_t tid);
NotifyMpduTransmited

http://codereview.appspot.com/144050/diff/7098/8006#newcode211
src/devices/wifi/block-ack-manager.h:211: void RefreshBlockAck (Mac48Address
recipient, uint8_t tid, uint16_t sequence);
what does "refresh" means ? Could you explain this a bit more ?

http://codereview.appspot.com/144050/diff/7098/8009
File src/devices/wifi/edca-txop-n.cc (right):

http://codereview.appspot.com/144050/diff/7098/8009#newcode133
src/devices/wifi/edca-txop-n.cc:133: m_blockAckType (COMPRESSED_BLOCK_ACK)
that variable is set here to a constant, and is never set anywhere else but it
is tested in a couple of places for other values. Did you test what happens with
the other values ? If you did, it probably makes sense to make that variable an
attribute of type enum. If you did not, just remove that variable and make the
code assume always COMPRESSED_BLOCK_ACK.

http://codereview.appspot.com/144050/diff/7098/8011
File src/devices/wifi/mac-low.cc (right):

http://codereview.appspot.com/144050/diff/7098/8011#newcode819
src/devices/wifi/mac-low.cc:819: ScheduleDelBaFrame (hdr.GetAddr2 (),
hdr.GetQosTid ());
MacLow should not be in the business of sending management frames. Namely, It
should merely notify a higher-layer listener that this event requires the
sending of a management frame. So, here, you should instead call some kind of
listener and pass the originator+tid to that listener which will then be
responsible for scheduling a delba frame for you.

http://codereview.appspot.com/144050/diff/7098/8011#newcode1510
src/devices/wifi/mac-low.cc:1510: MacLow::CreateBlockAckAgreement (const
MgtAddBaResponseHeader *respHdr, Mac48Address originator,
you could probably replace respHdr with a simple tid argument, just like you do
with DestrocBlockAckAgreement below

http://codereview.appspot.com/144050/diff/7098/8011#newcode1537
src/devices/wifi/mac-low.cc:1537: it->second.first.m_inactivityEvent =
Simulator::Schedule (timeout,
It is the caller of this function which should be responsible for maintaining
the per-queue inactivity timeout. Namely, this functionality belongs to
EdcaTxopN.

http://codereview.appspot.com/144050/diff/7098/8016
File src/devices/wifi/mgt-headers.h (right):

http://codereview.appspot.com/144050/diff/7098/8016#newcode150
src/devices/wifi/mgt-headers.h:150: * This header id used only to know what type
of action frame we have received.
These semantics (illegal to call Remove) will break existing code (notably,
Packet::Print). The right way to fix this is to move all your BA header classes
into a single class which knows how to serialize and deserialize all of them.

The mesh code does precisely this for mesh action headers. See
devices/mesh/dot11s/dot11s-mac-header.h WifiMeshActionHeader: they use a union
to allow you to get access to the underlying deserialized data but you could
also do what they do in devices/mesh/wifi-information-element-vector.h, that is,
use an Action base class which does not derive from Header but knows how to
serialize and deserialize itself and which is called by the main ActionHeader
class.

I would strongly support trying to make both the mesh and your new code share
the same ActionHeader class and move all of that code in src/devices/wifi/.
i.e., the best would be probably to use the WifiInformationElementVector
approach, with BA Action subclasses in src/devices/wifi and ask the mesh guys to
convert their Action code to this new Action class. Andrey, pavel could you help
with this ?

http://codereview.appspot.com/144050/diff/7098/8028
File src/devices/wifi/wifi-mac-queue.h (right):

http://codereview.appspot.com/144050/diff/7098/8028#newcode149
src/devices/wifi/wifi-mac-queue.h:149: BlockAckManager* m_baManager;
I think that this pointer is used to allow you to leave in the queue the packets
which have been sent already on the medium and for which you are waiting for a
block ack.

If so, this is not the way the code was designed to work: packets which have
been sent on the medium should not be in that queue anymore. They should be in a
queue/list in the EdcaTxopN class which sent them. 

i.e., there is an invariant in the current codebase: packets present in a
WifiMacQueue have never been sent on the medium before and they are always
removed from the queue and stored somewhere else while waiting for their acks.

http://codereview.appspot.com/144050/diff/7098/8030
File src/devices/wifi/wifi-mac.h (right):

http://codereview.appspot.com/144050/diff/7098/8030#newcode182
src/devices/wifi/wifi-mac.h:182: /* Next functions are not pure vitual so non
Qos WifiMacs are not
These functions should be moved to the qos MACs and the relevant parameters
should be exported as attributes. If you feel the need to share the relevant
code among macs, introduce a WifiQosMac base class which derives from WifiMac.

http://codereview.appspot.com/144050/diff/7098/8030#newcode233
src/devices/wifi/wifi-mac.h:233: static Time GetDefaultBasicBlockAckDelay
(void);
same here: moved to qos macs

[Mathieu Lacage, 2009-11-23 15:01:40]

I like the direction of the changes. I will wait until you have split the
BlockAckAgreement class and documented clearly the state transitions before
reviewing the block ack manager again.

http://codereview.appspot.com/144050/diff/10002/10014
File src/devices/wifi/mac-low.h (right):

http://codereview.appspot.com/144050/diff/10002/10014#newcode104
src/devices/wifi/mac-low.h:104: virtual void MissedBlockAck (void);
What happens with delayed block ack ?

http://codereview.appspot.com/144050/diff/10002/10014#newcode158
src/devices/wifi/mac-low.h:158: };
When is that method invoked ? Assume I am really dumb and I don't know what an
inactivity block ack is.

http://codereview.appspot.com/144050/diff/10002/10014#newcode472
src/devices/wifi/mac-low.h:472: void
RxCompleteBufferedPacketsWithSmallerSequence (uint16_t seq, Mac48Address
originator, uint8_t tid);
should this method not be private ?

http://codereview.appspot.com/144050/diff/10002/10014#newcode485
src/devices/wifi/mac-low.h:485: void RxCompleteBufferedPackets (Mac48Address
originator, uint8_t tid);
same here, private ?

http://codereview.appspot.com/144050/diff/10002/10030
File src/devices/wifi/wifi-mac-queue.h (right):

http://codereview.appspot.com/144050/diff/10002/10030#newcode35
src/devices/wifi/wifi-mac-queue.h:35: class BlockAckManager;
un-needed now.

http://codereview.appspot.com/144050/diff/10002/10030#newcode119
src/devices/wifi/wifi-mac-queue.h:119: void BlockQosPacket (Mac48Address dest,
uint8_t tid);
I really like this. 

I wonder if you could split this functionality out of this class into a separate
QosBlockedPackets class (pick a better name if you can) which has the
Block/Unblock/Is methods as public and contains the blocked list internally and,
then, make the new DequeueFirst/PeekFirst methods take a reference/pointer to
this new class. Then, the EdcaTxop class could allocate this QosBlockedPackets
class and forward the Block/Unblock callbacks from the BlockManager to this
class.

[Mirko Banchi, 2009-11-24 11:24:07]

On 2009/11/23 15:01:40, Mathieu Lacage wrote:
> I like the direction of the changes. I will wait until you have split the
> BlockAckAgreement class and documented clearly the state transitions before
> reviewing the block ack manager again.

Ok. This will come soon.

> http://codereview.appspot.com/144050/diff/10002/10014
> File src/devices/wifi/mac-low.h (right):
> 
> http://codereview.appspot.com/144050/diff/10002/10014#newcode104
> src/devices/wifi/mac-low.h:104: virtual void MissedBlockAck (void);
> What happens with delayed block ack ?

With delayed block ack will be called MissedAck method because upon receipt of a
block ack request rx station will reply with an ack frame. After that rx station
(in another txop) will send a block ack frame to tx station and tx station will
reply with an ack frame.
 
> http://codereview.appspot.com/144050/diff/10002/10014#newcode158
> src/devices/wifi/mac-low.h:158: };
> When is that method invoked ? Assume I am really dumb and I don't know what an
> inactivity block ack is.

Each station (rx and tx) maintain an inactivity timer for each block ack
agreement. Timer at rx is reset when a frame with ack policy block ack or a
block ack request are received. Timer at tx station is reset when a block ack is
received. When timer reaches zero a delba frame is scheduled for transmission.

> http://codereview.appspot.com/144050/diff/10002/10014#newcode472
> src/devices/wifi/mac-low.h:472: void
> RxCompleteBufferedPacketsWithSmallerSequence (uint16_t seq, Mac48Address
> originator, uint8_t tid);
> should this method not be private ?
> 
> http://codereview.appspot.com/144050/diff/10002/10014#newcode485
> src/devices/wifi/mac-low.h:485: void RxCompleteBufferedPackets (Mac48Address
> originator, uint8_t tid);
> same here, private ?

Yes, you are right. Maybe they are public because before they were called by
high macs.

> http://codereview.appspot.com/144050/diff/10002/10030
> File src/devices/wifi/wifi-mac-queue.h (right):
> 
> http://codereview.appspot.com/144050/diff/10002/10030#newcode35
> src/devices/wifi/wifi-mac-queue.h:35: class BlockAckManager;
> un-needed now.

good catch.Thanks

> http://codereview.appspot.com/144050/diff/10002/10030#newcode119
> src/devices/wifi/wifi-mac-queue.h:119: void BlockQosPacket (Mac48Address dest,
> uint8_t tid);
> I really like this. 
> 
> I wonder if you could split this functionality out of this class into a
separate
> QosBlockedPackets class (pick a better name if you can) which has the
> Block/Unblock/Is methods as public and contains the blocked list internally
and,
> then, make the new DequeueFirst/PeekFirst methods take a reference/pointer to
> this new class. Then, the EdcaTxop class could allocate this QosBlockedPackets
> class and forward the Block/Unblock callbacks from the BlockManager to this
> class.

I think that this is a bit complex to realize. Note that
DequeueFirstAvailable/PeekFirstAvailable methods of WifiMacQueue are used from
EdcaTxopN as a replacement for standard Dequeue/Peek methods and not only for
the control of blocked QosPackets.

[Mathieu Lacage, 2009-11-24 11:33:00]

On Tue, Nov 24, 2009 at 12:24 PM, <mk.banchi@gmail.com> wrote:

> Reviewers: faker.moatamri, Andrey Mazo, Nicola Baldo, and.kirill, Mathieu
> Lacage,
>
> Message:
>
> On 2009/11/23 15:01:40, Mathieu Lacage wrote:
>
>> I like the direction of the changes. I will wait until you have split
>>
> the
>
>> BlockAckAgreement class and documented clearly the state transitions
>>
> before
>
>> reviewing the block ack manager again.
>>
>
> Ok. This will come soon.
>
>
>  http://codereview.appspot.com/144050/diff/10002/10014
>> File src/devices/wifi/mac-low.h (right):
>>
>
>  http://codereview.appspot.com/144050/diff/10002/10014#newcode104
>> src/devices/wifi/mac-low.h:104: virtual void MissedBlockAck (void);
>> What happens with delayed block ack ?
>>
>
> With delayed block ack will be called MissedAck method because upon
> receipt of a block ack request rx station will reply with an ack frame.
> After that rx station (in another txop) will send a block ack frame to
> tx station and tx station will reply with an ack frame.


Documenting this in the callback definition would be helpful.


>
>
>  http://codereview.appspot.com/144050/diff/10002/10014#newcode158
>> src/devices/wifi/mac-low.h:158: };
>> When is that method invoked ? Assume I am really dumb and I don't know
>>
> what an
>
>> inactivity block ack is.
>>
>
> Each station (rx and tx) maintain an inactivity timer for each block ack
> agreement. Timer at rx is reset when a frame with ack policy block ack
> or a block ack request are received. Timer at tx station is reset when a
> block ack is received. When timer reaches zero a delba frame is
> scheduled for transmission.


Same here: make sure you add a relevant comment to the callback definition.

 http://codereview.appspot.com/144050/diff/10002/10030#newcode119
> src/devices/wifi/wifi-mac-queue.h:119: void BlockQosPacket
>
(Mac48Address dest,

> uint8_t tid);
> I really like this.
>

 I wonder if you could split this functionality out of this class into
>
a separate

> QosBlockedPackets class (pick a better name if you can) which has the
> Block/Unblock/Is methods as public and contains the blocked list
>
internally and,

> then, make the new DequeueFirst/PeekFirst methods take a
>
reference/pointer to

> this new class. Then, the EdcaTxop class could allocate this
>
QosBlockedPackets

> class and forward the Block/Unblock callbacks from the BlockManager to
>
this

> class.
>

I think that this is a bit complex to realize. Note that
> DequeueFirstAvailable/PeekFirstAvailable methods of WifiMacQueue are
> used from EdcaTxopN as a replacement for standard Dequeue/Peek methods
> and not only for the control of blocked QosPackets.
>

Yes, I have noticed this but it's really just a matter to add this extra
argument to the relevant calls, right ?

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

[Mirko Banchi, 2009-11-24 12:43:09]

>Yes, I have noticed this but it's really just a matter to add this >extra
>argument to the relevant calls, right ?

Ahhh ok. I understand what you are saying. Yes, i think this change is possible.
:)

Mirko

[Mirko Banchi, 2009-11-24 18:06:44]

Hi Mathieu and all,

a fourth review patch set is available.

What is new:

-OriginatorBlockAckAgreement class with documentation of state transitions for
an originator's block ack agreement.
-QosBlockedPackets class.
-Some tests.

Please take a look at them.

Thank you very much.

Mirko

[Mathieu Lacage, 2009-11-25 09:17:29]

Ok, a couple of small details, as well as some high-level questions.

http://codereview.appspot.com/144050/diff/10041/11008
File src/devices/wifi/block-ack-agreement.h (right):

http://codereview.appspot.com/144050/diff/10041/11008#newcode37
src/devices/wifi/block-ack-agreement.h:37: void SetTid (uint8_t tid);
it would be nice if you could get away with SetTid/SetPeer

http://codereview.appspot.com/144050/diff/10041/11008#newcode56
src/devices/wifi/block-ack-agreement.h:56: bool operator== (BlockAckAgreement
const &agreement);
friend bool operator == (const BlockAckAgreement &a, const BlockAckAgreement
&b);

This makes the operator symmetric (a == b <=> b == a), especially important when
you have subclassic.

http://codereview.appspot.com/144050/diff/10041/11010
File src/devices/wifi/block-ack-manager.h (right):

http://codereview.appspot.com/144050/diff/10041/11010#newcode47
src/devices/wifi/block-ack-manager.h:47: class BlockAckManager
I looked really really carefully at this code: here is a summary of my
understanding. Please, point out whatever I got wrong.

1) when you want to send a set of packets under a block ack to a
destination/tid, you send a ba request if no agreement exists yet.
2) when you receive a successful ADDBA response, you create an agreement with
CreateAgreement to record this agreement
3) now, you take a bunch of packets from the WifiMacQueue which could be sent
with a block ack, and you push them in BlockAckManager::StorePacket
4) At some later point, when EdcaTxop::AccessGranted is called, you call
BlockAckManager::GetNextPacket to get a packet to send under a block ack. 

GetNextPacket uses BlockAckManager::m_retryPackets but, at this point, I am
lost: who does the setup of m_retryPackets for the initial transmission ?

http://codereview.appspot.com/144050/diff/10041/11010#newcode215
src/devices/wifi/block-ack-manager.h:215: void SetUnblockPacketCallback
(Callback<void, Mac48Address, uint8_t> callback);
Set(Unb|B)lockPacket -> Set(Unb|B)Destination ?

Should match whatever name you choose for the class QosBlockedPackets

http://codereview.appspot.com/144050/diff/10041/11017
File src/devices/wifi/mac-low.h (right):

http://codereview.appspot.com/144050/diff/10041/11017#newcode102
src/devices/wifi/mac-low.h:102: * block ack variant. With delayed block ack will
be called
With delayed block ack, the MissedAck method will be called instead: upon
receipt of a block ack request, the rx station will reply with a normal ack
frame. Later, when the rx station gets a txop, it will send the block ack back
to the tx station which will reply with a normal ack to the rx station.

http://codereview.appspot.com/144050/diff/10041/11017#newcode479
src/devices/wifi/mac-low.h:479: void RegisterBlockAckListenerForAc (enum
AccessClass ac, MacLowBlockAckEventListener *listener);
The lifetime of the registered listener is typically equal to the lifetime of
the queue associated to this AC.

http://codereview.appspot.com/144050/diff/10041/11029
File src/devices/wifi/qos-blocked-packets.h (right):

http://codereview.appspot.com/144050/diff/10041/11029#newcode29
src/devices/wifi/qos-blocked-packets.h:29: class QosBlockedPackets
I feel that the class name could be improved somewhat: QosBlockedDestination ?
QosBlockedRemote ?

http://codereview.appspot.com/144050/diff/10041/11029#newcode35
src/devices/wifi/qos-blocked-packets.h:35: void BlockQosPacket (Mac48Address
dest, uint8_t tid);
rename to Block, Unblock, IsBlocked

http://codereview.appspot.com/144050/diff/10041/11037
File src/devices/wifi/wifi-mac-queue.h (right):

http://codereview.appspot.com/144050/diff/10041/11037#newcode100
src/devices/wifi/wifi-mac-queue.h:100: uint32_t GetNPacketsByTidAndAddress
(uint8_t tid,
I know that it's a bit late to ask this question but here it is anyway: from
reading the code of QosUtilsMapTidToAc (as the associated relevant bit of the
spec), I see that there are only two possible tid values for each AC so that
each queue contains packets with only two possible tids for each destination. 

Now, once you have mapped a packet tid to an AC, it's unclear to me why you need
to keep around both tids. That is, why could you not make all packets within an
AC use the same tid, even if the user specified a different tid ? In the end, I
fail to see what difference it would make in terms of qos, packet ordering, and
so on. I am somewhat curious because I feel that having only one tid per queue
would be a nice simplification in our codebase.

[Mirko Banchi, 2009-11-25 11:51:22]

On 2009/11/25 09:17:29, Mathieu Lacage wrote:
> Ok, a couple of small details, as well as some high-level questions.
> 
> http://codereview.appspot.com/144050/diff/10041/11008
> File src/devices/wifi/block-ack-agreement.h (right):
> 
> http://codereview.appspot.com/144050/diff/10041/11008#newcode37
> src/devices/wifi/block-ack-agreement.h:37: void SetTid (uint8_t tid);
> it would be nice if you could get away with SetTid/SetPeer

Sorry i don't undestand: only one method?

> http://codereview.appspot.com/144050/diff/10041/11008#newcode56
> src/devices/wifi/block-ack-agreement.h:56: bool operator== (BlockAckAgreement
> const &agreement);
> friend bool operator == (const BlockAckAgreement &a, const BlockAckAgreement
> &b);
> 
> This makes the operator symmetric (a == b <=> b == a), especially important
when
> you have subclassic.

Ok. But i saw that this overloading is useless because it's never used. So i'll
remove it.

> http://codereview.appspot.com/144050/diff/10041/11010
> File src/devices/wifi/block-ack-manager.h (right):
> 
> http://codereview.appspot.com/144050/diff/10041/11010#newcode47
> src/devices/wifi/block-ack-manager.h:47: class BlockAckManager
> I looked really really carefully at this code: here is a summary of my
> understanding. Please, point out whatever I got wrong.
> 
> 1) when you want to send a set of packets under a block ack to a
> destination/tid, you send a ba request if no agreement exists yet.

If with "ba request" you speak about ADDBARequest, yes, it's correct.

> 2) when you receive a successful ADDBA response, you create an agreement with
> CreateAgreement to record this agreement

Yes, exactly.

> 3) now, you take a bunch of packets from the WifiMacQueue which could be sent
> with a block ack, and you push them in BlockAckManager::StorePacket

Note that packets aren't stored in BlockAckManager at once. Each packet is
stored in the block ack manager only when it is transmitted with ack policy
block ack. This check and relative call to BlockAckManager::StorePacket is
performed in EdcaTxop::CompleteTx function.

> 4) At some later point, when EdcaTxop::AccessGranted is called, you call
> BlockAckManager::GetNextPacket to get a packet to send under a block ack. 
> 
> GetNextPacket uses BlockAckManager::m_retryPackets but, at this point, I am
> lost: who does the setup of m_retryPackets for the initial transmission ?

For the initial transmission there are no packets in m_retryPackets and
GetNextPacket will return 0. So, EdcaTxopN will dequeue a packet from
WifiMacQueue.

> http://codereview.appspot.com/144050/diff/10041/11010#newcode215
> src/devices/wifi/block-ack-manager.h:215: void SetUnblockPacketCallback
> (Callback<void, Mac48Address, uint8_t> callback);
> Set(Unb|B)lockPacket -> Set(Unb|B)Destination ?
>
> Should match whatever name you choose for the class QosBlockedPackets

Yes. I think that Set(Unb|B)lockDestination should be ok. 
 
> http://codereview.appspot.com/144050/diff/10041/11017
> File src/devices/wifi/mac-low.h (right):
> 
> http://codereview.appspot.com/144050/diff/10041/11017#newcode102
> src/devices/wifi/mac-low.h:102: * block ack variant. With delayed block ack
will
> be called
> With delayed block ack, the MissedAck method will be called instead: upon
> receipt of a block ack request, the rx station will reply with a normal ack
> frame. Later, when the rx station gets a txop, it will send the block ack back
> to the tx station which will reply with a normal ack to the rx station.

Ok. I will update the comment.

> http://codereview.appspot.com/144050/diff/10041/11017#newcode479
> src/devices/wifi/mac-low.h:479: void RegisterBlockAckListenerForAc (enum
> AccessClass ac, MacLowBlockAckEventListener *listener);
> The lifetime of the registered listener is typically equal to the lifetime of
> the queue associated to this AC.

Yes, but i don't undestand what you are suggesting. Sorry.
 
> http://codereview.appspot.com/144050/diff/10041/11029
> File src/devices/wifi/qos-blocked-packets.h (right):
> 
> http://codereview.appspot.com/144050/diff/10041/11029#newcode29
> src/devices/wifi/qos-blocked-packets.h:29: class QosBlockedPackets
> I feel that the class name could be improved somewhat: QosBlockedDestination ?
> QosBlockedRemote ?

Maybe the first is better. I will change its name.

> http://codereview.appspot.com/144050/diff/10041/11029#newcode35
> src/devices/wifi/qos-blocked-packets.h:35: void BlockQosPacket (Mac48Address
> dest, uint8_t tid);
> rename to Block, Unblock, IsBlocked

Done.

> http://codereview.appspot.com/144050/diff/10041/11037
> File src/devices/wifi/wifi-mac-queue.h (right):
> 
> http://codereview.appspot.com/144050/diff/10041/11037#newcode100
> src/devices/wifi/wifi-mac-queue.h:100: uint32_t GetNPacketsByTidAndAddress
> (uint8_t tid,
> I know that it's a bit late to ask this question but here it is anyway: from
> reading the code of QosUtilsMapTidToAc (as the associated relevant bit of the
> spec), I see that there are only two possible tid values for each AC so that
> each queue contains packets with only two possible tids for each destination. 
> 
> Now, once you have mapped a packet tid to an AC, it's unclear to me why you
need
> to keep around both tids. That is, why could you not make all packets within
an
> AC use the same tid, even if the user specified a different tid ? In the end,
I
> fail to see what difference it would make in terms of qos, packet ordering,
and
> so on. I am somewhat curious because I feel that having only one tid per queue
> would be a nice simplification in our codebase.

Yes you are right but keeping both tids is important, for example, for
aggregation. Standard 802.11n specifies that all MSDUs contained in an A-MSDU
must have the same tid.

Thank you for answering so promptly.

Mirko

[Mathieu Lacage, 2009-11-25 15:01:27]

On Wed, Nov 25, 2009 at 12:51 PM, <mk.banchi@gmail.com> wrote:

> On 2009/11/25 09:17:29, Mathieu Lacage wrote:
>
>> Ok, a couple of small details, as well as some high-level questions.
>>
>
>  http://codereview.appspot.com/144050/diff/10041/11008
>> File src/devices/wifi/block-ack-agreement.h (right):
>>
>
>  http://codereview.appspot.com/144050/diff/10041/11008#newcode37
>> src/devices/wifi/block-ack-agreement.h:37: void SetTid (uint8_t tid);
>> it would be nice if you could get away with SetTid/SetPeer
>>
>
> Sorry i don't undestand: only one method?


Ah, sorry for being unclear: can't you just use the constructor to set it
once and for all ?

http://codereview.appspot.com/144050/diff/10041/11017#newcode479
>> src/devices/wifi/mac-low.h:479: void RegisterBlockAckListenerForAc
>>
> (enum
>
>> AccessClass ac, MacLowBlockAckEventListener *listener);
>> The lifetime of the registered listener is typically equal to the
>>
> lifetime of
>
>> the queue associated to this AC.
>>
>
> Yes, but i don't undestand what you are suggesting. Sorry.


That was a suggested addition for the doxygen.

Now, once you have mapped a packet tid to an AC, it's unclear to me
>>
> why you need
>
>> to keep around both tids. That is, why could you not make all packets
>>
> within an
>
>> AC use the same tid, even if the user specified a different tid ? In
>>
> the end, I
>
>> fail to see what difference it would make in terms of qos, packet
>>
> ordering, and
>
>> so on. I am somewhat curious because I feel that having only one tid
>>
> per queue
>
>> would be a nice simplification in our codebase.
>>
>
> Yes you are right but keeping both tids is important, for example, for
> aggregation. Standard 802.11n specifies that all MSDUs contained in an
> A-MSDU must have the same tid.
>

My point is that I fail to see what would break if you just set the same tid
in the header, even  if this is not the tid the user requested.

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

[Mirko Banchi, 2009-11-26 15:29:35]

On 2009/11/25 15:01:27, Mathieu Lacage wrote:
> On Wed, Nov 25, 2009 at 12:51 PM, <mailto:mk.banchi@gmail.com> wrote:
> 
> > On 2009/11/25 09:17:29, Mathieu Lacage wrote:
> >
> >> Ok, a couple of small details, as well as some high-level questions.
> >>
> >
> >  http://codereview.appspot.com/144050/diff/10041/11008
> >> File src/devices/wifi/block-ack-agreement.h (right):
> >>
> >
> >  http://codereview.appspot.com/144050/diff/10041/11008#newcode37
> >> src/devices/wifi/block-ack-agreement.h:37: void SetTid (uint8_t tid);
> >> it would be nice if you could get away with SetTid/SetPeer
> >>
> >
> > Sorry i don't undestand: only one method?
> 
> 
> Ah, sorry for being unclear: can't you just use the constructor to set it
> once and for all ?

Yes. I removed those methods.

> http://codereview.appspot.com/144050/diff/10041/11017#newcode479
> >> src/devices/wifi/mac-low.h:479: void RegisterBlockAckListenerForAc
> >>
> > (enum
> >
> >> AccessClass ac, MacLowBlockAckEventListener *listener);
> >> The lifetime of the registered listener is typically equal to the
> >>
> > lifetime of
> >
> >> the queue associated to this AC.
> >>
> >
> > Yes, but i don't undestand what you are suggesting. Sorry.
> 
> 
> That was a suggested addition for the doxygen.

Added.

New patch set "fifth review" is available.

Mirko

[Mirko Banchi, 2009-12-02 18:06:53]

Hi Mathieu,

i have aligned my block ack implementation with ns-3-dev. Now implementation for
action frames are moved from mesh dir in wifi/mgt-headers.h/cc. I have also made
some corrections as you suggested. Please take a look at them.

Thank you very much,

Mirko

[Mathieu Lacage, 2009-12-14 12:58:40]

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.

[Mirko Banchi, 2009-12-14 14:15:01]

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.

>   - 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.

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

[Mathieu Lacage, 2009-12-14 15:09:07]

On Mon, Dec 14, 2009 at 3:15 PM, <mk.banchi@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.lacage@gmail.com>

[Tom Henderson, 2010-01-30 23:21:14]

Some comments from a superficial review of this patchset are included.  

Otherwise, the main comment that I have is that it would be nice to document
somewhere what this code modelizes, such as in src/devices/wifi.h.  I saw many
references to IEEE stds. sprinkled through the code.  Can you succintly
summarize what aspects of 802.11n mac (frame aggregation, block acks, others?)
are modelized now in ns-3, and which are left for futher study?

http://codereview.appspot.com/144050/diff/18001/18011
File src/devices/wifi/edca-txop-n.cc (right):

http://codereview.appspot.com/144050/diff/18001/18011#newcode621
src/devices/wifi/edca-txop-n.cc:621: MY_DEBUG ("missed block ack");
can you convert MY_DEBUG to NS_LOGs?

http://codereview.appspot.com/144050/diff/18001/18017
File src/devices/wifi/mgt-headers.cc (right):

http://codereview.appspot.com/144050/diff/18001/18017#newcode466
src/devices/wifi/mgt-headers.cc:466: case BLOCK_ACK_ADDBA_REQUEST:
try running Mathieu's check-style.py against these switch blocks; I think the
case statements should be indented two more spaces

http://codereview.appspot.com/144050/diff/18001/18018
File src/devices/wifi/mgt-headers.h (right):

http://codereview.appspot.com/144050/diff/18001/18018#newcode20
src/devices/wifi/mgt-headers.h:20: * Author: Mirko Banchi <mk.banchi@gmail.com>
In general, we prefer to not have people who work on preexisting files add
additional author and copyright statements.  Both authorship and copyright can
be determined by the mercurial history of the file.

http://codereview.appspot.com/144050/diff/18001/18022
File src/devices/wifi/qadhoc-wifi-mac.h (left):

http://codereview.appspot.com/144050/diff/18001/18022#oldcode42
src/devices/wifi/qadhoc-wifi-mac.h:42: class QadhocWifiMac : public WifiMac
This would be a good time to respond to and close the bug 766 filed against the
lack of doxygen for QoS wifi mac models.

http://codereview.appspot.com/144050/diff/18001/18023
File src/devices/wifi/qap-wifi-mac.cc (right):

http://codereview.appspot.com/144050/diff/18001/18023#newcode896
src/devices/wifi/qap-wifi-mac.cc:896: next equation:
minor nit; multi-line comment misses the "*" character on each line.

http://codereview.appspot.com/144050/diff/18001/18023#newcode916
src/devices/wifi/qap-wifi-mac.cc:916: //Better a management queue?
minor nit:  mixed C and C++ commenting styles within a few lines of source code.

[Mathieu Lacage, 2010-02-02 12:40:19]

On Sun, Jan 31, 2010 at 12:21 AM, <tomh.org@gmail.com> wrote:


> http://codereview.appspot.com/144050/diff/18001/18018#newcode20
> src/devices/wifi/mgt-headers.h:20: * Author: Mirko Banchi
> <mk.banchi@gmail.com>
> In general, we prefer to not have people who work on preexisting files
> add additional author and copyright statements.  Both authorship and
> copyright can be determined by the mercurial history of the file.
>

??? We prefer that people do not sprinkle "--mirko" comments throughout the
whole code but it's both appropriate and needed to update the copyright
statement at the top of a file when you have made more than a token
contribution to that file. The Author statement is purely documentary but I
think that it makes sense to update it if you update the Copyright
statement.


> http://codereview.appspot.com/144050/diff/18001/18023#newcode916
> src/devices/wifi/qap-wifi-mac.cc:916: //Better a management queue?
> minor nit:  mixed C and C++ commenting styles within a few lines of
> source code.
>

I was not aware that we had rules about C/C++ commenting. Do we ?

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

[Tom Henderson, 2010-02-02 16:55:26]

On 2010/02/02 12:40:19, Mathieu Lacage wrote:
> On Sun, Jan 31, 2010 at 12:21 AM, <mailto:tomh.org@gmail.com> wrote:
> 
> 
> > http://codereview.appspot.com/144050/diff/18001/18018#newcode20
> > src/devices/wifi/mgt-headers.h:20: * Author: Mirko Banchi
> > <mailto:mk.banchi@gmail.com>
> > In general, we prefer to not have people who work on preexisting files
> > add additional author and copyright statements.  Both authorship and
> > copyright can be determined by the mercurial history of the file.
> >
> 
> ??? We prefer that people do not sprinkle "--mirko" comments throughout the
> whole code but it's both appropriate and needed to update the copyright
> statement at the top of a file when you have made more than a token
> contribution to that file. The Author statement is purely documentary but I
> think that it makes sense to update it if you update the Copyright
> statement.

We had a discussion about this a very long time ago (might have been the kickoff
meeting), about trying to avoid what has occurred in ns-2 and other projects
where everyone who touches a file is listed as an author.  

Regarding copyright, as I understand it, every author who contributes gets
copyright regardless of what the "Copyright" statement says in the header,
according to the Berne convention.  All of the authors are logged in the
mercurial history.  This is unless the author explicitly assigns copyright to
another holder.  This is why we should use the hg -u option.  So for copyright,
I think it really makes no difference legally.

However, you are right; I checked that our code-submissions.html page says to do
just what Jens did, regarding both authorship and copyright, so I apologize for
my faulty memory and rescind this comment.

> 
> 
> > http://codereview.appspot.com/144050/diff/18001/18023#newcode916
> > src/devices/wifi/qap-wifi-mac.cc:916: //Better a management queue?
> > minor nit:  mixed C and C++ commenting styles within a few lines of
> > source code.
> >
> 
> I was not aware that we had rules about C/C++ commenting. Do we ?
> 

Yes, see the coding style document; there is a section on comment formatting.

Anyway, this was a minor nit I noticed in a cursory review, and it occurred to
me that it came up before in the context of someone who was mixing the use of
doxygen styles within the same file, so I just suggested to be internally
consistent.  It is not a blocker for me and not intended to annoy.

[Tom Henderson, 2010-02-02 16:57:08]

> However, you are right; I checked that our code-submissions.html page says to
do
> just what Jens 
(s/Mirko/Jens)

[Mathieu Lacage, 2010-02-04 17:04:52]

On Tue, Feb 2, 2010 at 5:55 PM, <tomh.org@gmail.com> wrote:

> We had a discussion about this a very long time ago (might have been the
> kickoff meeting), about trying to avoid what has occurred in ns-2 and
> other projects where everyone who touches a file is listed as an author.
>

Yes, I think we all agreed that it sucked.


> Regarding copyright, as I understand it, every author who contributes
> gets copyright regardless of what the "Copyright" statement says in the
> header, according to the Berne convention.  All of the authors are
> logged in the mercurial history.  This is unless the author explicitly
> assigns copyright to another holder.  This is why we should use the hg
> -u option.  So for copyright, I think it really makes no difference
> legally.
>
> However, you are right; I checked that our code-submissions.html page
> says to do just what Jens did, regarding both authorship and copyright,
> so I apologize for my faulty memory and rescind this comment.


I don't know the legalese part but all open source projects I have worked on
enforce the same policy: if you change/add +/-100 LOCs, you get to be in the
AUTHORS section and you update the Copyright field. Whether or not this is
sufficient/needed from a legal perspective in some/all countries, I would
not know.

 > http://codereview.appspot.com/144050/diff/18001/18023#newcode916
> > src/devices/wifi/qap-wifi-mac.cc:916: //Better a management queue?
> > minor nit:  mixed C and C++ commenting styles within a few lines of
> > source code.
> >
>

 I was not aware that we had rules about C/C++ commenting. Do we ?
>


Yes, see the coding style document; there is a section on comment
> formatting.
>
> Anyway, this was a minor nit I noticed in a cursory review, and it
> occurred to me that it came up before in the context of someone who was
> mixing the use of doxygen styles within the same file, so I just
>

I really don't care much but I really was not aware that we had a rule which
said that we were trying to enforce a uniform commenting style on a per-file
basis. I am not very rigorous about this in my code so, I will try to be
more careful.

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

[tomh_tomh.org, 2010-02-04 17:20:03]

On Thu, 4 Feb 2010 18:04:46 +0100, Mathieu Lacage
> 
> 
> I don't know the legalese part but all open source projects I have
worked
> on
> enforce the same policy: if you change/add +/-100 LOCs, you get to be in
> the
> AUTHORS section and you update the Copyright field. Whether or not this
is
> sufficient/needed from a legal perspective in some/all countries, I
would
> not know.

The only reason that I brought it up was that we don't have a problem with
this at the moment but we can set bad precedents.

Anyway, as I said, I think Mirko followed the posted guidance (which is
basically consistent with what you wrote above) and I am not suggesting to
change it so I think this is a moot point.

> 
> I really don't care much but I really was not aware that we had a rule
> which
> said that we were trying to enforce a uniform commenting style on a
> per-file
> basis. I am not very rigorous about this in my code so, I will try to be
> more careful.
>

It just caught my eye because it was within a couple of lines of each
other.  Again, not a big issue for me.

I think the bigger cleanup issue with the merge is that there is missing
doxygen and lots of MY_DEBUG statements still sprinkled in the code, which
I would like to see fixed.  

[Mirko Banchi, 2010-02-06 17:50:30]

On 2010/02/04 17:20:03, tomh_tomh.org wrote:
> 
> It just caught my eye because it was within a couple of lines of each
> other.  Again, not a big issue for me.
> 
> I think the bigger cleanup issue with the merge is that there is >missing
> doxygen and lots of MY_DEBUG statements still sprinkled in the
> code, which
> I would like to see fixed.  
> 

I think that use of MY_DEBUG macro is very helpful to figure out what happens in
a simulation because it prints also mac addresses. Without its use, when the
number of stations in a simulation increases, maybe the output could become
unreadable. However yes, it's possible to remove MY_DEBUG macro and use directly
NS_LOG_DEBUG (m_low->GetAddress ()<<" "<<...) instead.