Bug 2460

Tom, as proposed, it would mean signals below SensitivityThreshold will not be tracked in the InterferenceHelper, is it expected? If we have several signals at -101 dBm, I guess they should be tracked somehow.
SignalDetectThreshold is playing actually that role already, but keeps the InterferenceHelper informed.
Could you please comment?

(In reply to sebastien.deronne from comment #1)
> Tom, as proposed, it would mean signals below SensitivityThreshold will not
> be tracked in the InterferenceHelper, is it expected?   If we have several
> signals at -101 dBm, I guess they should be tracked somehow.
> SignalDetectThreshold is playing actually that role already, but keeps the
> InterferenceHelper informed.
> Could you please comment?

I would expect frames below sensitivity threshold to be ignored from InterferenceHelper perspective.  Whether default to -101 dBm or weaker, I do not have a strong opinion.

So that would be the difference with SignalDetectThreshold, which would still keep signals under that threshold in the interferenceHelper?

(In reply to sebastien.deronne from comment #3)
> So that would be the difference with SignalDetectThreshold, which would
> still keep signals under that threshold in the interferenceHelper?

Yes, I would keep those signals whose power is between SignalDetectThreshold and SensitivityThreshold since a future decoding error may occur due to the presence of such signals even if the STA is not deferring to such a signal.  The idea of SensitivityThreshold is to throw away events that do not have any impact on the STA reception or CCA process because of their low power.

(In reply to Tom Henderson from comment #4)
> (In reply to sebastien.deronne from comment #3)
> > So that would be the difference with SignalDetectThreshold, which would
> > still keep signals under that threshold in the interferenceHelper?
> 
> Yes, I would keep those signals whose power is between SignalDetectThreshold
> and SensitivityThreshold since a future decoding error may occur due to the
> presence of such signals even if the STA is not deferring to such a signal. 
> The idea of SensitivityThreshold is to throw away events that do not have
> any impact on the STA reception or CCA process because of their low power.

Ok, I'll go for a value of -114 dBm (lowest value that keeps all tests passing)

Tom, I do not see any -62 for the CCA threshold, I found this in the std:

For OFDM PHY operation with CCA-ED, the thresholds shall be less than or equal to –72 dBm for 20 MHz channel widths, –75 dBm for 10 MHz channel widths, and –78 dBm for 5 MHz channel widths (minimum sensitivity for BPSK, R=1/2 + 10 dB in Table 18-14).

Also, I guess this threshold will be different based on the operating channel (2.4 or 5) and on the channel width. Should we make it going dynamic based on the chosen parameters, in case the user did not manually change the default?

Note also that according to the std, threshold should depend on the txpower:

The ED threshold shall be ≤ –80 dBm for TXpower > 100mW, –76 dBm for 50mW < TXpower ≤ 100 mW, and –70 dBm for TX power ≤ 50 mW.

(In reply to sebastien.deronne from comment #6)
> Tom, I do not see any -62 for the CCA threshold, I found this in the std:
> 
> For OFDM PHY operation with CCA-ED, the thresholds shall be less than or
> equal to –72 dBm for 20 MHz channel widths, –75 dBm for 10 MHz channel
> widths, and –78 dBm for 5 MHz channel widths (minimum sensitivity for BPSK,
> R=1/2 + 10 dB in Table 18-14).
> 
> Also, I guess this threshold will be different based on the operating
> channel (2.4 or 5) and on the channel width. Should we make it going dynamic
> based on the chosen parameters, in case the user did not manually change the
> default?

Tom, didn't you mean -72 iso -62 ?
Where does the -91 come from? Shouldn't it be -92 to have a 20 dB difference as used in practice?

(In reply to sebastien.deronne from comment #6)
> Tom, I do not see any -62 for the CCA threshold, I found this in the std:
> 
> For OFDM PHY operation with CCA-ED, the thresholds shall be less than or
> equal to –72 dBm for 20 MHz channel widths, –75 dBm for 10 MHz channel
> widths, and –78 dBm for 5 MHz channel widths (minimum sensitivity for BPSK,
> R=1/2 + 10 dB in Table 18-14).

See 18.3.10.6 CCA requirements

I'm not sure about the applicability of the Annex D text above that you cite.  In LAA work, we used -62 dBm as per 3GPP TR36.889 Appendix A.2.1 Additional Wi-Fi system evaluation assumptions.


> 
> Also, I guess this threshold will be different based on the operating
> channel (2.4 or 5) and on the channel width. Should we make it going dynamic
> based on the chosen parameters, in case the user did not manually change the
> default?

I would be fine to accept a patch that automatically aligned it if someone could work out the details.  This gets into issues we have discussed before about how to generally detect "in case the user did not manually change the default".  For instance, if the default were set to an invalid value, then the code could use a lookup in case the default value was found.

(In reply to Tom Henderson from comment #9)
> (In reply to sebastien.deronne from comment #6)
> > Tom, I do not see any -62 for the CCA threshold, I found this in the std:
> > 
> > For OFDM PHY operation with CCA-ED, the thresholds shall be less than or
> > equal to –72 dBm for 20 MHz channel widths, –75 dBm for 10 MHz channel
> > widths, and –78 dBm for 5 MHz channel widths (minimum sensitivity for BPSK,
> > R=1/2 + 10 dB in Table 18-14).
> 
> See 18.3.10.6 CCA requirements

It seems indeed the correct place to look at, so I'll keep this value as default. I will work on a patch that changes this based on the selected channel width.

> 
> I'm not sure about the applicability of the Annex D text above that you
> cite.  In LAA work, we used -62 dBm as per 3GPP TR36.889 Appendix A.2.1
> Additional Wi-Fi system evaluation assumptions.
> 
> 
> > 
> > Also, I guess this threshold will be different based on the operating
> > channel (2.4 or 5) and on the channel width. Should we make it going dynamic
> > based on the chosen parameters, in case the user did not manually change the
> > default?
> 
> I would be fine to accept a patch that automatically aligned it if someone
> could work out the details.  This gets into issues we have discussed before
> about how to generally detect "in case the user did not manually change the
> default".  For instance, if the default were set to an invalid value, then
> the code could use a lookup in case the default value was found.

(In reply to sebastien.deronne from comment #8)
> (In reply to sebastien.deronne from comment #6)
> > Tom, I do not see any -62 for the CCA threshold, I found this in the std:
> > 
> > For OFDM PHY operation with CCA-ED, the thresholds shall be less than or
> > equal to –72 dBm for 20 MHz channel widths, –75 dBm for 10 MHz channel
> > widths, and –78 dBm for 5 MHz channel widths (minimum sensitivity for BPSK,
> > R=1/2 + 10 dB in Table 18-14).
> > 
> > Also, I guess this threshold will be different based on the operating
> > channel (2.4 or 5) and on the channel width. Should we make it going dynamic
> > based on the chosen parameters, in case the user did not manually change the
> > default?
> 
> Tom, didn't you mean -72 iso -62 ?
> Where does the -91 come from? Shouldn't it be -92 to have a 20 dB difference
> as used in practice?

Tom, I still do not get why you proposed -91dbm?
It seems -92 dBm is a typical value.

Created attachment 2784 [details]
New wifi phy thresholds

I had a discussion offline with Tom.
Based on that, I made a patch with the new wifi phy thresholds and I adapted tests so that there is no need to rescan pcap files (those tests are anyway expected to be changed in the future so that there is no pcap files dependency).

(In reply to sebastien.deronne from comment #12)
> Created attachment 2784 [details]
> New wifi phy thresholds
> 
> I had a discussion offline with Tom.
> Based on that, I made a patch with the new wifi phy thresholds and I adapted
> tests so that there is no need to rescan pcap files (those tests are anyway
> expected to be changed in the future so that there is no pcap files
> dependency).

Still doc to be slightly updated once approved

Sebastien, thanks for this patch; some comments:

1)  I suggest to test >= here instead of >

rxPowerW > GetSignalDetectionThresholdW ()

Reason:  Users could reasonably expect that a value equal to a threshold would be acceptable, rather than needing to be strictly greater.  Granted, in practice, this rxPowerW is going to be some double value due to path loss, but I'm thinking about tests and debugging, and the other threshold works this way.

2) Doxygen

I would like to adjust the doxygen as follows:

-    .AddAttribute ("EnergyDetectionThreshold",
+    .AddAttribute ("SignalDetectionThreshold",
                    "The energy of a received signal should be higher than "
                    "this threshold (dbm) to allow the PHY layer to detect the signal.",

Here, I would instead say:
                    "The energy of a received signal must be >= to this "
                    "threshold (dBm) for the PHY to attempt to decode this signal.",

3) Please leave EnergyDetectionThreshold in as a DEPRECATED attribute

4) (optional)  consider to add a check in WifiPhy::DoInitialize to abort with msg if the following check fails:  ccaMode1 >= signalDetect >= sensitivity

5) 

+  if (GetCcaMode1Threshold () == 0)
+    {
+      //todo: what if channel width has changed but we already set CCAMode1 threshold value?

what circumstances will lead to CcaMode1Threshold == 0 if a non-zero default is already set in the attribute?

Regarding the question of how to tie this to channel width and other factors, one possibility is to not couple the two together and instead add to the doxygen string:  Note:  this parameter does not automatically change with other changes (e.g. channel bonding) made to the Phy.

Another possibility is to auto-scale this through some method that tries to coordinate changes, but my experience is that this is painful to try to anticipate and deal with all possible 'order of setting attributes'.

6) please patch CHANGES.html for changed behavior section

7) I am fine with how you handled the tests

(In reply to Tom Henderson from comment #14)
> Sebastien, thanks for this patch; some comments:
> 
> 1)  I suggest to test >= here instead of >
> 
> rxPowerW > GetSignalDetectionThresholdW ()
> 
> Reason:  Users could reasonably expect that a value equal to a threshold
> would be acceptable, rather than needing to be strictly greater.  Granted,
> in practice, this rxPowerW is going to be some double value due to path
> loss, but I'm thinking about tests and debugging, and the other threshold
> works this way.

OK.

> 
> 2) Doxygen
> 
> I would like to adjust the doxygen as follows:
> 
> -    .AddAttribute ("EnergyDetectionThreshold",
> +    .AddAttribute ("SignalDetectionThreshold",
>                     "The energy of a received signal should be higher than "
>                     "this threshold (dbm) to allow the PHY layer to detect
> the signal.",
> 
> Here, I would instead say:
>                     "The energy of a received signal must be >= to this "
>                     "threshold (dBm) for the PHY to attempt to decode this
> signal.",
> 

OK.

> 3) Please leave EnergyDetectionThreshold in as a DEPRECATED attribute

Yes I always forget that, thanks.

> 
> 4) (optional)  consider to add a check in WifiPhy::DoInitialize to abort
> with msg if the following check fails:  ccaMode1 >= signalDetect >=
> sensitivity
> 

It sounds better.

> 5) 
> 
> +  if (GetCcaMode1Threshold () == 0)
> +    {
> +      //todo: what if channel width has changed but we already set CCAMode1
> threshold value?
> 
> what circumstances will lead to CcaMode1Threshold == 0 if a non-zero default
> is already set in the attribute?
> 
> Regarding the question of how to tie this to channel width and other
> factors, one possibility is to not couple the two together and instead add
> to the doxygen string:  Note:  this parameter does not automatically change
> with other changes (e.g. channel bonding) made to the Phy.
> 
> Another possibility is to auto-scale this through some method that tries to
> coordinate changes, but my experience is that this is painful to try to
> anticipate and deal with all possible 'order of setting attributes'.

Oups, I forgot to clean this in the patch, I wanted first to make this threshold dependent on the channel width. Is this something we want?

> 
> 6) please patch CHANGES.html for changed behavior section

OK.

> 
> 7) I am fine with how you handled the tests

> > 
> > Another possibility is to auto-scale this through some method that tries to
> > coordinate changes, but my experience is that this is painful to try to
> > anticipate and deal with all possible 'order of setting attributes'.
> 
> Oups, I forgot to clean this in the patch, I wanted first to make this
> threshold dependent on the channel width. Is this something we want?
> 

If we do this, we need to:

1) factor in channel width into the GetCcaMode1Threshold () and document in the attribute that this is the 20 MHz value being set and others will scale appropriately.

2) consider whether the other two should scale as well

3) write a test for the behavior


I think that it probably makes sense to adjust if it doesn't get messy.

(In reply to Tom Henderson from comment #16)
> > > 
> > > Another possibility is to auto-scale this through some method that tries to
> > > coordinate changes, but my experience is that this is painful to try to
> > > anticipate and deal with all possible 'order of setting attributes'.
> > 
> > Oups, I forgot to clean this in the patch, I wanted first to make this
> > threshold dependent on the channel width. Is this something we want?
> > 
> 
> If we do this, we need to:
> 
> 1) factor in channel width into the GetCcaMode1Threshold () and document in
> the attribute that this is the 20 MHz value being set and others will scale
> appropriately.
> 
> 2) consider whether the other two should scale as well
> 
> 3) write a test for the behavior
> 
> 
> I think that it probably makes sense to adjust if it doesn't get messy.

