[Ns-developers] Preliminary release of NS-3 WiMAX module

[Mathieu Lacage]

hi tippu,

On Wed, 2008-05-28 at 19:38 +0200, Jahanzeb Farooq wrote: 
> Hi all,
> It's been a while being a silent member of the mailing list. I hereby
> announce the preliminary release (version 0.9.0 ??) of the WiMAX module
> for NS-3. The module currently implements the fundamental
> functionalities of the WiMAX standard. Following is a brief outline of
> the functionalities implemented:

This is all pretty cool ! 

I went through the repository you posted:

> http://yans.inria.fr/code/ns-3-wimax/

So, here are couple of comments:
  1) your repository contains a lot of automatically-generated files.
For examples, it contains a lot of files under the build directory and
under the .waf-1.3.2-b67d68bfb915473cfa1a2e2290022f98 directory. All of
these should be removed.

  2) The code does not build by default unless we install itpp: I think
that you need to conditionally compile the itpp-dependent PHY
implementation based on whether the itpp library could be found. For
those who are interested, on my ubuntu system, I had to run the
following command: "sudo apt-get install libitpp-dev"

  3) After installing itpp 3.10.11, I still get the following
compilation error:
../src/devices/wimax/wimax-ofdm-phy/modulator80216.h:51: error: ‘BPSK_c’
does not name a type
I suspect that I need to get the right version of itpp to get that
symbol defined: which version should I get ?

  4) the files examples/simple-wimax*.cc and src/devices/wimax/*.h|*.cc
are executables: you should remove their exec bit with chmod and commit
them. i.e., "chmod a-x file".

  5) It is very unclear to me which version of ns-3 you have based your
work on. I looked at the mercurial history contained in your tree and
the most recent common ancestor I could find between your tree and
ns-3-dev is 1393:d0f6f85074685c859f3e5d5c58382c681c7ab915 which is a
changeset which happened just after release 3.0.10, that is, somewhere
in september 2007. So, there are 2 options:
  - you really have based your work on that version and, if this is the
case, you _must_ update your tree to ns-3-dev: there is little point in
reviewing it as-is.
  - you have merged your tree with ns-3-dev _by hand_ without using
mercurial's merging capabilities. If this is the case, I am afraid that
this is going to make merging your code back to ns-3-dev impossible
because it will trigger thousands of lines of code of conflicts in our
core code. It also makes reviewing the current code vastly harder than
it should be.

I suspect that you are in the second case above and, if I am right, you
have a couple of options:

  a) try to run "hg pull http://code.nsnam.org/ns-3-dev" in your tree,
deal with the conflicts if there are any, and repost a repo for review

  b) create a clean copy of ns-3-dev, copy your own code in there, and
hg add && hg ci it in the new repository. Please, make sure you do not
add unecessary and unwanted files. i.e., add each file separately.

  c) attempt to re-do your merge.I suspect very much that the
problematic changeset is 1439:9bc15c0b4233 and, if this is the case,
what you need to do is create a clone with the previous changeset: "hg
clone -r 1438:9ff95f45ab40 ns-3-wimax ns-3-wimax-for-merge", pull
ns-3-dev in ns-3-wimax-for-merge, and run "hg merge" (or "hg imerge",
depending on what you like best). Before doing this, I suggest strongly
that you install the _latest_ version of mercurial and read a bit about
it and about maintaining branches and merging (see, for example:
http://hgbook.red-bean.com/hgbookch3.html#x7-550003)

a) might be painful and create a scary and horrible mercurial history.
b) will destroy all _your_ development history, c) will preserve both
your history and the ns-3 history correctly. Based on the issue 1)
outlined above (lots of unwanted files present in your
repository/history), I would suggest that you go with b) because we
really do not want to keep in our history all the files you have added
there.


6) I read through the README which seems to be doing a good job of
explaining what the code implements, and how. The rationale behind the
structure of the code is, however, much less clear to me. 

For example, I see that you seem to be a using a PacketClassifier base
class and you seem to be requesting each user to provide an explicit
classifier for each device: the classifier is set at construction time
and cannot change ever, even for the BS. That implementation choice
seems to imply that it is not possible to create dynamic wimax
connections with your code because each connection needs to be
associated with a set of classification rules sent by the BS to the SSs
once the connection is established to allow each SS to classify its
traffic to different connections. I have to confess that I would have
expected to see a single PacketClassifier class (not virtual) which
would be able to parse and execute the classification rules as defined
by the spec and which would be fed the classification rules sent by the
BS to the SS and the classification rules built by the BS for itself.

7) It would be really helpful to know what your TODO is over the next
couple of months: it is hard, as-is, to figure out what is really just
code waiting to be implemented shortly and what is ignored because you
don't care about modelizing it. The end of README attempts to answer
that question but it would be very helpful to have a more detailed plan
of action about what you want to do. For example, the README seems to
imply that scheduling will be implemented before the dynamic creation
and management of service flows which seems really backward: I would
have expected the latter to be implemented first properly with a very
stupid scheduler.

8) There is a lot of code to digest here (14 KLOC) so, it is going to
take me a while to go through it all and make more interesting comments
about the code itself. Generally, I have to say that such big code drops
make it very very hard to provide good reviews: it would have been
vastly easier to comment on this work incrementally and piece-by-piece
rather than as a whole. 


regards,
Mathieu