Bug 2945

Related to bug 881. I'd like to come up with a clean solution for mesh and other extensions like wigig.

This patch propose to use a map instead of a vector, which I think is a better solution.

Hello Sébastien,
I have the impression that the content of the IE is no longer printed out (PrintInformationElements is empty whereas other methods like SerializeInformationElements iterate over all the elements).
Would the following do the trick?

void
MgtCommonHeader::PrintInformationElements (std::ostream &os) const
{
  for (const auto& elem : m_map)
    {
      switch (elem->first)
        {
          [...]
          case IE_HT_CAPABILITIES:
              os << "HT Capabilities=" << *(elem->second);
          [...]
        }
      if (elem != m_map.end ())
      {
        os << " , ";

    }
}

(In reply to Rediet from comment #2)
> Hello Sébastien,
> I have the impression that the content of the IE is no longer printed out
> (PrintInformationElements is empty whereas other methods like
> SerializeInformationElements iterate over all the elements).
> Would the following do the trick?
> 
> void
> MgtCommonHeader::PrintInformationElements (std::ostream &os) const
> {
>   for (const auto& elem : m_map)
>     {
>       switch (elem->first)
>         {
>           [...]
>           case IE_HT_CAPABILITIES:
>               os << "HT Capabilities=" << *(elem->second);
>           [...]
>         }
>       if (elem != m_map.end ())
>       {
>         os << " , ";
> 
>     }
> }

Correct, this was like this in Hany's code, I still need to update that, thanks.

Created attachment 3138 [details]
second proposal

I made some updates to previous patch, mainly updating Print so that we can now visualize IEs.

I want to merge those changes with WifiInformationElementVector, so that later mesh, wifi, wigig, etc. can use the same way of working.

WifiInformationElementVector uses std::vector, whereas this proposal makes use of std::map, which I think is better here to have a map (id, IE). Do everyone agrees to change WifiInformationElementVector to WifiInformationElementMap? If yes, I'll work on a third patch update.

The heavy use of static cast suggests to me that something could be improved in the design (use of inheritance, templates, TLVs ?).  Were other alternatives considered and discarded?  DynamicCast, even?

Created attachment 3145 [details]
third proposal

Tom, I changed StaticCast to DynamicCast, I had let those StaticCast from Hany's changes.
I do not think we can really avoid such casts here. Or do you have a better approach in mind?

Can you also address my comment with regards to the use of a map instead of a vector?
Thanks.

Regarding the use of map vs vector, I agree that map is probably the better container because the vector will be sparse (unless IE ordering matters).

Regarding the overall patch, I am not sure that it completely accomplishes the goal to simplify future extensions, because I still see in mgt-headers.cc that there is switch() code based on the IE type code, and the user has to edit this class to add new IE types to pretty-print.

My understanding is that we want a solution that requires no 'wifi' module changes to add a new IE relevant to a particular standard defined in another module such as 11ad.

One thing that has changed recently in the network module is the ability to have variable-length headers (bug 2505), so we could consider to redo this in terms of header code, since the ns3::Header class was designed to handle this use case.

So for instance, a variable-length WifiManagementFrameBody (derived from ns3::Header) could be defined to hold all of these TLVs (IEs), and then the WifiManagementFrameBody would be able to hold and access IEs, each of which are themselves headers, and store them in a map.

On the sending side, IEs could just be directly added to the packet as usual (AddHeader()).  On the receiving side, the MAC could deserialize into a WifiManagementFrameBody which could be passed by value or by const reference to methods that need to access the IEs.

In the (e.g.) 11ad code then, suppose you have an IE named 'ElevenAdIe' and a WifiManagementFrameBody 'body'.  Define a method on WifiManagementFrameBody as follows:

  bool DeserializeIfPresent (Header& h);

this method can then peek the type/length field and deserialize the variable-length header using virtual Deserialize methods.

and then the client code looks like:

  ElevenAdIe hdr;
  bool found = body.DeserializeIfPresent (hdr);
  if (found)
    {
      // process header
    }

instead of 

  Ptr<ElevenIdIe> hdr = DynamicCast<ElevenAdIe> (assocResp.GetInformationElement (IE_ELEVEN_AD));
  if (hdr)
    {
       // process header
    }

I guess it is not so much different than what you have in this patch except it avoids casts and would solve the Print() issue.

I g

OK, I will first move to map everywhere, then I will further investigate your proposal that seams appealing since it would avoid all those casts.

Tom, if I am right, the first step would be to make WifiInformationElement derive from Header. Does it make sense?

Tom, do we still need a map with your proposal? If you add the header, why would you also hold the map? I guess this is not needed anymore, or am I misunderstanding your idea?

(In reply to sebastien.deronne from comment #9)
> Tom, if I am right, the first step would be to make WifiInformationElement
> derive from Header. Does it make sense?

I hadn't thought about sequence of changes (not sure if that change in isolation will work).

(In reply to sebastien.deronne from comment #10)
> Tom, do we still need a map with your proposal? If you add the header, why
> would you also hold the map? I guess this is not needed anymore, or am I
> misunderstanding your idea?

What I was thinking was that WifiManagementFrameBody was a special type of Header, one that contained many variable-sized headers (IEs).  Upon deserialization of WifiManagementFrameBody from the Packet, this Header would be a concatenation of other Headers (IEs).  To extract a single IE, my thought was that the WifiManagementFrameBody dumped them all into a map (as a deserialization step) so they could be searched by TID value.

Note that I was proposing that this WifiManagementFrameBody was not needed when constructing a packet, just when deserializing.

(In reply to Tom Henderson from comment #12)
> (In reply to sebastien.deronne from comment #10)
> > Tom, do we still need a map with your proposal? If you add the header, why
> > would you also hold the map? I guess this is not needed anymore, or am I
> > misunderstanding your idea?
> 
> What I was thinking was that WifiManagementFrameBody was a special type of
> Header, one that contained many variable-sized headers (IEs).  Upon
> deserialization of WifiManagementFrameBody from the Packet, this Header
> would be a concatenation of other Headers (IEs).  To extract a single IE, my
> thought was that the WifiManagementFrameBody dumped them all into a map (as
> a deserialization step) so they could be searched by TID value.

But then we still need a kind of switch case as I have in CommonMgtHeader to search for every IEs in the map, so adding an IE would also require an update in WifiManagementFrameBody.

> 
> Note that I was proposing that this WifiManagementFrameBody was not needed
> when constructing a packet, just when deserializing.

Note that in the meantime I tried to have WifiInformationElement inherit from Header, but then I have issues since I can no longer use dynamic_cast, so I need to implement all at once and cannot go simply step by step. But maybe with your proposal WifiInformationElement is even no longer needed, or should be adapted to work as your
WifiManagementFrameBody class.

*** Bug 881 has been marked as a duplicate of this bug. ***

Moved to https://gitlab.com/nsnam/ns-3-dev/-/issues/173