Wimax module review

[faker.moatamri, 2009-07-01 09:24:25]

http://codereview.appspot.com/75057/diff/1/2
File examples/Mysimple-wimax.cc (right):

http://codereview.appspot.com/75057/diff/1/2#newcode1
Line 1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
Please verify coding style in all the files, specially the spaces when defining
a class, function and space before parentheses... those king of errors are in
almost all the files in the wimax package.

http://codereview.appspot.com/75057/diff/1/2#newcode37
Line 37: //LogComponentEnable ("SimpleWimaxSimulation", LOG_LEVEL_INFO);
Please remove commented code in all files...

http://codereview.appspot.com/75057/diff/1/2#newcode107
Line 107: 
some comments would be welcome in this file to describe the steps of configuring
a simple wimax example...

http://codereview.appspot.com/75057/diff/1/3
File examples/simple-wimax.cc (right):

http://codereview.appspot.com/75057/diff/1/3#newcode61
Line 61: NS_ASSERT (false);
Please use NS_ERROR instead of asserting false, with an understandable message.
The following line will never be reached because of the NS_ASSERT(false)
statement

http://codereview.appspot.com/75057/diff/1/3#newcode78
Line 78: SSRxTrace (std::string context, Ptr<const Packet> packet, Mac48Address
address, Ptr<ConnectionIdentifier> cid)
those displaying functions could be encapsulated into a utility helper class
that will have the responsibility to display wimax trace. This will be much more
useful as you will be able to reuse it everywhere in the code.

http://codereview.appspot.com/75057/diff/1/3#newcode84
Line 84: //<< ", packet: " << *packet
again commented code have to be removed

http://codereview.appspot.com/75057/diff/1/3#newcode204
Line 204: //Config::Connect ("/NodeList/*/DeviceList/*/Phy/Rx", MakeCallback
(&OfdmPhyRxTrace));
same thing, remove commented code.
Also again the file as important as this one needs more comments to say what you
are doing at each step, this file and the other example will be most probably
the entry point of new users, so it needs to be explained more by adding
comments.

http://codereview.appspot.com/75057/diff/1/5
File src/applications/UdpClientServer/udpClient.cc (right):

http://codereview.appspot.com/75057/diff/1/5#newcode100
Line 100: m_socket = Socket::CreateSocket(GetNode(), tid);
coding style for all of your code...

