[Ns-developers] How to maintain a repository for merge

[Mathieu Lacage]

On Sun, 2009-03-15 at 20:19 +0100, Mirko Banchi wrote:

> Added files:

All the code looks great ! Comments below.

> 
> src/devices/wifi/qsta-wifi-mac.h
> src/devices/wifi/qsta-wifi-mac.cc

The above looks good: do you think you could come up with a way of
sharing the common code between nqsta-wifi-mac and qsta-wifi-mac ? It
seems that the association state handling is fairly similar: maybe it
could be pushed in a common base class StaWifiMac which delegates some
work to the two subclasses NqstaWifiMac and QstaWifiMac ? The same would
also hold for NqapWifiMac and QapWifiMac. Note that I am not saying that
you _must_ do this because I suspect that there are some very subtle
differences which might make this very painful so, I am just asking that
you try to see if it is feasible without making the code unreadable.

> src/devices/wifi/qap-wifi-mac.h
> src/devices/wifi/qap-wifi-mac.cc

This signature does not make me very happy:

  void ForwardDown (Ptr<const Packet> packet, Mac48Address from,
Mac48Address to,
                    WifiMacHeader const *qosHeader = 0);

In the past, we have had a lot of debates about function arguments with
default values: I know that some disagree with me, but, please, don't
use them here: I find that they make it harder for me to follow which
methods are called. 

In this case, I think that you should add two extra arguments to
ForwardDown: a tid and a qospolicy and just make them unconditional and
make the callers pass the right values.


Also, the following can't possibly work:

+  if (qosHeader != 0)
+    {
...
+    }
+  else
+    {
+      uint8_t tid = GetTidForPacket (packet);
+      if (tid < 8)
+        {
+          hdr.SetQosTid (tid);
+          uint8_t ac = MapTidToAc (qosHeader->GetQosTid ());

ooh, qosHeader is 0...

The two functions  uint8_t GetTidForPacket (Ptr<const Packet> packet)
and uint8_t MapTidToAc (uint8_t tid) could be moved to
src/devices/wifi/qos-utils.h or something similar, although, really, not
a big deal.

> src/devices/wifi/edca-txop-n.h
> src/devices/wifi/edca-txop-n.cc

Although I see that you use this only for debugging, the m_accessClass
field looked pretty bad: ideally, EdcaTxopN would not need to know which
class it belongs to. 

The other thing I am a bit nervous about is m_typeOfStation which I see
is used in MapSrcAddress and MapDstAddress. Clearly, using a string
comparison against a magic string value here looks fairly bad. If you
really must parameterize behavior based on a parameter value, please,
use an enum instead of a string here. Anyway, I would be mostly curious
to know what other behaviors MapSrcAddress and MapDstAddress could
implement.

> src/devices/wifi/edca-parameter-set.h
> src/devices/wifi/edca-parameter-set.cc

The two files above don't look quite right: serializing all these
integer arguments in a single string blob looks a bit horrible. It seems
to me that each of these parameter could be changed directly as an
attribute of the EdcaTxopN class and, then, QstaWifiMac and QapWifiMac
could export a set of attributes such as VO_EdcaTxopN, VI_EdcaTxopN,
etc. This would allow you to kill the whole EdcaParameterSet class as
well as make it easier for users to specify a new value for these
attributes (they would not have to learn your serialization format).

> src/devices/wifi/msdu-aggregator.h
> src/devices/wifi/msdu-aggregator.cc

I can't really comment on your aggregation API but,
MsgAggregator::Deaggregate:

+     //a new packet is created
+     Packet *deaggregatedPacket = new Packet (buffer, hdr.GetLength
());
+     Ptr<Packet> p (deaggregatedPacket);

Use Create<Packet> (buffer, ...) instead of operator new which will get
the reference counting right. The above is leaking a count so, you are
leaking a packet each time you get there. (for reference, look at
Create<> in src/core/ptr.h to see the critical bit you missed :)

> src/devices/wifi/msdu-standard-aggregator.h
> src/devices/wifi/msdu-standard-aggregator.cc

nothing to say here.

> src/devices/wifi/amsdu-subframe-header.h
> src/devices/wifi/amsdu-subframe-header.cc

both look good.

> src/helper/msdu-aggregator-helper.h
> src/helper/msdu-aggregator-helper.cc

I have to confess that I am, again, uneasy about the string-based AC
control in this API, but the real issue here is the DynamicCast in
Install. It would be really nice to be able to avoid it and one way to
do this would be to do what we do for the PHY layer where the creation
of the PHY object is delated to a WifiPhyHelper class in
src/helper/wifi-helper.h. I think that you could create a similar
WifiMacHelper class with a set of subclasses, some of which could export
extra information to build the msdu aggregators for the Qos MACs.

i.e.,

class WifiMacHelper
{
public:
  virtual Ptr<WifiMac> *Create (void);
};

class WifiHelper
{
public:
  NetDeviceContainer Install (const WifiPhyHelper &phy, const WifiMacHelper &mac, NodeContainer c) const;
  ...
};

and:

class NqapWifiMacHelper : public WifiMacHelper
{
  void SetDcaAttribute (name, value, ...);
};

class NqstaWifiMacHelper : public WifiMacHelper
{
...
};

class AdhocWifiMacHelper : public WifiMacHelper
{
...
};

class QapWifiMacHelper : public WifiMacHelper
{
  void SetMsduAggregator (...);
  void SetEdcaAttribute (type, name, value, ...);
};

class QstaWifiMacHelper : public WifiMacHelper
{
  void SetMsduAggregator (...);
  void SetEdcaAttribute (type, name, value, ...);
};

Of course, you could decrease the number of subclasses with this:
class NqosWifiMacHelper : public WifiMacHelper
{
  void SetType (std::string typeid);
  ...
};
class QosWifiMacHelper : public WifiMacHelper
{
  void SetType (std::string typeid);
  ...
};

(either of these option would be fine with me)

Ideally, you would also pass an enum-based AC to SetMsduAggregator
instead of the string-based AC.


> Modified files:

> src/devices/wifi/wifi-mac-header.h
> src/devices/wifi/wifi-mac-header.cc

both look ok to me as-is.

> src/devices/wifi/mac-tx-middle.h
> src/devices/wifi/mac-tx-middle.cc

looks good to me modulo coding style
(http://www.nsnam.org/codingstyle.html).

For example:

+  for (; i != m_qosSequences.end (); i++)
+    delete [] i->second;

should be written as:
for ()
  {
    // XXX
  }

+      else
+        {

also, wrong indentation:

+                 retval = 0;
+          std::pair <Mac48Address,uint16_t*> newSeq (hdr->GetAddr1 (),
new uint16_t[16]);



> src/devices/wifi/wifi-mac-queue.h
> src/devices/wifi/wifi-mac-queue.cc

the above looks good to me modulo:
+  Ptr<const Packet> Dequeue (WifiMacHeader *hdr, uint8_t index,
Mac48Address addr);

the index argument should probably be an enum instead and the
documentation for this function should probably try to explain why it is
needed.

this is really cool: keep up the good work !

Mathieu