802.11 PCF implementation

Implementation of 802.11 PCF in ns-3

[Rediet, 2018-03-29 16:40:22]

Hey Sébastien,

Here are my comments. Didn't have time to check with the standard though...

Cheers,

Rediet

https://codereview.appspot.com/333660043/diff/20001/src/wifi/doc/source/wifi-...
File src/wifi/doc/source/wifi-design.rst (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/doc/source/wifi-...
src/wifi/doc/source/wifi-design.rst:112: The ``ns3::Txop`` object is used high
MACs that are not QoS-enabled,
used by [missing] high

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-ma...
File src/wifi/model/ap-wifi-mac.cc (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:170: NS_LOG_FUNCTION (this);
No verification whether PCF-capable AP

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:210: if ((duration.GetMicroSeconds () % 1024) !=
0)
No verification whether PCF-capable AP

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:561: cfParameterSet.SetCFPDurRemainingUs
(GetCfpMaxDuration ().GetMicroSeconds ());
Is it normal that it's always set to max?

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:922: SendCfEnd ();
Why do we send a CF-END without sending another poll if its a CFP period?

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.cc:962: SendCfEnd ();
CF-END sent for all beacons? Is it because the DTIM period corresponds to beacon
period?

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-mac.h
File src/wifi/model/ap-wifi-mac.h (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.h:330: Time m_beaconInterval;                    
//!< Interval between beacons
No longer used

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/infrastruc...
File src/wifi/model/infrastructure-wifi-mac.h (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/infrastruc...
src/wifi/model/infrastructure-wifi-mac.h:78: #endif /* REGULAR_WIFI_MAC_H */
comment to update

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:935: m_currentTxop->GotCfEnd ();
Not notified if CF-ACK expected?
Shouldn't it better to just do a if (instead of else if)?

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1015: else if ((hdr.IsData () /*&& hdr.HasData ()*/)||
hdr.IsMgt ())
Should keep it if commented out?

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1085: if (m_cfAckInfo.expectCfAck && hdr.IsCfAck ())
This code can be refactored as method

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1395: Simulator::Schedule (GetPifs () +
m_phy->CalculateTxDuration (packet->GetSize (), txVector, m_phy->GetFrequency
()), &MacLow::CfPollTimeout, this);
Shouldn't it be SIFS?

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1857: m_cfAckInfo.address = Mac48Address ();
Any address will do the trick here?

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:3084: uint32_t maxMacFrameSize = MAX_MSDU_SIZE + 34;
// 34 bytes (FCS + 4 address fields included) /* + GetMaxMacHeaderSize ()*/;
Not always 34 bytes, depends upon presence of QoS control field and other fields
(namely HT/VHT/HE control fields...)

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:3084: uint32_t maxMacFrameSize = MAX_MSDU_SIZE + 34;
// 34 bytes (FCS + 4 address fields included) /* + GetMaxMacHeaderSize ()*/;
Can't find GetMaxMacHeaderSize in this file or in ns-3-dev...

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.h
File src/wifi/model/mac-low.h (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.h#...
src/wifi/model/mac-low.h:231: * \return the expected interval between two beacon
transmissions..
..

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-rx-mid...
File src/wifi/model/mac-rx-middle.h (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-rx-mid...
src/wifi/model/mac-rx-middle.h:135: Callback<void> m_pcfCallback; //!< pcf
callback
PCF

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/wifi-mac-h...
File src/wifi/model/wifi-mac-header.cc (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/wifi-mac-h...
src/wifi/model/wifi-mac-header.cc:272: m_duration = duration;
A verification on the max duration could be useful here

[S. Deronne, 2018-03-30 08:58:43]

Rediet,
Thanks for your review.

No problem wrt to the standard, I think Varun has more knowledge about PCF
standard, but I am also interested in other feedback (typos, coding style, code
improvement, ...) as you did.

I still have some questions, please see below.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/doc/source/wifi-...
File src/wifi/doc/source/wifi-design.rst (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/doc/source/wifi-...
src/wifi/doc/source/wifi-design.rst:112: The ``ns3::Txop`` object is used high
MACs that are not QoS-enabled,
nice spot, originally missing, I will directly fix it in ns-3-dev :-)

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-mac.h
File src/wifi/model/ap-wifi-mac.h (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.h:330: Time m_beaconInterval;                    
//!< Interval between beacons
Correct, thanks :-)
More globally speaking, any idea of a "tool" to spot some unused variables
defined in header files?

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/infrastruc...
File src/wifi/model/infrastructure-wifi-mac.h (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/infrastruc...
src/wifi/model/infrastructure-wifi-mac.h:78: #endif /* REGULAR_WIFI_MAC_H */
Correct, I will do.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:935: m_currentTxop->GotCfEnd ();
No, the TXOP is informed via GotAck or MissedAck.
I will anyway check if a cleaner solution is possible.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1015: else if ((hdr.IsData () /*&& hdr.HasData ()*/)||
hdr.IsMgt ())
No, I will clean it.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1085: if (m_cfAckInfo.expectCfAck && hdr.IsCfAck ())
Which part?

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1395: Simulator::Schedule (GetPifs () +
m_phy->CalculateTxDuration (packet->GetSize (), txVector, m_phy->GetFrequency
()), &MacLow::CfPollTimeout, this);
No, this checks whether a response has been received within PIFS duration.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:3084: uint32_t maxMacFrameSize = MAX_MSDU_SIZE + 34;
// 34 bytes (FCS + 4 address fields included) /* + GetMaxMacHeaderSize ()*/;
this is commented code, I have never defined this function.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:3084: uint32_t maxMacFrameSize = MAX_MSDU_SIZE + 34;
// 34 bytes (FCS + 4 address fields included) /* + GetMaxMacHeaderSize ()*/;
correct, this was the purpose of GetMaxMacHeader, but this was never implemented
in the end. I should see how to fix this.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.h
File src/wifi/model/mac-low.h (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.h#...
src/wifi/model/mac-low.h:231: * \return the expected interval between two beacon
transmissions..
I will clean this.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-rx-mid...
File src/wifi/model/mac-rx-middle.h (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-rx-mid...
src/wifi/model/mac-rx-middle.h:135: Callback<void> m_pcfCallback; //!< pcf
callback
OK

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/wifi-mac-h...
File src/wifi/model/wifi-mac-header.cc (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/wifi-mac-h...
src/wifi/model/wifi-mac-header.cc:272: m_duration = duration;
This is implicit by the uint16_t boundary.
Do you have a case in mind where it might be an issue?

[Tom Henderson, 2018-03-31 23:27:03]

I did not have time to go through everything yet (large patch) so just took a
first pass.  It seems like there are no PCF-specific unit tests or Sphinx .rst
updates?

https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
File examples/wireless/wifi-pcf.cc (right):

https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
examples/wireless/wifi-pcf.cc:25: #include "ns3/internet-module.h"
based on 'include less' principle of the other recent bug 2900, what about just
including only the necessary headers?

https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
examples/wireless/wifi-pcf.cc:29: 
Can you say more about the interesting combinations of options to try (what is
the expected output)?  For instance, what command line option combinations
highlight the benefit or drawback of PCF the most (when run in comparison with
DCF mode)?  Can UDP data rate be changed?  Does RTS/CTS enable have any big
effect?  What if UDP data is disabled; what kind of frames are sent between AP
and STAs?

https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
examples/wireless/wifi-pcf.cc:157: "PcfSupported", BooleanValue (enablePcf));
what about 'PcfMode' or 'PcfEnabled' (boolean value true or false)? 
'PcfSupported' seems to imply more that it is supported rather than enabled.?

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h
File src/wifi/model/txop.h (right):

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
src/wifi/model/txop.h:40: * This class implements the packet fragmentation and
Is this doxygen up to date?  It seems that there is more going on here than
fragmentation management.

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
src/wifi/model/txop.h:58: class Txop : public Object
I wonder about the name of this object; could it be 'ChannelAccessManager' or
something like that?  To me, TXOP is a temporal concept, not a functional
object.

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
src/wifi/model/txop.h:103: void SetLow (const Ptr<MacLow> low);
SetMacLow

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
src/wifi/model/txop.h:109: void SetManager (const Ptr<DcfManager> manager);
SetDcfManager

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
src/wifi/model/txop.h:151: Ptr<WifiMacQueue > GetQueue () const;
Set/GetWifiMacQueue

[S. Deronne, 2018-04-01 15:03:51]

Tom, thanks for your review, I agree this is a big patch.

There is a small update in wifi-design.rst, but not that much to say about PCF
because we kept the same structure. Now that you point it, I could maybe add a
short section there to explain how PCF is handled, would this be fine for you?

For the unit tests, this is actually a more general comment (at least in wifi)
that we have a poor coverage. I do not see a point in starting covering PCF
whereas DCF is almost not covered. One of my action point for the future is to
go through every single class and write unit tests, but this will take a lot of
time and this is not expected to be started before this summer. If you know
people willing to help in that, they are more than welcome. Note that I am busy
right now to make sure our examples are passing through at least 90% of our wifi
code, and, once this is done, focus will be on utests.

https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
File examples/wireless/wifi-pcf.cc (right):

https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
examples/wireless/wifi-pcf.cc:25: #include "ns3/internet-module.h"
Right remark Tom, but I have not refactored examples includes yet, this will be
in a next part for bug 2900.

https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
examples/wireless/wifi-pcf.cc:29: 
I will provide more information.

https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
examples/wireless/wifi-pcf.cc:157: "PcfSupported", BooleanValue (enablePcf));
I used the same principale as for Qos, Ht, Vht, ...
I agree with you, but then shouldn't we be consistent?

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h
File src/wifi/model/txop.h (right):

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
src/wifi/model/txop.h:40: * This class implements the packet fragmentation and
yes, but this is still the main purpose of this class. But you are right, this
is also responsible, for example, to handle ADDBA sessions. Do you want me to
extend a bit this Doxygen? (maybe directly in ns-3-dev, this is not PCF
specific).

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
src/wifi/model/txop.h:58: class Txop : public Object
I have to be honest, I did not like the name Txop, but I have not find something
good so far.
ChannelAccessManager is already better. And for Qos, you would then have
QosChannelAccessManager?

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
src/wifi/model/txop.h:103: void SetLow (const Ptr<MacLow> low);
OK

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
src/wifi/model/txop.h:109: void SetManager (const Ptr<DcfManager> manager);
OK

https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
src/wifi/model/txop.h:151: Ptr<WifiMacQueue > GetQueue () const;
OK

[Tom Henderson, 2018-04-01 16:16:26]

On 2018/04/01 15:03:51, S. Deronne wrote:
> Tom, thanks for your review, I agree this is a big patch.
> 
> There is a small update in wifi-design.rst, but not that much to say about PCF
> because we kept the same structure. Now that you point it, I could maybe add a
> short section there to explain how PCF is handled, would this be fine for you?

Yes, I recommend at least covering for users, if user wants to enable PCF, what
is the basic configuration to do so, and what available options are there to
change the configuration (e.g. attributes available), and what available traces
can show the PCF operation?

> 
> For the unit tests, this is actually a more general comment (at least in wifi)
> that we have a poor coverage. I do not see a point in starting covering PCF
> whereas DCF is almost not covered. One of my action point for the future is to
> go through every single class and write unit tests, but this will take a lot
of
> time and this is not expected to be started before this summer. If you know
> people willing to help in that, they are more than welcome. Note that I am
busy
> right now to make sure our examples are passing through at least 90% of our
wifi
> code, and, once this is done, focus will be on utests.

I realize that current test coverage is far from complete, but we have to start
somewhere (DCF is somewhat covered in dcf-manager-test.cc, but other aspects of
wifi module are not well covered, I agree).  I think it is hard to make time in
the future to go back and write unit tests because there end up being always
more pressing priorities and the code details fade from memory (actually, IMO,
it would be helpful for a GSoC student project to work on unit and system
testing of a module like wifi-- maybe by next year if this isn't yet done?).

In place of unit tests, however, so you can make progress and not block on the
merge, some statement of how you as developer tested the code could be inserted
in this section:
https://www.nsnam.org/docs/models/html/wifi-testing.html#mac-validation

such as "PCF operation can be verified by running 'wifi-pcf' as follows... and
enabling PCAP, and observing the frame exchange using Wireshark.  A good example
of the performance difference between PCF and DCF can be observed by running
'wifi-pcf' as follows..."

This could also be used to handle my other comment about documenting in
wifi-pcf.cc what is interesting to do with that example.

In summary, full coverage with unit tests would be ideal, but I will settle for
at least some statements in documentation to future users that are clear about
"here is a feature, here is how it is designed to be configured by users, and
here is what is known about the correctness of the implementation and how one
can check behavior."

> 
>
https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
> File examples/wireless/wifi-pcf.cc (right):
> 
>
https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
> examples/wireless/wifi-pcf.cc:25: #include "ns3/internet-module.h"
> Right remark Tom, but I have not refactored examples includes yet, this will
be
> in a next part for bug 2900.
> 
>
https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
> examples/wireless/wifi-pcf.cc:29: 
> I will provide more information.
> 
>
https://codereview.appspot.com/333660043/diff/80001/examples/wireless/wifi-pc...
> examples/wireless/wifi-pcf.cc:157: "PcfSupported", BooleanValue (enablePcf));
> I used the same principale as for Qos, Ht, Vht, ...
> I agree with you, but then shouldn't we be consistent?

I see a difference between 'Supported' and 'Enabled' (we have many enabled
examples like GreenfieldEnabled, ShortGuardEnabled, etc.).  HtSupported means
that those modes are supported but whether they are used is subject to
negotiation, while others are not capability-based.  

I don't know much about PCF and whether it is a negotiated feature (that some
STAs may choose to use DCF with the access point while others use PCF).  If it
is negotiated, then I would keep 'PcfSupported' but if not negotiated, I would
suggest to change to 'Enabled'.

By the way, if there is a possibility of mixed mode operation, I suggest to
expand the wifi-pcf.cc example to allow user to configure nWifis with PCF and
another separate nWifis with DCF.
 
> 
> https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h
> File src/wifi/model/txop.h (right):
> 
>
https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
> src/wifi/model/txop.h:40: * This class implements the packet fragmentation and
> yes, but this is still the main purpose of this class. But you are right, this
> is also responsible, for example, to handle ADDBA sessions. Do you want me to
> extend a bit this Doxygen? (maybe directly in ns-3-dev, this is not PCF
> specific).

Yes, please.

> 
>
https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
> src/wifi/model/txop.h:58: class Txop : public Object
> I have to be honest, I did not like the name Txop, but I have not find
something
> good so far.
> ChannelAccessManager is already better. And for Qos, you would then have
> QosChannelAccessManager?

It seems more natural to me.

> 
>
https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
> src/wifi/model/txop.h:103: void SetLow (const Ptr<MacLow> low);
> OK
> 
>
https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
> src/wifi/model/txop.h:109: void SetManager (const Ptr<DcfManager> manager);
> OK
> 
>
https://codereview.appspot.com/333660043/diff/80001/src/wifi/model/txop.h#new...
> src/wifi/model/txop.h:151: Ptr<WifiMacQueue > GetQueue () const;
> OK

[Rediet, 2018-04-03 07:24:01]

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-mac.h
File src/wifi/model/ap-wifi-mac.h (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.h:330: Time m_beaconInterval;                    
//!< Interval between beacons
On 2018/03/30 08:58:43, S. Deronne wrote:
> Correct, thanks :-)
> More globally speaking, any idea of a "tool" to spot some unused variables
> defined in header files?

No, I'm not aware of such a tool. Just read through the code.
But it'd be clearly quite useful (btw, compiler warning don't cover this?)

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1085: if (m_cfAckInfo.expectCfAck && hdr.IsCfAck ())
On 2018/03/30 08:58:43, S. Deronne wrote:
> Which part?

This one:
   if (m_cfAckInfo.expectCfAck && hdr.IsCfAck ())
     {
       m_cfAckInfo.expectCfAck = false;
       NS_ASSERT (m_currentTxop != 0);
       m_currentTxop->GotAck ();
     }

Found at least lines 921-926, 1085-1090, and 1095-1100 (maybe more occurrences)

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1395: Simulator::Schedule (GetPifs () +
m_phy->CalculateTxDuration (packet->GetSize (), txVector, m_phy->GetFrequency
()), &MacLow::CfPollTimeout, this);
On 2018/03/30 08:58:43, S. Deronne wrote:
> No, this checks whether a response has been received within PIFS duration.

Quickly checked with 802.11-2016 standard. Figure 10-23 indicates that SIFS is
expected between poll and Data/NoData with ack.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/wifi-mac-h...
File src/wifi/model/wifi-mac-header.cc (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/wifi-mac-h...
src/wifi/model/wifi-mac-header.cc:272: m_duration = duration;
On 2018/03/30 08:58:43, S. Deronne wrote:
> This is implicit by the uint16_t boundary.
> Do you have a case in mind where it might be an issue?

I thought that the value was set to 32 768 during CF period.

[Stefano Avallone, 2018-04-08 08:40:43]

I would like to make a proposal that is related to one aspect of this code
review (renaming DcaTxop and EdcaTxopN). My proposal is to merge DcfState into
DcaTxop. The main reasons are that it is really difficult to understand the
separation of roles between the two, they introduce a circular dependency and
many methods of one class just call the corresponding method of the other class.
So, I went ahead and experimented with such an approach. You can find the result
in the wifi branch of my github fork:

https://github.com/stavallo/ns-3-dev-git/commits/wifi

("wifi: Merge DcfState into DcaTxop" is the commit to look for)

After merging DcfState into DcaTxop, I would rename DcaTxop into Dcf and
EdcaTxopN into Edcaf (and maybe DcfManager into ChannelAccessManager as you
suggested).

What do you think?

Thanks,
Stefano

[Tom Henderson, 2018-04-08 15:24:23]

On 2018/04/08 08:40:43, Stefano Avallone wrote:
> I would like to make a proposal that is related to one aspect of this code
> review (renaming DcaTxop and EdcaTxopN). My proposal is to merge DcfState into
> DcaTxop. The main reasons are that it is really difficult to understand the
> separation of roles between the two, they introduce a circular dependency and
> many methods of one class just call the corresponding method of the other
class.
> So, I went ahead and experimented with such an approach. You can find the
result
> in the wifi branch of my github fork:
> 
> https://github.com/stavallo/ns-3-dev-git/commits/wifi
> 
> ("wifi: Merge DcfState into DcaTxop" is the commit to look for)
> 
> After merging DcfState into DcaTxop, I would rename DcaTxop into Dcf and
> EdcaTxopN into Edcaf (and maybe DcfManager into ChannelAccessManager as you
> suggested).
> 
> What do you think?

I think it would be clearer to do so.

[S. Deronne, 2018-04-11 20:00:30]

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-mac.h
File src/wifi/model/ap-wifi-mac.h (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/ap-wifi-ma...
src/wifi/model/ap-wifi-mac.h:330: Time m_beaconInterval;                    
//!< Interval between beacons
Compilation succeeded on my mac, I guess it might fail on my Linux.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc
File src/wifi/model/mac-low.cc (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/mac-low.cc...
src/wifi/model/mac-low.cc:1395: Simulator::Schedule (GetPifs () +
m_phy->CalculateTxDuration (packet->GetSize (), txVector, m_phy->GetFrequency
()), &MacLow::CfPollTimeout, this);
This timeout is intended to check whether a reply to a CF poll was received
within PIFS, in that case the next pollable STA should be polled.

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/wifi-mac-h...
File src/wifi/model/wifi-mac-header.cc (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/wifi-mac-h...
src/wifi/model/wifi-mac-header.cc:272: m_duration = duration;
Correct, so you want to check here that raw duration is lower or equal to that
value?

[S. Deronne, 2018-04-11 20:02:04]

On 2018/04/08 08:40:43, Stefano Avallone wrote:
> I would like to make a proposal that is related to one aspect of this code
> review (renaming DcaTxop and EdcaTxopN). My proposal is to merge DcfState into
> DcaTxop. The main reasons are that it is really difficult to understand the
> separation of roles between the two, they introduce a circular dependency and
> many methods of one class just call the corresponding method of the other
class.
> So, I went ahead and experimented with such an approach. You can find the
result
> in the wifi branch of my github fork:
> 
> https://github.com/stavallo/ns-3-dev-git/commits/wifi
> 
> ("wifi: Merge DcfState into DcaTxop" is the commit to look for)
> 
> After merging DcfState into DcaTxop, I would rename DcaTxop into Dcf and
> EdcaTxopN into Edcaf (and maybe DcfManager into ChannelAccessManager as you
> suggested).
> 
> What do you think?
> 
> Thanks,
> Stefano

Thanks, I also agree, I will have a look and port your change.

[Stefano Avallone, 2018-04-11 20:50:03]

FYI I just rebased the patch on the current master.

On Wed, Apr 11, 2018 at 10:02 PM, <sebastien.deronne@gmail.com> wrote:

> On 2018/04/08 08:40:43, Stefano Avallone wrote:
>
>> I would like to make a proposal that is related to one aspect of this
>>
> code
>
>> review (renaming DcaTxop and EdcaTxopN). My proposal is to merge
>>
> DcfState into
>
>> DcaTxop. The main reasons are that it is really difficult to
>>
> understand the
>
>> separation of roles between the two, they introduce a circular
>>
> dependency and
>
>> many methods of one class just call the corresponding method of the
>>
> other class.
>
>> So, I went ahead and experimented with such an approach. You can find
>>
> the result
>
>> in the wifi branch of my github fork:
>>
>
> https://github.com/stavallo/ns-3-dev-git/commits/wifi
>>
>
> ("wifi: Merge DcfState into DcaTxop" is the commit to look for)
>>
>
> After merging DcfState into DcaTxop, I would rename DcaTxop into Dcf
>>
> and
>
>> EdcaTxopN into Edcaf (and maybe DcfManager into ChannelAccessManager
>>
> as you
>
>> suggested).
>>
>
> What do you think?
>>
>
> Thanks,
>> Stefano
>>
>
> Thanks, I also agree, I will have a look and port your change.
>
> https://codereview.appspot.com/333660043/
>

[Rediet, 2018-04-12 07:32:04]

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/wifi-mac-h...
File src/wifi/model/wifi-mac-header.cc (right):

https://codereview.appspot.com/333660043/diff/20001/src/wifi/model/wifi-mac-h...
src/wifi/model/wifi-mac-header.cc:272: m_duration = duration;
On 2018/04/11 20:00:30, S. Deronne wrote:
> Correct, so you want to check here that raw duration is lower or equal to that
> value?

I think it would be better (just like what's done below for SetDuration, unless
redundant).

[S. Deronne, 2018-05-10 13:18:43]

I'd like to push PCF code in ns-3-dev before end of this week, since we are
close to the RC date and I prefer to have this to the mainstream in order to be
sure we can detect bug as early as possible.

I mainly need to update documentation still and to extend example (+add to
regression), but this could still be done next week at the latest in ns-3-dev.
The only blocking point right now is that we should decide on a name for the
Txop classes.