ns-3 Bug 978: Consolidate Wi-Fi MAC high functionality

Initial patch. See ns-3 bug 978 for description and discussion.

[Nicola Baldo, 2010-08-27 13:45:37]

a few detailed comments here. More generic comments on bugzilla.

http://codereview.appspot.com/1926043/diff/1/41
File src/devices/wifi/sta-wifi-mac.cc (right):

http://codereview.appspot.com/1926043/diff/1/41#newcode179
src/devices/wifi/sta-wifi-mac.cc:179: if (m_qosSupported)
I agree that the standard does not say very clearly how to deal with management
frames in a QoS STA/AP. My interpretation is that the plain DCF parameters are
to be used for management frames. What is not clear to me is whether this means
that we need to use AC_BE, or m_dca. Maybe better to use m_dca, if we have both.

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

http://codereview.appspot.com/1926043/diff/1/44#newcode292
src/devices/wifi/wifi-mac.h:292: virtual void FinishConfigureStandard (enum
WifiPhyStandard standard);
when re-implementing this method, do we need to chain it up to the parent?

http://codereview.appspot.com/1926043/diff/1/44#newcode302
src/devices/wifi/wifi-mac.h:302: void SetTypeOfStation (TypeOfStation type);
I think that having this in wifi-mac conflicts with your proposed use case of
having an AP that supports both QoS and non-QoS STAs. What about moving this to
derived classes?

http://codereview.appspot.com/1926043/diff/1/44#newcode356
src/devices/wifi/wifi-mac.h:356: * state. Having a big switch seems like a good
intermediate stage,
I would move also this part to the derived classes (see previous comment)

[Dean, 2010-09-02 21:48:07]

Nicola,

