Matrix propagation loss model

[Mathieu Lacage, 2010-02-16 15:15:14]

http://codereview.appspot.com/207104/diff/1/3
File src/common/propagation-loss-model.cc (right):

http://codereview.appspot.com/207104/diff/1/3#newcode781
src/common/propagation-loss-model.cc:781: std::map<NodePair,
double>::const_iterator i = m_loss.find (std::make_pair (na, nb));
it would be easy to change the code in a single lookup if you made sure that na
< nb all the time. I don't know if it matters though

http://codereview.appspot.com/207104/diff/1/3#newcode783
src/common/propagation-loss-model.cc:783: if (i == m_loss.end ())
missing braces

http://codereview.appspot.com/207104/diff/1/3#newcode786
src/common/propagation-loss-model.cc:786: if (i != m_loss.end ())
same

http://codereview.appspot.com/207104/diff/1/4
File src/common/propagation-loss-model.h (right):

http://codereview.appspot.com/207104/diff/1/4#newcode487
src/common/propagation-loss-model.h:487: void SetLoss (Ptr<Node> a, Ptr<Node> b,
double loss, bool symmetric = true);
Is there a reason why don't you specify a and b as Ptr<MobilityModel> ?

[Pavel Boyko, 2010-02-16 15:38:54]

Updated

http://codereview.appspot.com/207104/diff/1/3
File src/common/propagation-loss-model.cc (right):

http://codereview.appspot.com/207104/diff/1/3#newcode781
src/common/propagation-loss-model.cc:781: std::map<NodePair,
double>::const_iterator i = m_loss.find (std::make_pair (na, nb));
On 2010/02/16 15:15:14, Mathieu Lacage wrote:
> it would be easy to change the code in a single lookup if you made sure that
na
> < nb all the time. I don't know if it matters though

Well, extra lookup was actually a bug.

http://codereview.appspot.com/207104/diff/1/3#newcode783
src/common/propagation-loss-model.cc:783: if (i == m_loss.end ())
On 2010/02/16 15:15:14, Mathieu Lacage wrote:
> missing braces

Done.

http://codereview.appspot.com/207104/diff/1/3#newcode786
src/common/propagation-loss-model.cc:786: if (i != m_loss.end ())
On 2010/02/16 15:15:14, Mathieu Lacage wrote:
> same

Done.

http://codereview.appspot.com/207104/diff/1/4
File src/common/propagation-loss-model.h (right):

http://codereview.appspot.com/207104/diff/1/4#newcode487
src/common/propagation-loss-model.h:487: void SetLoss (Ptr<Node> a, Ptr<Node> b,
double loss, bool symmetric = true);
On 2010/02/16 15:15:14, Mathieu Lacage wrote:
> Is there a reason why don't you specify a and b as Ptr<MobilityModel> ?

I think user shouldn't care about mobility models in this case (since nothing
depends on mobility in fact). I have removed extra GetObject<MobilityModel>() in
the updated version.

[Mathieu Lacage, 2010-02-16 15:50:01]

+1 for merging for me.

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

[Nicola Baldo, 2010-02-16 16:16:35]

I think this patch is ok. One minor comment below. Furthermore, it would be good
to have an example program to show how users are supposed to interact with this
model.

http://codereview.appspot.com/207104/diff/1/4
File src/common/propagation-loss-model.h (right):

http://codereview.appspot.com/207104/diff/1/4#newcode487
src/common/propagation-loss-model.h:487: void SetLoss (Ptr<Node> a, Ptr<Node> b,
double loss, bool symmetric = true);
On 2010/02/16 15:38:54, Pavel Boyko wrote:
> On 2010/02/16 15:15:14, Mathieu Lacage wrote:
> > Is there a reason why don't you specify a and b as Ptr<MobilityModel> ?
> 
> I think user shouldn't care about mobility models in this case (since nothing
> depends on mobility in fact). I have removed extra GetObject<MobilityModel>()
in
> the updated version.

Good point, but the user is supposed to create mobility objects in any case,
right? If so, maybe it makes sense to use Ptr<Mobility>, so the user is enforced
to create them.

[Pavel Boyko, 2010-02-16 16:24:44]

http://codereview.appspot.com/207104/diff/1/4
File src/common/propagation-loss-model.h (right):

