I've been looking over this code, and actually I'm not sure if the fix I proposed actually makes sense...



To edit my initial bug report, I'm not sure if the large SNR and small PER is actually a problem.  Now I think that the interference calculations are correct, except when delta=0.  More details are below...


So I think function (CalculateNoiseInterferenceW) introduces two areas of strange behavior:

First, it calculates the SNR by determining the noise ONLY at the very beginning that the packet is received.  I'm not really sure what this SNR goes on to be used for, so I'm not sure if this is important or not.

Second, an NiChange event is pushed back at the start time of the event (the start time that the packet is received).  This NiChange uses the noiseInterference calculated at the start time of the event.  If the attached program is run with delta=0, then noiseInterference is equal to the power of the interfering packet.  If, however, delta is >0 (even .001), then the interfering packet does not overlap with the start time of the intended packet, and noiseInterference is equal to 0.  This creates a very sudden change in PER from delta=0 and delta>0.  The problem here is that an interfering packet with the same transmit time as the intended packet is actually counted as overlapping twice, meaning there are two NiChange events added at the same time, and both with the interfering packet's power.  To fix this, I'd just make it so if the interfering packet 'overlaps' with the start time of the intended packet, and the intended packet 'overlaps' with the start time of the interfering packet, then only add to noiseInterference, and don't also push back an NiChange event at that time.  The result looks like this:

			if((*i)->Overlaps (event->GetStartTime ()))
			{
				noiseInterference += (*i)->GetRxPowerW ();
			} 		
			else if (event->Overlaps ((*i)->GetStartTime ())) 
			{
				ni->push_back (NiChange ((*i)->GetStartTime (), (*i)->GetRxPowerW ()));
			}
			if (event->Overlaps ((*i)->GetEndTime ())) 
			{
				ni->push_back (NiChange ((*i)->GetEndTime (), -(*i)->GetRxPowerW ()));
			}

a description of the intended behavior of the code can be found in: http://www.cutebugs.net/files/wns2-yans.pdf

After looking at the yans paper, I do think my fix proposed in the first comment does fix the bug in interference-helper.  Using the attached script, run these two tests:

./waf --run "wifi-simple-interference --Irss=-80  --numPackets=200 --delta=1"

and:

./waf --run "wifi-simple-interference --Irss=-80  --numPackets=200 --delta=0"

And you'll see a difference in the amount of received packets (184 when delta=0 versus 200 when delta=1).

Changing the if statements in 'CalculateNoiseInterferenceW' to:

                        if((*i)->Overlaps (event->GetStartTime ()))
                        {
                                noiseInterference += (*i)->GetRxPowerW ();
                        }               
                        else if (event->Overlaps ((*i)->GetStartTime ())) 
                        {
                                ni->push_back (NiChange ((*i)->GetStartTime (),
(*i)->GetRxPowerW ()));
                        }
                        if (event->Overlaps ((*i)->GetEndTime ())) 
                        {
                                ni->push_back (NiChange ((*i)->GetEndTime (),
-(*i)->GetRxPowerW ()));
                        }

Makes it so the bug when the interference packet is received at the same time as the intended packet, and therefore the noise is calculated twice for the interference packet, is now fixed.

Now running:

./waf --run "wifi-simple-interference --Irss=-80  --numPackets=200 --delta=1"
./waf --run "wifi-simple-interference --Irss=-80  --numPackets=200 --delta=0"

Has little difference in packets received (or if there is one the difference is by 1 or 2 packets received), as it should be.

I am not sure I understand the proposed fix. Can you provide a patch with "hg diff" ?

Created attachment 555 [details]
a potential patch ?

I believe that this patch reflects what you have in mind. I think I sort of understand what bug it is fixing. 

What would be really really cool is if you could find a way to convert your test program into a regression test added to interference-helper.cc (something along the lines of dcf-manager-test.cc). If you can't do this, I guess I can commit this patch if you confirm that it is what you expected but it's hard for me to feel very good about this since none of our existing tests outline this bug or change their output with or without the patch.

Hey,

Yeah, it looks like that patch does what I was talking about.  In the future I will include diff files with my bug submissions.

Also, I'll look into converting this script into a regression test over the next few days.  The reason this fix doesn't change any of the current test outputs is that this only changes the output when the interfering packet and the intended packet are received at exactly the same time (delta = 0 in this script)

To explain the situation again, in the function that populates the niChange array, (CalculateNoiseInterferenceW) we populate depending on whether the start or end time of the interfering packet overlaps with the given event.  the overlap method counts a given time as overlapping with an event if the given time is within the range of the event, or equals the event's start or end time.  When delta = 0, the intended packet arrives at the same time as the interfering packet, the interfering packet is counted as 'overlapping' with the intended packet, AND since the interfering packet contributes to the noise in the system when the intended packet's start time occurs, the interfering packet's noise is added to the 'noise interference at the start time' variable.  You can see the error in the niChange array if you print out the members of the array at the end of this function, because there will be two entries that correspond to the interfering packet's power.  This means for the rest of the duration of the intended signal, the interfering noise power will be too high by a level equal to the interfering packet power.

(I hope that explanation makes sense, essentially if you look at the members of the niChange array at the end of the CalculateNoiseInterferenceW function, it should be apparent that there's something wrong...)

(In reply to comment #6)

> Also, I'll look into converting this script into a regression test over the
> next few days.  The reason this fix doesn't change any of the current test

Ok, I will wait to push this patch until we can add the test. Thanks a lot.

Created attachment 581 [details]
Patch to add PER trace hook

Created attachment 582 [details]
Test for interference bug

I just attached a test that detects this bug.  When run, it calculates three interference tests, one when both packets are received at the same time, one when the interference packet is received 1 microsecond after the intended packet, and one when the interference packet is received 2 microseconds after the intended packet.  Then, using the attached patch to trace the PER, it calculates the difference in PER between the first and second test, and the second and third test.  In the current implementation, these values should be the same, so if they are the same, we 'pass', if they aren't, the test fails.  

Tom Henderson is going to transform this to a test in Craig's new framework, so if the source seems a bit rough right now, that should be fixed.  If you have any further comments tell me and I'll look into it/send updates to Tom.

+1 on the patch fixing this bug, and on the patch including the new PER trace source.  I will sign up to make sure this test is included in the release if the other two patches are pushed.

Okay after some thought I think I got what your problem is:

In case two packets have equal StartTimes, the power is added twice.

I agree with that, due to the <= in InterferenceHelper::Event::Overlap(). Your patch fixes that using elseif, as obviously the <= should stay.

There should also be an equivalent situation at the packet end time due to the >=. However, I believe it is irrelevant for further calculations.

So I also agree with the patch.

changeset: 7c3b105b6945

Since tom said that he would commit the test himself, I reassign this bug to him.

(In reply to comment #13)
> changeset: 7c3b105b6945
> 
> Since tom said that he would commit the test himself, I reassign this bug to
> him.
> 

This was closed out in changeset 30f7ef5b318a