Bug 787 - Addition of Two Ray Ground model to propagation loss model and tests
Addition of Two Ray Ground model to propagation loss model and tests
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: wifi
ns-3-dev
All All
: P5 normal
Assigned To: Mathieu Lacage
: patch
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-07 12:15 EST by Tom Hewer
Modified: 2010-02-08 07:15 EST (History)
3 users (show)

See Also:


Attachments
Patch to update files (44.91 KB, patch)
2010-01-07 12:17 EST, Tom Hewer
Details | Diff
Patch containing suggested changes (13.05 KB, patch)
2010-01-18 10:16 EST, Tom Hewer
Details | Diff
new version (13.24 KB, application/octet-stream)
2010-01-20 01:46 EST, Pavel Boyko
Details
New patch (14.47 KB, patch)
2010-01-20 11:50 EST, Tom Hewer
Details | Diff
Final Draft Patch (14.40 KB, patch)
2010-01-25 03:53 EST, Tom Hewer
Details | Diff
Final Draft v2 (25.88 KB, patch)
2010-01-25 04:24 EST, Tom Hewer
Details | Diff
Final Draft v3 (25.91 KB, patch)
2010-01-25 05:20 EST, Tom Hewer
Details | Diff
Final Draft v4 (14.43 KB, patch)
2010-02-04 09:57 EST, Tom Hewer
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Hewer 2010-01-07 12:15:56 EST
I have ported the Two Ray Ground model from the NS-2 code into NS-3.  It seemed suitable to add this to the propagation-loss-model code rather than it's own entity.

I've also created the test case, similar to Friis, and have checked the results pass on OSX and Linux.

Please could any users let me know if they find any problems or issues.

Many thanks,

Tom.
Comment 1 Tom Hewer 2010-01-07 12:17:05 EST
Created attachment 712 [details]
Patch to update files
Comment 2 Pavel Boyko 2010-01-13 06:33:13 EST
  Tom, 

