[Ns-developers] 802.11 first changesets to comment on.

[Mathieu Lacage]

hi timo,


On Thu, 2008-11-27 at 15:53 +0100, Timo Bingmann wrote:

> I've finished catching up on the wifi changes.
> It would be great, if you could give me a quick review or hints on the
> following changesets, which I've put into the hg repo at
> http://idlebox.net/2008/ns-3-wifi/code/ns-3-wifiex/

Ok, quick initial comments:
> 
> Brief change description:
> - NakagamiPropagationLossModel added.
> - Required for that: GammaVariable and ErlangVariable random
> variables.

Both look good to me. You will need approval from the random variable
maintainer, that is, michele weigle: just create a bug report and attach
a link to all relevant changesets:
http://idlebox.net/2008/ns-3-wifi/code/ns-3-wifiex/rev/4cc0bd0187f2
http://idlebox.net/2008/ns-3-wifi/code/ns-3-wifiex/rev/c2cc41757d7b
http://idlebox.net/2008/ns-3-wifi/code/ns-3-wifiex/rev/a2c275b23a7e

> - Change to PropagationLossModel::GetLoss() parameters. Required
> because Nakagami envelopes the absolute rxPower. It's not
> multiplicative.

Adding an extra parameter to the method sounds fine to me. However, the
DoGetLoss method was expected previously to return one of the components
of the final loss, not the final loss and that loss was then used by
PropagationLossModel::GetLoss to calculate the final aggregate loss with
the 'next' propagation loss model. Making the DoGetLoss method return
the final loss kills the whole idea of iterating over the list of loss
models to add their losses, thus, kills the idea of having a list of
loss models. Thus, while I understand that you might need an extra
parameter to DoGetLoss, and GetLoss, changing the semantics of its
return value seem really wrong to me.

> - Rewrote samples/main-random-variables to output all implemented RVs.

Looks pretty good. Will also require michele weigle approval.

> - Added samples/main-propagation-loss.cc, which outputs a plot of all
> loss models (except Jakes', which I couldnt get to work right or don't
> understand it.)

Looks pretty good: I don't know much about the Jakes model because I
never used it myself. I will try to see if I can find a simulation
script written by federico which uses it.

> - Lots of changes to the Gnuplot classes to make the two plot programs
> work.

1) I liked the ability to not have to allocate objects on the heap: is
there a specific reason why you made GnuplotDataset derive from
RefCountBase ? I am not saying that you should not: I am just trying to
understand the rationale.

2) what is the difference between a dataset title and a dataset
function ? (i.e., what is the purpose of GnuplotDataset::SetFunction).
it looks unused.

3) it would be cool to make DetectTerminal and SetTerminal private and
call them from the Gnuplot constructor.

4) I would tend to avoid the duplicate functionality provided by void
AddDataset (Ptr<GnuplotDataset> dataset, const std::string& title); and
just ask the user to set the title on the dataset himself.

5) What do you use extra for ?

6) GnuplotDataset::AddEmptyLine is totally evil. Can't you figure out a
way to get rid of it ?

7) GnuplotDataset::Type looks also pretty evil: would it not make sense
to simply create an unrelated Gnuplot3dDataset class and make the
Gnuplot class be able to also handle it ? This might help with (6)
above.

> 
> You can find the generated pdf plots at
> http://idlebox.net/2008/ns-3-wifi/results/

these do look pretty cool.

> 
> The output of Nakagami is cross-checked against ns-2. See the two
> plots: yes, one is with ns-2, the other with ns-3.
> 
> One conceptional idea: the code for NakagamiPropagationLossModel is
> taken from ns-2. It actually contains two propagation loss models: a
> 3-piece log distance loss model followed by a 3-piece Nakagami fast
> fading model. Conceptionally it would be better to separate them: use
> LogDistanceLossModel followed by the Nakagami model using the chaining
> feature. However this would require me to extend LogDistanceLossModel
> into the 3-piecewise composition. It introduces more complexity. Maybe
> just add a new class ThreeLogDistanceLossModel? What do you think?

This, indeed, seems like the right thing to do.

> 
> Just tell me if I'm on the right track.

you seem to be doing great :)

Mathieu