Re: 802.11e/n extensions
mathieu...@gmail.com
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 ?
mk.b...@gmail.com
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
Affected files:
M examples/mixed-wireless.cc
A examples/simple-802.11n-frame-aggregation.cc
M examples/stats/wifi-example-sim.cc
M examples/tap-wifi-dumbbell.cc
M examples/third.cc
M examples/wifi-adhoc.cc
M examples/wifi-ap.cc
M examples/wifi-wired-bridging.cc
A src/contrib/qos-tag.cc
A src/contrib/qos-tag.h
M src/contrib/wscript
M src/devices/wifi/adhoc-wifi-mac.cc
M src/devices/wifi/adhoc-wifi-mac.h
A src/devices/wifi/amsdu-subframe-header.cc
A src/devices/wifi/amsdu-subframe-header.h
M src/devices/wifi/dca-txop.cc
A src/devices/wifi/edca-txop-n.cc
A src/devices/wifi/edca-txop-n.h
M src/devices/wifi/mac-low.cc
M src/devices/wifi/mac-tx-middle.cc
M src/devices/wifi/mac-tx-middle.h
A src/devices/wifi/msdu-aggregator.cc
A src/devices/wifi/msdu-aggregator.h
A src/devices/wifi/msdu-standard-aggregator.cc
A src/devices/wifi/msdu-standard-aggregator.h
M src/devices/wifi/nqap-wifi-mac.cc
M src/devices/wifi/nqap-wifi-mac.h
M src/devices/wifi/nqsta-wifi-mac.cc
M src/devices/wifi/nqsta-wifi-mac.h
A src/devices/wifi/qadhoc-wifi-mac.cc
A src/devices/wifi/qadhoc-wifi-mac.h
A src/devices/wifi/qap-wifi-mac.cc
A src/devices/wifi/qap-wifi-mac.h
A src/devices/wifi/qos-utils.h
A src/devices/wifi/qsta-wifi-mac.cc
A src/devices/wifi/qsta-wifi-mac.h
M src/devices/wifi/wifi-mac-header.cc
M src/devices/wifi/wifi-mac-header.h
M src/devices/wifi/wifi-mac-queue.cc
M src/devices/wifi/wifi-mac-queue.h
M src/devices/wifi/wscript
A src/helper/nqos-wifi-mac-helper.cc
A src/helper/nqos-wifi-mac-helper.h
A src/helper/qos-wifi-mac-helper.cc
A src/helper/qos-wifi-mac-helper.h
M src/helper/wifi-helper.cc
M src/helper/wifi-helper.h
M src/helper/wscript
Mirko.Banchi
New issue for 802.11e/n extensions is at http://codereview.appspot.com/40088
Mathieu Lacage
rebased diff so, I don't really know what you changed compared to the
previous version. I think that when we are iterating like this, it
would be easier for me to see the patches relative to the last
iteration, and, then, once we have converged on the last iteration,
re-generate one final single big patch for one final review.
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
On Wed, Apr 15, 2009 at 12:47 AM, Mirko.Banchi <mk.b...@gmail.com> wrote:
>
> Issue http://codereview.appspot.com/41059 stopped to work.
>
> New issue for 802.11e/n extensions is at http://codereview.appspot.com/40088
> >
>
Mathieu Lacage <mathieu...@gmail.com>
Mirko.Banchi
packets form WifiMacQueue. For now i've created a method like this:
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,
Mirko
Mirko.Banchi
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,
Mirko
Mathieu Lacage
> 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);
Can't you share the NqapWifiMac::Receive method ? Or, at least, split
it in chunks which can be reused ?
> 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 all of these, I expected that the non-qos versions would be only
very slightly different with a couple more fields so, we could move in
the base class at least a method which builds the common fields in the
headers.
What you propose to move to the base class does not justify a base
class. I was hoping that it would be possible to move more code to the
base class by splitting some of the complex methods in smaller methods
first. If you think that this is not possible, then, I see no point in
creating a mostly-empty common base class
Mathieu
--
Mathieu Lacage <mathieu...@gmail.com>
Mathieu Lacage
<mathieu...@gmail.com> wrote:
> What you propose to move to the base class does not justify a base
> class. I was hoping that it would be possible to move more code to the
> base class by splitting some of the complex methods in smaller methods
> first. If you think that this is not possible, then, I see no point in
> creating a mostly-empty common base class
Erm, and I forgot to point out that this is why I originally suggested
that maybe a simple way to share some code might be to just move some
common code in a helper utils file or class which is reused by each
subclass. But, well, I could live without it.
Mirko.Banchi
On 17 Apr, 13:57, Mathieu Lacage <mathieu.lac...@gmail.com> wrote:
> On Fri, Apr 17, 2009 at 1:54 PM, Mathieu Lacage
>
> > What you propose to move to the base class does not justify a base
> > class. I was hoping that it would be possible to move more code to the
> > base class by splitting some of the complex methods in smaller methods
> > first. If you think that this is not possible, then, I see no point in
> > creating a mostly-empty common base class
>
> Erm, and I forgot to point out that this is why I originally suggested
> that maybe a simple way to share some code might be to just move some
> common code in a helper utils file or class which is reused by each
> subclass. But, well, I could live without it.
different (for example use of DeaggregateAndForward method), but as
you'll see for example with block ack in this method will be handled
action frames that are not handled in NqapWifiMac::Receive or handled
of association request frames could be different. This is only two
examples. I think having two separate methods mantains a certain
flexibility degree.
About other methods, as you told me, don't justify a creation of a
common base class.
If you need other patches or something else feel free to ask me.
Thank you very much,
Mirko
Mathieu Lacage
> If you need other patches or something else feel free to ask me.
Ok, so, can you prepare a final patch without spurious whitespace
changes ? I will review that patch as a whole, one last time, and,
then, we should be good ! Thank you very much for keeping up with all
of this.
Mirko.Banchi
On 17 Apr, 15:31, Mathieu Lacage <mathieu.lac...@gmail.com> wrote:
> > If you need other patches or something else feel free to ask me.
>
> Ok, so, can you prepare a final patch without spurious whitespace
> changes ? I will review that patch as a whole, one last time, and,
> then, we should be good ! Thank you very much for keeping up with all
> of this.
>
> Mathieu
QstaWifiMac, QapWifiMac, QadhocWifiMac; methods:
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).
Thanks,
Mirko
Mirko.Banchi
On 17 Apr, 16:26, "Mirko.Banchi" <mk.ban...@gmail.com> wrote:
> On 17 Apr, 15:31, Mathieu Lacage <mathieu.lac...@gmail.com> wrote:
>
> > On Fri, Apr 17, 2009 at 2:47 PM, Mirko.Banchi <mk.ban...@gmail.com> wrote:
> > > If you need other patches or something else feel free to ask me.
>
> > Ok, so, can you prepare a final patch without spurious whitespace
> > changes ? I will review that patch as a whole, one last time, and,
> > then, we should be good ! Thank you very much for keeping up with all
> > of this.
that you'll find, was in order to remove presence of tabs.If you
something else ask me.
Thank you for your collaboration,
Mirko
Mirko.Banchi
with new wifi helpers, the DcaTxop object isn't created with creation
of WifiMac object but it is created inside NqosWifiMacHelper::Create
method. So we have to modify wifi-test.cc accordingly. For now i have
made no changes to it.
Mirko
Mathieu Lacage
> 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?
How about :
QosUtilsGetTidForPacket
> The same fo MapTidToAc (uint8_t tid).
QosUtilsMapTidToAc
?
--
Mathieu Lacage <mathieu...@gmail.com>
Mirko.Banchi
> QosUtilsGetTidForPacket
>
> > The same fo MapTidToAc (uint8_t tid).
>
> QosUtilsMapTidToAc
>
> ?
Mirko
Mathieu Lacage
Where is that patch ?
Mirko.Banchi
> Where is that patch ?
>
> Mathieu
patch is Final patch v3. It's an incremental patch. If you prefer a
complete final patch i can upload it. But i've understood that this
method of reviewing is abolished. Should i send it by patchbomb?
However all code is at
http://code.nsnam.org/mirko/ns-3-80211n
Thanks,
Mirko
Mathieu Lacage
Please, don't introduce such changes to the final patch: if you want
to make these changes, spare them for _another_ patch.