ns-3: Extract WifiInformationElement(Vector) core to wifi module

This patch aims to bring the core of the current WifiInformationElement and WifiInformationElementVector classes into the wifi module, where they can then enjoy greater use (currently they are in the mesh module and contain mesh-specific aspects). It is proposed code for Bug 881 in the ns-3 Bugzilla module.

The latest patch is based on ns-3-dev changeset 6360:d8975477ff6a.

[Nicola Baldo, 2010-06-21 23:09:07]

The patch is ok for me, I only have a few minor comments (see below). After
these comments are resolved, you'll have my +1. 
However, since also the mesh code is affected by this patch, you should also get
a +1 by Kirill before pushing the change to ns-3-dev.

http://codereview.appspot.com/898045/diff/7001/5033
File src/devices/wifi/supported-rates.h (right):

http://codereview.appspot.com/898045/diff/7001/5033#newcode43
src/devices/wifi/supported-rates.h:43: WifiElementId ElementId () const { return
IE_SUPPORTED_RATES; }
function body should go into .cc file

http://codereview.appspot.com/898045/diff/7001/5037
File src/devices/wifi/wifi-information-element.h (right):

http://codereview.appspot.com/898045/diff/7001/5037#newcode65
src/devices/wifi/wifi-information-element.h:65: typedef uint8_t WifiElementId;
Maybe this should be called WifiInformationElementId

http://codereview.appspot.com/898045/diff/7001/5037#newcode137
src/devices/wifi/wifi-information-element.h:137: Buffer::Iterator DeserializeIE
(Buffer::Iterator i);
What's the difference between SerializeIE and SerializeInformation ()?

[Dean, 2010-06-22 13:21:54]

Nicola,

Thanks for your comments. I have addressed them in the updated patch available
on this issue.

Kirill,

This change proposed for Bug 881 affects mesh code, so I would be grateful if
you could cast your eye over it.

Thanks,
Dean.

http://codereview.appspot.com/898045/diff/7001/5033
File src/devices/wifi/supported-rates.h (right):

http://codereview.appspot.com/898045/diff/7001/5033#newcode43
src/devices/wifi/supported-rates.h:43: WifiElementId ElementId () const { return
IE_SUPPORTED_RATES; }
On 2010/06/21 23:09:07, Nicola Baldo wrote:
> function body should go into .cc file

Done.

http://codereview.appspot.com/898045/diff/7001/5037
File src/devices/wifi/wifi-information-element.h (right):

http://codereview.appspot.com/898045/diff/7001/5037#newcode65
src/devices/wifi/wifi-information-element.h:65: typedef uint8_t WifiElementId;
On 2010/06/21 23:09:07, Nicola Baldo wrote:
> Maybe this should be called WifiInformationElementId

Done.

http://codereview.appspot.com/898045/diff/7001/5037#newcode137
src/devices/wifi/wifi-information-element.h:137: Buffer::Iterator DeserializeIE
(Buffer::Iterator i);
On 2010/06/21 23:09:07, Nicola Baldo wrote:
> What's the difference between SerializeIE and SerializeInformation ()? 

I've made some comment changes in wifi_information_element.h in attempt to make
this clearer.

In attempt to answer your question...

WifiInformationElement::SerializeIE() is invoked to serialise the entire
Information Element, starting with the Element ID and length fields (an octet
each), and finishing with the "Information" (see IEEE Std. 802.11-2007, Figure
7-37).

The implementation of WifiInformationElement::SerializeIE() invokes
SerializeInformation() to do the work for the Information field.
WifiInformationElement::SerializeInformation() is pure virtual; individual IEs
must know how to serialise their information.

A class derived from WifiInformationElement hence need only implement the
latter.

DeserializeIE and DeserializeInformation are similar - a class deriving from
WifiInformationElement need only implement the latter which knows how to
deconstruct the Information field of the IE.

[Nicola Baldo, 2010-06-23 01:08:33]

http://codereview.appspot.com/898045/diff/7001/5037
File src/devices/wifi/wifi-information-element.h (right):

http://codereview.appspot.com/898045/diff/7001/5037#newcode137
src/devices/wifi/wifi-information-element.h:137: Buffer::Iterator DeserializeIE
(Buffer::Iterator i);

Thank for your explanation, now it is much clearer.
What about the following change in naming?

SerializeIE() --> Serialize()
(this method is a member of WifiInformationElement, so it should be obvious that
it is serializing the whole information element)

SerializeInformation() --> SerializeInformationField()

and similarly for deserialize methods

[and.kirill, 2010-06-25 08:01:22]

Thank you for your work. I have some minor comments:

Adding two methods for (de)serialization has resolved a problem with adding a
single IE inside management frame (peer-link-frame has become better).

Also I agree, that "#define" is now better than a enum, so you must not have a
huge enum and change it when a draft standard is approved. But may be it is
worth to add somewhere in comments, that conflicts may occur and developer of
new wifi-module should pay attention to this.

I'll give you +1 and sorry for late reply (is merging possible now?).

[Dean, 2010-06-25 09:54:41]

Kirill,

Thanks for the review.

> Adding two methods for (de)serialization has resolved a problem with adding a
> single IE inside management frame (peer-link-frame has become better).

Just to confirm...this is a comment, right - you are not requesting any change,
are you?

> Also I agree, that "#define" is now better than a enum, so you must not have a
> huge enum and change it when a draft standard is approved. But may be it is
> worth to add somewhere in comments, that conflicts may occur and developer of
> new wifi-module should pay attention to this.

Okay. So will you be happy if I put some text warning developers of this danger
in the comment preceding the #defines in wifi-information-element.h?

If you can give a couple of answers to the affirmative, then I'll update the
patch and merge it.

Thanks,
Dean.

[and.kirill, 2010-06-25 10:05:11]

2010/6/25  <deanarm@gmail.com>:
> Kirill,
>
> Thanks for the review.
>
>> Adding two methods for (de)serialization has resolved a problem with
>
> adding a
>>
>> single IE inside management frame (peer-link-frame has become better).
>
> Just to confirm...this is a comment, right - you are not requesting any
> change, are you?
>


This is a comment only, nothing to do:)


>> Also I agree, that "#define" is now better than a enum, so you must
>
> not have a
>>
>> huge enum and change it when a draft standard is approved. But may be
>
> it is
>>
>> worth to add somewhere in comments, that conflicts may occur and
>
> developer of
>>
>> new wifi-module should pay attention to this.
>
> Okay. So will you be happy if I put some text warning developers of this
> danger in the comment preceding the #defines in
> wifi-information-element.h?
>


Yes, maybe comment is needed where you define WifiElementId as uint8_t.


> If you can give a couple of answers to the affirmative, then I'll update
> the patch and merge it.
>
> Thanks,
> Dean.
>
> http://codereview.appspot.com/898045/show
>



-- 
Regards, Kirill Andreev
Wireless Software R&D Group,
IITP RAS