Thanks for your attention on this and your comments. As a result I've made some
changes and have updated this CodeReview Issue (and the hosted repository at
http://code.nsnam.org/deanarm/ns-3-dev-wifi-devel). I have, however, queried a
couple of the comments you made below; I may have misunderstood these, so I'd
appreciate clarification.

Kind regards,
Dean.

http://codereview.appspot.com/1926043/diff/1/41
File src/devices/wifi/sta-wifi-mac.cc (right):

http://codereview.appspot.com/1926043/diff/1/41#newcode179
src/devices/wifi/sta-wifi-mac.cc:179: if (m_qosSupported)
On 2010/08/27 13:45:37, Nicola Baldo wrote:
> I agree that the standard does not say very clearly how to deal with
management
> frames in a QoS STA/AP. My interpretation is that the plain DCF parameters are
> to be used for management frames. What is not clear to me is whether this
means
> that we need to use AC_BE, or m_dca. Maybe better to use m_dca, if we have
both.
> 

That seems sensible. I've done this for the WifiMac derived classes in the wifi
module. I've left MeshWifiInterfaceMac alone, because it appears to have been an
explicit choice there.

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

http://codereview.appspot.com/1926043/diff/1/44#newcode292
src/devices/wifi/wifi-mac.h:292: virtual void FinishConfigureStandard (enum
WifiPhyStandard standard);
On 2010/08/27 13:45:37, Nicola Baldo wrote:
> when re-implementing this method, do we need to chain it up to the parent?

I don't believe that this method should need to be reimplemented. I've thus
removed the virtual function specifier which I erroneously left in.

http://codereview.appspot.com/1926043/diff/1/44#newcode302
src/devices/wifi/wifi-mac.h:302: void SetTypeOfStation (TypeOfStation type);
On 2010/08/27 13:45:37, Nicola Baldo wrote:
> I think that having this in wifi-mac conflicts with your proposed use case of
> having an AP that supports both QoS and non-QoS STAs.

I'm not sure that it does. The TypeOfStation specifies whether the particular
WifiMac derivative is implementing an AP, (non-AP) STA, STA in IBSS, or Mesh
node. This information is required by the EdcaTxopN class and is thus only
relevant if QoS is in use, but I can't see anything that is precluded by
configuring it regardless.

> What about moving this to derived classes?

In what way do you think it would be implemented differently in derived classes?

http://codereview.appspot.com/1926043/diff/1/44#newcode356
src/devices/wifi/wifi-mac.h:356: * state. Having a big switch seems like a good
intermediate stage,
On 2010/08/27 13:45:37, Nicola Baldo wrote:
> I would move also this part to the derived classes (see previous comment)

So this is the Big Switch that controls whether this particular WifiMac instance
supports QoS or not. When support for dynamic QoS/non-QoS operation comes along
then this is likely to remain - it only indicates whether *QoS is supported*,
not whether it is actually in use on a given association (or in a given IBSS).
The latter would be (in the case of AP and STA) captured in yet-to-be-added
per-association state.

As with your previous comment, I don't quite understand how you think the
implementation here restricts functionality. How would you see this being
differently implemented in the derived classes?

[Nicola Baldo, 2010-09-06 12:40:18]

Dean, thanks for the updated patch and for the many clarifications. Several of
my doubts are gone now. Still, I think there are a few issues to be addressed -
see below comments.

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

http://codereview.appspot.com/1926043/diff/1/44#newcode292
src/devices/wifi/wifi-mac.h:292: virtual void FinishConfigureStandard (enum
WifiPhyStandard standard);
On 2010/09/02 21:48:07, Dean wrote:
> I don't believe that this method should need to be reimplemented. I've thus
> removed the virtual function specifier which I erroneously left in.

Actually I have a use case for having a virtual method here. The solution I had
in mind for bug 945 (fix support for vehicular communications (WAVE)) involved
the creation of a new MAC class, say WaveWifiMac, which would then use different
CWmin, CWmax and AIFSN parameters. How would you accommodate this in your
approach?

http://codereview.appspot.com/1926043/diff/1/44#newcode302
src/devices/wifi/wifi-mac.h:302: void SetTypeOfStation (TypeOfStation type);
After your explanation, I understand that I did a wrong interpretation of the
TypeOfStation parameter. The proposed approach is now ok for me. Sorry for the
confusion!

http://codereview.appspot.com/1926043/diff/1/44#newcode356
src/devices/wifi/wifi-mac.h:356: * state. Having a big switch seems like a good
intermediate stage,
On 2010/09/02 21:48:07, Dean wrote:
> So this is the Big Switch that controls whether this particular WifiMac
instance
> supports QoS or not. When support for dynamic QoS/non-QoS operation comes
along
> then this is likely to remain - it only indicates whether *QoS is supported*,
> not whether it is actually in use on a given association (or in a given IBSS).

So if it does not say whether QoS is actually *used*, what is the use case for
this Big Switch? I though that in your new design QoS was always supported (you
have both DcaTxop and EdcaTxop available), and that the only difference between
a QSTA and a NQSTA was what was actually being used. 

> The latter would be (in the case of AP and STA) captured in yet-to-be-added
> per-association state.
> 
> As with your previous comment, I don't quite understand how you think the
> implementation here restricts functionality. How would you see this being
> differently implemented in the derived classes?

Well, now I understand that I was actually thinking of the QosUsed switch. I
guess that this would be implemented in derived classes, is this correct?

[Dean, 2010-09-06 13:31:31]

Nicola,

Thanks for your comments; I've responded below.

I haven't updated the issue with the change I mention yet - I thought it worth
getting a response back to you quickly in case you are still looking at this.

Kind regards,
Dean.

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

http://codereview.appspot.com/1926043/diff/1/44#newcode292
src/devices/wifi/wifi-mac.h:292: virtual void FinishConfigureStandard (enum
WifiPhyStandard standard);
On 2010/09/06 12:40:18, Nicola Baldo wrote:
> Actually I have a use case for having a virtual method here. The solution I
had
> in mind for bug 945 (fix support for vehicular communications (WAVE)) involved
> the creation of a new MAC class, say WaveWifiMac, which would then use
different
> CWmin, CWmax and AIFSN parameters. How would you accommodate this in your
> approach?

That is a good point. I will make this function virtual again, and improve the
associated comment to indicate that a derived class may choose to reimplement it
in order to support different channel access parameters. The comment will also
note that the reimplemented method may chain up to the parent for certain values
of the WifiPhyStandard enumeration.

Do you think this covers it?

http://codereview.appspot.com/1926043/diff/1/44#newcode356
src/devices/wifi/wifi-mac.h:356: * state. Having a big switch seems like a good
intermediate stage,
On 2010/09/06 12:40:18, Nicola Baldo wrote:
> So if it does not say whether QoS is actually *used*, what is the use case for
> this Big Switch? I though that in your new design QoS was always supported
(you
> have both DcaTxop and EdcaTxop available), and that the only difference
between
> a QSTA and a NQSTA was what was actually being used. 

Okay - I can see that I have not explained this clearly. The key distinction is
between what the ns-3 WifiMac(-derived class) model supports, and what the
device that is being modelled notionally (i.e., in terms of the simulated world)
supports.

To see the use case (that I claim exists) for this switch, consider a time when
ApWifiMac supports both QoS and non-QoS associated STAs. At this point, one may
wish to model a BSS involving a QoS AP, and two associated STAs - one with a QoS
association, and one with a non-QoS association.

The Big Switch would in this case allow (notional) QoS support to be turned off
at one of the STAs, so that when both associate to the AP we end up with the
simulation topology we want.

A less interesting example (again, assuming forthcoming StaWifiMac support for
fall-back to non-QoS association if the AP does not support QoS) is the case
where we actually want to simulate a non-QoS BSS. In this case, all we need to
do is throw the Big Switch to off on the AP, and all associating STAs will do
The Right Thing.

> Well, now I understand that I was actually thinking of the QosUsed switch. I
> guess that this would be implemented in derived classes, is this correct?

Above I've alluded to my opinion that there is no need to have a "QosUsed
switch" - control of "notional" support for QoS on the STAs (whether AP, IBSS,
or non-AP infrastructure) will allow the user to configure the topology they
want through the attribute subsystem.

