Requesting Review of Wifi Enhancement

[Timo Bingmann]

Hello Mathieu,

I've uploaded on the codereview platform a hopefully readable version of the first set of my wifi contributions.

http://codereview.appspot.com/65051

It contains multiple added independent features:

- Ns2ExtWifiPhy
which works exactly like ns-2's WirelessPhyExt from the DSN.

- NakagamiPropagationLossModel
Based on GammaVariable and ErlangVariable

- TrafficApplication
based on OnOffApplication but much simpler and useful for stochastic streams. creates packets using a factory class.

There are also many many other small non-substantial changes, which I made while coding, generally improving code quality or readability.

We will have to talk about the WifiPhyTag, but I suggest you look first at how much easier it makes function calls.

I renamed WifiStateHelper to YansWifiStateHelper, but ultimately refrained from splitting it up into Ns2ExtWifiStateHelper. Oh well.

Many const changes, because the other order is very uncommon.

Removing a duplicate Eifs variable.

Well. Let's use that review tool. Thanks in advance.

Greetings
Timo

[Tom Henderson]

Timo Bingmann wrote:
> - TrafficApplication
> based on OnOffApplication but much simpler and useful for stochastic streams. creates packets using a factory class.
>

Timo, is TrafficApplication intended to replace OnOffApplication, or sit
side-by-side with it? It seems very similar.

Tom

[Timo Bingmann]

Yes, it is similar. But, it definitely was not _intended_ to replace OnOff. However, for most CBR scenarios either can be used.
There are subtle differences in packet sending times.

OnOff calculates the wait interval from DataRate and packet size. It sends at the _end_ of each interval and can only be used for CBR. The residualBits part in OnOff is used to make the data rate exact even for Off-state interruptions.

Whereas TrafficApp doesnt care about CBR or data rate, it only pulls the wait interval from a RandomVariable. Thus
Interval = ExponentialVariable(0.1) can be used just like Interval = ConstantVariable(0.1).

I also added the factory part to allow things like exponentially/normal/whatever distributed packet sizes.

The basic use case I required was to send beacon packet at specific intervals. That was possible with OnOff. However, once I wanted to add a jitter to the beacon interval, making to more real and avoiding some unnatural packet collisions, OnOff didnt work anymore, because the jitter breaks the wait interval calculations.

So if you want a CBR stream with this-and-that data rate, OnOff is the thing to use; if you want to manually set packet send intervals or specially configure packets TrafficApp is the right choice.
Yes, for most CBR cases TrafficApp with ConstantVariable is equal to OnOff, except for packet sending time point.

Well. I added TrafficApp (and also called it by that name) because its follows such a simple packet generator pattern that was not available in ns-3. However, if you ask me, I'm not fully satisfied with the class, because the simplest configuration with a fixed packet size (SimplePacketFactory) needs lots of extra code lines compared to OnOff. But that's the price of flexibility.

Greetings
Timo

[Mathieu Lacage]

hi timo,

I went through the patchset quickly but it's big and there are a lot
of unrelated changes together. If you really want to help merge as
much of this code as soon as possible, it would be really helpful to
try to split this patchset in smaller pieces and resend separately all
the parts which are non-controversial. Also, if you don't want to push
the rename of WifiStateHelper, then, undo it to decrease the set of
the patchset.

So, could you please resend separately the nakagami model, as well as
the small const+typo+small fixes ?

Mathieu

--
Mathieu Lacage <mathieu...@gmail.com>

[Mathieu Lacage]

I pushed all the small changes, including the nakagami propagation
model. Things which still need fixing for the rest of your patchset:
- leave out the Traffic stuff
- separate the ns2 phy model from the rest: if you really want to
make it easy to push your changes, you should not include the phy tag
changes or include them separately to allow me to review them in
isolation: I am far from convinced by this change.
- the ErrorRateModel changes are utterly evil: you have two
subclasses, and each subclass implements a different method from the
base class, leave the other virtual method unimplemented. This is
screaming for removing the base class.

I might get to do these things myself sometime, like I did for the
small change splitting, but, if you really care about pushing your
changes upstream, you most likely will need to do some more work now.

Mathieu

On Tue, May 12, 2009 at 4:56 PM, Timo Bingmann
<timo.b...@student.kit.edu> wrote:
>

--
Mathieu Lacage <mathieu...@gmail.com>