http://codereview.appspot.com/207104/diff/1/4#newcode487
src/common/propagation-loss-model.h:487: void SetLoss (Ptr<Node> a, Ptr<Node> b,
double loss, bool symmetric = true);
On 2010/02/16 16:16:35, Nicola Baldo wrote:
> Good point, but the user is supposed to create mobility objects in any case,
> right? If so, maybe it makes sense to use Ptr<Mobility>, so the user is
enforced
> to create them.

 I find NS_ASSERT (senderMobility != 0) in YansWifiChannel::Send () enough to
enforce user to create mobility models :)

[Nicola Baldo, 2010-02-22 10:56:02]

On Tue, Feb 16, 2010 at 5:24 PM,  <pavel.boyko@gmail.com> wrote:
>
>  I find NS_ASSERT (senderMobility != 0) in YansWifiChannel::Send ()
> enough to enforce user to create mobility models :)
>
> http://codereview.appspot.com/207104/show
>

I am fine if you leave the code as is, but please put at least one
example program, so that it is clearly shown to the users that they
need to create mobility objects anyway.

Nicola

[Pavel Boyko, 2010-03-05 12:29:48]

Hi, Nicola,

  I have added simple example, please take a look. Unfortunately there seems to
be a bug in flow monitor, so please consider applying patch from
http://www.nsnam.org/bugzilla/show_bug.cgi?id=834 to make the example working. 

  Usage:

$ ./waf --run wifi-hidden-terminal
Hidden station experiment with RTS/CTS disabled:
Flow 1 (10.0.0.1 -> 10.0.0.2)
  Tx Bytes:   14249772
  Rx Bytes:   1029876
  Throughput: 0.785733 Mbps
Flow 2 (10.0.0.3 -> 10.0.0.2)
  Tx Bytes:   14249772
  Rx Bytes:   0
  Throughput: 0 Mbps
------------------------------------------------
Hidden station experiment with RTS/CTS enabled:
Flow 1 (10.0.0.1 -> 10.0.0.2)
  Tx Bytes:   14249772
  Rx Bytes:   628140
  Throughput: 0.479233 Mbps
Flow 2 (10.0.0.3 -> 10.0.0.2)
  Tx Bytes:   14249772
  Rx Bytes:   351576
  Throughput: 0.268231 Mbps

  Pavel

[Josh Pelkey, 2010-03-05 16:16:23]

On 2010/03/05 12:29:48, Pavel Boyko wrote:
> Hi, Nicola,
> 
>   I have added simple example, please take a look. Unfortunately there seems
to
> be a bug in flow monitor, so please consider applying patch from
> http://www.nsnam.org/bugzilla/show_bug.cgi?id=834 to make the example working.

> 
>   Usage:
> 
> $ ./waf --run wifi-hidden-terminal
> Hidden station experiment with RTS/CTS disabled:
> Flow 1 (10.0.0.1 -> 10.0.0.2)
>   Tx Bytes:   14249772
>   Rx Bytes:   1029876
>   Throughput: 0.785733 Mbps
> Flow 2 (10.0.0.3 -> 10.0.0.2)
>   Tx Bytes:   14249772
>   Rx Bytes:   0
>   Throughput: 0 Mbps
> ------------------------------------------------
> Hidden station experiment with RTS/CTS enabled:
> Flow 1 (10.0.0.1 -> 10.0.0.2)
>   Tx Bytes:   14249772
>   Rx Bytes:   628140
>   Throughput: 0.479233 Mbps
> Flow 2 (10.0.0.3 -> 10.0.0.2)
>   Tx Bytes:   14249772
>   Rx Bytes:   351576
>   Throughput: 0.268231 Mbps
> 
>   Pavel

Looks good.  Go ahead and merge.

[Nicola Baldo, 2010-03-05 17:57:09]

On 2010/03/05 12:29:48, Pavel Boyko wrote:
>   I have added simple example, please take a look. Unfortunately there seems
to
> be a bug in flow monitor, so please consider applying patch from
> http://www.nsnam.org/bugzilla/show_bug.cgi?id=834 to make the example working.

> 


Cool! I think it is a very interesting example!
+1 to merge after your patch for bug 834 has been committed.

Nicola