Bugzilla – Bug 2551
wifi preamble should be part of TXVECTOR
Last modified: 2016-11-27 15:56:21 EST
Created attachment 2670 [details] patch to move preamble to txvector Preamble should be moved to TXVECTOR: - it is defined as such in the standard - it reduces signature of most of our functions - it removes code duplication See provided patch, all tests are passing.
Note that I would like to drop it asap in the mainstream once reviewed ok, because it will result in a lot of merge conflicts if we wait too long.
In general, I support the change, but here are some small comments: 1) change to packet tag size --- a/src/network/model/packet-tag-list.h Sun Nov 06 11:34:21 2016 +0100 +++ b/src/network/model/packet-tag-list.h Sun Nov 06 12:04:10 2016 +0100 @@ -158,7 +158,7 @@ */ enum TagData_e { - MAX_SIZE = 21 /**< Size of serialization buffer #data */ + MAX_SIZE = 26 /**< Size of serialization buffer #data */ }; We already have this bug open: https://www.nsnam.org/bugzilla/show_bug.cgi?id=2221 It would be nice to have Peter's opinion on making this change and possibly closing bug 2221 (if there is no near-term intent to do the refactoring that Mathieu proposed). In any case, I would like to see the packet tag documentation (e.g. doxygen) be updated if we are changing its size. 2) There is a somewhat unrelated cleanup of removal of 'enum' keyword from parameter declarations; this could be separated to a different patch, or the patch commit message should at least say that this commit moves the WifiPreamble and also removes scattered 'enum' keywords from declarations (but IMO separate patches are better). Now that we are supporting C++11, we could consider to migrate enums to enum classes (or start to introduce new enums as enum classes). 3) Where in the standard is this? I looked in Table 18.1 and couldn't find. I couldn't find either in 18.5.4.3 PMD_SAP sublayer-to-sublayer service primitives I think that as part of this patch, Doxygen for WifiTxVector should be updated at least to no longer reference 802.11-2007 but 2012 version instead, and also to identify which pieces of that standard are encapsulated in this class, because it seems to me that it is not aligned with what is called WifiTxVector (Table 18.1) in the standard.
(In reply to Tom Henderson from comment #2) > In general, I support the change, but here are some small comments: > > 1) change to packet tag size > > --- a/src/network/model/packet-tag-list.h Sun Nov 06 11:34:21 2016 > +0100 > +++ b/src/network/model/packet-tag-list.h Sun Nov 06 12:04:10 2016 > +0100 > @@ -158,7 +158,7 @@ > */ > enum TagData_e > { > - MAX_SIZE = 21 /**< Size of serialization buffer #data */ > + MAX_SIZE = 26 /**< Size of serialization buffer #data */ > }; > > We already have this bug open: > https://www.nsnam.org/bugzilla/show_bug.cgi?id=2221 > > It would be nice to have Peter's opinion on making this change and possibly > closing bug 2221 (if there is no near-term intent to do the refactoring that > Mathieu proposed). > > In any case, I would like to see the packet tag documentation (e.g. doxygen) > be updated if we are changing its size. Ok, I suggest to wait for bug 2221, but we cannot postpone too much because it affects lots of files, and thus merge conflicts are to be feared. > > 2) There is a somewhat unrelated cleanup of removal of 'enum' keyword from > parameter declarations; this could be separated to a different patch, or the > patch commit message should at least say that this commit moves the > WifiPreamble and also removes scattered 'enum' keywords from declarations > (but IMO separate patches are better). > > Now that we are supporting C++11, we could consider to migrate enums to enum > classes (or start to introduce new enums as enum classes). Actually I wanted to clean inconsistencies, but I can decouple this in another patch. Just let me know if we change all to enum classes, but then I guess it won't be limited to the wifi module. > > 3) Where in the standard is this? I looked in Table 18.1 and couldn't find. > I couldn't find either in 18.5.4.3 PMD_SAP sublayer-to-sublayer service > primitives > > I think that as part of this patch, Doxygen for WifiTxVector should be > updated at least to no longer reference 802.11-2007 but 2012 version > instead, and also to identify which pieces of that standard are encapsulated > in this class, because it seems to me that it is not aligned with what is > called WifiTxVector (Table 18.1) in the standard. This combines FORMAT and PREAMBLE_TYPE parameters, I suggest to combine those so that we can reuse preamble type already defined iso having two different defines.
(In reply to sebastien.deronne from comment #3) > (In reply to Tom Henderson from comment #2) > > In general, I support the change, but here are some small comments: > > > > 1) change to packet tag size > > > > --- a/src/network/model/packet-tag-list.h Sun Nov 06 11:34:21 2016 > > +0100 > > +++ b/src/network/model/packet-tag-list.h Sun Nov 06 12:04:10 2016 > > +0100 > > @@ -158,7 +158,7 @@ > > */ > > enum TagData_e > > { > > - MAX_SIZE = 21 /**< Size of serialization buffer #data */ > > + MAX_SIZE = 26 /**< Size of serialization buffer #data */ > > }; > > > > We already have this bug open: > > https://www.nsnam.org/bugzilla/show_bug.cgi?id=2221 > > > > It would be nice to have Peter's opinion on making this change and possibly > > closing bug 2221 (if there is no near-term intent to do the refactoring that > > Mathieu proposed). > > > > In any case, I would like to see the packet tag documentation (e.g. doxygen) > > be updated if we are changing its size. > > Ok, I suggest to wait for bug 2221, but we cannot postpone too much because > it affects lots of files, and thus merge conflicts are to be feared. > > > > > 2) There is a somewhat unrelated cleanup of removal of 'enum' keyword from > > parameter declarations; this could be separated to a different patch, or the > > patch commit message should at least say that this commit moves the > > WifiPreamble and also removes scattered 'enum' keywords from declarations > > (but IMO separate patches are better). > > > > Now that we are supporting C++11, we could consider to migrate enums to enum > > classes (or start to introduce new enums as enum classes). > > Actually I wanted to clean inconsistencies, but I can decouple this in > another patch. > Just let me know if we change all to enum classes, but then I guess it won't > be limited to the wifi module. Note that I prefer to keep one patch since it affects same files, and it would avoid overhead to rework all. But I could make a different commit on top if we choose to go for changing to enum classes. > > > > > 3) Where in the standard is this? I looked in Table 18.1 and couldn't find. > > I couldn't find either in 18.5.4.3 PMD_SAP sublayer-to-sublayer service > > primitives > > > > I think that as part of this patch, Doxygen for WifiTxVector should be > > updated at least to no longer reference 802.11-2007 but 2012 version > > instead, and also to identify which pieces of that standard are encapsulated > > in this class, because it seems to me that it is not aligned with what is > > called WifiTxVector (Table 18.1) in the standard. > > This combines FORMAT and PREAMBLE_TYPE parameters, I suggest to combine > those so that we can reuse preamble type already defined iso having two > different defines.
To be pushed once fix for bug 2221 is delivered
pushed in changeset 12429:d12f1640acca