802.11s mesh stack model

[Mathieu Lacage, 2009-07-16 11:39:05]

Overall, the code looks pretty good. Comments below.

1) if the metric is ratio of lines of comments over lines of code, then, the
result is pretty close to what you would get if you ran it on my code. i.e.,
craig is going to go balistic if he ever reads so given the amount of
commenting. I think that it's sort of ok as-is but you probably should try to
increase the value of that metric in the future.

2) the doxygen documentation is often lacking: you really need more of it

3) style: I added comments in a couple of locations but I did not try to catch
all coding style errors. It would be nice if you could go through the codebase
and try to fix it:
  - always add brackets, even for one-line statements:
if ()
  {
    // statement
  }
  - the same goes for 'for' statements
  - always increase the indent when adding a bracket:
if ()
  {// note indent
    // indented again
  }// end of indent
//end of indent
  - same for 'for' statements
  - the doxygen comments you add use the legal '///' doxygen marker. This is
really legal but the current codebase uses /** + */ so it make me feel really
weird when I read your code, especially because some of your doxygen
documentation actually uses /** + */. Please, try to follow what we do in the
rest of the code

4) the helper API needs some cleanup, but nothing big and it's fairly trivial to
do I think

5) the biggest technical comment I have is about the way you have information
elements. Specifically, on the way you deserialize them from FindFirst. You
really can't use PeekData the way you are using it. Here is a proposal to do
this differently:

