Download/Upload scheduler for wimax

[faker.moatamri, 2009-08-18 07:57:21]

Here is a review of uplink/Downlink scheduler for wimax

http://codereview.appspot.com/107047/diff/1/5
File src/applications/trace-based-streamer/trace-based-streamer.cc (right):

http://codereview.appspot.com/107047/diff/1/5#newcode97
Line 97: strcpy(m_traceFile, "/home/peppe/Desktop/Jurassik_MPEG4hq_.dat");
This is a bad, hardcoded paths are not suitable, can you please find another way
to code that?

http://codereview.appspot.com/107047/diff/1/6
File src/devices/wimax/bandwidth-manager.cc (right):

http://codereview.appspot.com/107047/diff/1/6#newcode137
Line 137: //packet = serviceFlow->GetQueue()->Peek(
please remove all commented code in all your patches... it is code to reread
your patch before submitting it.

http://codereview.appspot.com/107047/diff/1/6#newcode207
Line 207: std::cout << "\tGP_DEBUG: ProcessBandwidthRequest" << std::endl;
it looks like your are debugging with std::out, please make sure that the user
needs those outputs

http://codereview.appspot.com/107047/diff/1/7
File src/devices/wimax/bandwidth-manager.h (right):

http://codereview.appspot.com/107047/diff/1/7#newcode61
Line 61: 
white space changes are not appreciated, please remove them or use -w with diff

http://codereview.appspot.com/107047/diff/1/8
File src/devices/wimax/bs-scheduler-rtps.cc (right):

http://codereview.appspot.com/107047/diff/1/8#newcode116
Line 116: /*
please put it as doxygen so that it can appear in the documentation.

http://codereview.appspot.com/107047/diff/1/8#newcode158
Line 158: /*
can you isolate each bloc into a different function for a matter of clarity and
modularity. this will make the schedule function less huge.

http://codereview.appspot.com/107047/diff/1/10
File src/devices/wimax/bs-scheduler-simple.cc (right):

http://codereview.appspot.com/107047/diff/1/10#newcode268
Line 268: for (iter2 = serviceFlows->begin(); iter2 != serviceFlows->end();
++iter2)
those two for loops can be factorized into one function taking a serviceFlows
and returning a boolean, this will make the code more modular

http://codereview.appspot.com/107047/diff/1/10#newcode302
Line 302: for (iter2 = serviceFlows->begin(); iter2 != serviceFlows->end();
++iter2)
same for these two for loops, they are pretty much copy and paste code.

http://codereview.appspot.com/107047/diff/1/12
File src/devices/wimax/bs-scheduler.cc (right):

http://codereview.appspot.com/107047/diff/1/12#newcode68
Line 68: 
should you here use the bs variable? otherwise this constructor is useless and
should be removed.

http://codereview.appspot.com/107047/diff/1/12#newcode357
Line 357: }*/
please remove all the commented code!

http://codereview.appspot.com/107047/diff/1/12#newcode362
Line 362: m_bs = bs;
this line should be copied to the constructor accepting bs as an entry

http://codereview.appspot.com/107047/diff/1/16
File src/devices/wimax/service-flow-record.cc (right):

http://codereview.appspot.com/107047/diff/1/16#newcode221
Line 221: ServiceFlowRecord::UpdateBacklogged(uint32_t backlogged)
may be call it IncreaseBacklogged (uint32...) to say that it increases
BackLogged by this amount.

http://codereview.appspot.com/107047/diff/1/17
File src/devices/wimax/service-flow-record.h (right):

http://codereview.appspot.com/107047/diff/1/17#newcode139
Line 139: uint32_t m_grantedBandwidthTemp;
why did you add this temporary variable and the one below? can you put some
comments? is it meant to be removed?

http://codereview.appspot.com/107047/diff/1/18
File src/devices/wimax/simple-wimax-phy.cc (right):

http://codereview.appspot.com/107047/diff/1/18#newcode290
Line 290: SimpleWimaxPhy::DoGetNrBytes(uint32_t symbols,
any comments why this function returns always 0?

http://codereview.appspot.com/107047/diff/1/18#newcode294
Line 294: //return (uint64_t)floor((transmissionTime.GetSeconds() *
DoGetDataRate(modulationType))
remove commented code.

http://codereview.appspot.com/107047/diff/1/23
File src/devices/wimax/ul-job.cc (right):