Does that make sense?

[Dean, 2010-09-16 19:28:25]

All,

I've updated the code at this issue along the lines that Nicola suggested in a
comment on Bug 978. Specifically, I have reverted the WifiMac class to how it
was before I came along, and have instead inserted a RegularWifiMac class which
derives from WifiMac and from which StaWifiMac, ApWifiMac, AdhocWifiMac, and
MeshWifiInterfaceMac all derive. It is this class to which the Wi-Fi MAC high
functionality is now consolidated, leaving the more abstract WifiMac as a base
that allows for more custom efforts.

Comments appreciated.

Kind regards,
Dean.

[Gary Pei, 2010-09-17 23:05:42]

http://codereview.appspot.com/1926043/diff/17001/src/devices/wifi/regular-wif...
File src/devices/wifi/regular-wifi-mac.h (right):

http://codereview.appspot.com/1926043/diff/17001/src/devices/wifi/regular-wif...
src/devices/wifi/regular-wifi-mac.h:322: void SetupEdcaQueue (enum AcIndex ac);
Can you move this to "protected" rather than privte. For virtual access point ( 
Issue 2104052), if I want to implement VapResetWithSharedLowerLayers() in
ap-wifi-mac.cc. I need to use this function to reset the queues so that all
virtual APs share the same queues.

[Dean, 2010-09-30 20:13:46]

Gary,

Thanks for your comment. I've updated the code here per your request.

Kind regards,
Dean.

http://codereview.appspot.com/1926043/diff/17001/src/devices/wifi/regular-wif...
File src/devices/wifi/regular-wifi-mac.h (right):

http://codereview.appspot.com/1926043/diff/17001/src/devices/wifi/regular-wif...
src/devices/wifi/regular-wifi-mac.h:322: void SetupEdcaQueue (enum AcIndex ac);
On 2010/09/17 23:05:42, Gary Pei wrote:
> Can you move this to "protected" rather than privte. For virtual access point
( 
> Issue 2104052), if I want to implement VapResetWithSharedLowerLayers() in
> ap-wifi-mac.cc. I need to use this function to reset the queues so that all
> virtual APs share the same queues. 

Done.

[Nicola Baldo, 2010-10-04 16:54:25]

I reviewed as carefully as I could all the code. The most critical issue that is
left is to make beaconing consistent between AP and MESH (e.g., by making the
latter using m_beaconDca), and most importantly to fix the initialization order
of m_beaconDca, so that it has the highest priority.

