Bug 787

Created attachment 712 [details]
Patch to update files

  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.

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.

Also amended \brief in *.h for doxygen.

Tom.

Created attachment 730 [details]
new version

  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.

Created attachment 731 [details]
New patch

New patch with updated optimisations and addition of test suite for non-zero z values.

  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.

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.

(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

Created attachment 735 [details]
Final Draft v2

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

T.

(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.

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.

Created attachment 752 [details]
Final Draft v4

For files moved to src/common/...

T.

please, commit.

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.

you need to generate a priv/pub ssh key and send me a username+pub key by email.

Shouldn't this bug be closed as fixed in def0efbb0fd5?

Bug closed as added to ns-3-dev in changeset def0efbb0fd5.