TrafficApplication and a Wifi maximum throughput example.

[Timo]

First extract from my wifiex repo:

TrafficApplication is a modification of OnOffApplication to allow
a) stochastic inter-packet intervals like Poisson packet streams
b) a versatile packet factory to create packets with different sizes (e.g. from
some random distribution), data contents and maybe special Tags.
c) always accept incoming Rx as a sink, see below.

Also included is an example simulation, which tests the maximum throughput
between two wifi nodes: one has a TrafficApplication, the other a PacketSink.

The reason for TrafficApp's Rx functions becomes plain here: it no Rx is
installed, then _all_ data is queues somewhere in the PacketSocket stack and the
experiment explodes.

So tell me what you think?
Coming soon are the wifi modifications.

Timo

[Mathieu Lacage]

http://codereview.appspot.com/154183/diff/1/4
File src/applications/traffic/traffic-application.cc (right):

http://codereview.appspot.com/154183/diff/1/4#newcode123
src/applications/traffic/traffic-application.cc:123: if (!m_socket)
(m_socket != 0)

!m_socket relies on an implicit type conversion which is something we generally
frown upon.

http://codereview.appspot.com/154183/diff/1/5
File src/applications/traffic/traffic-application.h (right):

http://codereview.appspot.com/154183/diff/1/5#newcode79
src/applications/traffic/traffic-application.h:79: class TrafficApplication :
public Application
I don't care much but I guess that the name (TrafficApplication) might be too
generic.

http://codereview.appspot.com/154183/diff/1/5#newcode194
src/applications/traffic/traffic-application.h:194: protected:
should be private ?

http://codereview.appspot.com/154183/diff/1/5#newcode232
src/applications/traffic/traffic-application.h:232: protected:
should be private ?

http://codereview.appspot.com/154183/diff/1/7
File src/helper/traffic-helper.cc (right):

http://codereview.appspot.com/154183/diff/1/7#newcode3
src/helper/traffic-helper.cc:3: * Copyright (c) 2008 INRIA
I am sure no one at the inria did write this code :)

[Timo]

Short replies.

http://codereview.appspot.com/154183/diff/1/4
File src/applications/traffic/traffic-application.cc (right):

http://codereview.appspot.com/154183/diff/1/4#newcode123
src/applications/traffic/traffic-application.cc:123: if (!m_socket)
On 2009/11/17 14:58:26, Mathieu Lacage wrote:
> (m_socket != 0)
> 
> !m_socket relies on an implicit type conversion which is something we
generally
> frown upon.

Says !m_socket, which is a call to Ptr<T>::operator!(), so
no type conv. And it's code copied from OnOff.

http://codereview.appspot.com/154183/diff/1/5
File src/applications/traffic/traffic-application.h (right):

http://codereview.appspot.com/154183/diff/1/5#newcode79
src/applications/traffic/traffic-application.h:79: class TrafficApplication :
public Application
On 2009/11/17 14:58:26, Mathieu Lacage wrote:
> I don't care much but I guess that the name (TrafficApplication) might be too
> generic.

Yes I agree, but havent got a better idea.

Maybe IntervalApplication or StochasticTrafficApplication, but they are no good.

And in any case I believe OnOffApp should be renamed to CbrTrafficAppli.

No good ideas here.

http://codereview.appspot.com/154183/diff/1/5#newcode194
src/applications/traffic/traffic-application.h:194: protected:
On 2009/11/17 14:58:26, Mathieu Lacage wrote:
> should be private ?

Well who says these classes are final, I could derive from them.

http://codereview.appspot.com/154183/diff/1/7
File src/helper/traffic-helper.cc (right):

http://codereview.appspot.com/154183/diff/1/7#newcode3
src/helper/traffic-helper.cc:3: * Copyright (c) 2008 INRIA
On 2009/11/17 14:58:26, Mathieu Lacage wrote:
> I am sure no one at the inria did write this code :)

Are you sure? I don't consider sed s/OnOff/Traffic/ onoff-helper.cc >
traffic-helper.cc 
to be "writing code". ;)

[Tom Henderson]

I support this from a functional perspective; my main concern is how to
differentiate from OnOffApplication (avoid confusing users) since they are very
similar but the differences are subtle.

On the surface, this seems to be a lot of duplication with OnOffApplication.  I
raised a question about this back in May and this was the answer:
http://groups.google.com/group/ns-3-reviews/browse_thread/thread/f0b36d737342...

