[Ns-developers] WiMAX review request
Ismail Amine
Amine.Ismail at sophia.inria.fr
Wed Sep 23 08:39:42 PDT 2009
Dear Craig,
The WiMAX module has been reviewed by Mathieu and he gives us +1, Now we
need one more +1 to merge.
As I said in my previous email I have made all the changes you have
requested except points 4 and 26.
If you don't have other comments I propose to merge the WiMAX module for
the release of 14 October. What do you think?
Thank you
Best regards
Amine
Amine.Ismail at sophia.inria.fr wrote:
> Dear Craig,
>
> I have made all the changes you have requested in your previous email
> except points 4 and 26.
> Regarding point 4 I have modified the tests and now they don't print any
> output except the status of the test (OK or Failed). I think that these
> tests are very useful for developers to avoid regression during the
> future phase of development. These are basically regression tests for
> wimax module.
>
> Regarding point 26: These parts of the code are incomplete and of course
> they could be removed. However from my point of view it is very
> important to keep them here since they provide the skeleton of some
> features not implemented yet and they indicates were we should insert
> some code to implement them.
>
> I have also run the code through valgrind and I have found a lot of
> memory leaks, I have spend a lot of time to fix all of them. Actually
> the new version of the module is clean from the valgrind point of view
> in term of memory management.
>
> The new code is available on
> http://code.nsnam.org/iamine/ns-3-wimax-release/
>
> Thank you
>
> Best regards
> Amine
>
>
>
> craigdo at ee.washington.edu wrote:
>
>>> Hi all,
>>> I would like to ask you kindly to review the WiMAX module code.
>>>
>>>
>> Wow. Lots of code. Didn't make it through everything. I know this is
>> coming late, but I have comments.
>>
>> 1. It's just a nit, but there is a tendency in wimax.h to use acronyms
>> before definition. In many cases you find things like "MAC Common Part
>> Sublayer (CPS)" which is perfect, but many other acronyms are used well
>> before you find the definition and they are never explicitly tied together.
>> Overall this is a quite nice file, though.
>>
>> 2. In wimax.h, "The process to determine if a block is received correctly
>> has x steps" should be fixed.
>>
>> 3. In wimax,h, "From a pre-generated trace files determine the bloc error
>> rate for the calculated SNR". I don't anyone will understand what this
>> means at all unless they've read the code. Need to explain this further
>> since this is the first thing a user will read about the model.
>>
>> 4. The following seems wrong: "2 regression tests based on example
>> scenarios have been developed. To execute these tests we should compile the
>> module with "WIMAX_TEST" flag: CXXFLAGS="-DWIMAX_TEST" ./waf configure;
>> ./waf"
>>
>> Regression tests are run on examples which are compiled with and link to
>> standard ns-3 libraries; and use trace files to determine PASS or FAIL
>> status. You can't have a regression test that depends on a separately
>> compiled model and which generates no data. This may be something you can
>> use to test WiMAX, but it isn't an ns-3 regression test runnable in the
>> nightly build and test cycle like every other regression test.
>>
>> Regression tests typically don't print output. Some of the examples do.
>>
>> It sounds like this should be integrated into the new test framework before
>> release. Luckily this is easy to do and I can help.
>>
>> 5. examples/wimax-ipv4.cc: Why does this invent a new argument parsing
>> mechanism? This makes it different from all other examples in program
>> structure and usage.
>>
>> 6. examples/wimax-ipv4.cc: I think you should make it more clear what
>> topology you are creating, what the example is supposed to do and provide
>> comments in the code.
>>
>> wimax.ClassifierConfig (ss[i + (nbSS / 2)], ss[i], bs[0],
>> Sinterfaces.GetAddress (i + (nbSS / 2)),
>> SSinterfaces.GetAddress (i), 0, 100 + (i * 10), 17, 100);
>>
>> Is not terribly helpful in terms of understanding what the example is
>>
> doing.
>
>> 7. examples/wimax-multicast.cc: This shouldn't have:
>> NS_LOG_COMPONENT_DEFINE ("MySimpleWimaxSimulation");
>>
>> 8. examples/wimax-multicast.cc: See (6).
>>
>> 9. examples/wimax-simple.cc: See (6).
>>
>> 10. examples/wimax.cc: See (6) and (7),
>>
>> 11. The examples all seem to have different argument parsing routines with
>> different argument styles (i.e., -c vs. --Help).
>>
>> 12. trace-based-streamer.cc: You push characteristics of the trace to five
>> separate vectors. I would've expected to see these things combined into an
>> object and pushed into one vector. Something like,
>>
>> typedef struct {
>> Time tSend;
>> uint32_t size;
>> uint32_t frameIndex;
>> char[n] frameType;
>> } TraceEntry;
>>
>> TraceEntry entry;
>> entry.tSend = ...
>> m_entries.push_back (entry);
>>
>> 13. trace-based-streamer.cc: You load the trace when the application is
>> starting. This is okay unless you are running in real time. IN this case
>> it would be better to load the trace before the time-critical things start
>> happening in the simulation -- i.e., at configuration time.
>>
>> 14. trace-based-streamer.cc: In TraceBasedStreamer::Send (void) I find
>>
> lots
>
>> of malloc() but no corresponding free(). Has this been run through
>> valgrind?
>>
>> 15. trace-based-streamer.cc: The strcpy in
>> TraceBasedStreamer::TraceBasedStreamer() is redundant
>>
>> memset (m_traceFile, 0, 1024);
>> strcpy (m_traceFile, "");
>>
>> 16. trace-based-streamer.h: Doxygen is \ingroup udpecho with \brief A Udp
>> Echo Client.
>>
>> 17. trace-based-streamer.h: No Doxygen for methods of TraceBasedStreamer
>>
>> 18. src/applications/udp-client-server: I don't understand what this is
>>
> all
>
>> about. Why is this different than a udp-echo application? What does
>> udp-client-server mean? The Doxygen calls it a Udp Echo client and server.
>> Why not use or extend existing Udp Echo? Did you duplicate an application
>> to add a sequence number?
>>
>> 19. src/applications/udp-client-server: This all needs Doxygen.
>>
>> 20. What is this?
>>
>> diff -r 7dd4ad5ac045 src/devices/wimax/.directory
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/src/devices/wimax/.directory Thu Sep 03 15:07:19 2009 +0200
>> @@ -0,0 +1,3 @@
>> +[Dolphin]
>> +Timestamp=2009,8,27,9,21,49
>> +ViewMode=1
>>
>> 21. src/devices/wimax/README: Doesn't this just duplicate wimax.h? Why
>> have two places with the same information in it? One will get stale.
>>
>> 22. bs-link-manager.cc: Do you really want to LogComponentEnable
>> ("BSLinkManager", LOG_LEVEL_INFO) in BSLinkManager::BSLinkManager()?
>>
>> 23. bs-link-manager.cc: In BSLinkManager::CalculateRangingOppsToAllocate
>> (void)
>>
>> //randomly selecting TOs up to 10, shall actually be decided by scheduler
>> return rand () % 8 + 2;
>>
>> Is this really what you want? Should probably say randomly select TO
>>
> from 2
>
>> to 10, then. Hard coded numbers could be changed to const uint8_t for
>> clarity.
>>
>> 24. in BSLinkManager::PerformRanging(): If it was worthwhile putting in
>> debug printfs, it would probably good to use NS_LOG.
>>
>> #ifdef JDEBUG1
>> std::cout << std::endl << "RNG-REQ"; rngreq.Print (std::cout);
>>
> std::cout
>
>> << std::endl;
>> #endif
>>
>>
>> 25. In BSLinkManager::PerformInitialRanging() I see
>>
>> SSRecord *ssRecord = new SSRecord ();
>>
>> bool isOldSS = m_bs->GetSSManager ()->IsInRecord (rngreq->GetMacAddress
>> ());
>> if (isOldSS)
>> {
>> ssRecord = m_bs->GetSSManager ()->GetSSRecord (rngreq->GetMacAddress
>> ());
>> //if this fails it would mean the RNG-RSP with success status was not
>> received by the SS
>> }
>>
>> New SSRecord is overwritten with no delete? Has this been run through
>> valgrind? In fact, I don't see a delete anywhere. If there is supposed to
>> be responsibility for the object passed somewhere else, this kind of thing
>> should be explicitly stated.
>>
>> 26. In bs-link-manager.cc, this kind of stuff shouldn't be in released
>>
> code
>
>> ...
>>
>> void
>> BSLinkManager::DeallocateCids (Ptr<const ConnectionIdentifier> cid)
>> {
>> //if necessary, delete entire connections or simply set CIDs to 0
>> }
>>
>> uint64_t
>> BSLinkManager::SelectDlChannel (void)
>> {
>> //temporarily set to 1 for quick scanning
>> return m_bs->GetChannel (1);
>> }
>>
>> bool
>> BSLinkManager::ChangeDlChannel (void)
>> {
>> //code to decide if SS shall move to a new channel/frequency goes here
>> return false; //temp
>> }
>>
>> uint32_t
>> BSLinkManager::GetNewDlChannel (void)
>> {
>> //code to determine suggested new frequency goes here
>>
>> return 100; //temp
>> }
>>
>> 27. bs-link-manager.h: Needs Doxygen.
>>
>> 28. bs-scheduler-rtps.cc:
>>
>> a newed list goes in m_downlinkBursts during construction
>>
>> BSSchedulerRtps::BSSchedulerRtps (Ptr<WimaxBaseStationNetDevice> bs) :
>> m_downlinkBursts (
>> new std::list<std::pair<OfdmDlMapIe*, Ptr<PacketBurst> > > ())
>> {
>> SetBs (bs);
>> }
>>
>> m_downlinkBursts is just deleted on destruction
>>
>> BSSchedulerRtps::~BSSchedulerRtps (void)
>> {
>> SetBs (0);
>> delete m_downlinkBursts;
>> m_downlinkBursts = 0;
>> }
>>
>> but other things are newed up and put in
>>
>> void
>> BSSchedulerRtps::AddDownlinkBurst (Ptr<const WimaxConnection> connection,
>> uint8_t diuc, WimaxPhy::ModulationType modulationType,
>> Ptr<PacketBurst> burst)
>> {
>> OfdmDlMapIe *dlMapIe = new OfdmDlMapIe ();
>>
>> [ ... ]
>>
>> m_downlinkBursts->push_back (std::make_pair (dlMapIe, burst));
>> }
>>
>> Has this been run through valgrind? If this is handled somewhere else, the
>> responsibility for allocating and freeing objects should be written down
>> explicitly.
>>
>> 29. in BSSchedulerRtps::Schedule (void) there is ifdef JDEBUG2. See (24).
>>
>> 30. In BSSchedulerRtps::CreateUgsBurst, I see
>>
>> NS_ASSERT (burst->GetSize ());
>>
>> I really like to see messages associated with asserts (NS_ASSERT_MSG)
>>
>> 31. in BSSchedulerRtps::BSSchedulerRTPSConnection() there are several of
>>
>> std::cout << "\tDL Scheduler for rtPS flows \n" <<
>>
> "\t\tavailableSymbols =
>
>> "
>> << availableSymbols << std::endl;
>>
>> You shouldn't just write things to stdout. This should be NS_LOG
>>
> something.
>
>> 32. bs-scheduler-rtps.h: Needs more Doxygen.
>>
>> 33. bs-scheduler-simple.cc: Similar memory management questions as (28).
>>
>> 34. bs-scheduler-simple.cc: JDEBUGs sould be NS_LOGs
>>
>> 35. bs-scheduler-simple.h needs Doxygen.
>>
>> 36. bs-scheduler.h needs Doxygen.
>>
>> 37. burst-profile-manager.h
>>
>> /*Note: This class shall actually be a baseclass subclassed by
>> the BS and SS since the two will have different functionaities*/
>>
>> A confusing statement.
>>
>> 38. burst-profile-manager.h needs Doxygen
>>
>> 39. connection-identifier-factory.cc: Lots of hardcoded hex numbers.
>>
>> 40. connection-identifier-factory.h needs Doxygen
>>
>> 41. connection-manager.cc
>>
>> ConnectionManager::ConnectionManager (void) :
>> m_cidFactory (0), m_connectionSetupList (new
>> std::list<std::pair<std::pair<
>> Mac48Address, Ptr<ConnectionIdentifier> >, std::pair<Mac48Address,
>> Ptr<
>> ConnectionIdentifier> > > >), m_connectionSetupList2 (new
>>
> std::list<
>
>> std::pair<Mac48Address, Ptr<ConnectionIdentifier> > >)
>> {
>> }
>>
>> Two new operations
>>
>> ConnectionManager::~ConnectionManager (void)
>> {
>> delete m_connectionSetupList;
>> m_connectionSetupList = 0;
>> }
>>
>> But only one delete. Has this been run through valgrind?
>>
>> My eyes are starting to cross and patterns are developing so I'll just
>>
> finsh
>
>> here. I am concerned about the understandability of examples and the
>> different argument parsing. I don't like the scattered writes to standard
>> out very much. I am concerned about Doxygen completeness, the lack of
>> standard tests and I am very concerned about memory management.
>>
>> I'd like to see valgrind-clean runs before trying to merge any of this ...
>>
>> Regards,
>>
>> -- Craig
>>
>>
>>
>>> -----Original Message-----
>>> From: ns-developers-bounces at ISI.EDU
>>> [mailto:ns-developers-bounces at ISI.EDU] On Behalf Of Faker Moatamri
>>> Sent: Tuesday, September 08, 2009 5:16 AM
>>> To: ns-3-reviews at googlegroups.com; ns-developers at ISI.EDU
>>> Subject: [Ns-developers] WiMAX review request
>>>
>>> Hi all,
>>> I would like to ask you kindly to review the WiMAX module code. The
>>> WiMAX module implements the 802.16 specification.
>>> Here is the list of new/changed files:
>>>
>>> New files:
>>> Added a new directories:
>>> src/devices/wimax
>>> src/applications/trace-based-streamer
>>> src/applications/udp-client-server
>>> Added new files:
>>> examples/wimax.cc
>>> examples/wimax-ipv4.cc
>>> examples/wimax-mulicast.cc
>>> examples/wimax-simple.cc
>>> src/helper/trace-based-streamer-helper.cc/h
>>> src/helper/udp-helper.cc/h
>>> src/helper/wimax-helper.cc/h
>>> Changed:
>>> src/common/pcap-writer.cc/h
>>> src/core/config.h
>>>
>>> Documentation:
>>> It can be found in src/devices/wimax/README, there is also
>>> the attached
>>> wimax.h file which will be finalized and added to the tree
>>> once finished.
>>>
>>> Compiling and building:
>>> Requirments: This module requires the presence of the *IT++
>>> library*, under linux you can install it simply using yum/apt-get
>>> depending on your distribution. Without this library, the wimax code
>>> will be ignored and it will not be compiled. We plan to remove this
>>> dependency on IT++ for next release.
>>> Getting the code:
>>> - You can download the code from
>>> http://code.nsnam.org/iamine/ns-3-wimax-release/ , it is
>>> merged with the
>>> revision 4750 of ns-3-dev
>>> - You can use the attached patch and apply it on
>>> revision 4750
>>> of ns-3-dev
>>> Compile and build:
>>> - You use the usual ./waf configure, you should have WiMAX
>>> module : Enabled to be sure that wimax will be compiled and built
>>> - ./waf to compile and build
>>> Testing:
>>> WiMAX module testing is based on 2 example scenarios that are
>>> used for
>>> regression testing.
>>> To enable the testing you compile with WIMAX_TEST flag:
>>> CXXFLAGS="-DWIMAX_TEST" ./waf configure; ./waf
>>>
>>> You run the tests simply by running the examples:
>>> wimax-ipv4
>>> wimax-multicast
>>> You can also specify the scheduling type using -s option: [-s
>>> scheduling_type]
>>> The scheduling type can be:
>>> - 0 for SCHED_TYPE_SIMPLE (default)
>>> - 1 for SCHED_TYPE_MBQOS
>>> - 2 for SCHED_TYPE_RTPS
>>>
>>> You should get ok as output of the tests.
>>>
>>> Reviewing the code:
>>> The code has been uploaded to the Rietveld reviewing tool:
>>> http://codereview.appspot.com/115051
>>> You can review and put comments on the code.
>>> Thank you very much
>>> Faker Moatamri
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
>
>
More information about the Ns-developers
mailing list