new OFDM physical layer model

[Mathieu Lacage, 2010-05-19 06:48:04]

my only comment is that the change of private to protected looks ugly. Other
than that, this looks really nice.

http://codereview.appspot.com/690041/diff/1/7
File src/devices/wifi/yans-error-rate-model.h (right):

http://codereview.appspot.com/690041/diff/1/7#newcode78
src/devices/wifi/yans-error-rate-model.h:78: protected:
this smells bad: it would be really nice if you could find a way to avoid it.
Maybe moving the shared 'helper' private member methods to error-rate-model.h as
non-member functions and making NistErrorRateModel derive directly from
ErrorRateModel ? Or something similar...

[Nicola Baldo, 2010-05-26 17:02:35]

I think the contribution is very good and it should be feasible to merge it in
the near future. The main thing is that we need to address properly the
interfacing with  ErrorRateModel.

http://codereview.appspot.com/690041/diff/1/5
File src/devices/wifi/nist-error-rate-model.h (right):

http://codereview.appspot.com/690041/diff/1/5#newcode28
src/devices/wifi/nist-error-rate-model.h:28: * \brief Model the error rate for
different modulations.
I think you should put some reference here to the model implemented, for example
citing your technical report "Validation of OFDM error rate model in ns-3" as
well as the references therein.

http://codereview.appspot.com/690041/diff/1/7
File src/devices/wifi/yans-error-rate-model.h (right):

http://codereview.appspot.com/690041/diff/1/7#newcode78
src/devices/wifi/yans-error-rate-model.h:78: protected:
On 2010/05/19 06:48:05, Mathieu Lacage wrote:
> this smells bad: it would be really nice if you could find a way to avoid it.
> Maybe moving the shared 'helper' private member methods to error-rate-model.h
as
> non-member functions and making NistErrorRateModel derive directly from
> ErrorRateModel ? Or something similar...

I agree with Mathieu on avoiding that NistErrorRateModel inherits from
YansErrorRateModel.

A quick solution would be just to have NistErrorRateModel inherit directly from
ErrorRateModel, and duplicate whatever code is needed from YansErrorRateModel.
This avoids that weird inheritance, but we would duplicate all the DSSS error
model code...

Since the differences in the two models are only on OFDM, what about splitting
the ErrorRateModel interface in two, one part for DSSS and the other for DSSS?
Each interface would contain methods to get chunk success rates for each of the
modulation schemes of the PHY specification considered. 

Or, any other proposal?

[Gary Pei, 2010-05-26 23:23:56]

On 2010/05/26 17:02:35, Nicola Baldo wrote:
> I think the contribution is very good and it should be feasible to merge it in
> the near future. The main thing is that we need to address properly the
> interfacing with  ErrorRateModel.
> 
> http://codereview.appspot.com/690041/diff/1/5
> File src/devices/wifi/nist-error-rate-model.h (right):
> 
> http://codereview.appspot.com/690041/diff/1/5#newcode28
> src/devices/wifi/nist-error-rate-model.h:28: * \brief Model the error rate for
> different modulations.
> I think you should put some reference here to the model implemented, for
example
> citing your technical report "Validation of OFDM error rate model in ns-3" as
> well as the references therein.
> 
> http://codereview.appspot.com/690041/diff/1/7
> File src/devices/wifi/yans-error-rate-model.h (right):
> 
> http://codereview.appspot.com/690041/diff/1/7#newcode78
> src/devices/wifi/yans-error-rate-model.h:78: protected:
> On 2010/05/19 06:48:05, Mathieu Lacage wrote:
> > this smells bad: it would be really nice if you could find a way to avoid
it.
> > Maybe moving the shared 'helper' private member methods to
error-rate-model.h
> as
> > non-member functions and making NistErrorRateModel derive directly from
> > ErrorRateModel ? Or something similar...
> 
> I agree with Mathieu on avoiding that NistErrorRateModel inherits from
> YansErrorRateModel.
> 
> A quick solution would be just to have NistErrorRateModel inherit directly
from
> ErrorRateModel, and duplicate whatever code is needed from YansErrorRateModel.
> This avoids that weird inheritance, but we would duplicate all the DSSS error
> model code...
> 
> Since the differences in the two models are only on OFDM, what about splitting
> the ErrorRateModel interface in two, one part for DSSS and the other for DSSS?
> Each interface would contain methods to get chunk success rates for each of
the
> modulation schemes of the PHY specification considered. 
> 
> Or, any other proposal?

I am fine with duplicating DSSS code. However, it seems better to split
ErrorRateModel. If so, I would like you and/or Mathieu to do the split first and
I will follow your path.

[Tom Henderson, 2010-05-31 04:32:01]

> I am fine with duplicating DSSS code. However, it seems better to split
> ErrorRateModel. If so, I would like you and/or Mathieu to do the split first
and
> I will follow your path.

