[Ns-developers] How to maintain a repository for merge
Mirko Banchi
mk.banchi at gmail.com
Fri Mar 20 05:59:40 PDT 2009
Mathieu Lacage ha scritto:
> 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...
>
Yes :) You're right! I have to change this method signature...
> 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.
>
qos-utils.h seems to make sense...we could move also qos-tag (Which
currently is defined in src/contrib/qos-tag.h and src/contrib/qos-tag.cc
in this unit.
>> 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.
>
These two functions are used only to sets correctly sourceAddr and
DestAddr in an A-MSDU subframe header. If the A-MSDU is for an ap
all MSDUs forming the aggregated packet must have the correct
destination address (address 3 in this case) otherwise ap will not able
to forward each single MSDU to correct station.
On the other hand, if the A-MSDU is for a station, destination address
in each single A-MSDU subframe header must be setted to address 1.
>> 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).
>
If i'm understanding, should be there four new classes:
VO_EdcaTxopN
VI_EdcaTxopN
BE_EdcaTxopN
BK_EdcaTxopN
?
>> 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 :)
>
template <typename T, typename T1, typename T2>
Ptr<T> Create (T1 a1, T2 a2)
{
return Ptr<T> (new T (a1, a2), false);
}
Is there a real difference between:
Packet *deaggregatedPacket = new Packet (buffer, hdr.GetLength());
Ptr<Packet> p (deaggregatedPacket);
and
Ptr<Packet> p = Create<Packet> (buffer, hdr.GetLength());
?
>From the above method definition two options seems to operate in the
same manner.
However no problems...i can use Create<Packet> instead of operator new.
>> 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.
>
>
Ok. A new class WifiMacHelper seems good. I think that the second is
better.
>> 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]);
>
>
Yes, sorry...a tab character is present there...:( I did't see it
>
>> 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.
Ok.
>
> this is really cool: keep up the good work !
>
Thank you very much and sorry for the late answer.
Mirko
--
Mirko Banchi
e-mail: mk.banchi at gmail.com
e-mail: mk.banchi at virgilio.it
id-jabber: mk.banchi at jabber.org
id-msn: mb11684 at hotmail.com
PGP key fingerprint:
308F BFB1 4E67 2522 C88E
DC69 7631 52ED 32A5 6456
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3410 bytes
Desc: S/MIME Cryptographic Signature
Url : http://mailman.isi.edu/pipermail/ns-developers/attachments/20090320/4f1d4dea/smime.bin
More information about the Ns-developers
mailing list