Refactor WifiPhy and WifiPhyStateHelper to eliminate circular declaration of classes

Refactor WifiPhy and WifiPhyStateHelper to eliminate circular declaration of classes

[S. Deronne, 2017-08-05 12:13:38]

https://codereview.appspot.com/329860043/diff/1/examples/wireless/wifi-ap.cc
File examples/wireless/wifi-ap.cc (right):

https://codereview.appspot.com/329860043/diff/1/examples/wireless/wifi-ap.cc#...
examples/wireless/wifi-ap.cc:73: PhyStateTrace (std::string context, Time start,
Time duration, enum WifiPhyState::State state)
I think the enum is not needed, i.e. WifiPhyState::State state is enough. This
comment could be repeated in all places in the code you changed this.

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-liste...
File src/wifi/model/wifi-phy-listener.h (right):

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-liste...
src/wifi/model/wifi-phy-listener.h:112: #endif /* WIFI_PHY_LISTENER_H */
Is there not other listeners that could also be moved to separated files?

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-state...
File src/wifi/model/wifi-phy-state-helper.cc (right):

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-state...
src/wifi/model/wifi-phy-state-helper.cc:28: class WifiPhyListener;
needed?

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-state.h
File src/wifi/model/wifi-phy-state.h (right):

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-state...
src/wifi/model/wifi-phy-state.h:70: };
I am a bit annoyed this class does not do anything, is there not another
solution?
For example, just define the enum, outside of a new class that is anyway not
used as such.

[ammo6818-vandals.uidaho.edu, 2018-02-12 04:02:16]

Patch updated to address review comments.

https://codereview.appspot.com/329860043/diff/1/examples/wireless/wifi-ap.cc
File examples/wireless/wifi-ap.cc (right):

https://codereview.appspot.com/329860043/diff/1/examples/wireless/wifi-ap.cc#...
examples/wireless/wifi-ap.cc:73: PhyStateTrace (std::string context, Time start,
Time duration, enum WifiPhyState::State state)
On 2017/08/05 12:13:37, S. Deronne wrote:
> I think the enum is not needed, i.e. WifiPhyState::State state is enough. This
> comment could be repeated in all places in the code you changed this.

updated files to remove enum

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-liste...
File src/wifi/model/wifi-phy-listener.h (right):

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-liste...
src/wifi/model/wifi-phy-listener.h:112: #endif /* WIFI_PHY_LISTENER_H */
On 2017/08/05 12:13:37, S. Deronne wrote:
> Is there not other listeners that could also be moved to separated files?

This was the only one that needed to me moved to compile under Visual Studio

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-state...
File src/wifi/model/wifi-phy-state-helper.cc (right):

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-state...
src/wifi/model/wifi-phy-state-helper.cc:28: class WifiPhyListener;
On 2017/08/05 12:13:37, S. Deronne wrote:
> needed?

this is a forward declaration of the WifiPhyListener class

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-state.h
File src/wifi/model/wifi-phy-state.h (right):

https://codereview.appspot.com/329860043/diff/1/src/wifi/model/wifi-phy-state...
src/wifi/model/wifi-phy-state.h:70: };
On 2017/08/05 12:13:37, S. Deronne wrote:
> I am a bit annoyed this class does not do anything, is there not another
> solution?
> For example, just define the enum, outside of a new class that is anyway not
> used as such.

this has been changed to not use a class and just declare an named enum

[S. Deronne, 2018-02-13 21:08:16]

OK in general, but please also invite Tom and Stefano to this review.

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-mac-q...
File src/wifi/model/wifi-mac-queue-item.h (right):

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-mac-q...
src/wifi/model/wifi-mac-queue-item.h:116: #ifndef _WIN32
Why do we need this? This change should also be checked with Stefano.

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-phy-l...
File src/wifi/model/wifi-phy-listener.cc (right):

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-phy-l...
src/wifi/model/wifi-phy-listener.cc:32: }
I don't like to have this empty file, how should we handle with pure interface
class? Maybe check this with Tom.

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-phy-s...
File src/wifi/model/wifi-phy-state.h (right):

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-phy-s...
src/wifi/model/wifi-phy-state.h:54: SLEEP,
check indent

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-phy-s...
src/wifi/model/wifi-phy-state.h:58: OFF
here also

[ammo6818-vandals.uidaho.edu, 2018-02-18 05:37:45]