I think the less messy is to document that this value was set for 20 MHz, and might need to be adjusted if channel width is changed.

> > I think that it probably makes sense to adjust if it doesn't get messy.
> 
> I think the less messy is to document that this value was set for 20 MHz,
> and might need to be adjusted if channel width is changed.

I would be fine with this outcome.

I have 2 questions:

1) If I add the assert, I have a problem because the condition was not respected before those changes, so if I set the previous values for the failing tests, the assert is triggered and tests are now crashing. Should I rescan pcap files for the failing tests? Or do you have another preferred solution?


2) For the deprecated attribute, how should I handle this since the new and the old attribute are both linked to the same variable? Can I link those 2 to the same variable with no issue? I've not tested whether this works fine, but maybe you have already faced a similar case.

(In reply to sebastien.deronne from comment #19)
> I have 2 questions:
> 
> 1) If I add the assert, I have a problem because the condition was not
> respected before those changes, so if I set the previous values for the
> failing tests, the assert is triggered and tests are now crashing. Should I
> rescan pcap files for the failing tests? Or do you have another preferred
> solution?

Actually, I realize a different problem now.  The (new) energy detection threshold should only apply to non-WiFi signals, but it is implemented in the WifiPhy (instead of SpectrumWifiPhy).

It seems to me that you could write the assert and preserve legacy behavior by making the pcap-generating tests use Yans, if you could make YansWifiPhy ignore this new -62 dBm attribute.