Overall, the work is very good, thank you very much for the excellent work!

http://codereview.appspot.com/1926043/diff/25001/src/devices/mesh/mesh-wifi-i...
File src/devices/mesh/mesh-wifi-interface-mac.cc (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/mesh/mesh-wifi-i...
src/devices/mesh/mesh-wifi-interface-mac.cc:529: m_dca->SetMinCw (0);
I think we should have a solution coherent with ApWifiMac. For example, having a
separate m_beaconDca. Maybe we should put it in RegularWifiMac to avoid the
initialization order problem.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/adhoc-wifi-...
File src/devices/wifi/adhoc-wifi-mac.cc (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/adhoc-wifi-...
src/devices/wifi/adhoc-wifi-mac.cc:41: using namespace ns3;
we normally use "namespace ns3 {". I agree it doesn't make much difference, but
you know, just for consistency...

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/adhoc-wifi-...
File src/devices/wifi/adhoc-wifi-mac.h (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/adhoc-wifi-...
src/devices/wifi/adhoc-wifi-mac.h:20: * Author: Mirko Banchi
<mk.banchi@gmail.com>
Is there really any code by Mirko which you added to this file?

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/adhoc-wifi-...
src/devices/wifi/adhoc-wifi-mac.h:27: #include "amsdu-subframe-header.h"
is this really needed?

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac.cc
File src/devices/wifi/ap-wifi-mac.cc (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.cc:43: using namespace ns3;
maybe "namespace nse {"

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.cc:75: m_beaconDca->SetManager (m_dcfManager);
we have a problem here. EdcaTxopN::SetManager() calls DcfManager::Set(); the
order in which the latter is called determines the priority of virtual
collisions. See DcfManager::DoGrantAccess(). This means that a beacon could be
cancelled because of virtual collision with a data frame. I think this is not
the intended behavior.

A way to solve this is to initialize DCAs in the correct order. Maybe with a
RegularWifiMac::InitializeDcas() method to be called explicitly by child classes
such as ApWifiMac.

Alternatively, you could simply add m_beaconDca into RegularWifiMac. It would be
used by the Ap and Mesh WifiMacs only.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.cc:178: // QosUtilsMapTidToAc()), so we use that as
our default here.
This comment is not correct, because if (m_qosSupported == false) then you are
actually calling m_dca->Queue (), and not m_edca[QosUtilsMapTidToAc (0)]

You have a similar approach in sta-wifi-mac.cc, so the same consideration that I
made here holds there as well.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.cc:199: Mac48Address to, uint8_t tid)
My understanding is that you overloaded the ForwardDown method in order to allow
preserving the UP when the AP is forwarding a packet between two STAs. Is this
correct?

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac.h
File src/devices/wifi/ap-wifi-mac.h (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.h:27: #include "amsdu-subframe-header.h"
not needed, right?

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.h:63: */
Can you please concentrate the documentation of virtual functions in a single
place (the parent class)? 
In the child classes, I will just put a list of all inherited functions
one-per-line without any doxygen, and prepended by a comment like "// inherited
from XXX". 
I think this helps maintaining the documentation, and makes it easier to
identify parent/child relationship when you browse header files.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/sta-wifi-ma...
File src/devices/wifi/sta-wifi-mac.cc (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/sta-wifi-ma...
src/devices/wifi/sta-wifi-mac.cc:321: uint8_t tid = 0;
see identical comment in ap-wifi-mac.cc

http://codereview.appspot.com/1926043/diff/25001/src/helper/wifi-helper.cc
File src/helper/wifi-helper.cc (right):

http://codereview.appspot.com/1926043/diff/25001/src/helper/wifi-helper.cc#ne...
src/helper/wifi-helper.cc:148: LogComponentEnable ("NqstaWifiMac",
LOG_LEVEL_ALL);
remove

http://codereview.appspot.com/1926043/diff/25001/src/helper/wifi-helper.cc#ne...
src/helper/wifi-helper.cc:151: LogComponentEnable ("QadhocWifiMac",
LOG_LEVEL_ALL);
remove

http://codereview.appspot.com/1926043/diff/25001/src/helper/wifi-helper.cc#ne...
src/helper/wifi-helper.cc:152: LogComponentEnable ("QstaWifiMac",
LOG_LEVEL_ALL);
change to StaWifiMac

[Dean, 2010-10-10 20:10:52]

Nicola,

Thanks for this new set of useful comments. I believe I have addressed them in
the latest patch.

I'll also shortly push the corresponding change to
http://code.nsnam.org/deanarm/ns-3-dev-wifi-devel. The history is still not
pretty in that repository, but if you think we are getting close to approval to
push, then I'll look at tidying it up.

I'll note that check-style.py points out a few issues in the files I've touched.
I haven't fixed these up yet, but will do this as part of the final tidy before
push, if that is acceptable.

Kind regards,
Dean.

http://codereview.appspot.com/1926043/diff/25001/src/devices/mesh/mesh-wifi-i...
File src/devices/mesh/mesh-wifi-interface-mac.cc (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/mesh/mesh-wifi-i...
src/devices/mesh/mesh-wifi-interface-mac.cc:529: m_dca->SetMinCw (0);
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> I think we should have a solution coherent with ApWifiMac. For example, having
a
> separate m_beaconDca. Maybe we should put it in RegularWifiMac to avoid the
> initialization order problem.

This is a good suggestion and I have followed it. A DCF for beacon generation is
now instantiated in RegularWifiMac and the (protected, there) m_beaconDca points
to it.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/adhoc-wifi-...
File src/devices/wifi/adhoc-wifi-mac.cc (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/adhoc-wifi-...
src/devices/wifi/adhoc-wifi-mac.cc:41: using namespace ns3;
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> we normally use "namespace ns3 {". I agree it doesn't make much difference,
but
> you know, just for consistency...

Done.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/adhoc-wifi-...
File src/devices/wifi/adhoc-wifi-mac.h (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/adhoc-wifi-...
src/devices/wifi/adhoc-wifi-mac.h:20: * Author: Mirko Banchi
<mk.banchi@gmail.com>
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> Is there really any code by Mirko which you added to this file?

Possibly not, but because I was merging qadhoc-wifi-mac.* with adhoc-wifi-mac.*,
I also merged the authors to be safe.

I suspect, however, that Mirko's bit was the Block Ack stuff which has been
moved to regular-wifi-mac.*. Based on your comment and this assumption, I've
removed Mirko's copyright from sta-wifi-mac.*, adhoc-wifi-mac.*, and
ap-wifi-mac.*, and added his name to regular-wifi-mac.*.

It's all a bit hard to track - I hope that someone will pipe up if this is now a
misrepresentation.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/adhoc-wifi-...
src/devices/wifi/adhoc-wifi-mac.h:27: #include "amsdu-subframe-header.h"
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> is this really needed?

No. I've removed it and (at least some) other pointless inclusions in related
files.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac.cc
File src/devices/wifi/ap-wifi-mac.cc (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.cc:43: using namespace ns3;
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> maybe "namespace nse {" 

Done.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.cc:75: m_beaconDca->SetManager (m_dcfManager);
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> we have a problem here. EdcaTxopN::SetManager() calls DcfManager::Set(); the
> order in which the latter is called determines the priority of virtual
> collisions. See DcfManager::DoGrantAccess(). This means that a beacon could be
> cancelled because of virtual collision with a data frame. I think this is not
> the intended behavior.
> 
> A way to solve this is to initialize DCAs in the correct order. Maybe with a
> RegularWifiMac::InitializeDcas() method to be called explicitly by child
classes
> such as ApWifiMac.
> 
> Alternatively, you could simply add m_beaconDca into RegularWifiMac. It would
be
> used by the Ap and Mesh WifiMacs only.

As mentioned in response to earlier comment. The last suggestion has been
followed.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.cc:178: // QosUtilsMapTidToAc()), so we use that as
our default here.
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> This comment is not correct, because if (m_qosSupported == false) then you are
> actually calling m_dca->Queue (), and not m_edca[QosUtilsMapTidToAc (0)]
> 
> You have a similar approach in sta-wifi-mac.cc, so the same consideration that
I
> made here holds there as well.

Good point. Comment reworked here and elsewhere.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.cc:199: Mac48Address to, uint8_t tid)
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> My understanding is that you overloaded the ForwardDown method in order to
allow
> preserving the UP when the AP is forwarding a packet between two STAs. Is this
> correct?

Yes. That is precisely why this ForwardDown method exists. I've added some
commenting to this effect in ap-wifi-mac.h.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac.h
File src/devices/wifi/ap-wifi-mac.h (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.h:27: #include "amsdu-subframe-header.h"
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> not needed, right?

Done.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/ap-wifi-mac...
src/devices/wifi/ap-wifi-mac.h:63: */
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> Can you please concentrate the documentation of virtual functions in a single
> place (the parent class)? 
> In the child classes, I will just put a list of all inherited functions
> one-per-line without any doxygen, and prepended by a comment like "//
inherited
> from XXX". 
> I think this helps maintaining the documentation, and makes it easier to
> identify parent/child relationship when you browse header files.

Okay. I've done this, using a doxygen member group to contain the inherited
methods. I think there's a single exception, where I document a reimplemented
member which, in this case, is to note the reasons for reimplementing it.

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/sta-wifi-ma...
File src/devices/wifi/sta-wifi-mac.cc (right):

http://codereview.appspot.com/1926043/diff/25001/src/devices/wifi/sta-wifi-ma...
src/devices/wifi/sta-wifi-mac.cc:321: uint8_t tid = 0;
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> see identical comment in ap-wifi-mac.cc

Done.

http://codereview.appspot.com/1926043/diff/25001/src/helper/wifi-helper.cc
File src/helper/wifi-helper.cc (right):

http://codereview.appspot.com/1926043/diff/25001/src/helper/wifi-helper.cc#ne...
src/helper/wifi-helper.cc:148: LogComponentEnable ("NqstaWifiMac",
LOG_LEVEL_ALL);
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> remove

Done.

http://codereview.appspot.com/1926043/diff/25001/src/helper/wifi-helper.cc#ne...
src/helper/wifi-helper.cc:151: LogComponentEnable ("QadhocWifiMac",
LOG_LEVEL_ALL);
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> remove

Done.

http://codereview.appspot.com/1926043/diff/25001/src/helper/wifi-helper.cc#ne...
src/helper/wifi-helper.cc:152: LogComponentEnable ("QstaWifiMac",
LOG_LEVEL_ALL);
On 2010/10/04 16:54:25, Nicola Baldo wrote:
> change to StaWifiMac

Done.

[nbaldo_cttc.es, 2010-11-09 17:39:33]

Hi Dean,

On 10/10/2010 10:10 PM, deanarm@gmail.com wrote:
 > Thanks for this new set of useful comments. I believe I have addressed
 > them in the latest patch.

I reviewed your latest patch. I think it is now satisfactory, so you 
have my +1 to merge, provided that the following is done:

1) Kirill should approve the changes on the mesh device

2) fix history + check-style.py, as you proposed (see below comments)

3) add a meaningful description to RELEASE_NOTES and CHANGES.html
This is very important since this change is a major one for the wifi 
device.


> I'll also shortly push the corresponding change to
> http://code.nsnam.org/deanarm/ns-3-dev-wifi-devel. The history is still
> not pretty in that repository, but if you think we are getting close to
> approval to push, then I'll look at tidying it up.

I would be fine with having just one big changeset labeled "bug 978". 
It's going to break things for many people, but I don't see how a more 
fine-grained history would be better. It could actually be worse, 
because with many incremental changes then people won't know exactly 
what to blame when they find a change that breaks their code. With a 
single change, they can be redirected to the overall description of the 
change (which should be in CHANGES.html and should have the same title 
of the commit message).

What do you think?


> I'll note that check-style.py points out a few issues in the files I've
> touched. I haven't fixed these up yet, but will do this as part of the
> final tidy before push, if that is acceptable.

yes absolutely.


Thank you very much for your good work!

Regards,

Nicola



>
> http://codereview.appspot.com/1926043/
>