Somehow, this distinction should be preserved and well documented.  I don't see
it in the doxygen.

This could be called TwoState and in the future what is now called the "Off"
state could be extended to rather allow packet transmissions due to a different
distribution, with zero packet transmissions being the default?

http://codereview.appspot.com/154183/diff/1/2
File examples/wireless/wifi-throughput.cc (right):

http://codereview.appspot.com/154183/diff/1/2#newcode248
examples/wireless/wifi-throughput.cc:248: // simple statistics
Pavel merged a src/contrib/average.h class that may be leveraged here.

http://codereview.appspot.com/154183/diff/1/5
File src/applications/traffic/traffic-application.h (right):

http://codereview.appspot.com/154183/diff/1/5#newcode79
src/applications/traffic/traffic-application.h:79: class TrafficApplication :
public Application
On 2009/11/17 16:35:44, Timo wrote:
> On 2009/11/17 14:58:26, Mathieu Lacage wrote:
> > I don't care much but I guess that the name (TrafficApplication) might be
too
> > generic.
> 
> Yes I agree, but havent got a better idea.
> 
> Maybe IntervalApplication or StochasticTrafficApplication, but they are no
good.
> 
> And in any case I believe OnOffApp should be renamed to CbrTrafficAppli.


How about TwoStateTrafficApplication?

> No good ideas here.

http://codereview.appspot.com/154183/diff/1/5#newcode232
src/applications/traffic/traffic-application.h:232: protected:
On 2009/11/17 14:58:26, Mathieu Lacage wrote:
> should be private ?

we don't typically make raw data protected in ns-3.

http://codereview.appspot.com/154183/diff/1/8
File src/helper/traffic-helper.h (right):

http://codereview.appspot.com/154183/diff/1/8#newcode37
src/helper/traffic-helper.h:37: class TrafficHelper
Helper API also matches that of OnOff.  Agree with Mathieu that TrafficHelper
seems too generic of a name.

[Timo]

On 2009/11/25 06:46:15, Tom Henderson wrote:
> I support this from a functional perspective; my main concern is how to
> differentiate from OnOffApplication (avoid confusing users) since they are
very
> similar but the differences are subtle.
> 
> On the surface, this seems to be a lot of duplication with OnOffApplication. 
I
> raised a question about this back in May and this was the answer:
>
http://groups.google.com/group/ns-3-reviews/browse_thread/thread/f0b36d737342...

I agree with TrafficApp being too generic. My favorite is currently
StochasticTrafficApplication. Next problem is then the naming of
StochasticTrafficPacketFactory, but oh well.

I believe the main problem with OnOffApp is that the name just doesnt say what
the app does: send out CBR traffic. That's what the class is engineered to do,
the OnOff part is imho less important. Any application following the (very
common) on/off pattern will be confused with this CBR-generating one.

Now I know renaming OnOff is not really a good idea, as it is used all over the
examples and outside experiments. But still it should be considered.

> Somehow, this distinction should be preserved and well documented.  I don't
see
> it in the doxygen.

We're getting there.

> This could be called TwoState and in the future what is now called the "Off"
> state could be extended to rather allow packet transmissions due to a
different
> distribution, with zero packet transmissions being the default?

That is a good idea. And it raises the issue of why having only two states?

[Timo]

Fixed a bad calculation bug in average.h (I hope no one used that) and using it
in wifi-throughput.

http://codereview.appspot.com/154183/diff/1/2
File examples/wireless/wifi-throughput.cc (right):

http://codereview.appspot.com/154183/diff/1/2#newcode248
examples/wireless/wifi-throughput.cc:248: // simple statistics
On 2009/11/25 06:46:16, Tom Henderson wrote:
> Pavel merged a src/contrib/average.h class that may be leveraged here.

Thanks for pointing me to that contrib class. See new patch.

http://codereview.appspot.com/154183/diff/1/5
File src/applications/traffic/traffic-application.h (right):

http://codereview.appspot.com/154183/diff/1/5#newcode232
src/applications/traffic/traffic-application.h:232: protected:
On 2009/11/25 06:46:16, Tom Henderson wrote:
> On 2009/11/17 14:58:26, Mathieu Lacage wrote:
> > should be private ?
> 
> we don't typically make raw data protected in ns-3.

Okay will be private then (when we figure out a class name).