Merge wimax

[Faker Moatamri]

Hi Amine & all,
We are aiming, to merge wimax module into ns-3-dev before the end of
next week. In order to get the merge go smoothly and to anticipate
eventual problems I tested the builds under g++ 3.4.6, 4.0.4, 4.1.2,
4.2.4, 4.3.3 and 4.4.0 from the repository
http://code.nsnam.org/iamine/ns-3-wimax-release/.

Here are the results:
g++ 3.4.6, 4.0.4, 4.1.2:
Building error debug and optimized:

[698/893] cxx: src/devices/wimax/simple-wimax-channel.cc -> build/debug/src/devices/wimax/simple-wimax-channel_1.o
../src/devices/wimax/simple-ofdm-wimax-phy.cc: In member function `ns3::Ptr<ns3::PacketBurst> ns3::SimpleOfdmWimaxPhy::ConvertBitsToBurst(itpp::bvec, ns3::Ptr<ns3::PacketBurst>)':
../src/devices/wimax/simple-ofdm-wimax-phy.cc:597: warning: converting to `uint8_t' from `double'

g++ 4.2.4, 4.3.3 and 4.4.0: everything is green :-)

Can you please pull ns-3-dev code into your repository and merge it
specially that there are a new testing framework?
This will considerably speed up the merging time since it will be easier
to provide a patch based on ns-3-dev and you will anticipate eventual
small merging problems.
Also what is the status of bug 575 which is the only bug left for wimax?

Thanks
Best regards
Faker Moatamri

[iamine]

Hi Faker and all,

I have merged the WiMAX module with the latest version of ns-3-dev. The
code I available at the same directory
(http://code.nsnam.org/iamine/ns-3-wimax-release/).

I have tested builds under g++-4.0.4, 4.1.2 and 4.2.4; everything is
green! Could you rerun the build for g++-3.4.6, 4.3.3 and 4.4.0?
Regarding bug 575 the status is still open, however from my point of
view this bug is not critical and should not block an eventual  release.

Thank you in advance
Amine

Faker Moatamri wrote:

> Hi Amine & all,
> We are aiming, to merge wimax module into ns-3-dev before the end of
> next week. In order to get the merge go smoothly and to anticipate
> eventual problems I tested the builds under g++ 3.4.6, 4.0.4, 4.1.2,
> 4.2.4, 4.3.3 and 4.4.0 from the repository
> http://code.nsnam.org/iamine/ns-3-wimax-release/.
>
> Here are the results:
> g++ 3.4.6, 4.0.4, 4.1.2:
> Building error debug and optimized:
>
> [698/893] cxx: src/devices/wimax/simple-wimax-channel.cc ->
> build/debug/src/devices/wimax/simple-wimax-channel_1.o
> ../src/devices/wimax/simple-ofdm-wimax-phy.cc: In member function
> `ns3::Ptr<ns3::PacketBurst>
> ns3::SimpleOfdmWimaxPhy::ConvertBitsToBurst(itpp::bvec,
> ns3::Ptr<ns3::PacketBurst>)':
> ../src/devices/wimax/simple-ofdm-wimax-phy.cc:597: warning: converting
> to `uint8_t' from `double'
>
> g++ 4.2.4, 4.3.3 and 4.4.0: everything is green :-)
>
> Can you please pull ns-3-dev code into your repository and merge it
> specially that there are a new testing framework?
> This will considerably speed up the merging time since it will be
> easier to provide a patch based on ns-3-dev and you will anticipate
> eventual small merging problems.
> Also what is the status of bug 575 which is the only bug left for wimax?
>
> Thanks
> Best regards
> Faker Moatamri
>
>

[Faker Moatamri]

Ismail Amine wrote:

