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 ?

Message:

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.

Please review this at http://codereview.appspot.com/41059

To summarize, I think that:
1) just remove m_lastPeeked, and let's defer to a later patch dealing with this
2) try to come up with a base class for qap and qsta to share code
3) in the future, try to produce patches relative to the previous review
4) in the future, try to split your work in smaller separate patches.
For example, all the changes you did to DoGetXXX, you could have moved
to a separate followup patch. This makes it easier to merge each patch
because it is smaller and decreases review work for me.

Mathieu

bool Remove (Ptr<const Packet> packet);

that performs remotion in linear time (O(n));

About creation a common base class for Ap and Sta objects I suggest:

For Ap:

                WifiMac
                     ^
                      |
               ApWifiMac
               /            \
             /                \
           /                   \
NqapWifiMac      QapWifiMac

Code we could move in the base class ApWifiMac:

bool GetBeconGeneration (void) const;
Time GetBeaconInterval
SupportedRates GetSupportedRates (void) const;
void TxOk (WifiMacHeader const &hdr);
void TxFailed (WifiMacHeader const &hdr);

For now i wouldn't move in a base class following methods:

1) void SendOneBeacon (void);
Beacon for nQos Ap and Qos Ap are different. We could use a flag to
indicate which kind of beacon we have to send but i would avoid this.

2) void SetBeaconGeneration (bool enable);
Because we have two schedule two different methods.

3)void SendAssocResp (Mac48Address to, bool success);
Association format for Qos and nQos Ap are different. In the future we
could have need to
use different formats.

4)void SendProbeResp (Mac48Address to);
Same as 3).

For sta:

              WifiMac
                   ^
                    |
            StaWifiMac
              /            \
             /              \
            /                \
NqstaWifiMac      QstaWifiMac

Code we could move in the base class StaWifiMac:

enum State;
void SetMaxMissedBeacons (uint32_t missed);
void SetProbeRequestTimeout (Time timeout);
void SetAssocRequestTimeout (Time timeout);
void SetActiveProbing (bool enable);
void SetState (enum MacState state);
SupportedRates GetSupportedRates (void);
bool IsAssociated (void) const;
bool SupportSendFrom (void);

void AssocRequestTimeout (void);
This calls SendAssociationRequest () that has a different behaviour in
Nqsta and Qsta so we have to define an empty method that must be
redefined in subclasses.

void ProbeRequestTimeout (void);
Same as AssocRequestTimeout.

void TryToEnsureAssociated (void);
Same as above because calls SendProbeRequest ().

For now i wouldn't move in a base class following methods:

1)void MissedBeacons ();
Because we have two schedule two different methods.

2)void RestartBeaconWatchDog (Time delay);
Same as MissedBeacons

What do you think about?
Do you think that a creation of common base classes is justified ?

Thanks,

i spend last two days to analize how we could share common code
between QapWifiMac and NqapWifiMac and the same for QstaWifiMac and
NqstaWifiMac. As i wrote in the previous post, i think that there are
some troubles about it. I think that the creation of a common base
class could make code unreadeable. The main problems are in the
schedulation of functions. Really, mainly fo ap objects there are few
common functions that don't are implementation of function defined in
WifiMac. In addition, functions like SendProbeResp or SendAssocResp
could vary for QapWifiMac in comparison to NqapWifiMac so they are not
so common. Same fo QstaWifiMac and NqstaWifiMac. About creation of an
helper, i don't know how it could resolve the problem :(. What do you
think about?

About aggregators i uploaded a patch...i moved some controls about
presence of a single MSDU in an aggregated packet from
MsduStandardAggregator to EdcaTxopN. The logic was too complex and
useless...In addition this doesn't force other aggregator
implementations to have same controls.

Let me know about common base class for Qap and Qsta.

Thank you,

On 17 Apr, 13:57, Mathieu Lacage <mathieu.lac...@gmail.com> wrote:

Thank you very much,

On 17 Apr, 15:31, Mathieu Lacage <mathieu.lac...@gmail.com> wrote:

uint8_t GetTidForPacket (Ptr<Packet> packet)
AccessClass MapTidToAc (uint8_t tid)

as you adviced in a previuos post, would be great move them in qos-
utils.h. The signature for that methods should be changed in order to
collocate them in the correct unit?
For instance

uint8_t GetTidForPacket (Ptr<Packet> packet) should become
uint8_t QosGetTidForPacket (Ptr<Packet> packet) or something similar?
Or we can mantain that signature?

The same fo MapTidToAc (uint8_t tid).

On 17 Apr, 16:26, "Mirko.Banchi" <mk.ban...@gmail.com> wrote:

?

Mathieu

Thanks,