> 
> 
> 2) For the deprecated attribute, how should I handle this since the new and
> the old attribute are both linked to the same variable? Can I link those 2
> to the same variable with no issue? I've not tested whether this works fine,
> but maybe you have already faced a similar case.

Remove the variable linkage from the deprecated attribute; keep the methods that the attribute indirects through, but make these methods no-ops.

(In reply to Tom Henderson from comment #20)
> (In reply to sebastien.deronne from comment #19)
> > I have 2 questions:
> > 
> > 1) If I add the assert, I have a problem because the condition was not
> > respected before those changes, so if I set the previous values for the
> > failing tests, the assert is triggered and tests are now crashing. Should I
> > rescan pcap files for the failing tests? Or do you have another preferred
> > solution?
> 
> Actually, I realize a different problem now.  The (new) energy detection
> threshold should only apply to non-WiFi signals, but it is implemented in
> the WifiPhy (instead of SpectrumWifiPhy).
> 
> It seems to me that you could write the assert and preserve legacy behavior
> by making the pcap-generating tests use Yans, if you could make YansWifiPhy
> ignore this new -62 dBm attribute.

Tom, it's not clear for me, this CcaMode1Threshold was already existing in our code with a much lower value (-99 by default). Should this be deleted from yans? If yes, it means some code that were common to yans and spectrum located in wifi-phy should not be split. Or do you mean the default value should be changed in spectrum only? But if it concerns only non-wifi signals, I guess we should have two thresholds: one for wifi signals and one for non-wifi signals, and do a check in spectrum whether this is a wifi signal and whether this is a non-wifi signal.

> 
> > 
> > 
> > 2) For the deprecated attribute, how should I handle this since the new and
> > the old attribute are both linked to the same variable? Can I link those 2
> > to the same variable with no issue? I've not tested whether this works fine,
> > but maybe you have already faced a similar case.
> 
> Remove the variable linkage from the deprecated attribute; keep the methods
> that the attribute indirects through, but make these methods no-ops.