My sense is that the DSSS pieces could be factored out; of all of the methods
that Gary turned from private to protected, only these four are needed:

   double Get80211bDsssDbpskSuccessRate (double sinr, uint32_t nbits) const;
   double Get80211bDsssDqpskSuccessRate (double sinr,uint32_t nbits) const;
   double Get80211bDsssDqpskCck5_5SuccessRate (double sinr,uint32_t nbits)
const;
   double Get80211bDsssDqpskCck11SuccessRate (double sinr,uint32_t nbits) const;

which could be added to some other private class such as DsssErrorRateModel used
by both YansErrorRateModel and NistErrorRateModel.  Then make NistErrorRateModel
derive from ErrorRateModel and allow either Yans or Nist to be selectable by
user code.

It also seems to me that this will need to be aligned a bit after Dean's
patchsets.

[nbaldo_cttc.es, 2010-06-01 17:21:21]

Hi Tom,

On 05/31/2010 06:32 AM, tomh.org@gmail.com wrote:
>> I am fine with duplicating DSSS code. However, it seems better to
> split
>> ErrorRateModel. If so, I would like you and/or Mathieu to do the split
> first and
>> I will follow your path.
>
> My sense is that the DSSS pieces could be factored out; of all of the
> methods that Gary turned from private to protected, only these four are
> needed:
>
> double Get80211bDsssDbpskSuccessRate (double sinr, uint32_t nbits)
> const;
> double Get80211bDsssDqpskSuccessRate (double sinr,uint32_t nbits)
> const;
> double Get80211bDsssDqpskCck5_5SuccessRate (double sinr,uint32_t
> nbits) const;
> double Get80211bDsssDqpskCck11SuccessRate (double sinr,uint32_t
> nbits) const;
>
> which could be added to some other private class such as
> DsssErrorRateModel used by both YansErrorRateModel and
> NistErrorRateModel. Then make NistErrorRateModel derive from
> ErrorRateModel and allow either Yans or Nist to be selectable by user
> code.

This would leave the ErrorRateModel interface as-is, right?
If this is the case, I am ok with this proposal.


>
> It also seems to me that this will need to be aligned a bit after Dean's
> patchsets.

Your proposed code should work as-is with Dean's patches.
The only thing that needs to be aligned is the naming. It should be 
enough to use the following naming:

double GetDsssDbpskSuccessRate (double sinr, uint32_t nbits)
const;
double GetDsssDqpskSuccessRate (double sinr,uint32_t nbits)
const;
double GetDsssDqpskCck5_5SuccessRate (double sinr,uint32_t
nbits) const;
double GetDsssDqpskCck11SuccessRate (double sinr,uint32_t
nbits) const;

but I see no problem even if we change the naming in the future.


Nicola

[Gary Pei, 2010-06-08 22:56:32]

On 2010/06/01 17:21:21, nbaldo_cttc.es wrote:
> Hi Tom,
> 
> On 05/31/2010 06:32 AM, mailto:tomh.org@gmail.com wrote:
> >> I am fine with duplicating DSSS code. However, it seems better to
> > split
> >> ErrorRateModel. If so, I would like you and/or Mathieu to do the split
> > first and
> >> I will follow your path.
> >
> > My sense is that the DSSS pieces could be factored out; of all of the
> > methods that Gary turned from private to protected, only these four are
> > needed:
> >
> > double Get80211bDsssDbpskSuccessRate (double sinr, uint32_t nbits)
> > const;
> > double Get80211bDsssDqpskSuccessRate (double sinr,uint32_t nbits)
> > const;
> > double Get80211bDsssDqpskCck5_5SuccessRate (double sinr,uint32_t
> > nbits) const;
> > double Get80211bDsssDqpskCck11SuccessRate (double sinr,uint32_t
> > nbits) const;
> >
> > which could be added to some other private class such as
> > DsssErrorRateModel used by both YansErrorRateModel and
> > NistErrorRateModel. Then make NistErrorRateModel derive from
> > ErrorRateModel and allow either Yans or Nist to be selectable by user
> > code.
> 
> This would leave the ErrorRateModel interface as-is, right?
> If this is the case, I am ok with this proposal.
> 
> 
> >
> > It also seems to me that this will need to be aligned a bit after Dean's
> > patchsets.
> 
> Your proposed code should work as-is with Dean's patches.
> The only thing that needs to be aligned is the naming. It should be 
> enough to use the following naming:
> 
> double GetDsssDbpskSuccessRate (double sinr, uint32_t nbits)
> const;
> double GetDsssDqpskSuccessRate (double sinr,uint32_t nbits)
> const;
> double GetDsssDqpskCck5_5SuccessRate (double sinr,uint32_t
> nbits) const;
> double GetDsssDqpskCck11SuccessRate (double sinr,uint32_t
> nbits) const;
> 
> but I see no problem even if we change the naming in the future.
> 
> 
> Nicola
> 
> 
> 
> 
> 

