Bug 738

Summary:	CsmaNetDevice calls ReceiveErrorModel too late
Product:	ns-3	Reporter:	Florian Schmidt <Florian.Schmidt>
Component:	devices	Assignee:	Craig Dowell <craigdo>
Status:	RESOLVED FIXED
Severity:	normal	CC:	tomh
Priority:	P3
Version:	ns-3-dev
Hardware:	All
OS:	All
Attachments:	patch to fix the bug revised patch

Description Florian Schmidt 2009-11-12 04:49:58 UTC

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 Florian Schmidt 2009-11-16 06:27:07 UTC

Created attachment 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 Tom Henderson 2009-11-16 10:18:36 UTC

(In reply to comment #1)
> 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.


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 Tom Henderson 2009-11-16 10:19:28 UTC

Created attachment 654 [details]
revised patch

Comment 4 Tom Henderson 2009-11-16 10:20:16 UTC

Craig, please review and push if you are OK with this.