Wifi code clanup/refactoring phase 1

Wifi code clanup/refactoring phase 1

[Tom Henderson, 2016-12-24 15:25:07]

In general, I support the patch.  I have a couple of requests and questions.

(request):  If this is not an intermediate step and is intended to be more
long-running API, can you complete the missing doxygen?

(request):  Most of the API changes are internal to the device, but there are a
couple that might have user-visible impact (e.g. no longer a WifiChannel type
exported by WifiNetDevice).  Can you provide summary of user-facing changes to
the API, for CHANGES.html?

(question/comment):  The current 'Listener' class framework seems questionable
to me as is.  I understand the original goal to reduce coupling between objects.
 However, as implemented now, these seem to be simple forwarding objects and are
not general.  Why not just give MacLow the EdcaTxopN pointers directly and avoid
indirecting through this listener class?

I think there are two possible paths forward but would appreciate your
perspective.  First, if we retain the current framework, I would try to make
more general by removing the need to pass a pointer to a forwarding object in
the constructor, and consider to make this base listening class pure virtual. 
Then, consider to provide more than one implementation, one that provides the
internal forwarding to EdcaTxopN, and one that either exports data to one or
more trace sources (perhaps the trace sources are members of WifiNetDevice) or
exports data to a test case.  

The second path forward is to refactor away the listener objects when there is
only one listener involved.

Even if only for testing purposes, it is useful to get state out of the lower
layers of these models, so I lean towards the first path outlined above, but if
you agree, I think we should probably try to steer this listener framework in
that direction by better documenting that this is the rationale for this
framework (and to provide a constructor that does not require an EdcaTxopN
pointer...).

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
File src/wifi/model/aggregation-capable-transmission-listener.h (right):

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
src/wifi/model/aggregation-capable-transmission-listener.h:42:
AggregationCapableTransmissionListener (EdcaTxopN * txop);
Can you change to Ptr<EdcaTxopN>?

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
src/wifi/model/aggregation-capable-transmission-listener.h:56: virtual void
BlockAckInactivityTimeout (Mac48Address originator, uint8_t tid);
why is every method in this class virtual but there are no subclasses?  Do you
expect specialization?  If so, explain the specialization strategy.  However, it
just seems to be a pure forwarding class so I don't see what might change here.

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
src/wifi/model/aggregation-capable-transmission-listener.h:69: virtual void
SetAmpdu (Mac48Address dest, bool enableAmpdu);
some methods in this class are missing Doxygen

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
src/wifi/model/aggregation-capable-transmission-listener.h:82: virtual uint16_t
GetNextSequenceNumberfor (WifiMacHeader *hdr) const;
missing capital F on For (also next method)

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
src/wifi/model/aggregation-capable-transmission-listener.h:138: EdcaTxopN
*m_txop;
can you change this to Ptr<EdcaTxopN>?

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/dca-txop.h
File src/wifi/model/dca-txop.h (right):

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/dca-txop.h#new...
src/wifi/model/dca-txop.h:142: Time GetTxopLimit (void) const;
still missing Doxygen

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/dca-txop.h#new...
src/wifi/model/dca-txop.h:338: * \return true if the curren fragment is the last
fragment,
current is misspelled

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/mac-low-transm...
File src/wifi/model/mac-low-transmission-listener.h (right):

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/mac-low-transm...
src/wifi/model/mac-low-transmission-listener.h:42: MacLowTransmissionListener
(DcaTxop *txop);
In my opinion, since you change DcaTxop to inherit from ns3::Object, you should
scrub all usages of raw pointers and instead use reference counted pointer.

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/mac-low.h
File src/wifi/model/mac-low.h (left):

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/mac-low.h#oldc...
src/wifi/model/mac-low.h:283: };
Agree to move these out of MacLow

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/spectrum-wifi-...
File src/wifi/model/spectrum-wifi-phy.cc (right):

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/spectrum-wifi-...
src/wifi/model/spectrum-wifi-phy.cc:119: return Ptr<Channel> (0);
should return m_channel

[S. Deronne, 2016-12-28 16:21:37]

On 2016/12/24 15:25:07, Tom Henderson wrote:
> In general, I support the patch.  I have a couple of requests and questions.
> 
> (request):  If this is not an intermediate step and is intended to be more
> long-running API, can you complete the missing doxygen?
> 

Ok.

> (request):  Most of the API changes are internal to the device, but there are
a
> couple that might have user-visible impact (e.g. no longer a WifiChannel type
> exported by WifiNetDevice).  Can you provide summary of user-facing changes to
> the API, for CHANGES.html?
> 

