802.11e/n extensions

[Mathieu Lacage, 2009-04-14 11:56:33]

There are really a lot of small details but, here are the biggest issues left:

1) copyright: make sure you add your copyright where needed
2) lots of uneeded whitespace/indentation changes which make reviewing a bit
painful
3) I would like to know if you can do what I suggested in an earlier review,
that is, move common code from Nqap and Qap to a base class, and the same for
the Nqsta and Qsta classes. I don't know how painful that would be but I see a
lot of copy/pasted code and I wonder if we could minimize the amount of
duplication. Maybe you could move some common code in a separate 'helper' class
if the base class idea is too painful ?

http://codereview.appspot.com/41059/diff/1/3
File examples/simple-802.11n-frame-aggregation.cc (right):

http://codereview.appspot.com/41059/diff/1/3#newcode3
Line 3: * Copyright (c) 2005, 2009 INRIA
add your copyright

http://codereview.appspot.com/41059/diff/1/10
File src/applications/udp-echo/udp-echo-client.cc (right):

http://codereview.appspot.com/41059/diff/1/10#newcode143
Line 143: 
uneeded whitespace change

http://codereview.appspot.com/41059/diff/1/11
File src/contrib/qos-tag.cc (right):

http://codereview.appspot.com/41059/diff/1/11#newcode52
Line 52: return sizeof (uint8_t);
1 is clearer for me

http://codereview.appspot.com/41059/diff/1/14
File src/core/object-factory.cc (left):

http://codereview.appspot.com/41059/diff/1/14#oldcode22
Line 22: 
uneeded whitespace change

http://codereview.appspot.com/41059/diff/1/15
File src/devices/wifi/adhoc-wifi-mac.cc (right):

http://codereview.appspot.com/41059/diff/1/15#newcode49
Line 49: MakePointerAccessor (&AdhocWifiMac::DoGetDcaTxop,
It would make sense to make the name of the getter match the setter. i.e.,
either GetDcaTxop/SetDcaTxop or Do*/Do*

http://codereview.appspot.com/41059/diff/1/16
File src/devices/wifi/adhoc-wifi-mac.h (right):

http://codereview.appspot.com/41059/diff/1/16#newcode81
Line 81: 
please, avoid uneeded whitespace changes

http://codereview.appspot.com/41059/diff/1/17
File src/devices/wifi/amsdu-subframe-header.cc (right):

http://codereview.appspot.com/41059/diff/1/17#newcode3
Line 3: * Copyright (c) 2009
copyright to who ? Add something. Same for all files missing a copyright owner

http://codereview.appspot.com/41059/diff/1/19
File src/devices/wifi/dca-txop.cc (right):

http://codereview.appspot.com/41059/diff/1/19#newcode101
Line 101: static TypeId tid = TypeId ("ns3::DcaTxop")
good catch !

http://codereview.appspot.com/41059/diff/1/19#newcode380
Line 380: 
uneeded whitespace

http://codereview.appspot.com/41059/diff/1/19#newcode420
Line 420: MY_DEBUG ("fragmenting last fragment size=" << fragment->GetSize ());
uneeded layout change

http://codereview.appspot.com/41059/diff/1/19#newcode425
Line 425: MY_DEBUG ("fragmenting size=" << fragment->GetSize ());
same uneeded change

http://codereview.appspot.com/41059/diff/1/19#newcode519
Line 519: MY_DEBUG ("got ack. tx not done, size=" << m_currentPacket->GetSize
());
same

http://codereview.appspot.com/41059/diff/1/20
File src/devices/wifi/dcf-manager.cc (left):

http://codereview.appspot.com/41059/diff/1/20#oldcode296
Line 296: 
uneeded change

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

http://codereview.appspot.com/41059/diff/1/21#newcode316
Line 316: m_currentHdr.IsData ()))
bad alignment

http://codereview.appspot.com/41059/diff/1/21#newcode350
Line 350: while ((m_currentPacket = m_queue->PeekByTidAndAddress (&peekedHdr,
please, don't assign _within_ a conditional statement:
foo =bar
while (foo)
...
  foo = bar

http://codereview.appspot.com/41059/diff/1/28
File src/devices/wifi/msdu-standard-aggregator.cc (right):

http://codereview.appspot.com/41059/diff/1/28#newcode96
Line 96: }
alignemnt of closing brace

http://codereview.appspot.com/41059/diff/1/42
File src/devices/wifi/wifi-mac-header.h (right):

http://codereview.appspot.com/41059/diff/1/42#newcode64
Line 64: };
It would be nice to move these enum definitions to public members of class
WifiMacHeader.

class WifiMacHeader {
public:
  enum AddressType {
    ADDR1,
    ADDR2, 
    ...
  }
  enum QosAckPolicy {
    NORMAL_ACK,
    ...
  }

(and yes, let's try to kill these trailing _e to conform to the coding style)

http://codereview.appspot.com/41059/diff/1/42#newcode121
Line 121: void SetQosAckPolicy (QosAckPolicy_e);
repeat the 'enum' keyword

http://codereview.appspot.com/41059/diff/1/42#newcode132
Line 132: //uint8_t GetCtrlType (void) const;
if this is dead code, don't add it

http://codereview.appspot.com/41059/diff/1/44
File src/devices/wifi/wifi-mac-queue.h (right):

http://codereview.appspot.com/41059/diff/1/44#newcode69
Line 69: * Is tipically used by ns3::EdcaTxopN in order to perform correct MSDU
typo: typically

http://codereview.appspot.com/41059/diff/1/44#newcode80
Line 80: * Is tipically used by ns3::EdcaTxopN in order to perform correct MSDU
same typo

http://codereview.appspot.com/41059/diff/1/44#newcode93
Line 93: bool RemoveLastPeeked (void);
the m_lastPeeked variable and this method are really evil. Instead, what you
want is the much cleaner:

bool Remove (Ptr<const Packet> p);

(cleaner because it does not need to maintain any extra state in the queue)

Btw, I don't believe that your statement about O(1) is true: deletion is not
O(1) because you call erase and erase is not O(1).

http://codereview.appspot.com/41059/diff/1/44#newcode120
Line 120: PacketQueueI m_lastPeeked;
please, kill this

http://codereview.appspot.com/41059/diff/1/46
File src/helper/nqos-wifi-mac-helper.cc (right):

http://codereview.appspot.com/41059/diff/1/46#newcode40
Line 40: helper.SetDcaParameters ("MinCw", UintegerValue (15),
these are the default values defined in DcaTxop. Why do you reset them here ?

[Mirko Banchi, 2009-04-14 20:26:58]

I've publish a new patch (Patch Set 2) with changes to code as Mathieu asked me.
Only one thing I didn't modified: remotion of last peeked packet in
WifiMacQueue. I'm waiting feedback about it.