Updates for review comments uploaded.

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-mac-q...
File src/wifi/model/wifi-mac-queue-item.h (right):

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-mac-q...
src/wifi/model/wifi-mac-queue-item.h:116: #ifndef _WIN32
On 2018/02/13 21:08:16, S. Deronne wrote:
> Why do we need this? This change should also be checked with Stefano.

this was required to compile on Visual Studio.

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-phy-l...
File src/wifi/model/wifi-phy-listener.cc (right):

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-phy-l...
src/wifi/model/wifi-phy-listener.cc:32: }
On 2018/02/13 21:08:16, S. Deronne wrote:
> I don't like to have this empty file, how should we handle with pure interface
> class? Maybe check this with Tom.

This is the only function because the rest are pure virtual.  if we change the
declaration of the destructor to pure virtual, all of the classes that use this
class as a base class will have to be modified.

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-phy-s...
File src/wifi/model/wifi-phy-state.h (right):

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-phy-s...
src/wifi/model/wifi-phy-state.h:54: SLEEP,
On 2018/02/13 21:08:16, S. Deronne wrote:
> check indent

fixed

https://codereview.appspot.com/329860043/diff/20001/src/wifi/model/wifi-phy-s...
src/wifi/model/wifi-phy-state.h:58: OFF
On 2018/02/13 21:08:16, S. Deronne wrote:
> here also

fixed

[S. Deronne, 2018-02-23 20:25:56]

I would still like to have this included in the next release, could you address
my comments by the end of this weekend?