Ok, but there should not be much though.

> (question/comment):  The current 'Listener' class framework seems questionable
> to me as is.  I understand the original goal to reduce coupling between
objects.
>  However, as implemented now, these seem to be simple forwarding objects and
are
> not general.  Why not just give MacLow the EdcaTxopN pointers directly and
avoid
> indirecting through this listener class?

I guess this is also possible. If it looks better to you I can try to go with
pointers iso listeners and get rid of all listeners.
I indeed think it can be a better design, listeners can be a bit confusing

> 
> I think there are two possible paths forward but would appreciate your
> perspective.  First, if we retain the current framework, I would try to make
> more general by removing the need to pass a pointer to a forwarding object in
> the constructor, and consider to make this base listening class pure virtual. 
> Then, consider to provide more than one implementation, one that provides the
> internal forwarding to EdcaTxopN, and one that either exports data to one or
> more trace sources (perhaps the trace sources are members of WifiNetDevice) or
> exports data to a test case.  
> 
> The second path forward is to refactor away the listener objects when there is
> only one listener involved.
> 
> Even if only for testing purposes, it is useful to get state out of the lower
> layers of these models, so I lean towards the first path outlined above, but
if
> you agree, I think we should probably try to steer this listener framework in
> that direction by better documenting that this is the rationale for this
> framework (and to provide a constructor that does not require an EdcaTxopN
> pointer...).
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
> File src/wifi/model/aggregation-capable-transmission-listener.h (right):
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
> src/wifi/model/aggregation-capable-transmission-listener.h:42:
> AggregationCapableTransmissionListener (EdcaTxopN * txop);
> Can you change to Ptr<EdcaTxopN>?
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
> src/wifi/model/aggregation-capable-transmission-listener.h:56: virtual void
> BlockAckInactivityTimeout (Mac48Address originator, uint8_t tid);
> why is every method in this class virtual but there are no subclasses?  Do you
> expect specialization?  If so, explain the specialization strategy.  However,
it
> just seems to be a pure forwarding class so I don't see what might change
here.
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
> src/wifi/model/aggregation-capable-transmission-listener.h:69: virtual void
> SetAmpdu (Mac48Address dest, bool enableAmpdu);
> some methods in this class are missing Doxygen
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
> src/wifi/model/aggregation-capable-transmission-listener.h:82: virtual
uint16_t
> GetNextSequenceNumberfor (WifiMacHeader *hdr) const;
> missing capital F on For (also next method)
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
> src/wifi/model/aggregation-capable-transmission-listener.h:138: EdcaTxopN
> *m_txop;
> can you change this to Ptr<EdcaTxopN>?
> 
> https://codereview.appspot.com/311400043/diff/1/src/wifi/model/dca-txop.h
> File src/wifi/model/dca-txop.h (right):
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/dca-txop.h#new...
> src/wifi/model/dca-txop.h:142: Time GetTxopLimit (void) const;
> still missing Doxygen
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/dca-txop.h#new...
> src/wifi/model/dca-txop.h:338: * \return true if the curren fragment is the
last
> fragment,
> current is misspelled
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/mac-low-transm...
> File src/wifi/model/mac-low-transmission-listener.h (right):
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/mac-low-transm...
> src/wifi/model/mac-low-transmission-listener.h:42: MacLowTransmissionListener
> (DcaTxop *txop);
> In my opinion, since you change DcaTxop to inherit from ns3::Object, you
should
> scrub all usages of raw pointers and instead use reference counted pointer.
> 
> https://codereview.appspot.com/311400043/diff/1/src/wifi/model/mac-low.h
> File src/wifi/model/mac-low.h (left):
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/mac-low.h#oldc...
> src/wifi/model/mac-low.h:283: };
> Agree to move these out of MacLow
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/spectrum-wifi-...
> File src/wifi/model/spectrum-wifi-phy.cc (right):
> 
>
https://codereview.appspot.com/311400043/diff/1/src/wifi/model/spectrum-wifi-...
> src/wifi/model/spectrum-wifi-phy.cc:119: return Ptr<Channel> (0);
> should return m_channel

[S. Deronne, 2016-12-28 16:21:46]

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
File src/wifi/model/aggregation-capable-transmission-listener.h (right):

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
src/wifi/model/aggregation-capable-transmission-listener.h:42:
AggregationCapableTransmissionListener (EdcaTxopN * txop);
On 2016/12/24 15:25:07, Tom Henderson wrote:
> Can you change to Ptr<EdcaTxopN>?