http://codereview.appspot.com/75057/diff/1/5#newcode145
Line 145: NS_LOG_INFO("Error while sending " << m_size << " bytes to "
should be an NS_ERROR instead no?

http://codereview.appspot.com/75057/diff/1/6
File src/applications/UdpClientServer/udpClient.h (right):

http://codereview.appspot.com/75057/diff/1/6#newcode53
Line 53: SetRemote(Ipv4Address ip, uint16_t port);
space before the parentheses SetRemote () in all files

http://codereview.appspot.com/75057/diff/1/6#newcode63
Line 63: virtual void
why use two lines when only one is needed, specially if the name of the function
is small?

http://codereview.appspot.com/75057/diff/1/7
File src/applications/UdpClientServer/udpServer.cc (right):

http://codereview.appspot.com/75057/diff/1/7#newcode38
Line 38: UdpServer::GetTypeId(void)
coding style GetTypeId () ... for all the file, please space before parentheses

http://codereview.appspot.com/75057/diff/1/14
File src/devices/wimax/IPCS_BS_Bind_Parameter_Record.cc (right):

http://codereview.appspot.com/75057/diff/1/14#newcode1
Line 1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
Those files names should be changed for example
ipcs-bs-bind-parameter-record.cc, please refer to coding and naming style.
The class name would be BindParameterRecord...

http://codereview.appspot.com/75057/diff/1/14#newcode41
Line 41: IPCS_BS_Bind_Parameter_Record::~IPCS_BS_Bind_Parameter_Record(void)
space before brackets

http://codereview.appspot.com/75057/diff/1/14#newcode107
Line 107: iter2->Get_m_liste_parameter_record();
should verify that iter2 is not 0 before calling any method on it, since the
GetIPCS... can return a null pointer. Same for the following functions.

http://codereview.appspot.com/75057/diff/1/14#newcode108
Line 108: return mm_liste_parameter_record->IsInRecord(dst_ip_adr,
check also if mm_liste_parameter_record is not null also.

http://codereview.appspot.com/75057/diff/1/14#newcode151
Line 151: IPCS_BS_Bind_Parameter_Record::GetNSSs(void) const
should be named GetNSSSize for more clarity.

http://codereview.appspot.com/75057/diff/1/15
File src/devices/wimax/IPCS_BS_Bind_Parameter_Record.h (right):

http://codereview.appspot.com/75057/diff/1/15#newcode21
Line 21: 
Brief description will be very welcome here...

http://codereview.appspot.com/75057/diff/1/15#newcode32
Line 32: IPCS_BS_Bind_Parameter_Record(void);
indentation + space before parentheses

http://codereview.appspot.com/75057/diff/1/15#newcode43
Line 43: std::vector<SSRecord*> *m_liste_SS_parameter_record;
again

http://codereview.appspot.com/75057/diff/1/16
File src/devices/wimax/IPCS_BS_Packet_classifier.cc (right):

http://codereview.appspot.com/75057/diff/1/16#newcode30
Line 30: #include "../../common/packet.h"
we should not have that kind of include statements with ../../header.h, we
should discuss about the way to replace that by using attributes for example.

http://codereview.appspot.com/75057/diff/1/16#newcode41
Line 41: {
please coding style

http://codereview.appspot.com/75057/diff/1/16#newcode77
Line 77: C_Packet->RemoveHeader(llc);
space before ()

http://codereview.appspot.com/75057/diff/1/16#newcode78
Line 78: C_Packet->RemoveHeader(IPV4_Header);
Some comments will be welcome in this function or at least doxygen in the header
file.

http://codereview.appspot.com/75057/diff/1/16#newcode110
Line 110: if (ss_record == true)
no need to ==, just if (ss_record) is sufficient

http://codereview.appspot.com/75057/diff/1/16#newcode118
Line 118: if (packet_record == true)
same as above

http://codereview.appspot.com/75057/diff/1/16#newcode125
Line 125: if (SFID == ServiceFlow::SFID_TYPE_UGS)
here please use if else statements or switch() case statement to avoid checking
SFID multiple times uselessly

http://codereview.appspot.com/75057/diff/1/17
File src/devices/wimax/IPCS_BS_Packet_classifier.h (right):

http://codereview.appspot.com/75057/diff/1/17#newcode36
Line 36: 
coding style:
namespace n3 {

/**
 * \brief short one-line description of the purpose of your class
 * 
 * A longer description of the purpose of your class after a blank
 * empty line.
 */
class MyClass
{
public:
 MyClass ();
 /**
  * \param firstParam a short description of the purpose of this parameter
  * \returns a short description of what is returned from this function.
  *
  * A detailed description of the purpose of the method.
  */
 int DoSomething (int firstParam);
private:
 void MyPrivateMethod (void);
 int m_myPrivateMemberVariable;
};

} // namespace ns3

http://codereview.appspot.com/75057/diff/1/17#newcode48
Line 48: ServiceFlow
again, please revise the whole file for coding style errors

http://codereview.appspot.com/75057/diff/1/17#newcode52
Line 52: IPCS_BS_Bind_Parameter_Record *m_IPCS_BS_Bind_Parameter_Record;
why this member variable is public? member variables should never be accessed
directly

http://codereview.appspot.com/75057/diff/1/18
File src/devices/wimax/IPCS_Bind_parameter_manager.cc (right):

http://codereview.appspot.com/75057/diff/1/18#newcode33
Line 33: {
coding style, please refer to coding style web site

http://codereview.appspot.com/75057/diff/1/19
File src/devices/wimax/IPCS_Bind_parameter_manager.h (right):

http://codereview.appspot.com/75057/diff/1/19#newcode34
Line 34: IPCS_Bind_parameter_manager(void);
why indentation, remove commented code

http://codereview.appspot.com/75057/diff/1/20
File src/devices/wimax/IPCS_Classifier_Record.cc (right):

http://codereview.appspot.com/75057/diff/1/20#newcode28
Line 28: 
again coding style

http://codereview.appspot.com/75057/diff/1/20#newcode75
Line 75: {
coding style

http://codereview.appspot.com/75057/diff/1/21
File src/devices/wimax/IPCS_Classifier_Record.h (right):

http://codereview.appspot.com/75057/diff/1/21#newcode31
Line 31: 
Coding style

http://codereview.appspot.com/75057/diff/1/22
File src/devices/wimax/IPCS_SS_Packet_classifier.cc (right):

http://codereview.appspot.com/75057/diff/1/22#newcode29
Line 29: #include "../../common/packet.h"
no, please manage to remove ../../ or talk with mathieu to remove that. Usage of
attributes system would resolve that.

http://codereview.appspot.com/75057/diff/1/22#newcode63
Line 63: ServiceFlow*
adding at least doxygen is really a must...

http://codereview.appspot.com/75057/diff/1/22#newcode109
Line 109: if (record == true)
if (record) is sufficient

http://codereview.appspot.com/75057/diff/1/22#newcode116
Line 116: if (SFID == ServiceFlow::SFID_TYPE_UGS)
please use if else statement or switch case, we have multiple if checks for
nothing

http://codereview.appspot.com/75057/diff/1/22#newcode143
Line 143: return NULL;
please return 0 instead (refer to coding style web page)

http://codereview.appspot.com/75057/diff/1/23
File src/devices/wimax/IPCS_SS_Packet_classifier.h (right):

http://codereview.appspot.com/75057/diff/1/23#newcode46
Line 46: IPCS_Bind_parameter_manager *m_IPCS_Bind_parameter_manager;
member variables should never be public and should never accessed directly

http://codereview.appspot.com/75057/diff/1/24
File src/devices/wimax/Mac2Mac.cc (right):

http://codereview.appspot.com/75057/diff/1/24#newcode27
Line 27: 
should we include here the ensure registred statement for the class to be able
to use the attribute system NS_OBJECT_ENSURE_REGISTERED (Mac2Mac);?

http://codereview.appspot.com/75057/diff/1/24#newcode43
Line 43: return 0;
why return 0? unless the program fails in which case you will see it it will
always return 0. So I think it is useless here.

http://codereview.appspot.com/75057/diff/1/25
File src/devices/wimax/Mac2Mac.h (right):

http://codereview.appspot.com/75057/diff/1/25#newcode26
Line 26: {
coding style

http://codereview.appspot.com/75057/diff/1/25#newcode28
Line 28: class Mac2Mac : public Header
A little bit of doxygen won't hurt here!

http://codereview.appspot.com/75057/diff/1/26
File src/devices/wimax/README (right):

http://codereview.appspot.com/75057/diff/1/26#newcode1
Line 1: Summary of the NS-3 IEEE 802.16 WiMAX module
This file needs a lot of changes:
1- It has to be updated to the latest state of the development, please read it
carefully and change it as needed and please take into account the point 2.
2- Reading 180 lines of text in one shot without any partition is really
unhuman, there is no introduction as of the functionalities that will be
presented. I would suggest having titles for related paragraphs and to have a
small paragraph at the beginning saying what we will be talking about.

http://codereview.appspot.com/75057/diff/1/27
File src/devices/wimax/RELEASE_NOTES (right):

http://codereview.appspot.com/75057/diff/1/27#newcode2
Line 2: ns-3-wimax RELEASE NOTES
Is there any known issues with the current release, in that case it will be nice
to add them here.

http://codereview.appspot.com/75057/diff/1/28
File src/devices/wimax/TLVs.cc (right):

http://codereview.appspot.com/75057/diff/1/28#newcode26
Line 26: 
coding style again

http://codereview.appspot.com/75057/diff/1/28#newcode28
Line 28: {
coding style

http://codereview.appspot.com/75057/diff/1/28#newcode29
Line 29: // TODO Auto-generated constructor stub
please remove the TODO if you will not need to do nothing about it.

http://codereview.appspot.com/75057/diff/1/28#newcode38
Line 38: return 0;
why return 0?

http://codereview.appspot.com/75057/diff/1/28#newcode129
Line 129: // TODO Auto-generated destructor stub
remove the TODO if you don't plan to change anything about it

http://codereview.appspot.com/75057/diff/1/29
File src/devices/wimax/TLVs.h (right):

http://codereview.appspot.com/75057/diff/1/29#newcode25
Line 25: namespace ns3 {
coding style again

http://codereview.appspot.com/75057/diff/1/29#newcode30
Line 30: TLVs();
coding style, please use two spaces not 2 tabs as spacement.

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

http://codereview.appspot.com/75057/diff/1/31#newcode38
Line 38: 
coding style

http://codereview.appspot.com/75057/diff/1/31#newcode41
Line 41: {
same as above...

http://codereview.appspot.com/75057/diff/1/31#newcode67
Line 67: {
coding style

http://codereview.appspot.com/75057/diff/1/31#newcode105
Line 105: NS_ASSERT(false);
please generate an NS_ERROR

http://codereview.appspot.com/75057/diff/1/31#newcode115
Line 115: ServiceFlow *serviceFlow = NULL;
please use 0 instead of NULL

http://codereview.appspot.com/75057/diff/1/31#newcode144
Line 144: return serviceFlow;
you are returning the last service flow of serviceFlows which is the one before
serviceFlows->End(), was that done on purpose?

http://codereview.appspot.com/75057/diff/1/31#newcode170
Line 170: bwRequestHdr.SetType(
coding style,space before parentheses

http://codereview.appspot.com/75057/diff/1/31#newcode219
Line 219: /*XXX, temporarily divided in half (360 symbols each), shall actually
be determined based on UL and DL traffic*/
Is there a bug report or something to remind developers about this point?

http://codereview.appspot.com/75057/diff/1/31#newcode244
Line 244: uint32_t allocatedSymbols = ugsAllocationPerFrame +
rtpsAllocationPerFrame
only ugsAllocationPerFrame is not 0 right? I don't see why we're using the 3
other variables which remain 0.

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

http://codereview.appspot.com/75057/diff/1/32#newcode33
Line 33: 
coding style...

http://codereview.appspot.com/75057/diff/1/33
File src/devices/wimax/bs-link-manager.cc (right):

http://codereview.appspot.com/75057/diff/1/33#newcode38
Line 38: BSLinkManager::BSLinkManager(Ptr<WimaxBaseStationNetDevice> bs) :
coding style

http://codereview.appspot.com/75057/diff/1/33#newcode39
Line 39: m_bs(bs), m_signalQuality(10) //XXX, arbitrary value
unless you want to change this value, please remove XXX

http://codereview.appspot.com/75057/diff/1/33#newcode41
Line 41: tries = 0;//XXX, temp
remove the tries variable since it is temp!

http://codereview.appspot.com/75057/diff/1/33#newcode53
Line 53: //temp, randomly selecting TOs up to 10, shall actually be decided by
scheduler
a lot of temporary stuff in this class, needs to be cleaned up

http://codereview.appspot.com/75057/diff/1/33#newcode112
Line 112: //XXX, temp, assuming low power, packet lost or undecodable first 2
times
again temp

http://codereview.appspot.com/75057/diff/1/34
File src/devices/wimax/bs-link-manager.h (right):

http://codereview.appspot.com/75057/diff/1/34#newcode32
Line 32: {
coding style

http://codereview.appspot.com/75057/diff/1/34#newcode88
Line 88: int tries; //temp variable to test multiple RNG-REQ by the SS for
example undecodable at BS or lost RNG-RSP
shall be removed no?

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

http://codereview.appspot.com/75057/diff/1/35#newcode112
Line 112: //modulationType = m_bs->GetBurstProfileManager
()->GetModulationTypeForBurst (connection->GetCid ());
remove commented code

http://codereview.appspot.com/75057/diff/1/36
File src/devices/wimax/bs-scheduler.h (right):

http://codereview.appspot.com/75057/diff/1/36#newcode1
Line 1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
Please revise the coding style in all the files of this project, almost all
files are affected...

http://codereview.appspot.com/75057/diff/1/37
File src/devices/wimax/burst-profile-manager.cc (right):

http://codereview.appspot.com/75057/diff/1/37#newcode48
Line 48: /*XXX, assuming that always all the burst profiles are included in the
UCD, not clear
This might need a check???

http://codereview.appspot.com/75057/diff/1/37#newcode85
Line 85: NS_ASSERT(false);
Generate an error

http://codereview.appspot.com/75057/diff/1/37#newcode118
Line 118: NS_ASSERT(false);
same

http://codereview.appspot.com/75057/diff/1/37#newcode127
Line 127: /*during initial ranging or periodic ranging (or when RNG-REQ is used
instead of
does it need review ore please remove the temporary comment.

http://codereview.appspot.com/75057/diff/1/37#newcode145
Line 145: /*This function is not currently being used. It was previously being
used in
If still not used, please remove it.

http://codereview.appspot.com/75057/diff/1/38
File src/devices/wimax/burst-profile-manager.h (right):

http://codereview.appspot.com/75057/diff/1/38#newcode47
Line 47: /*these two functions are only required if DIUC/UIUC to modulation type
/**
 *
 */

http://codereview.appspot.com/75057/diff/1/41
File src/devices/wimax/connection-identifier.cc (right):

http://codereview.appspot.com/75057/diff/1/41#newcode35
Line 35: ConnectionIdentifier> () .AddAttribute("Identifier", "XXX",
please replace XXX by a phrase meaning something...

http://codereview.appspot.com/75057/diff/1/41#newcode73
Line 73: return !(lhs == rhs);
should we not compare the m_identifier like the above function?

http://codereview.appspot.com/75057/diff/1/43
File src/devices/wimax/connection-manager.cc (right):

http://codereview.appspot.com/75057/diff/1/43#newcode183
Line 183: NS_ASSERT(false);
please generate an error for all assert(false) statements

http://codereview.appspot.com/75057/diff/1/43#newcode335
Line 335: ; // namespace ns3
useless semi column

http://codereview.appspot.com/75057/diff/1/44
File src/devices/wimax/connection-manager.h (right):

http://codereview.appspot.com/75057/diff/1/44#newcode77
Line 77: ConnectionIdentifierFactory *m_cidFactory;
should be private and accessed using accessor getters/setters.

http://codereview.appspot.com/75057/diff/1/45
File src/devices/wimax/crc8.cc (right):

http://codereview.appspot.com/75057/diff/1/45#newcode24
Line 24: *   CRC8 table on the polynomial:
More comments on what this function is doing will be welcome

http://codereview.appspot.com/75057/diff/1/48
File src/devices/wimax/dl-mac-messages.h (right):

http://codereview.appspot.com/75057/diff/1/48#newcode1
Line 1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
why not using one file for each class, defining 4 classes in one file might be a
bad practice.

http://codereview.appspot.com/75057/diff/1/48#newcode55
Line 55: uint16_t m_eir_x_p_ir_max;
what's this name, it needs a small explanation eir_x_p_ir_max!!!

http://codereview.appspot.com/75057/diff/1/48#newcode59
Line 59: //uint8_t m_macVersion;
remove commented code

http://codereview.appspot.com/75057/diff/1/48#newcode265
Line 265: //m_extendedDiucDependentIe;
please kill all commented code

http://codereview.appspot.com/75057/diff/1/51
File src/devices/wimax/mac-messages.cc (right):

http://codereview.appspot.com/75057/diff/1/51#newcode444
Line 444: return 1 + 4 + 1 + 4 + 1 + 4 + 1 + 2 + 6 + m_basicCid.GetSize()
details of the numbers will be appreciated

http://codereview.appspot.com/75057/diff/1/52
File src/devices/wimax/mac-messages.h (right):

http://codereview.appspot.com/75057/diff/1/52#newcode1
Line 1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
why group 6 classes in one file???

http://codereview.appspot.com/75057/diff/1/52#newcode140
Line 140: //uint8_t m_aasBroadcastCapability;
kill all commented code

http://codereview.appspot.com/75057/diff/1/55
File src/devices/wimax/ofdm-wimax-channel.cc (right):

http://codereview.appspot.com/75057/diff/1/55#newcode59
Line 59: for (std::list<Ptr<OfdmWimaxPhy> >::const_iterator iter =
m_phyList.begin(); iter
you can also use 
for (int i = 0; i<index; i++) 
{
 ++iter;
}
return (*iter)->GetDevice();
This avoid the multiple if checks

http://codereview.appspot.com/75057/diff/1/55#newcode69
Line 69: NS_ASSERT(false);
NS_ERROR!

http://codereview.appspot.com/75057/diff/1/58
File src/devices/wimax/ofdm-wimax-phy.h (right):

http://codereview.appspot.com/75057/diff/1/58#newcode114
Line 114: static Ptr<PacketBurst> m_burstCopy; //copy of burst before it is
splitted into blocks, used at the receiving side to get packet metadata and tags
class member variables should always be private and remove temporary code

http://codereview.appspot.com/75057/diff/1/63
File src/devices/wimax/qos-parameter-set.cc (right):

http://codereview.appspot.com/75057/diff/1/63#newcode235
Line 235: #if 0
Why if 0? this is never executed then, please remove or correct in all the
files.

http://codereview.appspot.com/75057/diff/1/63#newcode332
Line 332: NS_ASSERT(false);
NS_ERROR

http://codereview.appspot.com/75057/diff/1/66
File src/devices/wimax/send-params.h (right):

http://codereview.appspot.com/75057/diff/1/66#newcode70
Line 70: //defining modulationType as WimaxPhy::ModulationType did not work for
some unknown reasons
Is it a bug reported?

http://codereview.appspot.com/75057/diff/1/66#newcode75
Line 75: 
The member variables should not be public and should not be accessed directly

http://codereview.appspot.com/75057/diff/1/67
File src/devices/wimax/service-flow-manager.cc (right):

http://codereview.appspot.com/75057/diff/1/67#newcode123
Line 123: //ServiceFlow *UGS_serviceFlow =
why this commented code? should remove it or explain.

http://codereview.appspot.com/75057/diff/1/67#newcode150
Line 150: else
you don't need the else here, and please return 0 not NULL.

http://codereview.appspot.com/75057/diff/1/67#newcode164
Line 164: //QoSParameterSet* m_parameterSet=serviceFlow->GetParameterSet();
delete

http://codereview.appspot.com/75057/diff/1/67#newcode168
Line 168: void
Please justify the presence of this function, it calls the function that is just
after it and it does the same thing, the parameter schedulingType is not used at
all.

http://codereview.appspot.com/75057/diff/1/67#newcode174
Line 174: 
schedulingType variable is not used, please remove it or justify its presence

http://codereview.appspot.com/75057/diff/1/67#newcode216
Line 216: NS_ASSERT(false);
NS_ERROR for all NS_ASSERT (false)

http://codereview.appspot.com/75057/diff/1/67#newcode398
Line 398: exit(0);
This should be an error no?

http://codereview.appspot.com/75057/diff/1/68
File src/devices/wimax/service-flow-manager.h (right):

http://codereview.appspot.com/75057/diff/1/68#newcode90
Line 90: ServiceFlow *UGS_serviceFlow;
They should not be public

[faker.moatamri, 2009-07-01 09:52:43]

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

http://codereview.appspot.com/75057/diff/1/70#newcode104
Line 104: //stats members
They should be private

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

http://codereview.appspot.com/75057/diff/1/71#newcode47
Line 47: if (schedulingType == QoSParameterSet::SCHEDULING_TYPE_UGS)
you'd rather use switch case here since you are switching behaviour based on the
schedulingType argument unless the Sfid can be set multiple times.

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

http://codereview.appspot.com/75057/diff/1/72#newcode55
Line 55: //ServiceFlow (Direction direction,QoSParameterSet::SchedulingType
schedulingType);
delete

[faker.moatamri, 2009-07-01 09:55:45]

http://codereview.appspot.com/75057/diff/1/73
File src/devices/wimax/simple-wimax-channel.cc (right):

http://codereview.appspot.com/75057/diff/1/73#newcode68
Line 68: NS_ASSERT(false);
NS_ERROR

[faker.moatamri, 2009-07-01 12:45:10]

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

http://codereview.appspot.com/75057/diff/1/75#newcode44
Line 44: "Rx", "XXX",
Real comments please, not XXX

http://codereview.appspot.com/75057/diff/1/75#newcode91
Line 91: //uint32_t dr=DoGetDataRate ((WimaxPhy::ModulationType)
s_params->m_modulationType);
kill

http://codereview.appspot.com/75057/diff/1/75#newcode127
Line 127: double distance = 10; //double distance = CalculateDistance (position,
m_node->GetPosition ());
is this a temporary change or will remain 10? if so remove commented code,
please remove all commented code in the code

http://codereview.appspot.com/75057/diff/1/75#newcode162
Line 162: }
Should put defaults and generate an error.

http://codereview.appspot.com/75057/diff/1/75#newcode221
Line 221: }
default behavior again

http://codereview.appspot.com/75057/diff/1/75#newcode268
Line 268: NS_ASSERT(false); //temp
I see that this function is not used at all since there is the NS_ASSERT(false),
please remove this function.

http://codereview.appspot.com/75057/diff/1/76
File src/devices/wimax/simple-wimax-phy.h (right):

http://codereview.appspot.com/75057/diff/1/76#newcode74
Line 74: //currently not used
if still not used please remove it

[faker.moatamri, 2009-07-06 14:27:01]

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

http://codereview.appspot.com/75057/diff/1/75#newcode268
Line 268: NS_ASSERT(false); //temp
Never mind, please generate an error instead

http://codereview.appspot.com/75057/diff/1/77
File src/devices/wimax/simpleOfdmWimaxChannel.cc (right):

http://codereview.appspot.com/75057/diff/1/77#newcode70
Line 70: NS_ASSERT(false);
Please generate an error!

http://codereview.appspot.com/75057/diff/1/79
File src/devices/wimax/simpleOfdmWimaxPhy.cc (right):

http://codereview.appspot.com/75057/diff/1/79#newcode1
Line 1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
please fix the file name

http://codereview.appspot.com/75057/diff/1/79#newcode50
Line 50: "Rx", "XXX", MakeTraceSourceAccessor(
Real comments will be appreciated instead of XXX

http://codereview.appspot.com/75057/diff/1/79#newcode109
Line 109: //std::cout << "Printing burst bytes original " << std::endl;
PrintBurstBuffer (burst);
please remove with all commented code

http://codereview.appspot.com/75057/diff/1/79#newcode225
Line 225: }
default behavior?

http://codereview.appspot.com/75057/diff/1/79#newcode234
Line 234: double distance = 10; //double distance = CalculateDistance (position,
m_node->GetPosition ());
remove commented code

http://codereview.appspot.com/75057/diff/1/79#newcode310
Line 310: itpp::bvec rFecBlock = ofdmPhySimulator.receive_symbol(fecBlock,
name of the function receive_symbol should be changed to ReceiveSymbol, please 
do that for all function names.

http://codereview.appspot.com/75057/diff/1/79#newcode368
Line 368: /*
Nice to see some comments on a function, we don't see that much in the Wimax
code. It would be really cool if you can add comments to functions and
variables, they will be useful to generate doxygen documents, but mostly useful
for users, developers and maintainers.

http://codereview.appspot.com/75057/diff/1/79#newcode469
Line 469: simpleOfdmWimaxPhy::GetModulationFecParams(
I've seen this function and the 2 next ones before, may be those functions
should be an abstract function in WimaxPhy with a default implementation and
might be overriden if needed. There is no use of repeating the same code several
times specially for maintainance and portablility issues.

http://codereview.appspot.com/75057/diff/1/79#newcode547
Line 547: NS_ASSERT(false); //temp
Again remove this please and generate an error

http://codereview.appspot.com/75057/diff/1/79#newcode651
Line 651: double duration = GetFrameDuration().GetMilliSeconds(); //to
milliseconds
please use switch case instead of the multiple if else blocks, also remove temp
code and put an NS_ERROR as default behavior

http://codereview.appspot.com/75057/diff/1/79#newcode771
Line 771: //double psDurationd1 = (double) 4 / samplingFrequency;
remove...

http://codereview.appspot.com/75057/diff/1/79#newcode796
Line 796: return 256; //Nfft, see pages 429 and 812
When refer to a question number or page number, is it documented somewhere what
you are referring to?

http://codereview.appspot.com/75057/diff/1/80
File src/devices/wimax/simpleOfdmWimaxPhy.h (right):

http://codereview.appspot.com/75057/diff/1/80#newcode119
Line 119: static Ptr<PacketBurst> m_burstCopy; //copy of burst before it is
splitted into blocks, used at the receiving side to get packet metadata and tags
why are they public? can't you just include a getter and put them private

http://codereview.appspot.com/75057/diff/1/80#newcode177
Line 177: //TracedCallback<WimaxPhy::ModulationType, uint16_t, uint16_t>
m_traceRx;
remove

http://codereview.appspot.com/75057/diff/1/81
File src/devices/wimax/ss-link-manager.cc (right):

http://codereview.appspot.com/75057/diff/1/81#newcode74
Line 74: m_eir_x_p_ir_max = eir_x_p_ir_max;
this name should be changed, it should be m_eirXPIrMax, the same for other names
using more than one '_'

http://codereview.appspot.com/75057/diff/1/81#newcode116
Line 116: //temp parameter "type" just to check on expiry of which event the
function was called
type parameter should be removed as it is not used

http://codereview.appspot.com/75057/diff/1/81#newcode194
Line 194: {
coding style space here

http://codereview.appspot.com/75057/diff/1/81#newcode208
Line 208: //XXX, temp, just to confirm
either remove the comment or remove the code and the comment

http://codereview.appspot.com/75057/diff/1/81#newcode208
Line 208: //XXX, temp, just to confirm
either remove the comment or remove the code and the comment

http://codereview.appspot.com/75057/diff/1/81#newcode301
Line 301: void
this function should be divided into smaller functions, it is too long

http://codereview.appspot.com/75057/diff/1/81#newcode339
Line 339: //either periodic ranging or an additional RNG-RSP during initial
ranging
is it normal that there is no instructions here?

http://codereview.appspot.com/75057/diff/1/81#newcode356
Line 356: //code to move to new channel/frequency goes here
???

http://codereview.appspot.com/75057/diff/1/81#newcode415
Line 415: temp = rngrsp.GetOffsetFreqAdjust ();
temp is overwritten 3 times and the successful boolean never updated, I'm
wondering if this function is useful/used?

http://codereview.appspot.com/75057/diff/1/83
File src/devices/wimax/ss-manager.cc (right):

http://codereview.appspot.com/75057/diff/1/83#newcode128
Line 128: return false;
This instruction is useless and is never reached, please delete

http://codereview.appspot.com/75057/diff/1/83#newcode132
Line 132: SSManager::DeleteSSRecord(Ptr<const ConnectionIdentifier> cid)
it might be useful to return true or false to have a feedback on whether the cid
was found and deleted or not.

http://codereview.appspot.com/75057/diff/1/85
File src/devices/wimax/ss-record.cc (right):

http://codereview.appspot.com/75057/diff/1/85#newcode57
Line 57: delete m_serviceFlows;
Please verify that you don't need recusive deletion of the vectors.

http://codereview.appspot.com/75057/diff/1/85#newcode237
Line 237: NS_ASSERT(false);
NS_ERROR

http://codereview.appspot.com/75057/diff/1/85#newcode260
Line 260: break;
Break are useless but you can keep them if you want

http://codereview.appspot.com/75057/diff/1/85#newcode268
Line 268: NS_ASSERT(false);
those two lines will never be reached so please delete them and use NS_ERROR
above.

http://codereview.appspot.com/75057/diff/1/85#newcode293
Line 293: 
same

http://codereview.appspot.com/75057/diff/1/86
File src/devices/wimax/ss-record.h (right):

http://codereview.appspot.com/75057/diff/1/86#newcode140
Line 140: IPCS_Bind_parameter_manager* m_liste_parameter_record;
why this variable is public? all the member variable should be private or
protected...

http://codereview.appspot.com/75057/diff/1/86#newcode161
Line 161: //IPCS_Bind_parameter_manager* m_liste_parameter_record;
remove

http://codereview.appspot.com/75057/diff/1/87
File src/devices/wimax/ss-scheduler.cc (right):

http://codereview.appspot.com/75057/diff/1/87#newcode40
Line 40: SSScheduler::SSScheduler (Ptr<WimaxSubscriberStationNetDevice> ss)
Should be called something more understandable like SubscriberStationScheduler,
SSScheduler is too vague...

http://codereview.appspot.com/75057/diff/1/87#newcode163
Line 163: serviceFlows = m_ss->GetServiceFlowManager ()->GetServiceFlows
(QoSParameterSet::SCHEDULING_TYPE_RTPS);
Those three blocks are very similar, the only difference is SCHEDULING_TYPE_...
it would be worth it to create a function that does that with the above
parameter as input and connection as output.

http://codereview.appspot.com/75057/diff/1/88
File src/devices/wimax/ss-scheduler.h (right):

http://codereview.appspot.com/75057/diff/1/88#newcode50
Line 50: //Ptr<WimaxSubscriberStationNetDevice> m_ss;
remove

http://codereview.appspot.com/75057/diff/1/89
File src/devices/wimax/ul-mac-messages.cc (right):

http://codereview.appspot.com/75057/diff/1/89#newcode677
Line 677: while (true)
why not putting while (ulMapIe.GetUiuc() != 14) instead?

http://codereview.appspot.com/75057/diff/1/90
File src/devices/wimax/ul-mac-messages.h (right):

http://codereview.appspot.com/75057/diff/1/90#newcode1
Line 1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
Too many classes in one file, it's better to do one class one file...

http://codereview.appspot.com/75057/diff/1/90#newcode51
Line 51: //may be required later on
remove all commented code...

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

http://codereview.appspot.com/75057/diff/1/91#newcode84
Line 84: /*XXX, DCD and UCD shall actually be updated when channel or burst
profile definitions
Will this be changed some time? otherwise remove the 'temporarily' in the
phrases

http://codereview.appspot.com/75057/diff/1/91#newcode146
Line 146: UplinkScheduler::Schedule (void)
this function is too long, can you please make it more partitioned

http://codereview.appspot.com/75057/diff/1/91#newcode193
Line 193: //WimaxPhy::ModulationType modulationType =
m_bs->GetBurstProfileManager ()->GetModulationTypeForBurst (cid);
remove all commented code

http://codereview.appspot.com/75057/diff/1/91#newcode525
Line 525: NS_ASSERT (false);
NS_ERROR

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

http://codereview.appspot.com/75057/diff/1/93#newcode64
Line 64: "XXX",
Please real comments

http://codereview.appspot.com/75057/diff/1/93#newcode97
Line 97: mbs_classifier = new IPCS_BS_Packet_classifier();
this new should be associated with a delete somewhere but I don't see any delete
mbs_classifier statement, by the way the name of the variable should be
m_mbsClassifier...

http://codereview.appspot.com/75057/diff/1/93#newcode354
Line 354: //#ifdef JDEBUG2
remove

http://codereview.appspot.com/75057/diff/1/93#newcode418
Line 418: StartFrame();
at EndFrame you call StartFrame? is this normal?

http://codereview.appspot.com/75057/diff/1/93#newcode441
Line 441: if (protocolNumber == 2048)
2048? can you please call as a static variable with a good name? #define
NAME_OF_THE_VARIABLE 2048

http://codereview.appspot.com/75057/diff/1/93#newcode485
Line 485: void
240 lines for one function is too much, consider dividing the function or
delegating some functionalities to other classes

http://codereview.appspot.com/75057/diff/1/93#newcode493
Line 493: RngReq rngreq;
rngReq

http://codereview.appspot.com/75057/diff/1/93#newcode501
Line 501: //packet->RemoveHeader (hdrType);
remove all commented code

http://codereview.appspot.com/75057/diff/1/93#newcode549
Line 549: NS_ASSERT(false);
NS_ERROR

http://codereview.appspot.com/75057/diff/1/93#newcode967
Line 967: Ptr<WimaxConnection> connection = NULL;
= 0

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

http://codereview.appspot.com/75057/diff/1/94#newcode1
Line 1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
This class is too long, you should delegate some of the functionalities to other
classes. 270 lines for the .h and 1000 lines for the .cc, too long!

http://codereview.appspot.com/75057/diff/1/94#newcode32
Line 32: #include "IPCS_BS_Packet_classifier.h"
ipcs-bs-packet-classifier.h

http://codereview.appspot.com/75057/diff/1/94#newcode158
Line 158: IPCS_BS_Packet_classifier* mbs_classifier;
Why this variable is public?

http://codereview.appspot.com/75057/diff/1/97
File src/devices/wimax/wimax-connection.cc (right):

http://codereview.appspot.com/75057/diff/1/97#newcode51
Line 51: "XXX", PointerValue(), MakePointerAccessor(
comments

http://codereview.appspot.com/75057/diff/1/97#newcode170
Line 170: NS_ASSERT(false);
Again

http://codereview.appspot.com/75057/diff/1/99
File src/devices/wimax/wimax-mac-header.cc (right):

http://codereview.appspot.com/75057/diff/1/99#newcode253
Line 253: // return 1 + 1 + 1 + 1 + 2 + m_cid.GetSize () + 1;
delete all commented code please

http://codereview.appspot.com/75057/diff/1/99#newcode287
Line 287: #if 0
This is always desactivated so please remove it...

http://codereview.appspot.com/75057/diff/1/100
File src/devices/wimax/wimax-mac-header.h (right):

http://codereview.appspot.com/75057/diff/1/100#newcode1
Line 1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
many classes in one file, please split, remove commented code and coding style

http://codereview.appspot.com/75057/diff/1/101
File src/devices/wimax/wimax-mac-queue.cc (right):

http://codereview.appspot.com/75057/diff/1/101#newcode69
Line 69: "XXX", MakeTraceSourceAccessor(&WimaxMacQueue::m_traceEnqueue))
.AddTraceSource(
Replace xxx by a good explanation

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

http://codereview.appspot.com/75057/diff/1/103#newcode58
Line 58: "Node", "XXX", PointerValue(), MakePointerAccessor(
No XXX...

http://codereview.appspot.com/75057/diff/1/103#newcode283
Line 283: //return true;
remove commented code...

http://codereview.appspot.com/75057/diff/1/103#newcode431
Line 431: m_initialRangingConnection = CreateObject<WimaxConnection> ("Cid",
Too long, please split it into few instructions

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

http://codereview.appspot.com/75057/diff/1/104#newcode40
Line 40: //#define JDEBUG1
remove

http://codereview.appspot.com/75057/diff/1/104#newcode152
Line 152: //temp, shall be private
put it as private...

http://codereview.appspot.com/75057/diff/1/104#newcode208
Line 208: TracedCallback<Ptr<const Packet> , const Mac48Address&> m_traceRx;
should be private also.

http://codereview.appspot.com/75057/diff/1/106
File src/devices/wimax/wimax-ofdm-phy/FEC80216.h (right):

http://codereview.appspot.com/75057/diff/1/106#newcode1
Line 1: #ifndef FEC80216_H
change file name, member functions and variables and please follow the coding
style

http://codereview.appspot.com/75057/diff/1/110
File src/devices/wimax/wimax-ofdm-phy/RS80216.cpp (right):

http://codereview.appspot.com/75057/diff/1/110#newcode102
Line 102: //return coded.right(coded.size()-padding_size);
remove

http://codereview.appspot.com/75057/diff/1/111
File src/devices/wimax/wimax-ofdm-phy/RS80216.h (right):

http://codereview.appspot.com/75057/diff/1/111#newcode1
Line 1: #ifndef RS80216_H
change name of the file, functions and member variables, please follow coding
style in all the files...

http://codereview.appspot.com/75057/diff/1/112
File src/devices/wimax/wimax-ofdm-phy/TransmissionSimulator.cpp (right):

http://codereview.appspot.com/75057/diff/1/112#newcode27
Line 27: //	double Ts = 0.000000625;
remove commented code

http://codereview.appspot.com/75057/diff/1/114
File src/devices/wimax/wimax-ofdm-phy/channelCoder80216.cpp (right):

http://codereview.appspot.com/75057/diff/1/114#newcode122
Line 122: delete FECcoder;
too many delete and new in this class, you can use smart pointers.

http://codereview.appspot.com/75057/diff/1/116
File src/devices/wimax/wimax-ofdm-phy/channelModulator80216.cpp (right):

http://codereview.appspot.com/75057/diff/1/116#newcode1
Line 1:
/***************************************************************************
same as previous files

http://codereview.appspot.com/75057/diff/1/118
File src/devices/wimax/wimax-ofdm-phy/convolutional.cpp (right):

http://codereview.appspot.com/75057/diff/1/118#newcode156
Line 156: /*
please remove commented code and comments in spanish :)

http://codereview.appspot.com/75057/diff/1/119
File src/devices/wimax/wimax-ofdm-phy/convolutional.h (right):

http://codereview.appspot.com/75057/diff/1/119#newcode1
Line 1: #ifndef CONVOLUTIONAL_H
coding style and naming for all those files please...

http://codereview.appspot.com/75057/diff/1/120
File src/devices/wimax/wimax-ofdm-phy/fadingChannel.cpp (right):

http://codereview.appspot.com/75057/diff/1/120#newcode1
Line 1:
/***************************************************************************
what is it used for? there is no implementation in here!

http://codereview.appspot.com/75057/diff/1/123
File src/devices/wimax/wimax-ofdm-phy/interleaver80216.h (right):

http://codereview.appspot.com/75057/diff/1/123#newcode1
Line 1: #ifndef INTERLEAVER80216_H
Coding style and naming...

http://codereview.appspot.com/75057/diff/1/123#newcode67
Line 67: int d;
very bad naming, please use significative variable names beginning with
m_variableName

http://codereview.appspot.com/75057/diff/1/124
File src/devices/wimax/wimax-ofdm-phy/modulator80216.cpp (right):

http://codereview.appspot.com/75057/diff/1/124#newcode29
Line 29: sixteenQAM = new QAM(16);
if the modulation is BPSK, why you create QAM?

http://codereview.appspot.com/75057/diff/1/124#newcode51
Line 51: switch (modulationOrder)
indenting, coding style

http://codereview.appspot.com/75057/diff/1/125
File src/devices/wimax/wimax-ofdm-phy/modulator80216.h (right):

http://codereview.appspot.com/75057/diff/1/125#newcode43
Line 43: set_mod_order(int modulationOrder);
method naming conventions, also classes/member variables naming conventions
should be respected

http://codereview.appspot.com/75057/diff/1/127
File src/devices/wimax/wimax-ofdm-phy/multicarrier80216.h (right):

http://codereview.appspot.com/75057/diff/1/127#newcode34
Line 34: class Multicarrier80216 {
functions and variables naming

http://codereview.appspot.com/75057/diff/1/128
File src/devices/wimax/wimax-ofdm-phy/packetCreator.cpp (right):

http://codereview.appspot.com/75057/diff/1/128#newcode22
Line 22: using std::cout;
Are you using them? please remove if not used.

http://codereview.appspot.com/75057/diff/1/129
File src/devices/wimax/wimax-ofdm-phy/packetCreator.h (right):

http://codereview.appspot.com/75057/diff/1/129#newcode35
Line 35: bifstream inputFile;
follow coding and naming style

http://codereview.appspot.com/75057/diff/1/130
File src/devices/wimax/wimax-ofdm-phy/pilotModulator80216.cpp (right):

http://codereview.appspot.com/75057/diff/1/130#newcode32
Line 32: if(link_direction== DOWNLINK) initializer = initializer_downlink;
coding style here and in all your files!

http://codereview.appspot.com/75057/diff/1/133
File src/devices/wimax/wimax-ofdm-phy/provaRSpuncturing/testRScoder.cpp (right):

http://codereview.appspot.com/75057/diff/1/133#newcode1
Line 1: 
Cool some testing, I didn't see any till now!

http://codereview.appspot.com/75057/diff/1/133#newcode11
Line 11: int main()
Please use NS_ASSERT for testing and use the NS testing standard for the tests
to be run with ./waf --check

http://codereview.appspot.com/75057/diff/1/133#newcode19
Line 19: /*
remove

http://codereview.appspot.com/75057/diff/1/134
File src/devices/wimax/wimax-ofdm-phy/randomizer80216.cpp (right):

http://codereview.appspot.com/75057/diff/1/134#newcode33
Line 33: bin tempoutput;
Code style and naming

http://codereview.appspot.com/75057/diff/1/134#newcode42
Line 42: //std::cout << "<<<< Randomizer80216::randomize: " <<  output.size ()
<< std::endl;
removfe commented code

http://codereview.appspot.com/75057/diff/1/136
File src/devices/wimax/wimax-ofdm-phy/readme (right):

http://codereview.appspot.com/75057/diff/1/136#newcode3
Line 3: 'make' will compile all the .cpp files necessary in this directory,
generating object .o files.
we use ./waf right? please update this file

http://codereview.appspot.com/75057/diff/1/137
File src/devices/wimax/wimax-ofdm-phy/sigmaGenerator.cpp (right):

http://codereview.appspot.com/75057/diff/1/137#newcode30
Line 30: vec sigmaGenerator::create_sigmaVector(double meanSNRdB, int
vectorSize){
naming & coding style

http://codereview.appspot.com/75057/diff/1/139
File src/devices/wimax/wimax-ofdm-phy/symbolCreator80216.cpp (right):

http://codereview.appspot.com/75057/diff/1/139#newcode23
Line 23: using std::cout;
remove them

http://codereview.appspot.com/75057/diff/1/140
File src/devices/wimax/wimax-ofdm-phy/symbolCreator80216.h (right):

http://codereview.appspot.com/75057/diff/1/140#newcode53
Line 53: int rate;
again naming and coding style

http://codereview.appspot.com/75057/diff/1/141
File src/devices/wimax/wimax-ofdm-phy/testfiles/TransmissionSimulation.cpp
(right):

http://codereview.appspot.com/75057/diff/1/141#newcode2
Line 2: #include "../TransmissionSimulator.h"
NO, something is wrong in here, please correct

http://codereview.appspot.com/75057/diff/1/141#newcode27
Line 27: //fd = open("result.txt", O_WRONLY | O_CREAT, 0600);	// open text file
to write results
remove commented code

http://codereview.appspot.com/75057/diff/1/142
File src/devices/wimax/wimax-ofdm-phy/testfiles/result.txt (right):

http://codereview.appspot.com/75057/diff/1/142#newcode1
Line 1: ____________________
is this used for comparison?

http://codereview.appspot.com/75057/diff/1/143
File src/devices/wimax/wimax-ofdm-phy/testfiles/testChannelModulator.cpp
(right):

http://codereview.appspot.com/75057/diff/1/143#newcode1
Line 1: #include "../types.h"
NO!

http://codereview.appspot.com/75057/diff/1/143#newcode11
Line 11: int main()
Please use the standard testing for NS3

http://codereview.appspot.com/75057/diff/1/144
File src/devices/wimax/wimax-ofdm-phy/testfiles/testFEC80216.cpp (right):

http://codereview.appspot.com/75057/diff/1/144#newcode1
Line 1: #include "../types.h"
same comments as other files and please no #include ../xxxx.h

http://codereview.appspot.com/75057/diff/1/144#newcode36
Line 36: //cout << "input: ............" << endl<< input<< endl;
remove

http://codereview.appspot.com/75057/diff/1/145
File src/devices/wimax/wimax-ofdm-phy/testfiles/testOFDMSymbolCreator.cpp
(right):

http://codereview.appspot.com/75057/diff/1/145#newcode1
Line 1: #include "../types.h"
Same things

http://codereview.appspot.com/75057/diff/1/145#newcode4
Line 4: using namespace itpp;
Those tests looks like visual tests I mean you run them and notice manually if
the result is as expected, can you make them automatic?

http://codereview.appspot.com/75057/diff/1/145#newcode35
Line 35: //cout << "coded: ............" << endl<< coded<< endl;
remove

http://codereview.appspot.com/75057/diff/1/146
File src/devices/wimax/wimax-ofdm-phy/testfiles/testPilotModulator.cpp (right):

http://codereview.appspot.com/75057/diff/1/146#newcode1
Line 1: #include "../types.h"
Same

http://codereview.appspot.com/75057/diff/1/147
File src/devices/wimax/wimax-ofdm-phy/testfiles/testRScoder.cpp (right):

http://codereview.appspot.com/75057/diff/1/147#newcode1
Line 1: #include "../types.h"
Same comments

http://codereview.appspot.com/75057/diff/1/148
File src/devices/wimax/wimax-ofdm-phy/testfiles/testSymbolCreator.cpp (right):

http://codereview.appspot.com/75057/diff/1/148#newcode1
Line 1: #include "../types.h"
same comments

http://codereview.appspot.com/75057/diff/1/149
File src/devices/wimax/wimax-ofdm-phy/testfiles/testTransmissionSimulator.cpp
(right):

http://codereview.appspot.com/75057/diff/1/149#newcode1
Line 1: #include "../TransmissionSimulator.h"
same comments

http://codereview.appspot.com/75057/diff/1/150
File src/devices/wimax/wimax-ofdm-phy/testfiles/testchannelcoding.cpp (right):

http://codereview.appspot.com/75057/diff/1/150#newcode1
Line 1: #include "../types.h"
...

http://codereview.appspot.com/75057/diff/1/153
File src/devices/wimax/wimax-phy.cc (right):

http://codereview.appspot.com/75057/diff/1/153#newcode43
Line 43: "XXX", PointerValue(), MakePointerAccessor(&WimaxPhy::GetChannel,
real comments please

http://codereview.appspot.com/75057/diff/1/154
File src/devices/wimax/wimax-phy.h (right):

http://codereview.appspot.com/75057/diff/1/154#newcode37
Line 37: {
coding style...

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

http://codereview.appspot.com/75057/diff/1/155#newcode68
Line 68: "XXX", MakeTraceSourceAccessor(
real comments

http://codereview.appspot.com/75057/diff/1/155#newcode75
Line 75: WimaxSubscriberStationNetDevice::WimaxSubscriberStationNetDevice(void)
:
please delegate some functionalities to other classes! The class looks like the
do everything class

http://codereview.appspot.com/75057/diff/1/155#newcode92
Line 92: new OfdmDlBurstProfile()), m_ulBurstProfile(
is it deleted somewhere?

http://codereview.appspot.com/75057/diff/1/155#newcode101
Line 101: m_classifier = new IPCS_SS_Packet_classifier(m_serviceFlowManager);
are they deleted somewhere?

http://codereview.appspot.com/75057/diff/1/155#newcode378
Line 378: //GetClassifier ()->SetDevice (this);
remove

http://codereview.appspot.com/75057/diff/1/155#newcode556
Line 556: #ifdef JDEBUG2
please remove

http://codereview.appspot.com/75057/diff/1/155#newcode599
Line 599: if (cid == GetBroadcastConnection()->GetCid())
you can use switch (cid) case.... plus this function is too long and you should
split it.

http://codereview.appspot.com/75057/diff/1/155#newcode617
Line 617: EVENT_LOST_DL_MAP, false, m_lostDlMapEvent); //see Question 35
what question 35??

http://codereview.appspot.com/75057/diff/1/155#newcode722
Line 722: NS_ASSERT(false);
NS_ERROR

http://codereview.appspot.com/75057/diff/1/155#newcode837
Line 837: //m_nrDlMapElements = dlMapElements.size ()-1;
remove

http://codereview.appspot.com/75057/diff/1/155#newcode967
Line 967: void
those two methods are really too similar, can you share the shared
functionalities

http://codereview.appspot.com/75057/diff/1/156
File src/devices/wimax/wimax-ss-net-device.h (right):

http://codereview.appspot.com/75057/diff/1/156#newcode1
Line 1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
This class is amazingly huge, you should delegate some functionalities to other
classes.
Also coding style...

http://codereview.appspot.com/75057/diff/1/160
File src/helper/wimax-helper.cc (right):

http://codereview.appspot.com/75057/diff/1/160#newcode164
Line 164: phy = CreateObject<simpleOfdmWimaxPhy> ();
SimpleOfdmWimaxPhy

http://codereview.appspot.com/75057/diff/1/160#newcode169
Line 169: break;
should never be the case so please generate an error.

http://codereview.appspot.com/75057/diff/1/160#newcode314
Line 314: //<< " " << *packet
remove

http://codereview.appspot.com/75057/diff/1/160#newcode389
Line 389: /*static void PcapPhyTxEvent (Ptr<PcapWriter> writer,
remove all commented code