Nicola, Mathieu and Tom

Please review and see if these change sets is what you have in your mind.

Thanks,

Gary

[Gary Pei, 2010-06-09 22:43:36]

http://codereview.appspot.com/690041/diff/1/5
File src/devices/wifi/nist-error-rate-model.h (right):

http://codereview.appspot.com/690041/diff/1/5#newcode28
src/devices/wifi/nist-error-rate-model.h:28: * \brief Model the error rate for
different modulations.
On 2010/05/26 17:02:35, Nicola Baldo wrote:
> I think you should put some reference here to the model implemented, for
example
> citing your technical report "Validation of OFDM error rate model in ns-3" as
> well as the references therein.

Done. I also added an appendix to provide plots by data mode.

[Nicola Baldo, 2010-06-11 17:02:10]

I think the patch is ok now. There are only a few trivial issues, once these are
fixed I think it is ok to merge.

http://codereview.appspot.com/690041/diff/12001/13002
File examples/wireless/wscript (left):

http://codereview.appspot.com/690041/diff/12001/13002#oldcode43
examples/wireless/wscript:43: 
whitespace change?

http://codereview.appspot.com/690041/diff/12001/13005
File src/devices/wifi/error-rate-model.h (right):

http://codereview.appspot.com/690041/diff/12001/13005#newcode26
src/devices/wifi/error-rate-model.h:26: #include "dsss-error-rate-model.h"
this should not be needed

http://codereview.appspot.com/690041/diff/12001/13007
File src/devices/wifi/nist-error-rate-model.h (right):

http://codereview.appspot.com/690041/diff/12001/13007#newcode23
src/devices/wifi/nist-error-rate-model.h:23: #include "yans-error-rate-model.h"
should include only error-rate-model.h

[Gary Pei, 2010-06-14 17:50:43]

http://codereview.appspot.com/690041/diff/12001/13002
File examples/wireless/wscript (left):

http://codereview.appspot.com/690041/diff/12001/13002#oldcode43
examples/wireless/wscript:43: 
On 2010/06/11 17:02:10, Nicola Baldo wrote:
> whitespace change?

Done.

http://codereview.appspot.com/690041/diff/12001/13005
File src/devices/wifi/error-rate-model.h (right):

http://codereview.appspot.com/690041/diff/12001/13005#newcode26
src/devices/wifi/error-rate-model.h:26: #include "dsss-error-rate-model.h"
On 2010/06/11 17:02:10, Nicola Baldo wrote:
> this should not be needed

I added here since both yans and nist needed Dsss functions. I can move this
statement to yans-error-rate-model.h and nist-error-rate-mode.h if you like that
way. Please let me know.

http://codereview.appspot.com/690041/diff/12001/13007
File src/devices/wifi/nist-error-rate-model.h (right):

http://codereview.appspot.com/690041/diff/12001/13007#newcode23
src/devices/wifi/nist-error-rate-model.h:23: #include "yans-error-rate-model.h"
On 2010/06/11 17:02:10, Nicola Baldo wrote:
> should include only error-rate-model.h

Done.

[Tom Henderson, 2010-06-14 19:32:14]

http://codereview.appspot.com/690041/diff/12001/13005
File src/devices/wifi/error-rate-model.h (right):

http://codereview.appspot.com/690041/diff/12001/13005#newcode26
src/devices/wifi/error-rate-model.h:26: #include "dsss-error-rate-model.h"
On 2010/06/14 17:50:43, Gary Pei wrote:
> On 2010/06/11 17:02:10, Nicola Baldo wrote:
> > this should not be needed
> 
> I added here since both yans and nist needed Dsss functions. I can move this
> statement to yans-error-rate-model.h and nist-error-rate-mode.h if you like
that
> way. Please let me know.

I agree with Nicola; put the dependency on yans and nist but not on this base
class.

[Gary Pei, 2010-06-14 22:20:30]

http://codereview.appspot.com/690041/diff/12001/13005
File src/devices/wifi/error-rate-model.h (right):

http://codereview.appspot.com/690041/diff/12001/13005#newcode26
src/devices/wifi/error-rate-model.h:26: #include "dsss-error-rate-model.h"
On 2010/06/14 19:32:15, Tom Henderson wrote:
> On 2010/06/14 17:50:43, Gary Pei wrote:
> > On 2010/06/11 17:02:10, Nicola Baldo wrote:
> > > this should not be needed
> > 
> > I added here since both yans and nist needed Dsss functions. I can move this
> > statement to yans-error-rate-model.h and nist-error-rate-mode.h if you like
> that
> > way. Please let me know.
> 
> I agree with Nicola; put the dependency on yans and nist but not on this base
> class.
> 

Done.