ok

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
src/wifi/model/aggregation-capable-transmission-listener.h:56: virtual void
BlockAckInactivityTimeout (Mac48Address originator, uint8_t tid);
They should indeed not be virtual

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
src/wifi/model/aggregation-capable-transmission-listener.h:69: virtual void
SetAmpdu (Mac48Address dest, bool enableAmpdu);
I will update

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
src/wifi/model/aggregation-capable-transmission-listener.h:82: virtual uint16_t
GetNextSequenceNumberfor (WifiMacHeader *hdr) const;
ok

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/aggregation-ca...
src/wifi/model/aggregation-capable-transmission-listener.h:138: EdcaTxopN
*m_txop;
ok

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/dca-txop.h
File src/wifi/model/dca-txop.h (right):

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/dca-txop.h#new...
src/wifi/model/dca-txop.h:142: Time GetTxopLimit (void) const;
I will add doxygen

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/dca-txop.h#new...
src/wifi/model/dca-txop.h:338: * \return true if the curren fragment is the last
fragment,
ok thanks

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/spectrum-wifi-...
File src/wifi/model/spectrum-wifi-phy.cc (right):

https://codereview.appspot.com/311400043/diff/1/src/wifi/model/spectrum-wifi-...
src/wifi/model/spectrum-wifi-phy.cc:119: return Ptr<Channel> (0);
ok

[Tom Henderson, 2017-01-21 22:03:37]

https://codereview.appspot.com/311400043/diff/20001/src/wave/model/wave-mac-l...
File src/wave/model/wave-mac-low.h (right):

https://codereview.appspot.com/311400043/diff/20001/src/wave/model/wave-mac-l...
src/wave/model/wave-mac-low.h:61: * \param todo.
fixme

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

https://codereview.appspot.com/311400043/diff/20001/src/wifi/doc/source/wifi-...
src/wifi/doc/source/wifi-design.rst:199: The YansWifiPhy framework uses the
``Channel`` frameworks.
suggest to delete the above sentence, since Spectrum framework is not in
opposition to Channel framework but is just a different specialization

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/dca-txop.cc
File src/wifi/model/dca-txop.cc (right):

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/dca-txop.c...
src/wifi/model/dca-txop.cc:593: {
Why are these null function bodies (also GotBlockAck() and MissedBlockAck ())? 
Should at least a NS_LOG_WARN be added if there is intended to be null
functionality?

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/dcf-state.cc
File src/wifi/model/dcf-state.cc (right):

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/dcf-state....
src/wifi/model/dcf-state.cc:49: NS_LOG_FUNCTION (this);
Throughout this file, the NS_LOG_FUNCTION calls could take also the function
arguments as in NS_LOG_FUNCTION (this << aifsn);

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/edca-txop-n.h
File src/wifi/model/edca-txop-n.h (right):

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/edca-txop-...
src/wifi/model/edca-txop-n.h:300: 
Incomplete doxygen in this header file

[S. Deronne, 2017-01-22 07:53:08]

Tom,
Thanks for your comments, I will make a new patch set.

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

https://codereview.appspot.com/311400043/diff/20001/src/wifi/doc/source/wifi-...
src/wifi/doc/source/wifi-design.rst:199: The YansWifiPhy framework uses the
``Channel`` frameworks.
On 2017/01/21 22:03:36, Tom Henderson wrote:
> suggest to delete the above sentence, since Spectrum framework is not in
> opposition to Channel framework but is just a different specialization

ok

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/dca-txop.cc
File src/wifi/model/dca-txop.cc (right):

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/dca-txop.c...
src/wifi/model/dca-txop.cc:593: {
If we want to avoid this, we need to define a pure virtual parent class. I want
to avoid that, so I made null implementation for few functions which will never
be called for non QoS. I agree to add a warning there.

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/dcf-state.cc
File src/wifi/model/dcf-state.cc (right):

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/dcf-state....
src/wifi/model/dcf-state.cc:49: NS_LOG_FUNCTION (this);
On 2017/01/21 22:03:36, Tom Henderson wrote:
> Throughout this file, the NS_LOG_FUNCTION calls could take also the function
> arguments as in NS_LOG_FUNCTION (this << aifsn);

Correct, thanks.

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/edca-txop-n.h
File src/wifi/model/edca-txop-n.h (right):

https://codereview.appspot.com/311400043/diff/20001/src/wifi/model/edca-txop-...
src/wifi/model/edca-txop-n.h:300: 
On 2017/01/21 22:03:37, Tom Henderson wrote:
> Incomplete doxygen in this header file

ok, I will update