Bug 3020

The SimplePreambleDetectionModel is fine (ThresholdPreambleDetectionModel may be more descriptive) but I question whether the Phy should move to RX at time zero.  What about instead keeping the PHY in idle and only moving to RX if after 4 us the preamble is detected?

(In reply to Tom Henderson from comment #1)
> The SimplePreambleDetectionModel is fine (ThresholdPreambleDetectionModel
> may be more descriptive) but I question whether the Phy should move to RX at
> time zero.  What about instead keeping the PHY in idle and only moving to RX
> if after 4 us the preamble is detected?

Tom, I do not get your question, the PHY is moving to RX after 4 microseconds.

(In reply to sebastien.deronne from comment #2)
> (In reply to Tom Henderson from comment #1)
> > The SimplePreambleDetectionModel is fine (ThresholdPreambleDetectionModel
> > may be more descriptive) but I question whether the Phy should move to RX at
> > time zero.  What about instead keeping the PHY in idle and only moving to RX
> > if after 4 us the preamble is detected?
> 
> Tom, I do not get your question, the PHY is moving to RX after 4
> microseconds.

I should have been more clear; at time 0, while the state machine transition is suppressed until later, there are several events that still occur that are moving things into a reception state:

1) EndReceive() is scheduled
2) NotifyRxBegin() is called
3) m_interference.NotifyRxStart() is called.

Should these things be suppressed also until after preamble is detected?  

There is a related issue, in the case in which preamble detection fails, should it call the RxDrop trace?  I see it similar to the 'below sensitivity' case in that the signal is too weak to detect, so I suggest to not call RxBegin at time 0, later followed by Drop for this situation.

In StartReceiveHeader(), I think this line will be problematic right now:

+  NS_ASSERT (m_endPlcpRxEvent.IsExpired ());

because it could be the case that StartReceiveHeader() is scheduled multiple times within the 4us window.  Unit tests should cover the case where multiple packets arrive within this window (strong followed by weak, and weak followed by strong).

(In reply to Tom Henderson from comment #3)
> (In reply to sebastien.deronne from comment #2)
> > (In reply to Tom Henderson from comment #1)
> > > The SimplePreambleDetectionModel is fine (ThresholdPreambleDetectionModel
> > > may be more descriptive) but I question whether the Phy should move to RX at
> > > time zero.  What about instead keeping the PHY in idle and only moving to RX
> > > if after 4 us the preamble is detected?
> > 
> > Tom, I do not get your question, the PHY is moving to RX after 4
> > microseconds.
> 
> I should have been more clear; at time 0, while the state machine transition
> is suppressed until later, there are several events that still occur that
> are moving things into a reception state:
> 
> 1) EndReceive() is scheduled
> 2) NotifyRxBegin() is called
> 3) m_interference.NotifyRxStart() is called.
> 
> Should these things be suppressed also until after preamble is detected?  

I kept what was done by Scott, but I can have a further look.

> 
> There is a related issue, in the case in which preamble detection fails,
> should it call the RxDrop trace?  I see it similar to the 'below
> sensitivity' case in that the signal is too weak to detect, so I suggest to
> not call RxBegin at time 0, later followed by Drop for this situation.
> 

Yes this is quite similar, I tend to agree.

> In StartReceiveHeader(), I think this line will be problematic right now:
> 
> +  NS_ASSERT (m_endPlcpRxEvent.IsExpired ());
> 
> because it could be the case that StartReceiveHeader() is scheduled multiple
> times within the 4us window.  Unit tests should cover the case where
> multiple packets arrive within this window (strong followed by weak, and
> weak followed by strong).

I still have to add some tests, this needs to be checked indeed.

> 1) EndReceive() is scheduled
> 2) NotifyRxBegin() is called
> 3) m_interference.NotifyRxStart() is called.

I managed to clean the first two as suggested.

The 3rd one needs to stay there. The reason is that once this is called, interference helper starts recording.
My opinion is that this function should be renamed (e.g. StartRecording).

I noticed 2 issues when writing tests:

1/ If another reception occurs during the first 4us window, the current reception is stopped and it jumps to the second reception.

         if (m_endPreambleDetectionEvent.IsRunning ())
           {
             m_endPreambleDetectionEvent.Cancel ();
           }

2/ InterferenceHelper should be changed in order to give the correct SNR value at the moment we check whether the preamble is detected. In the current patch, the moment against this check is done corresponds to the begin of the reception, whereas it should be at 4us (i.e. the moment now instead of the moment event was added). I made a change in InterferenceHelper but this causes regression issues so I am still busy investigating.

Created attachment 3238 [details]
updated patch + tests

I discovered several issues when writing tests (see comment above). In addition, I realized SNR calculation was also not correct (snr in SnrPer struct has never been really used so far). This is all fixed now, and tests are written. I also modified the name of the model as suggested by Tom.

(In reply to sebastien.deronne from comment #7)
> Created attachment 3238 [details]
> updated patch + tests
> 
> I discovered several issues when writing tests (see comment above). In
> addition, I realized SNR calculation was also not correct (snr in SnrPer
> struct has never been really used so far). This is all fixed now, and tests
> are written. I also modified the name of the model as suggested by Tom.

This all looks fine to me, but please edit wifi-design.rst as changes are made to the PHY model, and please remember to patch CHANGES.html and RELEASE_NOTES.

Created attachment 3239 [details]
updated doc

Tom, can you have a quick look at the documentation update (typos, content, ...) so that I can push this later today?
Thanks!

I merged with ns-3-dev and further updated documentation. Final patch pushed in changeset 13877:8ce4c7c7398c.