Bug 738 - CsmaNetDevice calls ReceiveErrorModel too late
: CsmaNetDevice calls ReceiveErrorModel too late
Status: RESOLVED FIXED
: ns-3
devices
: ns-3-dev
: All All
: P3 normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-11-12 04:49 EDT by
Modified: 2010-01-05 14:15 EDT (History)


Attachments
patch to fix the bug (1.13 KB, patch)
2009-11-16 06:27 EDT, Florian Schmidt
Details | Diff
revised patch (5.22 KB, patch)
2009-11-16 10:19 EDT, Tom Henderson
Details | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-12 04:49:58 EDT
Overview: CsmaNetDevice::Receive is responsible for calling a ReceiveErrorModel
if set. However, it removes the Ethernet header and trailer first. This means
that for the same bit error rate, the packet error rate is lower than it should
be, because Ethernet headers/trailers can never be corrupted.

Steps to reproduce: 
1) Use a simple setup that transfers packets from node A to node B.
2) Set a bit error rate.
3) Record the number of packets actually received by node B.

Actual result: The packet error rate is approximately (1-BER)^x, with x being
the packet size of payload + IP/UDP header in bits.

Expected result: The packet error rate should be approximately (1-BER)^y, with
y being the packet size of payload + IP/UDP + the 144 bits of Ethernet
header/trailer.
------- Comment #1 From 2009-11-16 06:27:07 EDT -------
Created an attachment (id=653) [details]
patch to fix the bug

This is a small patch to fix the bug. Not sure it's the most beautiful
solution, especially with the duplicated header/trailer removal in both
branches, but it changes as little as possible of the code. If this is not
acceptable, I can rewrite the section slightly more to eliminate the
duplication.
Patched code compiles, and in my testing setup, the PER now seems to match the
expected rate.
------- Comment #2 From 2009-11-16 10:18:36 EDT -------
(In reply to comment #1)
> Created an attachment (id=653) [details] [details]
> patch to fix the bug
> 
> This is a small patch to fix the bug. Not sure it's the most beautiful
> solution, especially with the duplicated header/trailer removal in both
> branches, but it changes as little as possible of the code. If this is not
> acceptable, I can rewrite the section slightly more to eliminate the
> duplication.
> Patched code compiles, and in my testing setup, the PER now seems to match the
> expected rate.


I agree that this bug needs fixed.  It pointed out another problem, too, which
is that the m_phyRxDropTrace inconsistently gets a packet with an Ethernet
header in one call above this code block, and without the header in the error
model call.

Attached is a patch which fixes also the drop trace issue, and removes the long
else clause.
------- Comment #3 From 2009-11-16 10:19:28 EDT -------
Created an attachment (id=654) [details]
revised patch
------- Comment #4 From 2009-11-16 10:20:16 EDT -------
Craig, please review and push if you are OK with this.