Bug 978

I've had a bit of a crack at this, and the results can be seen in CodeReview Issue 1926043 (one big hit), and at http://code.nsnam.org/deanarm/ns-3-dev-wifi-devel

As a couple of disclaimers... I've performed no history rewriting in the repository cited above; I'd like to get broad agreement on the approach before spending too much time tidying things like this up. I think I've improved the doxygen coverage in the code I've touched, but it is still far from complete, and I would like to do more here as part of this bug.

Many lines of code have changed but, I claim, functionality of the model has not (with the exception of an Adhoc-specific Block Ack bug being fixed as a consequence of consolidating the three-times-replicated code up into WifiMac).

The key architectural difference is that after my changes there exist only three "MAC high" classes in the wifi module (MeshWifiInterfaceMac, in the mesh module, is also affected, and I'll touch on it later). These are ApWifiMac, StaWifiMac, and AdhocWifiMac.

The first thing you'll notice is that there are no QoS/non-QoS variants. QoS/non-QoS selection is now achieved through manipulation of a Boolean attribute of WifiMac, called "QosSupported", which sets a protected member that WifiMac and derived classes use in the small amount of logic necessary to switch between QoS and non-QoS behaviour.

The WifiMac class has absorbed a lot of functionality (it was rather bare before I got hold of it). In particular, my aim was to have it provide sufficient functionality that ApWifiMac, StaWifiMac, and AdhocWifiMac, could forget about the routine of Wi-Fi, and focus on the specifics of being an AP, non-AP STA in an infrastructure network, or IBSS STA, respectively.

To achieve this I've had WifiMac take on responsibility for constructing related objects such as MacRxMiddle, MacTxMiddle, MacLow and DcfManager, and responsibility for creating a basic set of channel access functions. Specifically, WifiMac creates one DcaTxop (pointed to by the m_dca property), and the four EdcaTxopN's necessary for QoS.

Obviously, the four EdcaTxopN's will not be used on a MAC configured for non-QoS operation, and the DcaTxop will not be used on a MAC configured for QoS operation. This may seem a touch wasteful of memory, but my feeling was that it was acceptable given a general desire to keep related functionality in the same place. In a similar vein, I've also shifted all MAC high Block Ack handling into WifiMac, as the logic involved was common to the three previous QoS WifiMac-derived classes.

I think this approach puts us on the right path for the day when we need to model, for example, an AP with both QoS and non-QoS STAs associated, but I'd quite like to hear others' thoughts.

The best place to start looking at what all this has meant for WifiMac-derived classes is in the simplest of these - AdhocWifiMac.

This class is now looking considerably simpler, at well short of 200 lines of code, and I think that in a few bullets I can detail how it builds on WifiMac...

 - No attributes are needed beyond those provided by WifiMac, so AdhocWifiMac::GetTypeId() is trivial.

 - The constructor sets the type of station to be ADHOC_STA (EdcaTxopN's need to know this). The destructor is empty.

 - WifiMac::SetAddress() is overriden to also set the BSSID (not correct per the standard, but as per the existing implementation).

 - the Enqueue() method is implemented with code to cover the existing assumption about supported rates in IBSS, and To DS/From DS bits in frames set appropriately for IBSS.

 - WifiMac::SetLinkUpCallback() is overriden to implement the always-up-ness of IBSS.

 - Finally, the Receive() method handles QoS and non-QoS data frames, deferring everything else to WifiMac::Receive() (which, for example, deals with Block Ack Action frames).

ApWifiMac and StaWifiMac are obviously more complex than this, but with much of the common code gone (e.g., the set and get accessors for the various interframe spacings, and channel access function configuration code) I believe they are considerably easier to understand (and therefore maintain and extend).

MeshWifiInterfaceMac, as a WifiMac-derived class, has also been a beneficiary of these changes. Without going into the detail, a couple of hundred lines of code (which was common with other WifiMac's) have disappeared from mesh-wifi-interface-mac.cc. The one special note I'll make about MeshWifiInterfaceMac, is that it makes use of all five channel access functions provided by WifiMac. As a QoS MAC at heart, it obviously uses the EdcaTxopN's, and it reconfigures the DcaTxop (by overriding WifiMac::FinishConfigureStandard()) so that it is suitable for beacon generation (CWmin, CWmax, and AIFSN change).

The sum of the changes I have made do result in some regression test failures (PCAP trace mismatches). I have looked fairly hard at these and have convinced myself that the cause is channel access functions getting different random number streams due to the reorganisation of code that instantiates EdcaTxopN's and DcaTxop's.

So I'd appreciate comment on this bug in general, my approach, and the specifics of the changes I am proposing.

Hi!
I have not seen published code changes yet, but, in general, I have a couple of comments (maybe not so significant) to this huge code refactoring:

First is, that DcaTxop and EdcaTxop have a lot of common code (an aggregation is an only difference, as it seems to me), and if this two classes can be merged in a single class with "Enable-N-Aggregation" option.

Second is, that a difference between station, AP, and mesh station functionality consists of processing management frames, which is done by plug-in system in mesh module. I think, that this scheme is another possible ability to minimize the amount of code in wifi module.

-

(In reply to comment #1)
> I've had a bit of a crack at this, and the results can be seen in CodeReview
> Issue 1926043 (one big hit), and at
> http://code.nsnam.org/deanarm/ns-3-dev-wifi-devel
> 
> As a couple of disclaimers... I've performed no history rewriting in the
> repository cited above; I'd like to get broad agreement on the approach before
> spending too much time tidying things like this up. I think I've improved the
> doxygen coverage in the code I've touched, but it is still far from complete,
> and I would like to do more here as part of this bug.
> 
> Many lines of code have changed but, I claim, functionality of the model has
> not (with the exception of an Adhoc-specific Block Ack bug being fixed as a
> consequence of consolidating the three-times-replicated code up into WifiMac).

What is this bug? Could you please be more specific?

> The key architectural difference is that after my changes there exist only
> three "MAC high" classes in the wifi module (MeshWifiInterfaceMac, in the mesh
> module, is also affected, and I'll touch on it later). These are ApWifiMac,
> StaWifiMac, and AdhocWifiMac.
> 
> The first thing you'll notice is that there are no QoS/non-QoS variants.
> QoS/non-QoS selection is now achieved through manipulation of a Boolean
> attribute of WifiMac, called "QosSupported", which sets a protected member that
> WifiMac and derived classes use in the small amount of logic necessary to
> switch between QoS and non-QoS behaviour.
> 
> The WifiMac class has absorbed a lot of functionality (it was rather bare
> before I got hold of it). In particular, my aim was to have it provide
> sufficient functionality that ApWifiMac, StaWifiMac, and AdhocWifiMac, could
> forget about the routine of Wi-Fi, and focus on the specifics of being an AP,
> non-AP STA in an infrastructure network, or IBSS STA, respectively.
> 
> To achieve this I've had WifiMac take on responsibility for constructing
> related objects such as MacRxMiddle, MacTxMiddle, MacLow and DcfManager, and
> responsibility for creating a basic set of channel access functions.
> Specifically, WifiMac creates one DcaTxop (pointed to by the m_dca property),
> and the four EdcaTxopN's necessary for QoS.
> 
> Obviously, the four EdcaTxopN's will not be used on a MAC configured for
> non-QoS operation, and the DcaTxop will not be used on a MAC configured for QoS
> operation. This may seem a touch wasteful of memory, but my feeling was that it
> was acceptable given a general desire to keep related functionality in the same
> place. In a similar vein, I've also shifted all MAC high Block Ack handling
> into WifiMac, as the logic involved was common to the three previous QoS
> WifiMac-derived classes.
> 
> I think this approach puts us on the right path for the day when we need to
> model, for example, an AP with both QoS and non-QoS STAs associated, but I'd
> quite like to hear others' thoughts.
>
> The best place to start looking at what all this has meant for WifiMac-derived
> classes is in the simplest of these - AdhocWifiMac.
> 
> This class is now looking considerably simpler, at well short of 200 lines of
> code, and I think that in a few bullets I can detail how it builds on
> WifiMac...
> 
>  - No attributes are needed beyond those provided by WifiMac, so
> AdhocWifiMac::GetTypeId() is trivial.
> 
>  - The constructor sets the type of station to be ADHOC_STA (EdcaTxopN's need
> to know this). The destructor is empty.
> 
>  - WifiMac::SetAddress() is overriden to also set the BSSID (not correct per
> the standard, but as per the existing implementation).
> 
>  - the Enqueue() method is implemented with code to cover the existing
> assumption about supported rates in IBSS, and To DS/From DS bits in frames set
> appropriately for IBSS.
> 
>  - WifiMac::SetLinkUpCallback() is overriden to implement the always-up-ness of
> IBSS.
> 
>  - Finally, the Receive() method handles QoS and non-QoS data frames, deferring
> everything else to WifiMac::Receive() (which, for example, deals with Block Ack
> Action frames).
> 
> ApWifiMac and StaWifiMac are obviously more complex than this, but with much of
> the common code gone (e.g., the set and get accessors for the various
> interframe spacings, and channel access function configuration code) I believe
> they are considerably easier to understand (and therefore maintain and extend).
> 
> MeshWifiInterfaceMac, as a WifiMac-derived class, has also been a beneficiary
> of these changes. Without going into the detail, a couple of hundred lines of
> code (which was common with other WifiMac's) have disappeared from
> mesh-wifi-interface-mac.cc. The one special note I'll make about
> MeshWifiInterfaceMac, is that it makes use of all five channel access functions
> provided by WifiMac. As a QoS MAC at heart, it obviously uses the EdcaTxopN's,
> and it reconfigures the DcaTxop (by overriding
> WifiMac::FinishConfigureStandard()) so that it is suitable for beacon
> generation (CWmin, CWmax, and AIFSN change).
> 
> The sum of the changes I have made do result in some regression test failures
> (PCAP trace mismatches). I have looked fairly hard at these and have convinced
> myself that the cause is channel access functions getting different random
> number streams due to the reorganisation of code that instantiates EdcaTxopN's
> and DcaTxop's.
> 
> So I'd appreciate comment on this bug in general, my approach, and the
> specifics of the changes I am proposing.

I like very much the changes that you are proposing...i think that these changes should help very much to work in mixed mode so should be easier to manage for example the communication and the interaction among qos and non-qos stations. I also think that is correct to move the block ack code in WifiMac class in order to minimize code duplication. However for example i have tried the script examples/wireless/wifi-blockack.cc with your code but it doesn't work and raises a fatal error: "Unsupported action frame received".However this should be easy to fix. Keep up the good work Dean!

(In reply to comment #4)
> > Many lines of code have changed but, I claim, functionality of the model has
> > not (with the exception of an Adhoc-specific Block Ack bug being fixed as a
> > consequence of consolidating the three-times-replicated code up into WifiMac).
> 
> What is this bug? Could you please be more specific?

Sorry - I certainly should have initially, but was too tied up in the wider changes.

In the Action frame handling of QAdhocWifiMac::Receive() at current head of ns-3-dev, we have the following code:

else if (actionHdr.GetCategory () == WifiActionHeader::BLOCK_ACK &&
         actionHdr.GetAction().blockAck == WifiActionHeader::BLOCK_ACK_ADDBA_REQUEST)
{
  MgtAddBaResponseHeader respHdr;
  packet->RemoveHeader (respHdr);
  m_queues[QosUtilsMapTidToAc (respHdr.GetTid ())]->GotAddBaResponse (&respHdr, hdr->GetAddr2 ());
}

This code needs s/REQUEST/RESPONSE/. Compare with the equivalent in QStaWifiMac::Receive() and QApWifiMac::Receive().

This deserves to be properly tracked, so I'll raise a bug later on when I have a bit more time to look at this and the apparent problem you have found between my changeset and the wifi-blockack.cc example.

(In reply to comment #4)
> What is this bug? Could you please be more specific?

I've raised Bug 983 so that the issue I mentioned in my last post here is now properly tracked.

> However for example i have tried the script
> examples/wireless/wifi-blockack.cc with your code but it doesn't
> work and raises a fatal error: "Unsupported action frame received".

Thanks, Mirko, for pointing this out. I've fixed it in changeset 6563:ae5cfbe987a3 which is right now on the tip of http://code.nsnam.org/deanarm/ns-3-dev-wifi-devel (I haven't updated the CodeReview issue yet).

The failure was due to generally poor programming on my part, with WifiMac::Receive() not returning once it had successfully handled the Block Ack Action frame.

(In reply to comment #2)

Thanks for these comments, Kirill.

> First is, that DcaTxop and EdcaTxop have a lot of common code (an aggregation
> is an only difference, as it seems to me), and if this two classes can be
> merged in a single class with "Enable-N-Aggregation" option.

I hadn't looked at these classes in detail before your note, but now I can see what you are talking about. I think there is certainly the possibility of removing some code duplication here, but the only thing I will note is that such work should be done somewhere other than this bug, which is focused on "MAC high" functionality.

> Second is, that a difference between station, AP, and mesh station
> functionality consists of processing management frames, which is done by
> plug-in system in mesh module. I think, that this scheme is another possible
> ability to minimize the amount of code in wifi module.

I noticed this in my refactor and did wonder whether a generalisation of this could be used in the wifi module.

The one thing that seemed non-trivial to me was the matter of ensuring the correct ordering of (e.g., frame modification) actions performed by the plugins. Specifically, it wasn't immediately clear to me how we could tidily arrange such that, for example, the information elements introduced by more than one plugin would appear in the correct order in a management frame body.

Did you have thoughts on this?

(In reply to comment #7)
> (In reply to comment #2)
> > First is, that DcaTxop and EdcaTxop have a lot of common code (an aggregation
> > is an only difference, as it seems to me), and if this two classes can be
> > merged in a single class with "Enable-N-Aggregation" option.
> 
> I hadn't looked at these classes in detail before your note, but now I can see
> what you are talking about. I think there is certainly the possibility of
> removing some code duplication here, but the only thing I will note is that
> such work should be done somewhere other than this bug, which is focused on
> "MAC high" functionality.
> 

I agree with Dean that the [E]dcaTxOp issue is beyond the scope of this bug. I created bug 987 to keep track of this issue.

In general, I agree with Dean that there is a lot of duplicated code in the various files *-wifi-mac.*, and that's the reason I like this patch in general -  it is a significant simplification with respect to the existing code. I gave a quick look to it on codereview, and provided a few detailed comments (not exhaustive by any means). 

I think the most critical aspect that must be agreed upon is the elimination of the distinction between Qos and Nqos versions of the Ap/Sta/Adhoc MAC classes. I would appreciate if the people involved in this dicussion could explicitly state if they like/dislike/dontcare this idea.

(In reply to comment #9)
> In general, I agree with Dean that there is a lot of duplicated code in the
> various files *-wifi-mac.*, and that's the reason I like this patch in general
> -  it is a significant simplification with respect to the existing code. I gave
> a quick look to it on codereview, and provided a few detailed comments (not
> exhaustive by any means). 
> 
> I think the most critical aspect that must be agreed upon is the elimination of
> the distinction between Qos and Nqos versions of the Ap/Sta/Adhoc MAC classes.
> I would appreciate if the people involved in this dicussion could explicitly
> state if they like/dislike/dontcare this idea.

As i posted before, i agree with Dean about the elimination of the distinction between Qos and Nqos MAC classes. I think that this would make easier the implementation of mixed mode in particular the communication between a Qos access point and Nqos stations. Of course there would be also a reduction of code duplication.

> I noticed this in my refactor and did wonder whether a generalisation of this
> could be used in the wifi module.
> 
> The one thing that seemed non-trivial to me was the matter of ensuring the
> correct ordering of (e.g., frame modification) actions performed by the
> plugins. Specifically, it wasn't immediately clear to me how we could tidily
> arrange such that, for example, the information elements introduced by more
> than one plugin would appear in the correct order in a management frame body.
> 
> Did you have thoughts on this?

Well, mandatory information elements should be added by first plug-in (that must add all mandatory IEs and this plug-in is a replacement for mac-high model), and other non-mandatory IEs will be added by other plug-ins (if you want to add a vendor specific IE, you should write a new plug-in). All frames, that have mandatory IEs (like supported rates in beacon frame) must not be dropped by any plug-in. This may be a solution.

As it seems to me, mandatory IEs are strictly ordered, but vendor specific - not.

In current wifi-model this problem does not seem serious to me, but, when we will try to construct mesh access point, for example, some problems may arise.

(In reply to comment #11)
> > The one thing that seemed non-trivial to me was the matter of ensuring the
> > correct ordering of (e.g., frame modification) actions performed by the
> > plugins. Specifically, it wasn't immediately clear to me how we could tidily
> > arrange such that, for example, the information elements introduced by more
> > than one plugin would appear in the correct order in a management frame body.
> > 
> > Did you have thoughts on this?
> 
> Well, mandatory information elements should be added by first plug-in (that
> must add all mandatory IEs and this plug-in is a replacement for mac-high
> model), and other non-mandatory IEs will be added by other plug-ins (if you
> want to add a vendor specific IE, you should write a new plug-in). All frames,
> that have mandatory IEs (like supported rates in beacon frame) must not be
> dropped by any plug-in. This may be a solution.
>
> As it seems to me, mandatory IEs are strictly ordered, but vendor specific -
> not.

I'm a little uncomfortable with (or misunderstanding of) the terminology "mandatory IE" in the context of this bug. Specifically, I am not sure whether you are using this to refer to IEs that must be included in a given management frame payload (like the example you give of the Supported Rates element), or whether you are referring to all IEs that are defined by the 802.11 standard (i.e., all IEs that are not "vendor specific" - a term which I am happier with because the standard defines this).

The reason I query this is that, considering the specific and pertinent example of "802.11 QoS support" (to use a vague term), we have IEs such as the EDCA Parameter Set and QoS Capability elements (or the Extended Supported Rates IE, to provide an example relevant to your point) which may or may not be included in a frame depending on the configured capabilities of the STA (which is an AP, in the specific example I'm thinking of), but which, if included, have a well-defined ordering with respect to other IEs that may or may not be included.

(In reply to comment #1)
> The WifiMac class has absorbed a lot of functionality (it was rather bare
> before I got hold of it). In particular, my aim was to have it provide
> sufficient functionality that ApWifiMac, StaWifiMac, and AdhocWifiMac, could
> forget about the routine of Wi-Fi, and focus on the specifics of being an AP,
> non-AP STA in an infrastructure network, or IBSS STA, respectively.
> 
> To achieve this I've had WifiMac take on responsibility for constructing
> related objects such as MacRxMiddle, MacTxMiddle, MacLow and DcfManager, and
> responsibility for creating a basic set of channel access functions.
> Specifically, WifiMac creates one DcaTxop (pointed to by the m_dca property),
> and the four EdcaTxopN's necessary for QoS.
> 
> Obviously, the four EdcaTxopN's will not be used on a MAC configured for
> non-QoS operation, and the DcaTxop will not be used on a MAC configured for QoS
> operation. This may seem a touch wasteful of memory, but my feeling was that it
> was acceptable given a general desire to keep related functionality in the same
> place. 


I am not too much concerned about the waste of memory, but I am a bit concerned about hard-coding the DcaTxop and EdcaTxopN's in WifiMac, so that all class will have the same architecture. 

I agree that the standard says that the architecture enforced by Dean's patch (one DcaTxop and 4 EdcaTxopN) is *the* architecture. See IEEE 802.11-2007  Figure 9-1 "MAC architecture" and Figure 9-17 "Reference implementation model". 
(Well, HCCA and PCF are missing, but that's not the point: we could easily add them to the architecture if someone developed them)

Still, there might be interest in implementing different architecture, for example:

1)  Wireless Access in Vehicular Environments (WAVE) has a different architecture, see IEEE Std. 1609.4-2006, Figure 4 "Reference architecture of the MAC with channel coordination"

2) eventual custom modifications of Wifi that people might desire to implement


To summarize, there is a tradeoff between flexibility and simplicity. The current code has a more abstract interface but lot of duplication. The proposed patch simplifies code a lot, but focuses strictly on supporting the current MAC implementations. I think the key point that we need to understand is whether we are interested in supporting these feature at this stage or not.

Created attachment 968 [details]
WAVE MAC architecture

For your reference, here is a picture of the WAVE MAC architecture.

(In reply to comment #13)

Thanks for the comments, Nicola. This is certainly the discussion we need to have in consideration of the proposed change.

> Still, there might be interest in implementing different architecture, for
> example:
> 
> 1)  Wireless Access in Vehicular Environments (WAVE) has a different
> architecture, see IEEE Std. 1609.4-2006, Figure 4 "Reference architecture of
> the MAC with channel coordination"

Logically, the WAVE architecture does indeed require distinct channel access functions for use on the Control and Service Channels, but it is significant that simultaneous operation on CCH and SCH does not occur - TDM is used. For this reason, I think it quite likely that practical implementations, in order to keep costs reasonable, will simple reconfigure the five 802.11-defined channel access functions at the same time as the necessary PHY channel switch is occurring.

While the nature of likely practical implementations need not influence our approach to producing a simulation model, this does allude to one way the the WifiMac architecture proposed herein could readily support a derived class that is attempting to implement WAVE.

Another way to do it would be to use the EDCA queues that WifiMac provides to cater for one of the CCH or SCH, and simply instantiate another set of EdcaTxopN's in the derived class to cater for the other channel.

Either way, I am arguing that the proposed approach provides well for a forthcoming WAVE model.

> 2) eventual custom modifications of Wifi that people might desire to implement

I don't think anything in the proposed WifiMac is likely to restrict options in a derived class; it is always possible, in principle, to ignore the infrastructure that the WifiMac base attempts to provide and go it alone.

That said, I would note that there is only a certain amount of customisation one can undertake before we are no longer talking about a "Wi-Fi" (or even, IEEE 802.11-based) MAC. I expect that ability to customise is likely to be restricted by the logically lower-layer classes (e.g., MacLow, MacTx/RxMiddle, WifiPhy and derived classes) before the content of WifiMac causes problems.

Hi Dean,

thank you for your reply. Some further comments below.

(In reply to comment #15)
> Logically, the WAVE architecture does indeed require distinct channel access
> functions for use on the Control and Service Channels, but it is significant
> that simultaneous operation on CCH and SCH does not occur - TDM is used. For
> this reason, I think it quite likely that practical implementations, in order
> to keep costs reasonable, will simple reconfigure the five 802.11-defined
> channel access functions at the same time as the necessary PHY channel switch
> is occurring.

I am not sure this would be good approach, because you would have to flush the queues whenever you switch from CCH to SCH and vice-versa. 
 

> Another way to do it would be to use the EDCA queues that WifiMac provides to
> cater for one of the CCH or SCH, and simply instantiate another set of
> EdcaTxopN's in the derived class to cater for the other channel.

Ok this could work in practice.


> Either way, I am arguing that the proposed approach provides well for a
> forthcoming WAVE model.

I agree that it is possible to implement it, but frankly I don't like the way it would be implemented, it seems to me that this way the WAVE device would be implemented as an hack over existing code rather than with a clean and readable design. (more details below)


> I don't think anything in the proposed WifiMac is likely to restrict options in
> a derived class; it is always possible, in principle, to ignore the
> infrastructure that the WifiMac base attempts to provide and go it alone.

Again I agree that it is possible, but I don't like it. For example, you might end up with a WaveWifiMac which doesn't use at all the EDCA queues defined in WifiMac, and which instead defines its own queues. I think this is detrimental to code readability and maintainability.


> That said, I would note that there is only a certain amount of customisation
> one can undertake before we are no longer talking about a "Wi-Fi" (or even,
> IEEE 802.11-based) MAC. I expect that ability to customise is likely to be
> restricted by the logically lower-layer classes (e.g., MacLow, MacTx/RxMiddle,
> WifiPhy and derived classes) before the content of WifiMac causes problems.

I agree! In fact, the exact type and amount of restriction imposed by the lower-layer classes is defined by the current abstract WifiMac interface. I think that having such an abstract interface is good since it exactly tells you how far you can go with customization. The more I think about it, the more I am convinced that the one and only think I really don't like of your patch is that you're throwing away that abstract interface. Let me stress that I am still convinced that the rest of the patch is a very good contribution (code is better organized, much less duplication, etc.). 

So, can we rescue the abstract WifiMac interface? Here is my proposal:

1) you leave the existing abstract WifiMac class as-is

2) you take the WifiMac class in your patch, rename it to RegularWifiMac (or CommonWifiMac, or VanillaWifiMac... whatever), and make it inherit from the abstract WifiMac

3) the rest of your patch stays as-is, you just need to make all the *WifiMac classes inherit from the new RegularWifiMac instead of WifiMac 

4) Wave implementation could be done by having two RegularWifiMac instances, one for SCH and one for CCH, contained in a wrapper class WaveWifiMac that inherits directly from the abstract WifiMac

5) people that want to develop their own MAC can choose between inheriting from RegularWifiMac (if they plan to reuse a lot of functionality), or inheriting directly from WifiMac (if they need a highly customized solution).

What do you think?

(In reply to comment #16)
> Here is my proposal:
[snip]
> What do you think?

I think this is a good proposal. I will knock something up along these lines in the next few days, and offer for review.

(In reply to comment #17)
> I will knock something up along these lines in the next few
> days, and offer for review.

So I've done this and the new code is available under Issue 1926043.

It occurred to me while doing this, that WifiMac isn't actually as abstract as you might like it to be. Specifically:

 - It contains Block Ack functionality, including (essentially) empty implementations of methods Set/GetCompressedBlockAckTimeout(), with comments saying that these must be reimplemented in derived classes that use them. Should these shift to the new RegularWifiMac?

 - It defines methods (some pure virtual) that deal with SSIDs and BSSIDs - things that are irrelevant to, e.g., a derived class attempting to model WAVE. Should these shift to the new RegularWifiMac?

 - It defines and implements methods to deal with CCH and SCH - things that are irrelevant to all bar classes attempting to model WAVE. For now these have nowhere else to go currently, so I can readily understand why they are here - when a WAVE derivative of WifiMac comes along can I assume they'll shift there?

 - I also suspect that methods like Configure80211a/b/_10MHz/_5MHz and ConfigureDcf should move up to RegularWifiMac, as they are specific to configuration for as-defined-in-the-standard operation.

Do you have thoughts on all this, Nicola (or other)?

(In reply to comment #18)

> 
>  - It defines methods (some pure virtual) that deal with SSIDs and BSSIDs -
> things that are irrelevant to, e.g., a derived class attempting to model WAVE.
> Should these shift to the new RegularWifiMac?
> 

These functions need to be changed for virtual access point case (see Issue  2104052). It seems to me that it is better to make changes directly in ap-wifi-mac.cc. However, I have to do it in regular-wifi-mac.cc now. Can we move these functions to ap-wifi-mac.cc?

(In reply to comment #19)
> (In reply to comment #18)
> >  - It defines methods (some pure virtual) that deal with SSIDs and BSSIDs -
> > things that are irrelevant to, e.g., a derived class attempting to model WAVE.
> > Should these shift to the new RegularWifiMac?
> 
> These functions need to be changed for virtual access point case (see Issue 
> 2104052). It seems to me that it is better to make changes directly in
> ap-wifi-mac.cc. However, I have to do it in regular-wifi-mac.cc now. Can we
> move these functions to ap-wifi-mac.cc?

I had a go at pushing these functions ({Get,Set}{Ssid,Bssid}, and the Ssid attribute) down the tree to ApWifiMac and peers, but it started to look so much like the duplicated code that I was trying get rid of in the first place that I got cold feet and gave up.

What is the generalised requirement for VAPs? Is it the case that the Virtualised AP needs to be told of N SSIDs to use, and will choose the corresponding BSSIDs based on it's MAC address?

If so, then could this be supported in a less intrusive (with respect to altering interfaces that correspond to stuff that is less proprietary than VAP) manner by simply adding API to ApWifiMac (or perhaps a new VapWifiMac?) that allows "additional" SSIDs to be specified.

Thoughts?

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > >  - It defines methods (some pure virtual) that deal with SSIDs and BSSIDs -
> > > things that are irrelevant to, e.g., a derived class attempting to model WAVE.
> > > Should these shift to the new RegularWifiMac?
> > 
> > These functions need to be changed for virtual access point case (see Issue 
> > 2104052). It seems to me that it is better to make changes directly in
> > ap-wifi-mac.cc. However, I have to do it in regular-wifi-mac.cc now. Can we
> > move these functions to ap-wifi-mac.cc?
> 
> I had a go at pushing these functions ({Get,Set}{Ssid,Bssid}, and the Ssid
> attribute) down the tree to ApWifiMac and peers, but it started to look so much
> like the duplicated code that I was trying get rid of in the first place that I
> got cold feet and gave up.
> 
> What is the generalised requirement for VAPs? Is it the case that the
> Virtualised AP needs to be told of N SSIDs to use, and will choose the
> corresponding BSSIDs based on it's MAC address?
> 
> If so, then could this be supported in a less intrusive (with respect to
> altering interfaces that correspond to stuff that is less proprietary than VAP)
> manner by simply adding API to ApWifiMac (or perhaps a new VapWifiMac?) that
> allows "additional" SSIDs to be specified.
> 
> Thoughts?

Actually, I was suggesting that you simply overload Get,Set{Ssid,Bssid} in ApWifiMac with same implementation while you don't need to do that for others. I will then apply the same patch to these functions in ApWifiMac instead of regular-wifi-mac.cc.

Another thought is that I could make a subclass from ApWifiMac to just overload these two functions. But, it seems easier to do it at ApwifiMac.

Hello,

This is the follow up to the mail I'd sent earlier today:
http://mailman.isi.edu/pipermail/ns-developers/2010-October/008361.html

As per Nicola and Dean's suggestion, I had a look through the code to see how ns-3-click-mac would fit into the picture. Simply speaking, here's how things work with Click when sending a packet down the stack:

1) At L3, an Ipv4L3ClickProtocol instance passes a packet on to Click for routing. Click then returns the packet to Ipv4L3ClickProtocol after processing it. The packet will have the MAC layer headers attached to it.

2) How ns-3-click handles it:
A constraint is imposed on the user. The user should avoid using Click elements that encapsulate MAC specific headers in his Click graph. The way Click works, this results in a packet with an ethernet header being received via Click's interface (and subsequently, being received at Ipv4L3ClickProtocol), regardless of what kind of device is being used. The ethernet header is then stripped off, and the packet is sent down the stack via NetDevice::Send (). ns-3's MAC layer code then handles the rest.

3) How ns-3-click-mac ought to handle it:
There should be no restriction on the user in terms of the use of Click. So any packet received via a Click interface, which is ready *with any L2 headers attached*, should be passed down appropriately.

I'm quite inexperienced as far as L2 is concerned, so I'm a little hesitant to write a lot of custom code to make ns-3-click-mac fit cleanly. The ns-2 implementation wrote an entire Click specific 802.11 MAC! This is something I want to avoid doing. What I'd really like is something along the lines of this which I can do from L3:

// Receive Packet from Click at Ipv4L3ClickProtocol
Ptr<NetDevice> netdev = GetNetDevice (ifid);
if (DynamicCast<WifiNetDevice> (netdev))
  {
    WifiMacHeader hdr;
    packet->RemoveHeader (hdr);
    DynamicCast<WifiNetDevice> (netdev)->GetMac ()->Enqueue (packet, hdr);
  }

This would execute the following piece of code that all *-wifi-mac.cc files seem to be using:

if (m_qosSupported)
  {
    NS_ASSERT (tid < 8);
    m_edca[QosUtilsMapTidToAc (tid)]->Queue (packet, hdr);
  }
else
  {
    m_dca->Queue (packet, hdr);
  }

The above, I believe, should do the trick. Hope I'm not being too vague here. And excellent job on the patch Dean. :)

(In reply to comment #21)
> Actually, I was suggesting that you simply overload Get,Set{Ssid,Bssid} in
> ApWifiMac with same implementation while you don't need to do that for others.
> I will then apply the same patch to these functions in ApWifiMac instead of
> regular-wifi-mac.cc.
> 
> Another thought is that I could make a subclass from ApWifiMac to just overload
> these two functions. But, it seems easier to do it at ApwifiMac.

I'm afraid I'm a bit hard of understanding, here, and I apologise for this, but...

If all you want to do is to reimplement RegularWifiMac::GetSsid() and RegularWifiMac::SetSsid() in ApWifiMac, then what is currently stopping you from doing so?

Hi Lalith,

(In reply to comment #22)
> 3) How ns-3-click-mac ought to handle it:
> There should be no restriction on the user in terms of the use of Click. 
> So any packet received via a Click interface, which is ready *with any L2 
> headers attached*, should be passed down appropriately.

It is not clear to me what a Click-based MAC implementation does. I tried to have a look at the available wifi Click elements
http://read.cs.ucla.edu/click/elements#wifi

are the above elements what you want to integrate with ns-3?
From a quick look I get the impression that these wifi elements basically re-implement all the wifi "MAC High" functionality (association & stuff alike). If this is true, then in order to implement your functionality it is probably better to inherit from WifiMac (rather than from RegularWifiMac). But to be sure we would need to know exactly what wifi functionality is provided by Click.

Also, you'll probably have to do something with WifiNetDevice, since the LLC/SNAP (ethernet) header is added there.

(In reply to comment #24)
> If this is true, then in order to implement your functionality it is probably
> better to inherit from WifiMac (rather than from RegularWifiMac). 

mmmmh... actually I'm probably wrong here... but let's discuss first about what Click actually does.

> 
> If all you want to do is to reimplement RegularWifiMac::GetSsid() and
> RegularWifiMac::SetSsid() in ApWifiMac, then what is currently stopping you
> from doing so?

Nothing. I just want to make sure that this is ok for everyone.

I just gave another review to Dean's code:
http://codereview.appspot.com/1926043/

If the beaconing issue is satisfactorily addressed, the patch is ready to merge for me. Still, we need Kirill to approve the changes to the mesh code.

Just so that I don't confuse anyone any further, I'll just explain in short how a packet travels down the stack in ns-3-click :):

   Application
        |
        |
        V
    Transport
        |
        |
        V
  Ipv4L3ClickProtocol ------> Click
                                |
                                |
                                V
                        (Traverse Click Graph)
                                |
                                |
                                V
                (exit via a Click Interface, say eth0)
                                |
  Ipv4L3ClickProtocol  <--------
 (Packet with L2 frame received at L3)
        |
        |
        V
 (Remove L2 Header and call NetDevice::Send())
        |
        |
        V
    NetDevice
(L2 headers attached after this)
 (ns-3 MAC layer code is used)
        

The above has the constraint that users cannot use Mac specific elements in their Click graph, since the ns-3 code strips off the L2 headers that come with the packets from Click, and send them down the stack, thus using ns-3's Mac code.

If we wanted to allow users to use Click's Mac layer elements, then we'd need to somehow bypass the MacHigh model itself, and be able to queue packets at m_dca/m_edca directly. And as I'll explain below, it would be better if the RegaularWifiMac class provided a method that allows external routers to directly enqueue at m_dca/m_edca.

In response to Nicola's comment:

> (In reply to comment #22)
> 
> It is not clear to me what a Click-based MAC implementation does. I tried to
> have a look at the available wifi Click elements
> http://read.cs.ucla.edu/click/elements#wifi
> 
> are the above elements what you want to integrate with ns-3?

Yes. With the current ns-3-click code, we basically tell the user _not_ to use any such Wifi specific element (mainly stuff like Wifi Encap: http://read.cs.ucla.edu/click/elements/wifiencap). So this defaults to a standard ethernet frame. Now in the ns-3 code, since we assume that the ns-3-click user is following the instructions as prescribed, we try and remove an ethernet header from the packet we receive from a Click interface (think of it like downgrading an L2 packet into an L3 one all over again), and then sending it down via the ns-3 NetDevice using NetDevice::Send(). From here on, it's the normal ns-3 path of a packet. This causes the ns-3 code to attach the LLC/SNAP header and then the WifiMacHeader as the packet goes through the NetDevice and WifiMac code.

> From a quick look I get the impression that these wifi elements basically
> re-implement all the wifi "MAC High" functionality (association & stuff alike).
> If this is true, then in order to implement your functionality it is probably
> better to inherit from WifiMac (rather than from RegularWifiMac). But to be
> sure we would need to know exactly what wifi functionality is provided by
> Click.

Yes, you're correct. But Click users usually have the need to try out their own custom MAC layer elements (beyond the normal association and stuff, which the ns-3 code does provide). To address such needs, it would be much easier if we had something like the below defined within RegularWifiMac:

void
RegularWifiMac::Enqueue (Packet packet, WifiMacHeader hdr)
{
  if (m_qosSupported)
    {
      NS_ASSERT (tid < 8);
      m_edca[QosUtilsMapTidToAc (tid)]->Queue (packet, hdr);
    }
  else
    {
      m_dca->Queue (packet, hdr);
    }
}

The advantage here is that we needn't write another WifiHigh model for external routers like Click, wherein a packet going down the stack is received from the external router with an L2 header on. We simply perform the following from within L3 (in my specific case, Ipv4L3ClickProtocol):

// Receive Packet going down the stack from Click at Ipv4L3ClickProtocol
Ptr<NetDevice> netdev = GetNetDevice (ifid);
if (DynamicCast<WifiNetDevice> (netdev))
  {
    WifiMacHeader hdr;
    packet->RemoveHeader (hdr);
    DynamicCast<WifiNetDevice> (netdev)->GetMac ()->Enqueue (packet, hdr); 
  }

Over here, we're not concerned with what kind of Wifi Mac we have underneath. It'll work seamlessly with {Q}Adhoc, {Q}AP or {Q}STA. The user can run a Click graph for a Click router running in (say) Adhoc mode _as it is_ on top of the ns-3 node. The current framework that we have requires the user to remove the Wifi specific Encapsulation/Decapsulation elements from his Click graph for the same, and it prevents the user from trying out his own custom MAC layer elements.

Furthermore, if we add another WifiHigh model by subclassing from WifiMac or RegularWifiMac (let's call it WifiExternalHigh), we'll also need to modify the Wifi related helpers to create/add WifiExternalHigh to the node instead of the normal ones when the node is supposed to use an external router, which again, isn't very clean in my opinion.

> 
> Also, you'll probably have to do something with WifiNetDevice, since the
> LLC/SNAP (ethernet) header is added there.

I've pinged Ruben regarding the discussion here. I'm hoping that he'll join soon.

Thanks all of you for your time! :)

(In reply to comment #28)
> If we wanted to allow users to use Click's Mac layer elements, then we'd need
> to somehow bypass the MacHigh model itself, and be able to queue packets at
> m_dca/m_edca directly. 

So this means that when Click's wifi elements are used, the MacHigh functions (association, beaconing, etc) are implemented in Click using elements such as:
http://read.cs.ucla.edu/click/elements/beaconscanner
http://read.cs.ucla.edu/click/elements/associationrequester
http://read.cs.ucla.edu/click/elements/beaconsource
am I right?



> But Click users usually have the need to try out their own
> custom MAC layer elements (beyond the normal association and stuff, which the
> ns-3 code does provide).

The references that I provided above actually suggest that the "association stuff" provided by ns-3 is not to be used when Click Wifi MAC elements are used, am I right?


> To address such needs, it would be much easier if we
> had something like the below defined within RegularWifiMac:
> 
> void
> RegularWifiMac::Enqueue (Packet packet, WifiMacHeader hdr)
> {
>   if (m_qosSupported)
>     {
>       NS_ASSERT (tid < 8);
>       m_edca[QosUtilsMapTidToAc (tid)]->Queue (packet, hdr);
>     }
>   else
>     {
>       m_dca->Queue (packet, hdr);
>     }
> }
> 
> The advantage here is that we needn't write another WifiHigh model for external
> routers like Click, wherein a packet going down the stack is received from the
> external router with an L2 header on. We simply perform the following from
> within L3 (in my specific case, Ipv4L3ClickProtocol):
> 
> // Receive Packet going down the stack from Click at Ipv4L3ClickProtocol
> Ptr<NetDevice> netdev = GetNetDevice (ifid);
> if (DynamicCast<WifiNetDevice> (netdev))
>   {
>     WifiMacHeader hdr;
>     packet->RemoveHeader (hdr);
>     DynamicCast<WifiNetDevice> (netdev)->GetMac ()->Enqueue (packet, hdr); 
>   }


I am not sure that this is a good approach, you're making an L3 implementation depend on the fact that you have a specific L2 implementation below it. I would like the Ipv4 developers/maintainers to comment on this.


> 
> Over here, we're not concerned with what kind of Wifi Mac we have underneath.
> It'll work seamlessly with {Q}Adhoc, {Q}AP or {Q}STA. The user can run a Click
> graph for a Click router running in (say) Adhoc mode _as it is_ on top of the
> ns-3 node. 

I think in this way you would have two MacHigh implementations running at the same time, one in Click and one in ns-3. In my opinion it wouldn't work at all.

Based on my current understanding, I suggest to implement both a ClickWifiNetDevice and a ClickWifiMac:

ClickWifiNetDevice will receive from L3 a packet that already has the L2 header. Therefore, unlike the current WifiNetDevice, it will *not* add the LLC/SNAP header. Then it will call a proper method of ClickWifiMac in order for it to process the packet with the already exisiting WifiMacHeader hdr. I think it is fine if you define your custom method for this purpose, given your specific needs.

ClickWifiMac will receive the packet with the WifiMacHeader, and directly enqueue it, without implementing any association, beaconing & stuff alike. This functionality is expected to be implemented in Click. 

Now, one question is whether ClickWifiMac will inherit from RegularWifiMac or directly from WifiMac. I think the most critical issue is whether ClickWifiMac can actually know the type of station (AP, STA, ADHOC, MESH). To my understanding, this depends on what elements are actually included in the Click graph. If you can have this information available, then you can inherit from RegularWifiMac and provide QoS support. On the other hand, you could have a simpler solution by dropping QoS support, inheriting from WifiMac directly and copying only the non-QoS code from regular-wifi-mac.cc; in this latter way, your ClickWifiMac implementation doesn't need to know the type of station which is implemented in the Click graph.



> Furthermore, if we add another WifiHigh model by subclassing from WifiMac or
> RegularWifiMac (let's call it WifiExternalHigh), we'll also need to modify the
> Wifi related helpers to create/add WifiExternalHigh to the node instead of the
> normal ones when the node is supposed to use an external router, which again,
> isn't very clean in my opinion.

I agree that you will probably have to develop specific helpers for the ClickWifi stuff. In my opinion, this is a good solution. I think the user needs to be aware that he is using the WifiMacHigh implemented in Click, rather than the one implemented in ns-3.

(In reply to comment #29)
> (In reply to comment #28)
> > If we wanted to allow users to use Click's Mac layer elements, then we'd need
> > to somehow bypass the MacHigh model itself, and be able to queue packets at
> > m_dca/m_edca directly. 
> 
> So this means that when Click's wifi elements are used, the MacHigh functions
> (association, beaconing, etc) are implemented in Click using elements such as:
> http://read.cs.ucla.edu/click/elements/beaconscanner
> http://read.cs.ucla.edu/click/elements/associationrequester
> http://read.cs.ucla.edu/click/elements/beaconsource
> am I right?

Right!

> 
> 
> 
> > But Click users usually have the need to try out their own
> > custom MAC layer elements (beyond the normal association and stuff, which the
> > ns-3 code does provide).
> 
> The references that I provided above actually suggest that the "association
> stuff" provided by ns-3 is not to be used when Click Wifi MAC elements are
> used, am I right?


Right again.

> 
> 
> > To address such needs, it would be much easier if we
> > had something like the below defined within RegularWifiMac:
> > 
> > void
> > RegularWifiMac::Enqueue (Packet packet, WifiMacHeader hdr)
> > {
> >   if (m_qosSupported)
> >     {
> >       NS_ASSERT (tid < 8);
> >       m_edca[QosUtilsMapTidToAc (tid)]->Queue (packet, hdr);
> >     }
> >   else
> >     {
> >       m_dca->Queue (packet, hdr);
> >     }
> > }
> > 
> > The advantage here is that we needn't write another WifiHigh model for external
> > routers like Click, wherein a packet going down the stack is received from the
> > external router with an L2 header on. We simply perform the following from
> > within L3 (in my specific case, Ipv4L3ClickProtocol):
> > 
> > // Receive Packet going down the stack from Click at Ipv4L3ClickProtocol
> > Ptr<NetDevice> netdev = GetNetDevice (ifid);
> > if (DynamicCast<WifiNetDevice> (netdev))
> >   {
> >     WifiMacHeader hdr;
> >     packet->RemoveHeader (hdr);
> >     DynamicCast<WifiNetDevice> (netdev)->GetMac ()->Enqueue (packet, hdr); 
> >   }
> 
> 
> I am not sure that this is a good approach, you're making an L3 implementation
> depend on the fact that you have a specific L2 implementation below it. I would
> like the Ipv4 developers/maintainers to comment on this.

Actually, this will go into a specific L3 I've written for Click named Ipv4L3ClickProtocol. Tom had suggested I write this class and keep it separate from Ipv4L3Protocol. It has already been reviewed.

> 
> 
> > 
> > Over here, we're not concerned with what kind of Wifi Mac we have underneath.
> > It'll work seamlessly with {Q}Adhoc, {Q}AP or {Q}STA. The user can run a Click
> > graph for a Click router running in (say) Adhoc mode _as it is_ on top of the
> > ns-3 node. 
> 
> I think in this way you would have two MacHigh implementations running at the
> same time, one in Click and one in ns-3. In my opinion it wouldn't work at all.
> 
> Based on my current understanding, I suggest to implement both a
> ClickWifiNetDevice and a ClickWifiMac:
> 
> ClickWifiNetDevice will receive from L3 a packet that already has the L2
> header. Therefore, unlike the current WifiNetDevice, it will *not* add the
> LLC/SNAP header. Then it will call a proper method of ClickWifiMac in order for
> it to process the packet with the already exisiting WifiMacHeader hdr. I think
> it is fine if you define your custom method for this purpose, given your
> specific needs.
> 
> ClickWifiMac will receive the packet with the WifiMacHeader, and directly
> enqueue it, without implementing any association, beaconing & stuff alike. This
> functionality is expected to be implemented in Click. 
> 
> Now, one question is whether ClickWifiMac will inherit from RegularWifiMac or
> directly from WifiMac. I think the most critical issue is whether ClickWifiMac
> can actually know the type of station (AP, STA, ADHOC, MESH). To my
> understanding, this depends on what elements are actually included in the Click
> graph. If you can have this information available, then you can inherit from
> RegularWifiMac and provide QoS support. On the other hand, you could have a
> simpler solution by dropping QoS support, inheriting from WifiMac directly and
> copying only the non-QoS code from regular-wifi-mac.cc; in this latter way,
> your ClickWifiMac implementation doesn't need to know the type of station which
> is implemented in the Click graph.
> 

I agree with you here. It's much better to keep my changes separate by making a ClickWifiNetDevice and ClickWifiMac. This way, it would also be easier to leave it to the user which Mac he'd like to use as well. (use regular ns-3-click or ns-3-click-mac)

> 
> 
> > Furthermore, if we add another WifiHigh model by subclassing from WifiMac or
> > RegularWifiMac (let's call it WifiExternalHigh), we'll also need to modify the
> > Wifi related helpers to create/add WifiExternalHigh to the node instead of the
> > normal ones when the node is supposed to use an external router, which again,
> > isn't very clean in my opinion.
> 
> I agree that you will probably have to develop specific helpers for the
> ClickWifi stuff. In my opinion, this is a good solution. I think the user needs
> to be aware that he is using the WifiMacHigh implemented in Click, rather than
> the one implemented in ns-3.

True. I'll rethink my approach on this one.

I'll just compile the discussion here into a full mail and post on the mailing list so that others can pitch in as well. Thanks a lot Nicola for your input! :)

(In reply to comment #27)
> I just gave another review to Dean's code:
> http://codereview.appspot.com/1926043/
> 
> If the beaconing issue is satisfactorily addressed, the patch is ready to merge
> for me. Still, we need Kirill to approve the changes to the mesh code.

I've just updated the issue and development repository, so any further comments are very much welcome.

Summarizing the discussion on codereview:

+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

3) add a meaningful description to RELEASE_NOTES and CHANGES.html 

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

Hi!
I am very sorry for so late comment.
I agree with all changes in the mesh part of patchset.

Fixed in changesets 6673:ec22aa763e2d and 6674:52f8688d6d01 on ns-3-dev. Python bindings will require update before release.