(In reply to comment #0)
> I have ported the Two Ray Ground model from the NS-2 code into NS-3.  It seemed
> suitable to add this to the propagation-loss-model code rather than it's own
> entity.
> 
> I've also created the test case, similar to Friis, and have checked the results
> pass on OSX and Linux.
> 
> Please could any users let me know if they find any problems or issues.

  Thank you for 2-ray ground model implementation & test. All formulas are correct to me, although number of multiplications can be optimized. The only important thing I don't like is that you have implemented TX and RX antenna heights as path loss model parameters. This is bad approach since every station potentially has it's own antenna height but there is always just single propagation loss model instance per radio channel (= ever). I propose to 
assume that every node has single antenna and use node's z coordinate as antenna height relative to some global reference "ground level" z_0 where z_0 is an attribute of TwoRayGroundPropagationLossModel. What do you think? 

  Also please remove extra indent after namespace ns3 { to minimize diff.
Comment 3 Tom Hewer 2010-01-18 10:16:29 EST
Created attachment 727 [details]
Patch containing suggested changes

I've made the changes so that the z coordinate of the node is the antenna height, but have also included a model parameter to add to this.  This is to allow for a standard height above z for antenna height, if a network needs it (i.e. a vehicular network where z=0 and the antenna is on the roof 1.5m above). The difference is defaulted to 0 for ease of use.

Please have a look and let me know what you think.  Also, could you indicate the multiplications that can be optimised?

Many thanks,

Tom.
Comment 4 Tom Hewer 2010-01-18 10:17:30 EST
Also amended \brief in *.h for doxygen.

Tom.
Comment 5 Pavel Boyko 2010-01-20 01:46:45 EST
Created attachment 730 [details]
new version
Comment 6 Pavel Boyko 2010-01-20 02:04:12 EST
  Hi, Tom, 

  please consider attached version: fixed indentaion, removed unnecessary GetObject() calls and a little bit optimized calculations.

  Also test must be extended to include nonzero z coordinates cases.

  Pavel

(In reply to comment #3)
> Created an attachment (id=727) [details]
> Patch containing suggested changes
> 
> I've made the changes so that the z coordinate of the node is the antenna
> height, but have also included a model parameter to add to this.  This is to
> allow for a standard height above z for antenna height, if a network needs it
> (i.e. a vehicular network where z=0 and the antenna is on the roof 1.5m above).
> The difference is defaulted to 0 for ease of use.
> 
> Please have a look and let me know what you think.  Also, could you indicate
> the multiplications that can be optimised?
> 
> Many thanks,
> 
> Tom.
Comment 7 Tom Hewer 2010-01-20 11:50:10 EST
Created attachment 731 [details]
New patch

New patch with updated optimisations and addition of test suite for non-zero z values.
Comment 8 Pavel Boyko 2010-01-25 02:49:55 EST
  Hi, Tom,

  New test is a Good Thing, thank you. I'm sorry to be tedious, but 

1. code style is still wrong (please setup your editor to use spaces instead of tabs and follow GNU braces indentation style)

2. I didn't catch why new form of  Use Two-Ray Pathloss  formulas is better that the previous one. There where 6 multiplications and now there are 8 of them. 

  Actually I can forgive you point 2 (probably all these "optimizations" are not serious), but point 1 must be fixed to merge your code.

  Pavel

(In reply to comment #7)
> Created an attachment (id=731) [details]
> New patch
> 
> New patch with updated optimisations and addition of test suite for non-zero z
> values.
Comment 9 Tom Hewer 2010-01-25 03:53:34 EST
Created attachment 734 [details]
Final Draft Patch

Hi Pavel,

You are right on point 2. I put those multiplications back in for testing, but have now returned them to the optimised version you wrote, much better for large simulations!

As to point 1 I have changed my tabs to spaces and tried to follow the GNU style.

Many thanks,

Tom.
Comment 10 Mathieu Lacage 2010-01-25 03:54:53 EST
(In reply to comment #9)

> As to point 1 I have changed my tabs to spaces and tried to follow the GNU
> style.

check-style.py is your friend:
http://code.nsnam.org/mathieu/indent
Comment 11 Tom Hewer 2010-01-25 04:24:15 EST
Created attachment 735 [details]
Final Draft v2

I ran Mathieu's check-style tool over the files and recreated the patch.

T.
Comment 12 Pavel Boyko 2010-01-25 04:51:50 EST
(In reply to comment #11)
> Created an attachment (id=735) [details]
> Final Draft v2
> 
> I ran Mathieu's check-style tool over the files and recreated the patch.

  Some more tabs left in the "Two-Ray Ground equation:" comment. 

  Well, I vote to commit right after ns-3.7 release day.
Comment 13 Tom Hewer 2010-01-25 05:20:42 EST
Created attachment 736 [details]
Final Draft v3

Fixed extra tabs.

I've noticed that running the check-style.py has corrected some other parts of the files; is this ok?

T.
Comment 14 Tom Hewer 2010-02-04 09:57:00 EST
Created attachment 752 [details]
Final Draft v4

For files moved to src/common/...

T.
Comment 15 Mathieu Lacage 2010-02-04 09:58:51 EST
please, commit.
Comment 16 Tom Hewer 2010-02-04 11:12:03 EST
Are there instructions on how to commit to ns-3-dev.

I have committed the changed files locally but when I try hg push it gives the response 'ssl required'

Thanks,

T.
Comment 17 Mathieu Lacage 2010-02-04 11:56:20 EST
you need to generate a priv/pub ssh key and send me a username+pub key by email.
Comment 18 Andrey Mazo 2010-02-08 07:14:15 EST
Shouldn't this bug be closed as fixed in def0efbb0fd5?
Comment 19 Tom Hewer 2010-02-08 07:15:57 EST
Bug closed as added to ns-3-dev in changeset def0efbb0fd5.