> 
> Tom, it's not clear for me, this CcaMode1Threshold was already existing in
> our code with a much lower value (-99 by default). Should this be deleted
> from yans? If yes, it means some code that were common to yans and spectrum
> located in wifi-phy should not be split. Or do you mean the default value
> should be changed in spectrum only? But if it concerns only non-wifi
> signals, 

One solution is to delete it from Yans and specialize it to Spectrum.  Another solution is to have Yans ignore it.

> I guess we should have two thresholds: one for wifi signals and one
> for non-wifi signals, and do a check in spectrum whether this is a wifi
> signal and whether this is a non-wifi signal.
> 

Agree, Spectrum should use both CCA-CS and CCA-ED thresholds, while Yans should only use CCA-CS since all signals are Wi-Fi.

Tom,

Then I have impression the logic in wifi-phy is NOK:

    case WifiPhy::CCA_BUSY:
    case WifiPhy::IDLE:
      if (rxPowerW >= GetSignalDetectionThresholdW ())
        {
          ..
        }

I assume we should have this instead in SpectrumWifiPhy then:

  if (wifiRxParams == 0 && rxPowerW > GetSignalDetectionThresholdW ())
    {
      NS_LOG_INFO ("Received non Wi-Fi signal");
      m_interference.AddForeignSignal (rxDuration, rxPowerW);
      SwitchMaybeToCcaBusy ();
      return;
    }

And no more if condition in wifi-phy:

    case WifiPhy::CCA_BUSY:
    case WifiPhy::IDLE:
        {
          ...
        }