https://codereview.appspot.com/329860043/diff/40001/src/wifi/helper/athstats-...
File src/wifi/helper/athstats-helper.cc (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/helper/athstats-...
src/wifi/helper/athstats-helper.cc:209: AthstatsWifiTraceSink::PhyRxOkTrace
(std::string context, Ptr<const Packet> packet, double snr, WifiMode mode, enum
WifiPreamble preamble)
"enum" not needed

https://codereview.appspot.com/329860043/diff/40001/src/wifi/helper/athstats-...
File src/wifi/helper/athstats-helper.h (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/helper/athstats-...
src/wifi/helper/athstats-helper.h:166: void PhyRxOkTrace (std::string context,
Ptr<const Packet> packet, double snr, WifiMode mode, enum WifiPreamble
preamble);
"enum" not needed

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-mac-q...
File src/wifi/model/wifi-mac-queue-item.cc (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-mac-q...
src/wifi/model/wifi-mac-queue-item.cc:33: NS_LOG_COMPONENT_DEFINE
("WifiMacQueueItem");
We should add it to EnableLogComponents

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-phy-l...
File src/wifi/model/wifi-phy-listener.cc (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-phy-l...
src/wifi/model/wifi-phy-listener.cc:31: {
I would then only have the header file, with destructeur (empty) implementation
in the header file itself.

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-phy-s...
File src/wifi/model/wifi-phy-state.h (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-phy-s...
src/wifi/model/wifi-phy-state.h:57: */
Alignment to be checked.

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-radio...
File src/wifi/model/wifi-radio-energy-model.cc (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-radio...
src/wifi/model/wifi-radio-energy-model.cc:470: NS_LOG_FUNCTION (this);
I would avoid adding logs on getters

[ammo6818-vandals.uidaho.edu, 2018-02-25 10:41:45]

Updated patch to address review comments uploaded.

https://codereview.appspot.com/329860043/diff/40001/src/wifi/helper/athstats-...
File src/wifi/helper/athstats-helper.cc (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/helper/athstats-...
src/wifi/helper/athstats-helper.cc:209: AthstatsWifiTraceSink::PhyRxOkTrace
(std::string context, Ptr<const Packet> packet, double snr, WifiMode mode, enum
WifiPreamble preamble)
On 2018/02/23 20:25:56, S. Deronne wrote:
> "enum" not needed

enum removed

https://codereview.appspot.com/329860043/diff/40001/src/wifi/helper/athstats-...
File src/wifi/helper/athstats-helper.h (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/helper/athstats-...
src/wifi/helper/athstats-helper.h:166: void PhyRxOkTrace (std::string context,
Ptr<const Packet> packet, double snr, WifiMode mode, enum WifiPreamble
preamble);
On 2018/02/23 20:25:56, S. Deronne wrote:
> "enum" not needed

enum removed

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-mac-q...
File src/wifi/model/wifi-mac-queue-item.cc (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-mac-q...
src/wifi/model/wifi-mac-queue-item.cc:33: NS_LOG_COMPONENT_DEFINE
("WifiMacQueueItem");
On 2018/02/23 20:25:56, S. Deronne wrote:
> We should add it to EnableLogComponents

Added

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-phy-l...
File src/wifi/model/wifi-phy-listener.cc (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-phy-l...
src/wifi/model/wifi-phy-listener.cc:31: {
On 2018/02/23 20:25:56, S. Deronne wrote:
> I would then only have the header file, with destructeur (empty)
implementation
> in the header file itself.

moved to header file and removed the source file

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-phy-s...
File src/wifi/model/wifi-phy-state.h (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-phy-s...
src/wifi/model/wifi-phy-state.h:57: */
On 2018/02/23 20:25:56, S. Deronne wrote:
> Alignment to be checked.

updated alignment

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-radio...
File src/wifi/model/wifi-radio-energy-model.cc (right):

https://codereview.appspot.com/329860043/diff/40001/src/wifi/model/wifi-radio...
src/wifi/model/wifi-radio-energy-model.cc:470: NS_LOG_FUNCTION (this);
On 2018/02/23 20:25:56, S. Deronne wrote:
> I would avoid adding logs on getters

removed on all getters

[S. Deronne, 2018-02-25 17:49:53]

https://codereview.appspot.com/329860043/diff/70001/examples/wireless/power-a...
File examples/wireless/power-adaptation-distance.cc (right):

https://codereview.appspot.com/329860043/diff/70001/examples/wireless/power-a...
examples/wireless/power-adaptation-distance.cc:350: wifi.SetRemoteStationManager
(manager, "DefaultTxPowerLevel", UintegerValue
(static_cast<uint64_t>(maxPower)), "RtsCtsThreshold", UintegerValue
(rtsThreshold));
why a cast? It seems not related to this review.

https://codereview.appspot.com/329860043/diff/70001/examples/wireless/power-a...
File examples/wireless/power-adaptation-interference.cc (right):

https://codereview.appspot.com/329860043/diff/70001/examples/wireless/power-a...
examples/wireless/power-adaptation-interference.cc:328: #endif
Please remove this

https://codereview.appspot.com/329860043/diff/70001/src/test/traced/traced-ca...
File src/test/traced/traced-callback-typedef-test-suite.cc (right):

https://codereview.appspot.com/329860043/diff/70001/src/test/traced/traced-ca...
src/test/traced/traced-callback-typedef-test-suite.cc:237: NS_UNUSED(a5);
This seems not related to this review (and NS_UNUSED should be avoided)

https://codereview.appspot.com/329860043/diff/70001/src/wifi/helper/wifi-help...
File src/wifi/helper/wifi-helper.cc (right):

https://codereview.appspot.com/329860043/diff/70001/src/wifi/helper/wifi-help...
src/wifi/helper/wifi-helper.cc:208: header.SetRate (static_cast<uint8_t>(rate));
This is not related to this review

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-mac-q...
File src/wifi/model/wifi-mac-queue-item.h (right):

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-mac-q...
src/wifi/model/wifi-mac-queue-item.h:120: #endif
@Tom, is this OK?

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-phy-s...
File src/wifi/model/wifi-phy-state-helper.h (right):

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-phy-s...
src/wifi/model/wifi-phy-state-helper.h:31: #include "wifi-tx-vector.h"
why all those additional headers?

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-phy-s...
File src/wifi/model/wifi-phy-state.h (right):

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-phy-s...
src/wifi/model/wifi-phy-state.h:53: */
wrong alignment

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-phy.cc
File src/wifi/model/wifi-phy.cc (right):

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-phy.c...
src/wifi/model/wifi-phy.cc:539: NS_LOG_FUNCTION (this << n);
we should wait for the patch to be pushed

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-phy.c...
src/wifi/model/wifi-phy.cc:1340: return
static_cast<uint8_t>(m_bssMembershipSelectorSet.size ());
not related to this change

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-phy.h
File src/wifi/model/wifi-phy.h (right):

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-phy.h...
src/wifi/model/wifi-phy.h:38: #include "wifi-phy-state-helper.h"
why all those headers?

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-radio...
File src/wifi/model/wifi-radio-energy-model.h (right):

https://codereview.appspot.com/329860043/diff/70001/src/wifi/model/wifi-radio...
src/wifi/model/wifi-radio-energy-model.h:29: #include
"ns3/wifi-tx-current-model.h"
is this needed??

[ammo6818-vandals.uidaho.edu, 2018-02-26 12:49:54]

Updated patch set uploaded to address review comments

[S. Deronne, 2018-02-26 13:33:59]

Thanks, please address remaining comments so that I can check with Tom if we can
push this tomorrow.

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-mac-...
File src/wifi/model/wifi-mac-queue-item.cc (right):

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-mac-...
src/wifi/model/wifi-mac-queue-item.cc:91: NS_OBJECT_TEMPLATE_CLASS_DEFINE(Queue,
WifiMacQueueItem);
I suggest to remove those ifdef, it looks more related to the  elimination of
Visual Studio compiler warnings than the refactoring of WifiPhyStateHelper.

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy-...
File src/wifi/model/wifi-phy-state-helper.h (right):

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy-...
src/wifi/model/wifi-phy-state-helper.h:31: #include "wifi-tx-vector.h"
are all those needed? please prefer forward declaration when possible.

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy-...
src/wifi/model/wifi-phy-state-helper.h:64: WifiPhyStateHelper ();
misalignment

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy.cc
File src/wifi/model/wifi-phy.cc (right):

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy....
src/wifi/model/wifi-phy.cc:34: #include "ns3/energy-source.h"
why?

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy.h
File src/wifi/model/wifi-phy.h (right):

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy....
src/wifi/model/wifi-phy.h:38: #include "wifi-phy-state-helper.h"
why??? We only remove code here

[ammo6818-vandals.uidaho.edu, 2018-02-27 12:31:17]

Updated patch to address review comments uploaded.

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-mac-...
File src/wifi/model/wifi-mac-queue-item.cc (right):

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-mac-...
src/wifi/model/wifi-mac-queue-item.cc:91: NS_OBJECT_TEMPLATE_CLASS_DEFINE(Queue,
WifiMacQueueItem);
On 2018/02/26 13:33:59, S. Deronne wrote:
> I suggest to remove those ifdef, it looks more related to the  elimination of
> Visual Studio compiler warnings than the refactoring of WifiPhyStateHelper.

removed.

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy-...
File src/wifi/model/wifi-phy-state-helper.h (right):

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy-...
src/wifi/model/wifi-phy-state-helper.h:31: #include "wifi-tx-vector.h"
On 2018/02/26 13:33:59, S. Deronne wrote:
> are all those needed? please prefer forward declaration when possible.

reduce to the minimum set required.

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy-...
src/wifi/model/wifi-phy-state-helper.h:64: WifiPhyStateHelper ();
On 2018/02/26 13:33:59, S. Deronne wrote:
> misalignment

fixed.

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy.cc
File src/wifi/model/wifi-phy.cc (right):

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy....
src/wifi/model/wifi-phy.cc:34: #include "ns3/energy-source.h"
On 2018/02/26 13:33:59, S. Deronne wrote:
> why?

eliminated.

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy.h
File src/wifi/model/wifi-phy.h (right):

https://codereview.appspot.com/329860043/diff/170001/src/wifi/model/wifi-phy....
src/wifi/model/wifi-phy.h:38: #include "wifi-phy-state-helper.h"
On 2018/02/26 13:33:59, S. Deronne wrote:
> why??? We only remove code here

reduce to the minimum set required to compile

[S. Deronne, 2018-02-27 12:42:05]

https://codereview.appspot.com/329860043/diff/190001/src/wifi/model/wifi-phy-...
File src/wifi/model/wifi-phy-state-helper.h (right):

https://codereview.appspot.com/329860043/diff/190001/src/wifi/model/wifi-phy-...
src/wifi/model/wifi-phy-state-helper.h:52: typedef Callback<void, Ptr<Packet>,
double> RxErrorCallback;
Why is it not removed from WifiPhy then?

[S. Deronne, 2018-02-28 09:59:22]

I am busy to update your patch, then I'll see with Tom how to proceed since I am
currently on travel.

[ammo6818-vandals.uidaho.edu, 2018-02-28 11:19:44]

Updates for last review comment uploaded.

https://codereview.appspot.com/329860043/diff/190001/src/wifi/model/wifi-phy-...
File src/wifi/model/wifi-phy-state-helper.h (right):

https://codereview.appspot.com/329860043/diff/190001/src/wifi/model/wifi-phy-...
src/wifi/model/wifi-phy-state-helper.h:52: typedef Callback<void, Ptr<Packet>,
double> RxErrorCallback;
On 2018/02/27 12:42:05, S. Deronne wrote:
> Why is it not removed from WifiPhy then?

removed.

[S. Deronne, 2018-03-02 21:21:52]

Thanks, I fixed some header inclusions and pushed to ns-3-dev.