class IeVector : public Header 
{
  typedef std::vector<Ptr<InformationElement> >::const_iterator Iterator;
public:
  void Add (Ptr<InformationElement> element);
  Iterator Begin (void);
  Iterator End (void);
  // maybe helper method:
  Ptr<InformationElement> FindFirst (uint8_t id);
private:
  void Serialize (...) {...}
  ... Deserialize (Buffer::Iterator i) {
  while (i.GetSize () > 2) {
    uint8_t id = i.ReadU8 ();
    uint8_t len = i.ReadU8 ();
    Ptr<InformationElement> element;
    switch (id) {
      case THIS_TYPE:
        element = Create<ThisType> ();
        break;
      ...
    }
    element->Deserialize (i);
    Add (element);
  }
  ...
};

If you do this, you can get rid of the multiple inheritance in
InformationElement (does not need to derive from Header directly: just needs to
re-define similar Serialize/Deserialize methods). This also allows you to not
abuse PeekData.

http://codereview.appspot.com/88094/diff/1070/1090
File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.h (right):

http://codereview.appspot.com/88094/diff/1070/1090#newcode88
Line 88: WifiElementId ElementId () const {
code belongs to .cc file

http://codereview.appspot.com/88094/diff/1070/1105
File src/devices/mesh/dot11s/ie-dot11s-preq.cc (right):

http://codereview.appspot.com/88094/diff/1070/1105#newcode241
Line 241: if ((*j)->IsDo ())
coding style: missing {}

http://codereview.appspot.com/88094/diff/1070/1105#newcode336
Line 336: for (std::vector<Ptr<DestinationAddressUnit> >::const_iterator i =
m_destinations.begin (); i != m_destinations.end(); i++ )
missing {}

http://codereview.appspot.com/88094/diff/1070/1105#newcode349
Line 349: for (std::vector<Ptr<DestinationAddressUnit> >::iterator i =
m_destinations.begin (); i != m_destinations.end(); i++)
missing {}

http://codereview.appspot.com/88094/diff/1070/1105#newcode361
Line 361: for (std::vector<Ptr<DestinationAddressUnit> >::iterator j =
m_destinations.begin (); j != m_destinations.end(); j++)
again, missing {}

http://codereview.appspot.com/88094/diff/1070/1121
File src/devices/mesh/mesh-point-device.cc (right):

http://codereview.appspot.com/88094/diff/1070/1121#newcode92
Line 92: {
coding style: missing indent

http://codereview.appspot.com/88094/diff/1070/1121#newcode423
Line 423: GetInterface (outIface)->SendFrom(packet, src, dst, protocol);
coding style: missing {}

http://codereview.appspot.com/88094/diff/1070/1122
File src/devices/mesh/mesh-point-device.h (right):

http://codereview.appspot.com/88094/diff/1070/1122#newcode194
Line 194: broadcastDataBytes (0)
constructor should be in .cc file

http://codereview.appspot.com/88094/diff/1070/1127
File src/devices/mesh/mesh-wifi-interface-mac.cc (right):

http://codereview.appspot.com/88094/diff/1070/1127#newcode291
Line 291: //Delete smart pointers:
don't forget m_mac->Dispose ()

http://codereview.appspot.com/88094/diff/1070/1127#newcode324
Line 324: if (m_phy != 0)
if you move the SetChannel method to the base class, you don't need this method
anymore

http://codereview.appspot.com/88094/diff/1070/1127#newcode339
Line 339: if (phy != 0)
coding style: missing {}

http://codereview.appspot.com/88094/diff/1070/1128
File src/devices/mesh/mesh-wifi-interface-mac.h (right):

http://codereview.appspot.com/88094/diff/1070/1128#newcode246
Line 246: Statistics () :
generally, even for trivial constructors, we put no code in public headers,
except for performance reasons.

http://codereview.appspot.com/88094/diff/1070/1131
File src/devices/mesh/wifi-information-element.cc (right):

http://codereview.appspot.com/88094/diff/1070/1131#newcode70
Line 70: const uint8_t * data = packet->PeekData();
You are really trying to deserialize the beacon IE information from a packet.
This should all be done from a ::Deserialize method instead.

http://codereview.appspot.com/88094/diff/1070/1132
File src/devices/mesh/wifi-information-element.h (right):

http://codereview.appspot.com/88094/diff/1070/1132#newcode21
Line 21: #ifndef WIFIINFORMATIONELEMENT_H_
would be nice to use '_' to separate word boundaries

http://codereview.appspot.com/88094/diff/1070/1132#newcode84
Line 84: class WifiInformationElement : public Header,
oh, my god. This multiple inheritance really works ? Where do you need Ptr<> for
these objects ?

http://codereview.appspot.com/88094/diff/1070/1132#newcode145
Line 145: bool FindFirst(Ptr<Packet> packet);
method smells bad

http://codereview.appspot.com/88094/diff/1070/1134
File src/helper/mesh-helper.cc (right):

http://codereview.appspot.com/88094/diff/1070/1134#newcode56
Line 56: {
coding style

http://codereview.appspot.com/88094/diff/1070/1134#newcode71
Line 71: {
coding style

http://codereview.appspot.com/88094/diff/1070/1135
File src/helper/mesh-helper.h (right):

http://codereview.appspot.com/88094/diff/1070/1135#newcode57
Line 57: * \param roots               List of root mesh points
I don't see any roots mesh points

http://codereview.appspot.com/88094/diff/1070/1135#newcode68
Line 68: * \param roots               List of root mesh points
same here: no root mesh points

http://codereview.appspot.com/88094/diff/1070/1135#newcode80
Line 80: void SetStackInstaller (std::string type);
given the way this method is implemented, I think that it would be more clear if
the argument was of type Ptr<MeshStack> instead of std::string

http://codereview.appspot.com/88094/diff/1070/1135#newcode81
Line 81: void Report (const ns3::Ptr<ns3::NetDevice>&, std::ostream&);
what does this method do ?

http://codereview.appspot.com/88094/diff/1070/1135#newcode82
Line 82: void ResetStats (const ns3::Ptr<ns3::NetDevice>&);
what does this method do ?

http://codereview.appspot.com/88094/diff/1070/1137
File src/helper/mesh-interface-helper.h (right):

http://codereview.appspot.com/88094/diff/1070/1137#newcode45
Line 45: void SetType (std::string n0 = "", const AttributeValue &v0 =
EmptyAttributeValue (),
better as:
void SetMacAttribute (std::string name, const AttributeValue &value);

http://codereview.appspot.com/88094/diff/1070/1137#newcode58
Line 58: void SetBeParameters ( std::string n0 = "", const AttributeValue &v0 =
EmptyAttributeValue (),
better as:
void SetBeAttribute (std::string name, const AttributeValue &value);

http://codereview.appspot.com/88094/diff/1070/1137#newcode67
Line 67: void SetVoParameters ( std::string n0 = "", const AttributeValue &v0 =
EmptyAttributeValue (),
same comment as SetBeAttribute

http://codereview.appspot.com/88094/diff/1070/1137#newcode88
Line 88: void SetMeshId (std::string s);
what is this ?

http://codereview.appspot.com/88094/diff/1070/1137#newcode96
Line 96: static void Report (const Ptr<WifiNetDevice>& device, std::ostream&
os);
more doc needed. Who calls this ? for what purpose ?

[Pavel Boyko, 2009-07-16 12:39:40]

On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> 5) the biggest technical comment I have is about the way you have information
> elements. Specifically, on the way you deserialize them from FindFirst. You
> really can't use PeekData the way you are using it. 

  Please explain why we "really can't"? 

> Here is a proposal to do
> this differently:

  Your solution looks fine (and performance is much better), but I'm afraid of
the giant switch inside -- can it be avoided?
 
> class IeVector : public Header 
> {
>   typedef std::vector<Ptr<InformationElement> >::const_iterator Iterator;
> public:
>   void Add (Ptr<InformationElement> element);
>   Iterator Begin (void);
>   Iterator End (void);
>   // maybe helper method:
>   Ptr<InformationElement> FindFirst (uint8_t id);
> private:
>   void Serialize (...) {...}
>   ... Deserialize (Buffer::Iterator i) {
>   while (i.GetSize () > 2) {
>     uint8_t id = i.ReadU8 ();
>     uint8_t len = i.ReadU8 ();
>     Ptr<InformationElement> element;
>     switch (id) {
>       case THIS_TYPE:
>         element = Create<ThisType> ();
>         break;
>       ...
>     }
>     element->Deserialize (i);
>     Add (element);
>   }
>   ...
> };
> 
> If you do this, you can get rid of the multiple inheritance in
> InformationElement (does not need to derive from Header directly: just needs
to
> re-define similar Serialize/Deserialize methods). This also allows you to not
> abuse PeekData.

[and.kirill, 2009-07-16 12:59:43]

Brief explanation of using Report and ResetStats methods

http://codereview.appspot.com/88094/diff/1070/1137
File src/helper/mesh-interface-helper.h (right):

http://codereview.appspot.com/88094/diff/1070/1137#newcode96
Line 96: static void Report (const Ptr<WifiNetDevice>& device, std::ostream&
os);
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> more doc needed. Who calls this ? for what purpose ?
The idea of collecting statistics is simple: you call Dot11sInstaller, which
creates dot11s stack and installs it to mesh point device. Alsoyou can obtain
statistics from user script using this installer at any moment of time(Report is
used for this purpose, and Report method is present almost in every mesh  module
class). When we want to collect statistics periodically, we must reset all
previously obtained statistics. So, user script calls Report (), and ResetStats
() of Dot11sInstaller periodically.

[and.kirill, 2009-07-16 13:35:11]

http://codereview.appspot.com/88094/diff/1070/1127
File src/devices/mesh/mesh-wifi-interface-mac.cc (right):

http://codereview.appspot.com/88094/diff/1070/1127#newcode291
Line 291: //Delete smart pointers:
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> don't forget m_mac->Dispose ()

Sorry, could you explain, what exactly I have forgotten here?

[and.kirill, 2009-07-16 13:55:54]

http://codereview.appspot.com/88094/diff/1070/1127
File src/devices/mesh/mesh-wifi-interface-mac.cc (right):

http://codereview.appspot.com/88094/diff/1070/1127#newcode324
Line 324: if (m_phy != 0)
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> if you move the SetChannel method to the base class, you don't need this
method
> anymore

Done.

http://codereview.appspot.com/88094/diff/1070/1135
File src/helper/mesh-helper.h (right):

http://codereview.appspot.com/88094/diff/1070/1135#newcode80
Line 80: void SetStackInstaller (std::string type);
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> given the way this method is implemented, I think that it would be more clear
if
> the argument was of type Ptr<MeshStack> instead of std::string

Well, the same idea is implemented in olsr-helper: create an object by type id,
and type-id is passed as a string: m_agentFactory.SetTypeId
("ns3::olsr::RoutingProtocol");
I have done this as a string to make possible passing a string as a command-line
argument of a script

[and.kirill, 2009-07-21 09:14:48]

http://codereview.appspot.com/88094/diff/1070/1090
File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.h (right):

http://codereview.appspot.com/88094/diff/1070/1090#newcode88
Line 88: WifiElementId ElementId () const {
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> code belongs to .cc file

Done.

http://codereview.appspot.com/88094/diff/1070/1105
File src/devices/mesh/dot11s/ie-dot11s-preq.cc (right):

http://codereview.appspot.com/88094/diff/1070/1105#newcode241
Line 241: if ((*j)->IsDo ())
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> coding style: missing {}

Done.

http://codereview.appspot.com/88094/diff/1070/1105#newcode336
Line 336: for (std::vector<Ptr<DestinationAddressUnit> >::const_iterator i =
m_destinations.begin (); i != m_destinations.end(); i++ )
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> missing {}

Done.

http://codereview.appspot.com/88094/diff/1070/1105#newcode349
Line 349: for (std::vector<Ptr<DestinationAddressUnit> >::iterator i =
m_destinations.begin (); i != m_destinations.end(); i++)
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> missing {}

Done.

http://codereview.appspot.com/88094/diff/1070/1105#newcode361
Line 361: for (std::vector<Ptr<DestinationAddressUnit> >::iterator j =
m_destinations.begin (); j != m_destinations.end(); j++)
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> again, missing {}

Done.

http://codereview.appspot.com/88094/diff/1070/1121
File src/devices/mesh/mesh-point-device.cc (right):

http://codereview.appspot.com/88094/diff/1070/1121#newcode423
Line 423: GetInterface (outIface)->SendFrom(packet, src, dst, protocol);
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> coding style: missing {}

Done.

http://codereview.appspot.com/88094/diff/1070/1122
File src/devices/mesh/mesh-point-device.h (right):

http://codereview.appspot.com/88094/diff/1070/1122#newcode194
Line 194: broadcastDataBytes (0)
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> constructor should be in .cc file

Done.

http://codereview.appspot.com/88094/diff/1070/1127
File src/devices/mesh/mesh-wifi-interface-mac.cc (right):

http://codereview.appspot.com/88094/diff/1070/1127#newcode339
Line 339: if (phy != 0)
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> coding style: missing {}

Done.

http://codereview.appspot.com/88094/diff/1070/1128
File src/devices/mesh/mesh-wifi-interface-mac.h (right):

http://codereview.appspot.com/88094/diff/1070/1128#newcode246
Line 246: Statistics () :
On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> generally, even for trivial constructors, we put no code in public headers,
> except for performance reasons.

Done.

[and.kirill, 2009-07-31 15:35:19]

Hi!
In new patchset I have fixed coding style. Also WifiInformationElement does not
use PeekData any more (it uses WifiInformationElementVector now, but
WifiInformationElement is stull inherited from Header for convenience). Alos all
code was removed from headers.

[Pavel Boyko, 2009-08-14 10:15:21]

On 2009/07/16 11:39:05, Mathieu Lacage wrote:
> Overall, the code looks pretty good. Comments below.

Two weeks passed since last update of this review. Mathieu, please take a look,
I hope we satisfy all your comments.

[Mathieu Lacage, 2009-08-21 14:09:29]

1) I think that you still need some small tweaks to the IE vector handling. I
added comments to explain this: please, ask me for more details if needed

2) small details:
  - don't use '\n' -> use std::endl (portable to win)
  - please, in headers, it would be really helpful if you could try to group
together type definitions, then method declarations, then, member variable
declarations and avoid to group types+related variable decls

3) I think that the code for the installer classes should be moved to the helper
code in src/helper. I will look at this more carefully later and try to come up
with a concrete suggestion as to what this would look like.

http://codereview.appspot.com/88094/diff/4001/4003
File examples/mesh.cc (right):

http://codereview.appspot.com/88094/diff/4001/4003#newcode50
Line 50: int       xSize;
coding style: m_ for class members.

http://codereview.appspot.com/88094/diff/4001/4003#newcode118
Line 118: SeedManager::SetSeed(seed);
you don't need this: --RngSeed= and --RngRun work by default

http://codereview.appspot.com/88094/diff/4001/4005
File src/devices/mesh/dot11s/airtime-metric.cc (right):

http://codereview.appspot.com/88094/diff/4001/4005#newcode33
Line 33: .AddAttribute ( "overheadNanosec",
OverheadNanosec

http://codereview.appspot.com/88094/diff/4001/4005#newcode39
Line 39: .AddAttribute ( "testLengt",
typo + CamelCaps

http://codereview.appspot.com/88094/diff/4001/4005#newcode46
Line 46: .AddAttribute ( "dot11HeaderLengt",
typo+CamelCaps

http://codereview.appspot.com/88094/diff/4001/4005#newcode53
Line 53: .AddAttribute ( "dot11sHeaderLength",
typo+CamelCaps

http://codereview.appspot.com/88094/diff/4001/4005#newcode91
Line 91: * (station->GetAvgSlrc () + 1));
It would be really cool if you could use the tracing hooks which are being added
by nicola to calculate this average slrc in your own code instead of asking the
wifi device to calculate it.

i.e., It would be nice to remove the need for WifiRemoteStation::GetAvgSlrc

http://codereview.appspot.com/88094/diff/4001/4006
File src/devices/mesh/dot11s/airtime-metric.h (right):

http://codereview.appspot.com/88094/diff/4001/4006#newcode49
Line 49: uint32_t m_overheadNanosec;
indent

http://codereview.appspot.com/88094/diff/4001/4008
File src/devices/mesh/dot11s/dot11s-installer.h (right):

http://codereview.appspot.com/88094/diff/4001/4008#newcode34
Line 34: ///\brief Installs 802.11s stack. needed by helper only
I have the feeling that this class and its base class belong to the helper
layer.

http://codereview.appspot.com/88094/diff/4001/4012
File src/devices/mesh/dot11s/hwmp-protocol-mac.cc (right):

http://codereview.appspot.com/88094/diff/4001/4012#newcode432
Line 432: os << "<Statistics "
std::endl is your friend

http://codereview.appspot.com/88094/diff/4001/4013
File src/devices/mesh/dot11s/hwmp-protocol-mac.h (right):

http://codereview.appspot.com/88094/diff/4001/4013#newcode104
Line 104: struct MyPerr {
usually, we try to move all type definitions together, and then, put together
all variable declarations

http://codereview.appspot.com/88094/diff/4001/4014
File src/devices/mesh/dot11s/hwmp-protocol.cc (right):

http://codereview.appspot.com/88094/diff/4001/4014#newcode56
Line 56: .AddAttribute ( "maxQueueSize",
CamelCase

http://codereview.appspot.com/88094/diff/4001/4014#newcode63
Line 63: .AddAttribute ( "dot11MeshHWMPmaxPREQretries",
CamelCase

http://codereview.appspot.com/88094/diff/4001/4014#newcode70
Line 70: .AddAttribute ( "dot11MeshHWMPnetDiameterTraversalTime",
CamelCase

same for all attributes below

http://codereview.appspot.com/88094/diff/4001/4014#newcode902
Line 902: {
indent

http://codereview.appspot.com/88094/diff/4001/4014#newcode923
Line 923: {
indent

http://codereview.appspot.com/88094/diff/4001/4014#newcode1111
Line 1111: "address=\"" << m_address << "\"\n"
In C++ "\n" is std::endl

http://codereview.appspot.com/88094/diff/4001/4014#newcode1130
Line 1130: {
indent

http://codereview.appspot.com/88094/diff/4001/4014#newcode1141
Line 1141: plugin->second->ResetStats ();
indent

http://codereview.appspot.com/88094/diff/4001/4015
File src/devices/mesh/dot11s/hwmp-protocol.h (right):

http://codereview.appspot.com/88094/diff/4001/4015#newcode176
Line 176: Statistics m_stats;
move this down with the other member variable declarations

http://codereview.appspot.com/88094/diff/4001/4020
File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.cc (right):

http://codereview.appspot.com/88094/diff/4001/4020#newcode113
Line 113: IeBeaconTiming::DelNeighboursTimingElementUnit (uint16_t aid, Time
last_beacon, //MicroSeconds!
probably the MicroSeconds comment is useless since you are using a Time type,
right ?

http://codereview.appspot.com/88094/diff/4001/4020#newcode203
Line 203: ;
??

http://codereview.appspot.com/88094/diff/4001/4023
File src/devices/mesh/dot11s/ie-dot11s-configuration.h (right):

http://codereview.appspot.com/88094/diff/4001/4023#newcode77
Line 77: class dot11sMeshCapability
CamelCaps

http://codereview.appspot.com/88094/diff/4001/4023#newcode84
Line 84: uint16_t GetUint16 () const;
if it's a class, it should have no public member variables, but well, I can see
the pain of adding all setters and getters. ignore me.

http://codereview.appspot.com/88094/diff/4001/4025
File src/devices/mesh/dot11s/ie-dot11s-id.h (right):

http://codereview.appspot.com/88094/diff/4001/4025#newcode47
Line 47: WifiElementId ElementId () const;
repeat virtual keyword to show those who read the code that this method is
overridden from the base class.

http://codereview.appspot.com/88094/diff/4001/4044
File src/devices/mesh/dot11s/peer-management-protocol-mac.cc (right):

http://codereview.appspot.com/88094/diff/4001/4044#newcode63
Line 63: WifiInformationElementVector elements =
WifiInformationElementVector::DeserializePacket (packet);
if WifiInformationElementVector is_a Header, you can do this:

WifiInformationElementVector elements;
packet->RemoveHeader (elements);

http://codereview.appspot.com/88094/diff/4001/4053
File src/devices/mesh/mesh-point-device.h (right):

http://codereview.appspot.com/88094/diff/4001/4053#newcode92
Line 92: virtual void SetName (const std::string name);
kill SetName and GetName

http://codereview.appspot.com/88094/diff/4001/4062
File src/devices/mesh/wifi-information-element-vector.cc (right):

http://codereview.appspot.com/88094/diff/4001/4062#newcode83
Line 83: WifiInformationElementVector::DeserializePacket (Ptr<Packet> packet)
if this becomes Header::Deserialize (Buffer::Iterator i),

you can read the id of the information element directly and, each
InformationElement subclass does not need to read the id itself.

http://codereview.appspot.com/88094/diff/4001/4062#newcode151
Line 151: if (sortByElementId)
Can't you just assume that sortByElementId is true all the time ?

http://codereview.appspot.com/88094/diff/4001/4062#newcode159
Line 159: packet->AddHeader (**i);
here, for symetry with Deserialize, you should write yourself the element id by
calling i.WriteU8 (i->ElementId ())
and then, delegate the IE serialization with i->Serialize (i); so the
InformationElement does not write the id itself.

http://codereview.appspot.com/88094/diff/4001/4063
File src/devices/mesh/wifi-information-element-vector.h (right):

http://codereview.appspot.com/88094/diff/4001/4063#newcode35
Line 35: class WifiInformationElementVector
this class should derive from Header

http://codereview.appspot.com/88094/diff/4001/4063#newcode46
Line 46: static WifiInformationElementVector DeserializePacket (Ptr<Packet>
packet);
and that static method would become the Deserialize method overriden from the
base class

http://codereview.appspot.com/88094/diff/4001/4063#newcode47
Line 47: Ptr<Packet> CreatePacket (bool sortByElementId = true);
if you derive from Header at line 35, this befores Serialize overriden from
Header base class

http://codereview.appspot.com/88094/diff/4001/4063#newcode55
Line 55: class EmptyIe : public Header
you don't need this class anymore if you derive from Header at line 35

http://codereview.appspot.com/88094/diff/4001/4065
File src/devices/mesh/wifi-information-element.h (right):

http://codereview.appspot.com/88094/diff/4001/4065#newcode84
Line 84: class WifiInformationElement : public Header,
you do not need to derive from Header anymore.

http://codereview.appspot.com/88094/diff/4001/4065#newcode148
Line 148: virtual uint8_t GetInformationSize () const = 0;
you can make these methods private

[and.kirill, 2009-08-21 19:32:25]

An only comment that is ignored by me - is sort type definitions and variables.
I shal fix it as soon as possible.

[and.kirill, 2009-08-21 19:33:33]

I have fixed most of comments and I have some questions about
WifiInformationElementVector.

http://codereview.appspot.com/88094/diff/4001/4003
File examples/mesh.cc (right):

http://codereview.appspot.com/88094/diff/4001/4003#newcode50
Line 50: int       xSize;
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4003#newcode118
Line 118: SeedManager::SetSeed(seed);
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4005
File src/devices/mesh/dot11s/airtime-metric.cc (right):

http://codereview.appspot.com/88094/diff/4001/4005#newcode33
Line 33: .AddAttribute ( "overheadNanosec",
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4005#newcode39
Line 39: .AddAttribute ( "testLengt",
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4005#newcode46
Line 46: .AddAttribute ( "dot11HeaderLengt",
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4005#newcode53
Line 53: .AddAttribute ( "dot11sHeaderLength",
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4005#newcode91
Line 91: * (station->GetAvgSlrc () + 1));
Well, I thought about using trace hooks, but I have no idea how to connect to
WifiRemoteStation, because list of remote stations changes due to link failure.
And I do not care about removing that stations, tat are unreachable more. When I
start to use trace hooks, I need to remember the list of remote stations at my
metric calculator. Could you, please, make an advice how to do this better?

http://codereview.appspot.com/88094/diff/4001/4006
File src/devices/mesh/dot11s/airtime-metric.h (right):

http://codereview.appspot.com/88094/diff/4001/4006#newcode49
Line 49: uint32_t m_overheadNanosec;
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4008
File src/devices/mesh/dot11s/dot11s-installer.h (right):

http://codereview.appspot.com/88094/diff/4001/4008#newcode34
Line 34: ///\brief Installs 802.11s stack. needed by helper only
I have removed this part of code from helper, because when I have done this, all
headers from dot11s namespace are not needed to be copied to ns3headers (and we
have less componets when building NS)

http://codereview.appspot.com/88094/diff/4001/4012
File src/devices/mesh/dot11s/hwmp-protocol-mac.cc (right):

http://codereview.appspot.com/88094/diff/4001/4012#newcode432
Line 432: os << "<Statistics "
Done in all other Statistics::Print

Done.

http://codereview.appspot.com/88094/diff/4001/4014
File src/devices/mesh/dot11s/hwmp-protocol.cc (right):

http://codereview.appspot.com/88094/diff/4001/4014#newcode56
Line 56: .AddAttribute ( "maxQueueSize",
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4014#newcode63
Line 63: .AddAttribute ( "dot11MeshHWMPmaxPREQretries",
The name of this attribute was copied from 11s standart, so it was written
wrong. Fixed.

http://codereview.appspot.com/88094/diff/4001/4014#newcode70
Line 70: .AddAttribute ( "dot11MeshHWMPnetDiameterTraversalTime",
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4014#newcode902
Line 902: {
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4014#newcode923
Line 923: {
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4014#newcode1111
Line 1111: "address=\"" << m_address << "\"\n"
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4014#newcode1130
Line 1130: {
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4014#newcode1141
Line 1141: plugin->second->ResetStats ();
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4015
File src/devices/mesh/dot11s/hwmp-protocol.h (right):

http://codereview.appspot.com/88094/diff/4001/4015#newcode176
Line 176: Statistics m_stats;
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4020
File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.cc (right):

http://codereview.appspot.com/88094/diff/4001/4020#newcode113
Line 113: IeBeaconTiming::DelNeighboursTimingElementUnit (uint16_t aid, Time
last_beacon, //MicroSeconds!
I have removed this comment.

http://codereview.appspot.com/88094/diff/4001/4020#newcode203
Line 203: ;
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4023
File src/devices/mesh/dot11s/ie-dot11s-configuration.h (right):

http://codereview.appspot.com/88094/diff/4001/4023#newcode77
Line 77: class dot11sMeshCapability
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4025
File src/devices/mesh/dot11s/ie-dot11s-id.h (right):

http://codereview.appspot.com/88094/diff/4001/4025#newcode47
Line 47: WifiElementId ElementId () const;
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4044
File src/devices/mesh/dot11s/peer-management-protocol-mac.cc (right):

http://codereview.appspot.com/88094/diff/4001/4044#newcode63
Line 63: WifiInformationElementVector elements =
WifiInformationElementVector::DeserializePacket (packet);
Threr is a little problem with doing a WifiInformationElementVector inerited
from Header: When I call RemoveHeader, I call Deserialize, but inside
Deserialize I have only Buffer::Iterator and my deserializator may go through
this buffer (hoping to next information element) in inifinite loop. When I parse
packet in WifiInformationElementVector, I suppose that the packet contains only
information elements and nothing else till end, and when I work with a packet
rather than with Buffer::Iterator, I always know, where shall I stop (I need
just to check the length of the rest packet). Can I control the end of Buffer
from Deserialize?

http://codereview.appspot.com/88094/diff/4001/4053
File src/devices/mesh/mesh-point-device.h (right):

http://codereview.appspot.com/88094/diff/4001/4053#newcode92
Line 92: virtual void SetName (const std::string name);
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.

http://codereview.appspot.com/88094/diff/4001/4062
File src/devices/mesh/wifi-information-element-vector.cc (right):

http://codereview.appspot.com/88094/diff/4001/4062#newcode83
Line 83: WifiInformationElementVector::DeserializePacket (Ptr<Packet> packet)
Yes, but how can I control, that I have reached the end of the Buffer? When I
try to remove header from the packet - it is obvious

http://codereview.appspot.com/88094/diff/4001/4062#newcode151
Line 151: if (sortByElementId)
I know, that sorting is needed in beacons, but in HWMP protocol we do not have
IDs of some information elements(path selection frames contain many IE), so I
have added this flag

http://codereview.appspot.com/88094/diff/4001/4062#newcode159
Line 159: packet->AddHeader (**i);
The problem is the same: how to control the end of buffer from Deserialize

http://codereview.appspot.com/88094/diff/4001/4065
File src/devices/mesh/wifi-information-element.h (right):

http://codereview.appspot.com/88094/diff/4001/4065#newcode84
Line 84: class WifiInformationElement : public Header,
In path selection frames in HWMP I add an information element as a header. Is it
really needed to wrap it into a vector?

http://codereview.appspot.com/88094/diff/4001/4065#newcode148
Line 148: virtual uint8_t GetInformationSize () const = 0;
On 2009/08/21 14:09:30, Mathieu Lacage wrote:

Done.