Review of 802.11s Mesh module

[faker.moatamri, 2009-06-10 16:19:58]

Hi all,
I reviewed the wifi mesh code, all comments and reviews are welcome.
Best regards
Faker Moatamri

http://codereview.appspot.com/63188/diff/1/2
File examples/mesh.cc (right):

http://codereview.appspot.com/63188/diff/1/2#newcode70
Line 70: /// Create nodes and setup theis mobility
spelling: their

http://codereview.appspot.com/63188/diff/1/3
File src/devices/mesh/dot11s/airtime-metric.cc (right):

http://codereview.appspot.com/63188/diff/1/3#newcode60
Line 60: AirtimeLinkMetricCalculator::CalculateMetric(Mac48Address peerAddress,
Ptr<MeshWifiInterfaceMac> mac)
What kind of metric are you calculating here? some comments will be welcome like
those in the header file.

http://codereview.appspot.com/63188/diff/1/3#newcode70
Line 70: uint32_t metric = (uint32_t) (
What is the 10240 number? or 8* 1e9? is it possible to use a constant in the
standard form instead of using numbers?

http://codereview.appspot.com/63188/diff/1/5
File src/devices/mesh/dot11s/dot11s-helper.cc (right):

http://codereview.appspot.com/63188/diff/1/5#newcode73
Line 73: if (channel > 0)
Kill commented code...

http://codereview.appspot.com/63188/diff/1/5#newcode132
Line 132: //if(*root_iterator == i.GetDistanceFrom(c.Begin ()))
kill commented code

http://codereview.appspot.com/63188/diff/1/5#newcode144
Line 144: return Install (phy, NodeContainer (node), roots, nInterfaces);
Here you are iterating on a list of one node. Is it possible to implement the
Install for one node and then to call it from the iterator in the above
function:
for (NodeContainer::Iterator i = c.Begin (); i != c.End (); ++i)
{
  Ptr<Node> node = *i;
  Install(...,node,...)
}
I think it might make more sense to do it this way...

http://codereview.appspot.com/63188/diff/1/6
File src/devices/mesh/dot11s/dot11s-helper.h (right):

http://codereview.appspot.com/63188/diff/1/6#newcode57
Line 57: * \brief Install 802.11s mesh device & protocols on given node
This installs on given node list right?

http://codereview.appspot.com/63188/diff/1/7
File src/devices/mesh/dot11s/dot11s-mac-header.cc (right):

http://codereview.appspot.com/63188/diff/1/7#newcode113
Line 113: return;
May be should give here a warning or throw an error that we cannot have more
than 3 addresses?

http://codereview.appspot.com/63188/diff/1/7#newcode256
Line 256: NS_ASSERT (false);
This will generate an assertion fault, the return above is useless since it will
stop anyway. I suggest putting output error since I guess this is an error (is
it?), it would make more sense to generate an error here, but if you really want
to keep NS_ASSERT, please output some text describing the error.

http://codereview.appspot.com/63188/diff/1/7#newcode275
Line 275: NS_ASSERT (false);
Same comment

http://codereview.appspot.com/63188/diff/1/7#newcode279
Line 279: // ???
If this code needs to be put, please put a TODO to be clear that is missing
code. If the behavior is not specified yet, please put a comment on that.

http://codereview.appspot.com/63188/diff/1/7#newcode286
Line 286: return retval;
same comment

http://codereview.appspot.com/63188/diff/1/7#newcode330
Line 330: {
did you mean Dot11sMacHeaderBits?

http://codereview.appspot.com/63188/diff/1/7#newcode340
Line 340: bool result (true);
Testing the serialization would be very recommended here to avoid unexpected
behavior.

http://codereview.appspot.com/63188/diff/1/8
File src/devices/mesh/dot11s/dot11s-mac-header.h (right):

http://codereview.appspot.com/63188/diff/1/8#newcode46
Line 46: void   SetAddr4 (Mac48Address address);
What are those addresses? why 4,5,6... not 1,2,3...?

http://codereview.appspot.com/63188/diff/1/10
File src/devices/mesh/dot11s/hwmp-mac-plugin.cc (right):

http://codereview.appspot.com/63188/diff/1/10#newcode52
Line 52: {
In this function, I would recommand creating two private functions that will
treat the mesh header and the action respectively. This will increase
readability specially if you plan to add more functionalities in here.

http://codereview.appspot.com/63188/diff/1/10#newcode75
Line 75: NS_ASSERT(false);
Message saying that extension is not supported yet would be appreciated. Same
for all the NS_ASSERT(false) in your code.

http://codereview.appspot.com/63188/diff/1/10#newcode81
Line 81: return false;
your return will never be called right?

http://codereview.appspot.com/63188/diff/1/10#newcode196
Line 196: NS_ASSERT (!m_preqTimer.IsRunning());
This line and the if (m_preqTimer.IsRunning ())
	            return;
are doing the same job right?

http://codereview.appspot.com/63188/diff/1/11
File src/devices/mesh/dot11s/hwmp-mac-plugin.h (right):

http://codereview.appspot.com/63188/diff/1/11#newcode67
Line 67: //add a destination to exisying PREQ generated by me and stored in
existing, may be use the /**
                           *
                           */

http://codereview.appspot.com/63188/diff/1/12
File src/devices/mesh/dot11s/hwmp-protocol.cc (right):

http://codereview.appspot.com/63188/diff/1/12#newcode193
Line 193: HwmpProtocol::RequestRoute (
This functionality might be given to another class to make the protocol class
less heavy, and it would be called by the protocol as a service.

http://codereview.appspot.com/63188/diff/1/12#newcode208
Line 208: NS_ASSERT (false);
Again the NS_ASSERT(false)... please replace all of them in your code by
NS_ERROR and please write a user friendly message to explain the error or the
not yet supported feature.

http://codereview.appspot.com/63188/diff/1/12#newcode220
Line 220: return false;
again return never reached...

http://codereview.appspot.com/63188/diff/1/12#newcode239
Line 239: if(*chan == plugin->second->GetChannelId ())
for a matter of clarity, you should may be use brackets here to make it more
readable.

http://codereview.appspot.com/63188/diff/1/12#newcode472
Line 472: //chack if must retransmit:
check

http://codereview.appspot.com/63188/diff/1/12#newcode733
Line 733: retval.pkt = NULL;
Please use 0 instead, please refer to coding style.

http://codereview.appspot.com/63188/diff/1/12#newcode764
Line 764: while (1)
why not put while (packet.pkt != 0) instead of the infinite loop?

http://codereview.appspot.com/63188/diff/1/12#newcode767
Line 767: if (packet.pkt == NULL)
0 instead of NULL

http://codereview.appspot.com/63188/diff/1/12#newcode833
Line 833: while (1)
same as above

http://codereview.appspot.com/63188/diff/1/13
File src/devices/mesh/dot11s/hwmp-protocol.h (right):

http://codereview.appspot.com/63188/diff/1/13#newcode101
Line 101: ///\brief forms a path error information element when list of
destination fails on a given interface
This class is too huge, it should delegate some responsibilities to other
classes, it will be more readable and more maintainable and eventually reusable.
For example Path error handling can be in a separate class. Same thing for
queuing and dequeuing,stats... Always try to make the classes sepcialized and
their functionalities limited to ease, readibility, maintainability,
extensibility and reusability.

http://codereview.appspot.com/63188/diff/1/14
File src/devices/mesh/dot11s/hwmp-rtable.cc (right):

http://codereview.appspot.com/63188/diff/1/14#newcode230
Line 230: //We suppose that no dublicates here can be
duplicates

http://codereview.appspot.com/63188/diff/1/14#newcode311
Line 311: void HwmpRtableTest::Test1 ()
Please give a more signicative name to the tests you are executing, in order to
give an idea of what you are doing or add some comments about it.

http://codereview.appspot.com/63188/diff/1/14#newcode347
Line 347: // TODO: test AddPrecursor and GetPrecursors
will you be able to do it soon?

http://codereview.appspot.com/63188/diff/1/18
File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.cc (right):

http://codereview.appspot.com/63188/diff/1/18#newcode95
Line 95: return;
a warning would be nice here to say that the operation not completed...

http://codereview.appspot.com/63188/diff/1/18#newcode103
Line 103: return;
same as above

http://codereview.appspot.com/63188/diff/1/18#newcode190
Line 190: IeBeaconTiming::TimestampToU16 (Time x)
why not use a more significant variable name instead of x?

http://codereview.appspot.com/63188/diff/1/19
File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.h (right):

http://codereview.appspot.com/63188/diff/1/19#newcode49
Line 49: uint8_t m_aid;
member attributes of a class should not be public and should not be accessed
directly, they should be private and accessed only by accessor functions Getter
and Setter above.

http://codereview.appspot.com/63188/diff/1/20
File src/devices/mesh/dot11s/ie-dot11s-configuration.cc (right):

http://codereview.appspot.com/63188/diff/1/20#newcode158
Line 158: //TODO: print
to be done when?

http://codereview.appspot.com/63188/diff/1/21
File src/devices/mesh/dot11s/ie-dot11s-configuration.h (right):

http://codereview.appspot.com/63188/diff/1/21#newcode84
Line 84: bool acceptPeerLinks;
please use the convention m_name and please make the member variables not public
and use instead getters and setters.

http://codereview.appspot.com/63188/diff/1/22
File src/devices/mesh/dot11s/ie-dot11s-id.cc (right):

http://codereview.appspot.com/63188/diff/1/22#newcode111
Line 111: //TODO
can you do it please?

http://codereview.appspot.com/63188/diff/1/23
File src/devices/mesh/dot11s/ie-dot11s-id.h (right):

http://codereview.appspot.com/63188/diff/1/23#newcode39
Line 39: //IeMeshId (char const meshId[32]);
remove commented code

http://codereview.appspot.com/63188/diff/1/28
File src/devices/mesh/dot11s/ie-dot11s-peering-protocol.cc (right):

http://codereview.appspot.com/63188/diff/1/28#newcode63
Line 63: //TODO: print
can you please print...?

http://codereview.appspot.com/63188/diff/1/30
File src/devices/mesh/dot11s/ie-dot11s-perr.cc (right):

http://codereview.appspot.com/63188/diff/1/30#newcode43
Line 43: // FILL
please fill it...

http://codereview.appspot.com/63188/diff/1/32
File src/devices/mesh/dot11s/ie-dot11s-prep.cc (right):

http://codereview.appspot.com/63188/diff/1/32#newcode211
Line 211: //TODO
can you do it please...

http://codereview.appspot.com/63188/diff/1/34
File src/devices/mesh/dot11s/ie-dot11s-preq.cc (right):

http://codereview.appspot.com/63188/diff/1/34#newcode133
Line 133: //IePreq::SetFlags (uint8_t flags)
remove

http://codereview.appspot.com/63188/diff/1/38
File src/devices/mesh/dot11s/peer-link-frame.cc (right):

http://codereview.appspot.com/63188/diff/1/38#newcode131
Line 131: i.WriteHtolsbU16(m_capability);
space before parentheses

http://codereview.appspot.com/63188/diff/1/40
File src/devices/mesh/dot11s/peer-link.cc (right):

http://codereview.appspot.com/63188/diff/1/40#newcode283
Line 283: PeerLink::StateMachine (PeerEvent event,PmpReasonCode reasoncode)
this function is too long and it should be divided into several functions for a
matter of maintability and readability. I would suggest in each case
:IDLE,OPN_SNT,... to have a function that will treat the case.

http://codereview.appspot.com/63188/diff/1/40#newcode293
Line 293: // TODO Callback MLME-SignalPeerLinkStatus
when?

http://codereview.appspot.com/63188/diff/1/40#newcode310
Line 310: {}
should you put some code in here? is it an error to have values different from
those above?

http://codereview.appspot.com/63188/diff/1/40#newcode413
Line 413: // TODO Callback MLME-SignalPeerLinkStatus
Please check all the TODOs in your code, thanks

http://codereview.appspot.com/63188/diff/1/41
File src/devices/mesh/dot11s/peer-link.h (right):

http://codereview.appspot.com/63188/diff/1/41#newcode224
Line 224: /// When last beacon was sent (TODO or received?)
when you plan to do it?

http://codereview.appspot.com/63188/diff/1/41#newcode255
Line 255: /// ?
any comments?

http://codereview.appspot.com/63188/diff/1/44
File src/devices/mesh/dot11s/peer-management-protocol.cc (right):

http://codereview.appspot.com/63188/diff/1/44#newcode155
Line 155: if(i == m_neighbourBeacons.end())
coding style: space after if and two spaces after the return
if (...)
  {

  }
please change this for the whole code...

http://codereview.appspot.com/63188/diff/1/44#newcode229
Line 229: bool reject = ! (ShouldAcceptOpen (interface,
peerAddress,reasonCode));
why not having an accept boolean to avoid multiple negations:
it would be after if (accept) instead of if (!reject)...

http://codereview.appspot.com/63188/diff/1/44#newcode288
Line 288: NS_ASSERT (false);
please use NS_ERROR instead and write an understanble error message

http://codereview.appspot.com/63188/diff/1/44#newcode345
Line 345: if(peerLink != 0)
remove 2 spaces indentation, again please verify indentation for the whole file.

http://codereview.appspot.com/63188/diff/1/49
File src/devices/mesh/mesh-l2-routing-protocol.h (right):

http://codereview.appspot.com/63188/diff/1/49#newcode84
Line 84: * Note that route discobery works async. -- RequestRoute returns
immediately, while
discovery

http://codereview.appspot.com/63188/diff/1/49#newcode95
Line 95: *                      to really send packetusing routing information.
space: packet using

http://codereview.appspot.com/63188/diff/1/50
File src/devices/mesh/mesh-point-device.cc (right):

http://codereview.appspot.com/63188/diff/1/50#newcode305
Line 305: NS_ASSERT(false);
please generate an error, the return 0 statement will never be reached, so it is
useless.

http://codereview.appspot.com/63188/diff/1/50#newcode365
Line 365: return;
why type in return?

http://codereview.appspot.com/63188/diff/1/53
File src/devices/mesh/mesh-wifi-beacon.h (right):

http://codereview.appspot.com/63188/diff/1/53#newcode56
Line 56: /// Create wifi header for beacon frame. \param address is sender
address \param mpAddress is mesh point address
please put parmeters in new lines

http://codereview.appspot.com/63188/diff/1/55
File src/devices/mesh/mesh-wifi-interface-mac.cc (right):

http://codereview.appspot.com/63188/diff/1/55#newcode191
Line 191: NS_LOG_DEBUG("SetWifiPhy: Can switch channel now: " <<
CanSwitchChannel() ); // TMP
why is it TMP? will you need to remove it?

http://codereview.appspot.com/63188/diff/1/55#newcode575
Line 575: if (beacon_hdr.GetSsid ().IsEqual(GetSsid()))
space after IsEqual

[Pavel Boyko, 2009-06-15 09:03:26]

Once again I'd like to thank Faker for thorough review. Some points are not
answered yet, coming soon at https://forge.wenos.ru/hgprojects/ns3dev 

Now I propose to pause review until 3.5 release.

http://codereview.appspot.com/63188/diff/1/2
File examples/mesh.cc (right):

http://codereview.appspot.com/63188/diff/1/2#newcode70
Line 70: /// Create nodes and setup theis mobility
On 2009/06/10 16:19:58, faker.moatamri wrote:
> spelling: their

Done.

http://codereview.appspot.com/63188/diff/1/3
File src/devices/mesh/dot11s/airtime-metric.cc (right):

http://codereview.appspot.com/63188/diff/1/3#newcode60
Line 60: AirtimeLinkMetricCalculator::CalculateMetric(Mac48Address peerAddress,
Ptr<MeshWifiInterfaceMac> mac)
On 2009/06/10 16:19:58, faker.moatamri wrote:
> What kind of metric are you calculating here? some comments will be welcome
like
> those in the header file.

Done.

http://codereview.appspot.com/63188/diff/1/3#newcode70
Line 70: uint32_t metric = (uint32_t) (
On 2009/06/10 16:19:58, faker.moatamri wrote:
> What is the 10240 number? or 8* 1e9? is it possible to use a constant in the
> standard form instead of using numbers?

Done.

http://codereview.appspot.com/63188/diff/1/5
File src/devices/mesh/dot11s/dot11s-helper.cc (right):

http://codereview.appspot.com/63188/diff/1/5#newcode73
Line 73: if (channel > 0)
On 2009/06/10 16:19:58, faker.moatamri wrote:
> Kill commented code...

helpers reworked to device helper + mesh stack installer, see updated codebase.
Commented code killed.

http://codereview.appspot.com/63188/diff/1/5#newcode132
Line 132: //if(*root_iterator == i.GetDistanceFrom(c.Begin ()))
On 2009/06/10 16:19:58, faker.moatamri wrote:
> kill commented code

Done.

http://codereview.appspot.com/63188/diff/1/5#newcode144
Line 144: return Install (phy, NodeContainer (node), roots, nInterfaces);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> Here you are iterating on a list of one node. Is it possible to implement the
> Install for one node and then to call it from the iterator in the above
> function:
> for (NodeContainer::Iterator i = c.Begin (); i != c.End (); ++i)
> {
>   Ptr<Node> node = *i;
>   Install(...,node,...)
> }
> I think it might make more sense to do it this way...

It makes no difference

http://codereview.appspot.com/63188/diff/1/6
File src/devices/mesh/dot11s/dot11s-helper.h (right):

http://codereview.appspot.com/63188/diff/1/6#newcode57
Line 57: * \brief Install 802.11s mesh device & protocols on given node
On 2009/06/10 16:19:58, faker.moatamri wrote:
> This installs on given node list right?

right. fixed.

http://codereview.appspot.com/63188/diff/1/7
File src/devices/mesh/dot11s/dot11s-mac-header.cc (right):

http://codereview.appspot.com/63188/diff/1/7#newcode113
Line 113: return;
On 2009/06/10 16:19:58, faker.moatamri wrote:
> May be should give here a warning or throw an error that we cannot have more
> than 3 addresses?

changed to NS_ASSERT (num_of_addresses <= 3);

http://codereview.appspot.com/63188/diff/1/7#newcode256
Line 256: NS_ASSERT (false);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> This will generate an assertion fault, the return above is useless since it
will
> stop anyway. I suggest putting output error since I guess this is an error (is
> it?), it would make more sense to generate an error here, but if you really
want
> to keep NS_ASSERT, please output some text describing the error.

All NS_ASSERT (false) are changed to NS_FATAL_ERROR (meaningful message)

http://codereview.appspot.com/63188/diff/1/7#newcode275
Line 275: NS_ASSERT (false);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> Same comment

Done.

http://codereview.appspot.com/63188/diff/1/7#newcode279
Line 279: // ???
On 2009/06/10 16:19:58, faker.moatamri wrote:
> If this code needs to be put, please put a TODO to be clear that is missing
> code. If the behavior is not specified yet, please put a comment on that.

Done.

http://codereview.appspot.com/63188/diff/1/7#newcode286
Line 286: return retval;
On 2009/06/10 16:19:58, faker.moatamri wrote:
> same comment

Done.

http://codereview.appspot.com/63188/diff/1/7#newcode330
Line 330: {
On 2009/06/10 16:19:58, faker.moatamri wrote:
> did you mean Dot11sMacHeaderBits?

Nope, I mean Built-In-Self-Test.

http://codereview.appspot.com/63188/diff/1/7#newcode340
Line 340: bool result (true);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> Testing the serialization would be very recommended here to avoid unexpected
> behavior.

Nothing but serialization test follows.

http://codereview.appspot.com/63188/diff/1/8
File src/devices/mesh/dot11s/dot11s-mac-header.h (right):

http://codereview.appspot.com/63188/diff/1/8#newcode46
Line 46: void   SetAddr4 (Mac48Address address);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> What are those addresses? why 4,5,6... not 1,2,3...?

This is known as 4- and 6-address extensions (or address schemes) for multihop
wireless networks. Addresses 1 to 3 are already present in WifiMacHeader
(frankly speaking address 4 is also there, but it is never used for management
frames, so 802.11s has to redefine it in own header).

Please let me know if more detailed answer is needed.

http://codereview.appspot.com/63188/diff/1/10
File src/devices/mesh/dot11s/hwmp-mac-plugin.cc (right):

http://codereview.appspot.com/63188/diff/1/10#newcode52
Line 52: {
On 2009/06/10 16:19:58, faker.moatamri wrote:
> In this function, I would recommand creating two private functions that will
> treat the mesh header and the action respectively. This will increase
> readability specially if you plan to add more functionalities in here.

Done.

http://codereview.appspot.com/63188/diff/1/10#newcode75
Line 75: NS_ASSERT(false);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> Message saying that extension is not supported yet would be appreciated. Same
> for all the NS_ASSERT(false) in your code.

Done.

http://codereview.appspot.com/63188/diff/1/10#newcode81
Line 81: return false;
On 2009/06/10 16:19:58, faker.moatamri wrote:
> your return will never be called right?

Done.

http://codereview.appspot.com/63188/diff/1/10#newcode196
Line 196: NS_ASSERT (!m_preqTimer.IsRunning());
On 2009/06/10 16:19:58, faker.moatamri wrote:
> This line and the if (m_preqTimer.IsRunning ())
> 	            return;
> are doing the same job right?

Right. I think extra assertion is harmless but can be useful.

http://codereview.appspot.com/63188/diff/1/11
File src/devices/mesh/dot11s/hwmp-mac-plugin.h (right):

http://codereview.appspot.com/63188/diff/1/11#newcode67
Line 67: //add a destination to exisying PREQ generated by me and stored in
On 2009/06/10 16:19:58, faker.moatamri wrote:
> existing, may be use the /**
>                            *
>                            */

Done.

http://codereview.appspot.com/63188/diff/1/12
File src/devices/mesh/dot11s/hwmp-protocol.cc (right):

http://codereview.appspot.com/63188/diff/1/12#newcode193
Line 193: HwmpProtocol::RequestRoute (
On 2009/06/10 16:19:58, faker.moatamri wrote:
> This functionality might be given to another class to make the protocol class
> less heavy, and it would be called by the protocol as a service.

Will do this during the next refactoring wave.

http://codereview.appspot.com/63188/diff/1/12#newcode208
Line 208: NS_ASSERT (false);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> Again the NS_ASSERT(false)... please replace all of them in your code by
> NS_ERROR and please write a user friendly message to explain the error or the
> not yet supported feature.

Done.

http://codereview.appspot.com/63188/diff/1/12#newcode220
Line 220: return false;
On 2009/06/10 16:19:58, faker.moatamri wrote:
> again return never reached...

Done.

http://codereview.appspot.com/63188/diff/1/12#newcode239
Line 239: if(*chan == plugin->second->GetChannelId ())
On 2009/06/10 16:19:58, faker.moatamri wrote:
> for a matter of clarity, you should may be use brackets here to make it more
> readable.

Done.

http://codereview.appspot.com/63188/diff/1/12#newcode472
Line 472: //chack if must retransmit:
On 2009/06/10 16:19:58, faker.moatamri wrote:
> check

Done.

http://codereview.appspot.com/63188/diff/1/12#newcode733
Line 733: retval.pkt = NULL;
On 2009/06/10 16:19:58, faker.moatamri wrote:
> Please use 0 instead, please refer to coding style.

Done.

http://codereview.appspot.com/63188/diff/1/12#newcode764
Line 764: while (1)
On 2009/06/10 16:19:58, faker.moatamri wrote:
> why not put while (packet.pkt != 0) instead of the infinite loop?

Done.

http://codereview.appspot.com/63188/diff/1/12#newcode767
Line 767: if (packet.pkt == NULL)
On 2009/06/10 16:19:58, faker.moatamri wrote:
> 0 instead of NULL

Done.

http://codereview.appspot.com/63188/diff/1/12#newcode833
Line 833: while (1)
On 2009/06/10 16:19:58, faker.moatamri wrote:
> same as above

Done.

http://codereview.appspot.com/63188/diff/1/13
File src/devices/mesh/dot11s/hwmp-protocol.h (right):

http://codereview.appspot.com/63188/diff/1/13#newcode101
Line 101: ///\brief forms a path error information element when list of
destination fails on a given interface
On 2009/06/10 16:19:58, faker.moatamri wrote:
> This class is too huge, it should delegate some responsibilities to other
> classes, it will be more readable and more maintainable and eventually
reusable.
> For example Path error handling can be in a separate class. Same thing for
> queuing and dequeuing,stats... Always try to make the classes sepcialized and
> their functionalities limited to ease, readibility, maintainability,
> extensibility and reusability.

Agree. Will refactor HwmpProtocol in the next refactoring cycle.

http://codereview.appspot.com/63188/diff/1/14
File src/devices/mesh/dot11s/hwmp-rtable.cc (right):

http://codereview.appspot.com/63188/diff/1/14#newcode230
Line 230: //We suppose that no dublicates here can be
On 2009/06/10 16:19:58, faker.moatamri wrote:
> duplicates

Done.

http://codereview.appspot.com/63188/diff/1/14#newcode311
Line 311: void HwmpRtableTest::Test1 ()
On 2009/06/10 16:19:58, faker.moatamri wrote:
> Please give a more signicative name to the tests you are executing, in order
to
> give an idea of what you are doing or add some comments about it.

Will do this soon.

http://codereview.appspot.com/63188/diff/1/14#newcode347
Line 347: // TODO: test AddPrecursor and GetPrecursors
On 2009/06/10 16:19:58, faker.moatamri wrote:
> will you be able to do it soon?

Yes, will do this soon.

http://codereview.appspot.com/63188/diff/1/18
File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.cc (right):

http://codereview.appspot.com/63188/diff/1/18#newcode95
Line 95: return;
On 2009/06/10 16:19:58, faker.moatamri wrote:
> a warning would be nice here to say that the operation not completed...

Done.

http://codereview.appspot.com/63188/diff/1/18#newcode103
Line 103: return;
On 2009/06/10 16:19:58, faker.moatamri wrote:
> same as above

Done.

http://codereview.appspot.com/63188/diff/1/18#newcode190
Line 190: IeBeaconTiming::TimestampToU16 (Time x)
On 2009/06/10 16:19:58, faker.moatamri wrote:
> why not use a more significant variable name instead of x?

Renamed to t :))

http://codereview.appspot.com/63188/diff/1/19
File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.h (right):

http://codereview.appspot.com/63188/diff/1/19#newcode49
Line 49: uint8_t m_aid;
On 2009/06/10 16:19:58, faker.moatamri wrote:
> member attributes of a class should not be public and should not be accessed
> directly, they should be private and accessed only by accessor functions
Getter
> and Setter above.

Done.

http://codereview.appspot.com/63188/diff/1/20
File src/devices/mesh/dot11s/ie-dot11s-configuration.cc (right):

http://codereview.appspot.com/63188/diff/1/20#newcode158
Line 158: //TODO: print
On 2009/06/10 16:19:58, faker.moatamri wrote:
> to be done when?

TODO removed

http://codereview.appspot.com/63188/diff/1/21
File src/devices/mesh/dot11s/ie-dot11s-configuration.h (right):

http://codereview.appspot.com/63188/diff/1/21#newcode84
Line 84: bool acceptPeerLinks;
On 2009/06/10 16:19:58, faker.moatamri wrote:
> please use the convention m_name and please make the member variables not
public
> and use instead getters and setters.

IMO in this case this is an overkill.

http://codereview.appspot.com/63188/diff/1/22
File src/devices/mesh/dot11s/ie-dot11s-id.cc (right):

http://codereview.appspot.com/63188/diff/1/22#newcode111
Line 111: //TODO
On 2009/06/10 16:19:58, faker.moatamri wrote:
> can you do it please?

Done.

http://codereview.appspot.com/63188/diff/1/23
File src/devices/mesh/dot11s/ie-dot11s-id.h (right):

http://codereview.appspot.com/63188/diff/1/23#newcode39
Line 39: //IeMeshId (char const meshId[32]);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> remove commented code

Done.

http://codereview.appspot.com/63188/diff/1/28
File src/devices/mesh/dot11s/ie-dot11s-peering-protocol.cc (right):

http://codereview.appspot.com/63188/diff/1/28#newcode63
Line 63: //TODO: print
On 2009/06/10 16:19:58, faker.moatamri wrote:
> can you please print...?

Done.

http://codereview.appspot.com/63188/diff/1/30
File src/devices/mesh/dot11s/ie-dot11s-perr.cc (right):

http://codereview.appspot.com/63188/diff/1/30#newcode43
Line 43: // FILL
On 2009/06/10 16:19:58, faker.moatamri wrote:
> please fill it...

will be done asap

http://codereview.appspot.com/63188/diff/1/32
File src/devices/mesh/dot11s/ie-dot11s-prep.cc (right):

http://codereview.appspot.com/63188/diff/1/32#newcode211
Line 211: //TODO
On 2009/06/10 16:19:58, faker.moatamri wrote:
> can you do it please...

will be done asap

http://codereview.appspot.com/63188/diff/1/34
File src/devices/mesh/dot11s/ie-dot11s-preq.cc (right):

http://codereview.appspot.com/63188/diff/1/34#newcode133
Line 133: //IePreq::SetFlags (uint8_t flags)
On 2009/06/10 16:19:58, faker.moatamri wrote:
> remove

Done.

http://codereview.appspot.com/63188/diff/1/38
File src/devices/mesh/dot11s/peer-link-frame.cc (right):

http://codereview.appspot.com/63188/diff/1/38#newcode131
Line 131: i.WriteHtolsbU16(m_capability);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> space before parentheses

Done.

http://codereview.appspot.com/63188/diff/1/40
File src/devices/mesh/dot11s/peer-link.cc (right):

http://codereview.appspot.com/63188/diff/1/40#newcode283
Line 283: PeerLink::StateMachine (PeerEvent event,PmpReasonCode reasoncode)
On 2009/06/10 16:19:58, faker.moatamri wrote:
> this function is too long and it should be divided into several functions for
a
> matter of maintability and readability. I would suggest in each case
> :IDLE,OPN_SNT,... to have a function that will treat the case.

I'm afraid of touching state machine. The next one will be more structured, I
promise :)

http://codereview.appspot.com/63188/diff/1/40#newcode293
Line 293: // TODO Callback MLME-SignalPeerLinkStatus
On 2009/06/10 16:19:58, faker.moatamri wrote:
> when?

soon

http://codereview.appspot.com/63188/diff/1/40#newcode310
Line 310: {}
On 2009/06/10 16:19:58, faker.moatamri wrote:
> should you put some code in here? is it an error to have values different from
> those above?

NS_FATAL_ERROR ("") inserted

http://codereview.appspot.com/63188/diff/1/40#newcode413
Line 413: // TODO Callback MLME-SignalPeerLinkStatus
On 2009/06/10 16:19:58, faker.moatamri wrote:
> Please check all the TODOs in your code, thanks

Done.

http://codereview.appspot.com/63188/diff/1/41
File src/devices/mesh/dot11s/peer-link.h (right):

http://codereview.appspot.com/63188/diff/1/41#newcode224
Line 224: /// When last beacon was sent (TODO or received?)
On 2009/06/10 16:19:58, faker.moatamri wrote:
> when you plan to do it?

Done.

http://codereview.appspot.com/63188/diff/1/41#newcode255
Line 255: /// ?
On 2009/06/10 16:19:58, faker.moatamri wrote:
> any comments?

Done.

http://codereview.appspot.com/63188/diff/1/44
File src/devices/mesh/dot11s/peer-management-protocol.cc (right):

http://codereview.appspot.com/63188/diff/1/44#newcode229
Line 229: bool reject = ! (ShouldAcceptOpen (interface,
peerAddress,reasonCode));
On 2009/06/10 16:19:58, faker.moatamri wrote:
> why not having an accept boolean to avoid multiple negations:
> it would be after if (accept) instead of if (!reject)...

I see no difference

http://codereview.appspot.com/63188/diff/1/44#newcode288
Line 288: NS_ASSERT (false);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> please use NS_ERROR instead and write an understanble error message

Done.

http://codereview.appspot.com/63188/diff/1/44#newcode345
Line 345: if(peerLink != 0)
On 2009/06/10 16:19:58, faker.moatamri wrote:
> remove 2 spaces indentation, again please verify indentation for the whole
file.

Done.

http://codereview.appspot.com/63188/diff/1/49
File src/devices/mesh/mesh-l2-routing-protocol.h (right):

http://codereview.appspot.com/63188/diff/1/49#newcode84
Line 84: * Note that route discobery works async. -- RequestRoute returns
immediately, while
On 2009/06/10 16:19:58, faker.moatamri wrote:
> discovery

Done.

http://codereview.appspot.com/63188/diff/1/49#newcode95
Line 95: *                      to really send packetusing routing information.
On 2009/06/10 16:19:58, faker.moatamri wrote:
> space: packet using

Done.

http://codereview.appspot.com/63188/diff/1/50
File src/devices/mesh/mesh-point-device.cc (right):

http://codereview.appspot.com/63188/diff/1/50#newcode305
Line 305: NS_ASSERT(false);
On 2009/06/10 16:19:58, faker.moatamri wrote:
> please generate an error, the return 0 statement will never be reached, so it
is
> useless.

Done.

http://codereview.appspot.com/63188/diff/1/50#newcode365
Line 365: return;
On 2009/06/10 16:19:58, faker.moatamri wrote:
> why type in return?

I guess it wasn't void some time ago. Fixed.

http://codereview.appspot.com/63188/diff/1/53
File src/devices/mesh/mesh-wifi-beacon.h (right):

http://codereview.appspot.com/63188/diff/1/53#newcode56
Line 56: /// Create wifi header for beacon frame. \param address is sender
address \param mpAddress is mesh point address
On 2009/06/10 16:19:58, faker.moatamri wrote:
> please put parmeters in new lines

Done.

http://codereview.appspot.com/63188/diff/1/55
File src/devices/mesh/mesh-wifi-interface-mac.cc (right):

http://codereview.appspot.com/63188/diff/1/55#newcode191
Line 191: NS_LOG_DEBUG("SetWifiPhy: Can switch channel now: " <<
CanSwitchChannel() ); // TMP
On 2009/06/10 16:19:58, faker.moatamri wrote:
> why is it TMP? will you need to remove it?

Done.

http://codereview.appspot.com/63188/diff/1/55#newcode575
Line 575: if (beacon_hdr.GetSsid ().IsEqual(GetSsid()))
On 2009/06/10 16:19:58, faker.moatamri wrote:
> space after IsEqual

Done.