[Ns-developers] 802.11s code review

[Mathieu Lacage]

hi,

2 weeks ago, at the ns-3 workshop
(http://www.nsnam.org/wiki/index.php/Wns3-2009), Kirill Andreev
presented a 802.11s implementation for ns-3
(http://forge.wenos.ru/hgprojects/ns3dev/). 

There is really a _lot_ of new code here so, I have done a relatively
high-level first review and did not look at very many details. I have
explicitely CCed gustavo because I think he could helpfully comment on
the code and my own comments below (more specifically, comment 3):

1) I found routing-table-test.cc which, I think, is awesome. It would
make sense to make it a test just like src/devices/wifi/wifi-test.cc
which is run in an automated way with "./waf check".

2) There are a bunch of coding style issues:
http://www.nsnam.org/codingstyle.html summarizes some (but not all)
constraints: the goal is really to try to make all of the ns-3 code look
the same and make it impossible to figure out who wrote the code by just
looking at its indentation and layout. A few quick examples:

uint32_t
L2RoutingNetDevice::GetIfIndex() const
  {
    NS_LOG_FUNCTION_NOARGS ();
    return m_ifIndex;
  }

should be left-aligned:

uint32_t
L2RoutingNetDevice::GetIfIndex() const
{
  NS_LOG_FUNCTION_NOARGS ();
  return m_ifIndex;
}

this if statement:

  if (!Mac48Address::IsMatchingType (routingPort->GetAddress ()))
    NS_FATAL_ERROR ("Device does not support eui 48 addresses: cannot be
added to bridge.");

should use braces, even for a single-line statement:

  if (!Mac48Address::IsMatchingType (routingPort->GetAddress ()))
    {
      NS_FATAL_ERROR ("Device does not support eui 48 addresses: cannot
be added to bridge.");
    }

This inline method declaration (WifiBeaconTimingElement):

  uint16_t TIMESTAMP_TO_U16(Time x)
  {
    return ((uint16_t)((x.GetMicroSeconds() >> 8)&0xffff));
  };

should not be inline. i.e., it should be declared here and the
implementation moved to the .cc file to maximize the readability of the
header. (Of course, its name should follow the MixedCamel convention
rather than ALL_UPPER_CASE).

enum dot11sPathSelectionProtocol is declared in
src/devices/wifi/mesh-configuration-element.h which is a bit
un-intuitive: it would be nice if the name of the enum had at least some
sort of relationship as to which header it is defined in. The same goes
for DestinationAddressUnit defined in
mesh-wifi-preq-information-element.h: maybe making it an inner class of
the outer WifiPreqInformationElement class would help.

I know some people will think I am way too anal about this but, well,
that's fine: it's just my job to make everyone miserable :)

3) src/devices/l2-routing/l2-routing-main/l2-routing-net-device.h looks
like a more generic version of src/devices/bridge-net-device.cc which
delegates l2 forwarding decisions to another class. I wonder if it would
be possible to, instead, delegate the forwarding/routing decisions to a
subclass through a virtual method from a base class:

class L2RoutingNetDevice : public NetDevice
{
  // check device capabilities, register receive callback, 
  // manage associated BridgeChannel, call DoAddPort to notify subclass
  void AddPort (...);
  uint32_t GetNPorts (void) const;

  // Note: it might make sense to add EnablePort and Disable port, just
like Hwmp::EnablePort and Hwmp::DisablePort. If so, AddPort should
return a port index and Enable and Disable port should take that index.
private:
  // called by ::AddPort
  virtual void DoAddPort (Ptr<NetDevice> dev) = 0;
  virtual uint32_t DoGetNPorts (void) = 0;
  // called by ::ReceiveFromDevice and ::Send
  virtual void DoRoute (Ptr<NetDevice> from, Mac48Address src,
Mac48Address dst, Ptr<Packet> p, uint8_t protocol) = 0;
};

class HwmpNetDevice : public L2RoutingNetDevice
{
private:
  // implement all these methods.
  virtual void DoAddPort (Ptr<NetDevice> dev);
  virtual uint32_t DoGetNPorts (void);
  virtual void DoRoute (Ptr<NetDevice> from, Mac48Address src,
Mac48Address dst, Ptr<Packet> p, uint8_t protocol);

  // keep track of the port list.
  std::vector<Ptr<NetDevice> > m_ports;
};

class LearningBridgeNetDevice : L2RoutingNetDevice
{
private:
  // implement all these methods.
  virtual void DoAddPort (Ptr<NetDevice> dev);
  virtual uint32_t DoGetNPorts (void);
  virtual void DoRoute (Ptr<NetDevice> from, Mac48Address src,
Mac48Address dst, Ptr<Packet> p, uint8_t protocol);

  // keep track of the port list.
  std::vector<Ptr<NetDevice> > m_ports;
};

4) I remember one of the questions during the wns3 workshop was related
to tags and how to remove them. I can see that, in hwmp.cc, they are
removed with RemoveAllTags which could be problematic. I don't think we
have a better way to handle this for now but the plan is to solve this
problem as part of the ipv4-rework over the next 2 weeks (so, as soon as
I am done with this review, I will work on fixing this issue to solve
your problem here :)

5) Why are MeshWifiMac::SetPrepReceivedCallback,
SetPerrReceivedCallback, SetPeerStatusCallback, etc. virtual ? Do you
have subclasses of MeshWifiMac ?

6) What happens in MeshWifiMac if one of the packets to forward does not
have an HwmpTag ? Is this tag mandatory to make MeshWifiMac work ? If
so, maybe it should be renamed MeshWifiMacTag.

7) What is a WifiPeerManager ? What is the purpose of
WifiPeerManager::AttachPorts ? I don't see this method being called from
anywhere so, I wonder what it's used for.

Mathieu