Correct?

(In reply to sebastien.deronne from comment #23)
> Tom,
> 
> Then I have impression the logic in wifi-phy is NOK:
> 
>     case WifiPhy::CCA_BUSY:
>     case WifiPhy::IDLE:
>       if (rxPowerW >= GetSignalDetectionThresholdW ())
>         {
>           ..
>         }
> 

here in wifi-phy, I would say that this is OK since this is a preamble detection step.

(Maybe PreambleDetectionThreshold might be clearer name than SignalDetectionThreshold?)

> I assume we should have this instead in SpectrumWifiPhy then:
> 
>   if (wifiRxParams == 0 && rxPowerW > GetSignalDetectionThresholdW ())
>     {
>       NS_LOG_INFO ("Received non Wi-Fi signal");
>       m_interference.AddForeignSignal (rxDuration, rxPowerW);
>       SwitchMaybeToCcaBusy ();
>       return;
>     }

No, I do not think StartRx() needs to change in this regard with a threshold comparison.

> 
> And no more if condition in wifi-phy:
> 
>     case WifiPhy::CCA_BUSY:
>     case WifiPhy::IDLE:
>         {
>           ...
>         }
> 
> Correct?

No, see above.

I think the main idea is that SpectrumWifiPhy::StartRx () will detect whether it is foreign or Wi-Fi signal.  If foreign, it should check to determine whether CCA is raised, and return.  However, if it is a Wi-Fi signal, it can proceed to StartReceivePreambleAndHeader and share the same logic as Yans.

What needs to probably change, is this:

void
WifiPhy::SwitchMaybeToCcaBusy (void)
{
  NS_LOG_FUNCTION (this);
  //We are here because we have received the first bit of a packet and we are
  //not going to be able to synchronize on it
  //In this model, CCA becomes busy when the aggregation of all signals as
  //tracked by the InterferenceHelper class is higher than the CcaBusyThreshold

In YansWifiPhy, we would not switch to a busy state unless the receiver synched on a preamble, while SpectrumWifiPhy would do so if the non-wifi signal exceeded the CcaMode1Threshold.  That is, if preamble detection threshold was set to -82 dBm and a Wi-Fi signal of -85 dBm came in, it would be ignored (by both models).  Only in SpectrumWifiPhy would a (non Wi-Fi) signal also be able to trigger a busy state.

Tom, thanks, this part is clear now.
I think this is already how SwitchMaybeToCcaBusy is handled.

But I still do not get why you say CcaMode1Threshold should be removed in Yans.
Could you detail what you meant?

(In reply to sebastien.deronne from comment #25)
> Tom, thanks, this part is clear now.
> I think this is already how SwitchMaybeToCcaBusy is handled.
> 
> But I still do not get why you say CcaMode1Threshold should be removed in
> Yans.
> Could you detail what you meant?

CcaMode1Threshold is for energy detection.  Thinking about it again now, I suppose we can keep it for Yans, but it wouldn't have much effect since all signals are wifi when Yans is used, and the SignalDetectionThreshold would ensure that RX state would be entered.

(In reply to Tom Henderson from comment #26)
> (In reply to sebastien.deronne from comment #25)
> > Tom, thanks, this part is clear now.
> > I think this is already how SwitchMaybeToCcaBusy is handled.
> > 
> > But I still do not get why you say CcaMode1Threshold should be removed in
> > Yans.
> > Could you detail what you meant?
> 
> CcaMode1Threshold is for energy detection.  Thinking about it again now, I
> suppose we can keep it for Yans, but it wouldn't have much effect since all
> signals are wifi when Yans is used, and the SignalDetectionThreshold would
> ensure that RX state would be entered.

The problem is then still that I cannot respect ccaMode1 >= signalDetect >=
> sensitivity and keep tests working. Or we need this assert for Spectrum only.

Tom, any updates on this?

(In reply to sebastien.deronne from comment #28)
> Tom, any updates on this?

It has been a little while, and I sense that we should still talk about API/requirements here.

Attached is a new API-only patch that doesn't get into implementation but suggests some new attributes and deprecates two that we have been using.

The relevant sections in the spec are 18.3.10.6 and D.2.5.

The existing CcaMode1Threshold attribute is named after a DSSS-specific threshold, so I am proposing to consider using the OFDM naming convention (CCA-ED and CCA-CS) going forward and to deprecate the CcaMode1Threshold and EnergyDetectionThreshold attributes.

CCA-ED and CCA-CS pertain to CCA operation.  For decoding, successful decoding occurs based on the error model, but can artificially be constrained by a new proposed attribute, RxSopThreshold (Receiver Start of Packet Threshold) used to avoid associations with weak AP signals in dense deployments.

Finally, to improve scalability, I already mentioned the "SensitivityThreshold" to throw away weak signals that do not impact performance.

As for implementation behavior, the standard says that implicitly there is a CCA energy detection threshold that is 20 dB above the CS threshold.  So, CCA-ED value used in practice should be the minimum of (CCA-CS + 20 dB, CCA-ED).  For some parts of the band, CCA-ED is smaller (such as described in D.2.5).

The thresholds also should change with channel width.  I am not proposing to make these values automatically change with channel width changes at this time.  So, for instance, if the user sets CCA-CS to -82 dBm but then changes channel width to 10 MHz, the threshold would not drop automatically to -85 dBm but would have to be set by the user.

Created attachment 2841 [details]
proposed new attributes

Tom, what is the difference between SensitivityThreshold and RxSopThreshold?

I agree with your proposal, but I think we will still face regression issues.

(In reply to sebastien.deronne from comment #31)
> Tom, what is the difference between SensitivityThreshold and RxSopThreshold?
> 

Notionally, SensitivityThreshold is for run-time performance optimization, RxSopThreshold is for making a behavioral change (suppress reception of frames for dense enviroments).  SensitivityThreshold will affect reception, CCA-ED, and CCA-CS behavior.  I believe that RxSopThreshold would affect reception but not CCA-ED behavior (CCA-CS?-- need to confirm...).

I think you may be asking about whether a single attribute, SensitivityThreshold, can be used to implement both.  If set rather low (e.g. -110 dBm) it serves more as a run-time performance optimization, but if set high (e.g. -82 dBm) it can serve as a RxSopThreshold.  I think this may work if virtual carrier sense is to be suppressed (i.e. CCA-CS is suppressed, not just higher level MAC functions) by RxSopThreshold.

In 3GPP LAA coexistence studies, the CCA-CS value is specified as -82 dBm, but the AWGN error models typically permit CCA-CS down to about -91 dBm, so if one wants to make Wi-Fi not defer to weak signals (below -82 dBm) then a threshold is needed.  

Let me see whether I can get more information about whether a separate RxSopThreshold is needed; maybe this can be deferred till later.

Tom,
Can you maybe rework the patch I submitted with your proposal?
Thanks.

Created attachment 3198 [details]
new proposal

Here is my updated proposal.

1) Keep CcaMode1Threshold but set the threshold to -62 dBm (it is assumed for 20 MHz)
2) Deprecate (remove) 'EnergyDetectionThreshold' and replace it with 'RxSensitivity' attribute.  Functionally they are the same, but EnergyDetection name can be confusing with the traditional use of this term.  Set RxSensitivity default to -101 dBm (20 MHz thermal noise floor).

I plan to follow this with a proposal to add a PreambleDetection step that will function as the 'start of packet' detection threshold (that is required to be at -82 dBm but in practice can be lower).  This PreambleDetection block will allow users to replace with other algorithms or error models if needed.

So if acceptable, I will provide regression tests around this patch so it can be merged, and then follow up with a PreambleDetection patch that will cover that aspect.

I may just remove altogether EnergyDetectionThreshold rather than deprecate; I don't think the deprecation here is helpful (it is a no-op).  I don't want to redirect it to override the new attribute, though.

2 tests are failing with the proposed patch:

    devices-mesh-dot11s-regression
    routing-aodv-regression

I will have a further look.

Also, few comments:

* I also would remove the attribute instead of deprecating it.
* Was it not the intention to rename CcaMode1Threshold to CcaEdThreshold?
* Some tests/examples are not cleaned up in your patch.

(In reply to sebastien.deronne from comment #35)
> 2 tests are failing with the proposed patch:
> 
>     devices-mesh-dot11s-regression
>     routing-aodv-regression
> 
> I will have a further look.


I noticed these too but was not concerned because these are tests at a higher layer and they are pcap traces that are often perturbed (I would like to replace them at some point).  It is likely that there is some small perturbation and they just need to be regenerated.

However, I would like to have a solid unit/regression test on this patch before committing.

(In reply to sebastien.deronne from comment #36)
> Also, few comments:
> 
> * I also would remove the attribute instead of deprecating it.
> * Was it not the intention to rename CcaMode1Threshold to CcaEdThreshold?

I am open to this but neutral.  On the one hand, CcaEd is a little bit more conventional name for it.  On the other hand, CcaMode1 is technically correct per the standard, and keeping the same name minimizes the API change.

> * Some tests/examples are not cleaned up in your patch.

OK, thanks, will have a look.  I am just gauging interest on the overall plan before finishing off.

(In reply to Tom Henderson from comment #37)
> (In reply to sebastien.deronne from comment #35)
> > 2 tests are failing with the proposed patch:
> > 
> >     devices-mesh-dot11s-regression
> >     routing-aodv-regression
> > 
> > I will have a further look.
> 
> 
> I noticed these too but was not concerned because these are tests at a
> higher layer and they are pcap traces that are often perturbed (I would like
> to replace them at some point).  It is likely that there is some small
> perturbation and they just need to be regenerated.

I mostly regenerated them, also some minor changes to clean up some settings.

> 
> However, I would like to have a solid unit/regression test on this patch
> before committing.

I had started a test for this, if fine for you I could continue working on it tomorrow.

Created attachment 3199 [details]
update to apply on Tom's proposal

(In reply to Tom Henderson from comment #38)
> (In reply to sebastien.deronne from comment #36)
> > Also, few comments:
> > 
> > * I also would remove the attribute instead of deprecating it.
> > * Was it not the intention to rename CcaMode1Threshold to CcaEdThreshold?
> 
> I am open to this but neutral.  On the one hand, CcaEd is a little bit more
> conventional name for it.  On the other hand, CcaMode1 is technically
> correct per the standard, and keeping the same name minimizes the API change.

Ok, I've just added a patch to apply on top of yours.
I changed the API there, if not fine for you we can revert back, but my preference is to use CcaEdThreshold.

> 
> > * Some tests/examples are not cleaned up in your patch.
> 
> OK, thanks, will have a look.  I am just gauging interest on the overall
> plan before finishing off.


Please see my changes, all tests are now passing.

Tom, with your change we do not get a notification that the packet is dropped (NotifyRxDrop).

In my test list, one of the check was whether it gets dropped when a packet is received with a power below RxSensitivity.

So I suggest to have:

phy->NotifyRxDrop (packet);

Created attachment 3202 [details]
update to apply on Tom's proposal

Updated my previous patch to include call to NotifyRxDrop as I previously suggested.

Tom, is my additional fine for you? Then I can continue writing the test case.

(In reply to sebastien.deronne from comment #44)
> Tom, is my additional fine for you? Then I can continue writing the test
> case.

I am not sure about it; I had intentionally left it out.  I think it comes down to two issues: 1) semantics of the Drop trace, and 2) reducing wifi events.

It could be argued that the signal is not even there from the perspective of the receiver, so it should not be dropped but ignored.  Also, consider a dense-mode scenario, multiple BSSes; do you want to be processing more events and traces from very weak signals arriving from one corner of the scenario to another?

On the other hand, from a debugging perspective, it may help in some cases for someone to deduce what happened to their missing packet.

I guess I would still lean towards having no trace event (and perhaps even delete the NS_LOG statement too, or else make it conditional on a 'LogWeakSignals' attribute defaulting to false) and reserve the RxDrop trace for packets that are dropped despite being possibly strong enough to be received.

Sebastien, I also would prefer to remove all of your remappings of CcaMode1Threshold to EdThreshold in examples and tests, and just delete the attribute unless it is set to something realistic, or unless the example or test is intentionally trying to make use of a changed value.  Setting it to the value of 0 or -110 dBm is not helpful when people copy-paste the ns-3 examples to start their own work.

(In reply to Tom Henderson from comment #45)
> (In reply to sebastien.deronne from comment #44)
> > Tom, is my additional fine for you? Then I can continue writing the test
> > case.
> 
> I am not sure about it; I had intentionally left it out.  I think it comes
> down to two issues: 1) semantics of the Drop trace, and 2) reducing wifi
> events.
> 
> It could be argued that the signal is not even there from the perspective of
> the receiver, so it should not be dropped but ignored.  Also, consider a
> dense-mode scenario, multiple BSSes; do you want to be processing more
> events and traces from very weak signals arriving from one corner of the
> scenario to another?
> 
> On the other hand, from a debugging perspective, it may help in some cases
> for someone to deduce what happened to their missing packet.
> 
> I guess I would still lean towards having no trace event (and perhaps even
> delete the NS_LOG statement too, or else make it conditional on a
> 'LogWeakSignals' attribute defaulting to false) and reserve the RxDrop trace
> for packets that are dropped despite being possibly strong enough to be
> received.

OK I get your point, this was just because this was a behavioral change compared to current implementation, but this can be documented. I don't think we need to add an extra attribute to control that, if user is interested he can add this himself.

Created attachment 3205 [details]
new patch

I made a new patch merging the previous ones and taking into account your comments.
I still need to add a test case and update documentation.

Created attachment 3209 [details]
updated patch

Updated patch => I fixed an issue I discovered when writing test cases:

diff --git a/src/wifi/model/spectrum-wifi-phy.cc b/src/wifi/model/spectrum-wifi-phy.cc
index 892a7ca..a5051e2 100644
--- a/src/wifi/model/spectrum-wifi-phy.cc
+++ b/src/wifi/model/spectrum-wifi-phy.cc
@@ -245,7 +245,7 @@ SpectrumWifiPhy::StartRx (Ptr<SpectrumSignalParameters> rxParams)
 
   // Do no further processing if signal is too weak
   // Current implementation assumes constant rx power over the packet duration
-  if (WToDbm (rxPowerW) < GetRxSensitivity ())
+  if (rxPowerW < GetRxSensitivity ())
     {
       NS_LOG_INFO ("Received signal too weak to process: " << WToDbm (rxPowerW) << " dBm");
       return;

I also written some test case but it's not finished yet so I'll publish in the coming day.

Created attachment 3219 [details]
latest patch including tests

I updated the previous patch and included test cases for this bug.

I rely on those changes for a next phase so please let me know your remaining comments.

In general, latest patch looks good.

1) There is an inconsistency in the check against RxSensitivity; for YansWifiPhy, the check includes the RxGain, but for SpectrumWifiPhy, it does not.

2) are you not deprecating the old attributes?

3) 
+    .AddAttribute ("CcaEdThreshold",
                    "The energy of a received signal should be higher than "
                    "this threshold (dbm) to allow the PHY layer to declare CCA BUSY state.",

Should the help string clarify that this check should be done on the 20 MHz primary channel?  Should it clarify that this is for a non-wifi signal?

4) the unit test looks good, thanks for writing it.

(In reply to Tom Henderson from comment #52)
> In general, latest patch looks good.
> 
> 1) There is an inconsistency in the check against RxSensitivity; for
> YansWifiPhy, the check includes the RxGain, but for SpectrumWifiPhy, it does
> not.

No, this is correct, gain is already applied in Spectrum:

  double rxPowerW = Integral (filteredSignal) * DbToRatio (GetRxGain ());


> 
> 2) are you not deprecating the old attributes?

Sorry, I forgot.

> 
> 3) 
> +    .AddAttribute ("CcaEdThreshold",
>                     "The energy of a received signal should be higher than "
>                     "this threshold (dbm) to allow the PHY layer to declare
> CCA BUSY state.",
> 
> Should the help string clarify that this check should be done on the 20 MHz
> primary channel?  Should it clarify that this is for a non-wifi signal?

I will extend the help text.

> 
> 4) the unit test looks good, thanks for writing it.

Thanks.

I am finishing this and then it will be ready to be pushed.

Note: I also need to update documentation.

Created attachment 3225 [details]
FINAL patch

Handled review comments and updated documentation.

I am planning to push tomorrow.

(In reply to sebastien.deronne from comment #55)
> Created attachment 3225 [details]
> FINAL patch
> 
> Handled review comments and updated documentation.
> 
> I am planning to push tomorrow.

Looks good, except that:

+ TypeId::DEPRECATED, "Replaced by RxSensitivity, this attribute has no effect.")

Usually, deprecation means that it still continues to work for now, but developers should move off of it soon.  If deprecated means non-functional here, I would rather just remove it now because users may expect that it still works for now and skip through the warning.

(In reply to Tom Henderson from comment #56)
> (In reply to sebastien.deronne from comment #55)
> > Created attachment 3225 [details]
> > FINAL patch
> > 
> > Handled review comments and updated documentation.
> > 
> > I am planning to push tomorrow.
> 
> Looks good, except that:
> 
> + TypeId::DEPRECATED, "Replaced by RxSensitivity, this attribute has no
> effect.")
> 
> Usually, deprecation means that it still continues to work for now, but
> developers should move off of it soon.  If deprecated means non-functional
> here, I would rather just remove it now because users may expect that it
> still works for now and skip through the warning.

You are right.

Then we have two solutions: either we delete this attribute, or we keep it but we have to set its default value to the same as RxSensitivity. I am more in favor of the second solution.

> 
> Then we have two solutions: either we delete this attribute, or we keep it
> but we have to set its default value to the same as RxSensitivity. I am more
> in favor of the second solution.

Second solution is fine with me.

OK, I will change that and push.

Pushed in changeset 13866:cace17aa07da