http://codereview.appspot.com/107047/diff/1/23#newcode10
Line 10: //NS_OBJECT_ENSURE_REGISTERED(UlJob);
why is it commented? if not needed, please delete.

http://codereview.appspot.com/107047/diff/1/24
File src/devices/wimax/ul-job.h (right):

http://codereview.appspot.com/107047/diff/1/24#newcode35
Line 35: //TODO: verificar a necessidade de ReqType
did you verify that?

http://codereview.appspot.com/107047/diff/1/25
File src/devices/wimax/uplink-scheduler-mbqos.cc (right):

http://codereview.appspot.com/107047/diff/1/25#newcode82
Line 82: for now temporarily assuming DCD/UCD shall be updated everytime */
please update this comment

http://codereview.appspot.com/107047/diff/1/25#newcode202
Line 202: UplinkSchedulerMBQoS::Schedule (void)
again can you please make this function more modular? 250 lines for a function
make it unreadable

http://codereview.appspot.com/107047/diff/1/25#newcode249
Line 249: //WimaxPhy::ModulationType modulationType = GetBs
()->GetBurstProfileManager ()->GetModulationTypeForBurst (cid);
remove

http://codereview.appspot.com/107047/diff/1/25#newcode929
Line 929: NS_ASSERT (false);
Please use NS_FATAL_ERROR instead

http://codereview.appspot.com/107047/diff/1/25#newcode988
Line 988: UplinkSchedulerMBQoS::OnSetRequestedBandwidth (Ptr<ServiceFlowRecord>
sfr)
is this function used somewhere? should it be removed or should some comments be
added?

http://codereview.appspot.com/107047/diff/1/27
File src/devices/wimax/uplink-scheduler-rtps.cc (right):

http://codereview.appspot.com/107047/diff/1/27#newcode56
Line 56: UplinkSchedulerRtps::UplinkSchedulerRtps(Ptr<WimaxBaseStationNetDevice>
bs)
you can call the constructor with parameter from the constructor with void, i.e.

UplinkSchedulerRtps::UplinkSchedulerRtps ()
  {
    UplinkSchedulerRtps (0);
}
you avoid this way copy and paste.

http://codereview.appspot.com/107047/diff/1/27#newcode94
Line 94: for now temporarily assuming DCD/UCD shall be updated everytime */
this code have been used also in the other file, copy and paste is an easy way
but for maintainibility, it's not convenient at all, anyway, please update the
comments here and think about a way to make it more modular.

http://codereview.appspot.com/107047/diff/1/27#newcode406
Line 406: }
again this function is too huge. modularity

http://codereview.appspot.com/107047/diff/1/29
File src/devices/wimax/uplink-scheduler-simple.cc (right):

http://codereview.appspot.com/107047/diff/1/29#newcode53
Line 53:
UplinkSchedulerSimple::UplinkSchedulerSimple(Ptr<WimaxBaseStationNetDevice> bs)
again, you can call in the void constructor the non void one with bs = 0;

http://codereview.appspot.com/107047/diff/1/29#newcode227
Line 227: #ifdef JDEBUG2
remove

http://codereview.appspot.com/107047/diff/1/29#newcode324
Line 324: }
modulorize

http://codereview.appspot.com/107047/diff/1/34
File src/devices/wimax/wimax-bs-net-device.h (right):

http://codereview.appspot.com/107047/diff/1/34#newcode34
Line 34: //#include "uplink-scheduler.h"
remvoe

http://codereview.appspot.com/107047/diff/1/34#newcode272
Line 272: //UplinkScheduler *m_uplinkScheduler;
remove

http://codereview.appspot.com/107047/diff/1/36
File src/devices/wimax/wimax-mac-queue.h (right):

http://codereview.appspot.com/107047/diff/1/36#newcode92
Line 92: 
avoid white space changes

http://codereview.appspot.com/107047/diff/1/37
File src/devices/wimax/wimax-net-device.cc (right):

http://codereview.appspot.com/107047/diff/1/37#newcode60
Line 60: "Node", "XXX", PointerValue(), MakePointerAccessor(
please put real comments instead of xxx

http://codereview.appspot.com/107047/diff/1/37#newcode576
Line 576: /*
remove this code if it is commented

http://codereview.appspot.com/107047/diff/1/44
File src/helper/wimax-helper.h (right):

http://codereview.appspot.com/107047/diff/1/44#newcode195
Line 195: WimaxChannel> channel, SchedulerType schedulerType, SchedulerType
schedulerType);
why this functions is accepting two scheduler types?