> Hi Faker and all,
>
> I have merged the WiMAX module with the latest version of ns-3-dev.
> The code I available at the same directory
> (http://code.nsnam.org/iamine/ns-3-wimax-release/).
>
> I have tested builds under g++-4.0.4, 4.1.2 and 4.2.4; everything is
> green! Could you rerun the build for g++-3.4.6, 4.3.3 and 4.4.0?
> Regarding bug 575 the status is still open, however from my point of
> view this bug is not critical and should not block an eventual  release.
>

The following tests have failed on all the g++-s:
CRASH: TestSuite ns3-tcp-interoperability
FAIL: TestSuite pcap-file-object  
I tried to test the same thing with ns-3-dev and all the tests passed.
Anyone has an idea what the problem could be?
Best regards
Faker Moatamri

[craigdo]

> > I have merged the WiMAX module with the latest version of ns-3-dev.
> > The code I available at the same directory
> > (http://code.nsnam.org/iamine/ns-3-wimax-release/).
> >
> > I have tested builds under g++-4.0.4, 4.1.2 and 4.2.4; everything is
> > green! Could you rerun the build for g++-3.4.6, 4.3.3 and 4.4.0?
> > Regarding bug 575 the status is still open, however from my point of
> > view this bug is not critical and should not block an eventual
> release.
> >
> The following tests have failed on all the g++-s:
> CRASH: TestSuite ns3-tcp-interoperability
> FAIL: TestSuite pcap-file-object
> I tried to test the same thing with ns-3-dev and all the tests passed.
> Anyone has an idea what the problem could be?

Make sure your test.py and src/test is up to date.

ns3-tcp-interoperability:  Run

  ./test.py -s ns3-tcp-interoperability -v

And take a look at the standard error.  CRASH means the test-runner did not
return 0 (success) or 1 (failure).  

pcap-file-object:  Run

  ./test.py -s pcap-file-object -t results.txt

To create an error report.  Then look in results.txt for what exactly
happened.  I noticed last night that there is still a hardcoded location in
pcap-file-object (/tmp/<random-number>.pcap) that I need to fix.  If you
can't create a file in /tmp the test will fail.  I am going to fix this
morning.

-- Craig

[iamine]

Hi All,

Thank you Caig and Faker. Everything is green now under g++-4.0.4,
4.1.2, 4.2.4, 3.4.6, 4.3.3 and 4.4.0 in debug and optimized modes. And
all the tests return the status passed.

Faker, you can check this on http://mimas:8010/waterfall


Regards
Amine

craigdo@... wrote:

>>> I have merged the WiMAX module with the latest version of ns-3-dev.
>>> The code I available at the same directory
>>> (http://code.nsnam.org/iamine/ns-3-wimax-release/).
>>>
>>> I have tested builds under g++-4.0.4, 4.1.2 and 4.2.4; everything is
>>> green! Could you rerun the build for g++-3.4.6, 4.3.3 and 4.4.0?
>>> Regarding bug 575 the status is still open, however from my point of
>>> view this bug is not critical and should not block an eventual
>>>      
>> release.
>>    
>> The following tests have failed on all the g++-s:
>> CRASH: TestSuite ns3-tcp-interoperability
>> FAIL: TestSuite pcap-file-object
>> I tried to test the same thing with ns-3-dev and all the tests passed.
>> Anyone has an idea what the problem could be?
>>    
>
> Make sure your test.py and src/test is up to date.
>
> ns3-tcp-interoperability:  Run
>
>   ./test.py -s ns3-tcp-interoperability -v
>
> And take a look at the standard error.  CRASH means the test-runner did not
> return 0 (success) or 1 (failure).  
>
> pcap-file-object:  Run
>
>   ./test.py -s pcap-file-object -t results.txt
>
> To create an error report.  Then look in results.txt for what exactly
> happened.  I noticed last night that there is still a hardcoded location in
> pcap-file-object (/tmp/<random-number>.pcap) that I need to fix.  If you
> can't create a file in /tmp the test will fail.  I am going to fix this
> morning.
>
> -- Craig
>
>
>
>
>
>  

[Faker Moatamri]

Ismail Amine wrote:
> Hi All,
>
> Thank you Caig and Faker. Everything is green now under g++-4.0.4,
> 4.1.2, 4.2.4, 3.4.6, 4.3.3 and 4.4.0 in debug and optimized modes. And
> all the tests return the status passed.
Ok cool, just curious, what was the problem?
>
> Faker, you can check this on http://mimas:8010/waterfall
>
Do you have a ready tested patch to apply to ns-3-dev?
If so +1 for merge tomorrow in ns-3dev. Tom? Craig? others? what do you
think?
>

[Tom Henderson-2]

Faker Moatamri wrote:

> If so +1 for merge tomorrow in ns-3dev. Tom? Craig? others? what do you
> think?

I've been holding off a review of this since Craig posted his 40-point
review last month.  I have been looking at it tonight; unfortunately, I
think it needs more work and another review before merging, or some
commitments to fix these things before release if not all of it is done
prior to merging.

This is an impressive amount of code (40,000+ lines according to wc) but
my main concerns are:
- very little test code
- very little doxygen outside of the helper class
- very few attributes
- coding style of function declarations

There is no test code right now, AFAICS, that test.py would execute.
There are two regression tests that are in the examples/wimax directory,
and Craig pointed out previously that these should be converted to the
new framework (probably moved to src/test/wimax and new, simpler
examples added).  It is hard to go back after a model like this is done
and try to add unit tests and other test code, but I would like to ask
the authors what they feel are the main concerns for possible
regressions down the road and how we could write tests for them now?  Do
we even know (how?) that the current code is functioning correctly?

Classes missing doxygen-- I started to enumerate these but stopped as
the list grew too long-- consider that there are 57 header files and it
seems that most classes are missing doxygen.  I would start by
doxygenating the key classes such as WimaxNetDevice and its variants,
things like WimaxMacHeader, etc.  Maybe not every class and member needs
doxygen (we aren't 100% throughout ns-3 as is right now) but looking at
wscript, most headers are publicly exported so I think the general rule
should be to doxygenate all public headers.

Also, I don't think blocks such as the below are valid doxygen blocks
(this is pretty common throughout the proposed new code) because first
line is not "/**"-- have you checked whether doxygen picks these up?
   /*
    * set the destination IP address and port
    * \param ip the destination ip address to which the stream will be sent
    * \param port the destination udp port to which the stream will be sent
    */

I counted 28 calls to AddAttribute.  Many of these seem to involve
pointer accessors, outside of some propagation parameters.  This means
that most of the variables, parameters, and constants are not accessible
via the attribute and default value system.  Many values seem to be
initialized in constructors to magic numbers; for instance, see
QoSParameterSet.  Again, I would ask the authors to review all of these
constructors and, where there is a potential for future users to want to
change the value (note:  I counted hundreds of "setter" functions), make
it an attribute, and for other hard coded magic numbers, consider the
use of const integers.

I counted 12 trace sources and it didn't seem to me that the trace
sources were aligned with the ones that Craig tried to standardize
across devices a few months ago (for instance, the use of PromiscSniffer
as the pcap trace source).

 From Craig's previous comments, please revisit the following still
unaddressed comments, aside from the "lacks doxygen" comments:
3, 4 (already mentioned above), 20, 23, and 26.

 From a coding style standpoint, a major difference that was visibly
apparent to me with respect to the rest of ns-3 is that the class member
function declarations do not have the return type on the same line as
the function name.  Since most of these functions are also missing
Doxygen, this could probably be cleaned up at the same time.

Other comments:

Regarding logging, I do not think that this is a blocker for merging
because there is no required style for logging but there are only two
instances of NS_LOG_FUNCTION which makes it harder to trace control flow
via logging (something I do when working with with WiFi code, for
example).  I care much less about addressing this issue than all of the
above other concerns (tests, doxygen, attributes), however.

address is misspelled "adress" in several variable names (grep on adress)

Why is trace-based streamer going to the trouble of allocating and
copying zero-filled payloads into packets?  Couldn't you use the "fake
data" version of application packets here?

People may not be aware that there are two new applications (not
specific to WiMAX) being merged as well:
src/applications/trace-based-streamer, and
src/applications/udp-client-server
This wasn't apparent to me until I reviewed Craig's previous comments.
Maybe this will be a surprise to others unless called out somehow or
split out separately of this repo.

Given the size of this module, I think that trying to address all of my
comments to the fullest extent possible will be a mountain of work, so I
would like to hear other opinions about whether, and to what extent,
these comments should be addressed before merging.

- Tom