Bug 1008 - The next loss can not be used in propagation-loss-model
The next loss can not be used in propagation-loss-model
Status: RESOLVED INVALID
Product: ns-3
Classification: Unclassified
Component: core
pre-release
All All
: P5 normal
Assigned To: Mathieu Lacage
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-14 12:11 UTC by Gary Pei
Modified: 2010-10-15 15:54 UTC (History)
2 users (show)

See Also:


Attachments
The proposed patch and test shows that it fixed the problem. (3.37 KB, patch)
2010-10-14 12:11 UTC, Gary Pei
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gary Pei 2010-10-14 12:11:43 UTC
Created attachment 999 [details]
The proposed patch and test shows that it fixed the problem.

Although YansWifiChannelHelper::AddPropagationLoss() maintains a vector of propagation loss models, however only only model can be used. However, I believe the original intent was to support multiple loss models (e.g. large scale fading and small scale fading etc.). Thus, I think the attached patch should fulfill the intent.
Comment 1 Mathieu Lacage 2010-10-14 14:22:51 UTC
(In reply to comment #0)
> Created an attachment (id=999) [details]
> The proposed patch and test shows that it fixed the problem.
> 
> Although YansWifiChannelHelper::AddPropagationLoss() maintains a vector of
> propagation loss models, however only only model can be used. However, I
> believe the original intent was to support multiple loss models (e.g. large
> scale fading and small scale fading etc.). Thus, I think the attached patch
> should fulfill the intent.

The intent of the current code is that PropagationLossModel::CalcRxPower recursively calls PropagationLossModel::CalcRxPower on the next model until there are no models and the current code appears to be doing just that. What is the problem you are seeing ?
Comment 2 Gary Pei 2010-10-14 14:35:52 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > Created an attachment (id=999) [details] [details]
> > The proposed patch and test shows that it fixed the problem.
> > 
> > Although YansWifiChannelHelper::AddPropagationLoss() maintains a vector of
> > propagation loss models, however only only model can be used. However, I
> > believe the original intent was to support multiple loss models (e.g. large
> > scale fading and small scale fading etc.). Thus, I think the attached patch
> > should fulfill the intent.
> 
> The intent of the current code is that PropagationLossModel::CalcRxPower
> recursively calls PropagationLossModel::CalcRxPower on the next model until
> there are no models and the current code appears to be doing just that. What is
> the problem you are seeing ?

In the attached test program, it will give you answer 20 rather than 30. The problem I see is that only last model's value is returned since it did not do recursive calls in PropagationLossModel::CalcRxPower. If it does, it should give you 30.
Comment 3 Gary Pei 2010-10-14 14:51:58 UTC
> problem I see is that only last model's value is returned 
                             ^^^^
I want to clarify. Current PropagationLossModel::CalcRxPower only returns the 1st model result if there is only one model or the 2nd model result otherwise.
Comment 4 Mathieu Lacage 2010-10-14 14:58:36 UTC
(In reply to comment #3)
> > problem I see is that only last model's value is returned 
>                              ^^^^
> I want to clarify. Current PropagationLossModel::CalcRxPower only returns the
> 1st model result if there is only one model or the 2nd model result otherwise.

Yes, and the second result is expected to use the input value to return the correct value. i.e., if you get an incorrect value, it's because the model you are using is not correctly returning a value which uses the input value.
Comment 5 Gary Pei 2010-10-14 16:27:05 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > > problem I see is that only last model's value is returned 
> >                              ^^^^
> > I want to clarify. Current PropagationLossModel::CalcRxPower only returns the
> > 1st model result if there is only one model or the 2nd model result otherwise.
> 
> Yes, and the second result is expected to use the input value to return the
> correct value. i.e., if you get an incorrect value, it's because the model you
> are using is not correctly returning a value which uses the input value.

Thus, PropagationLossModel::CalcRxPower does not recursively calls PropagationLossModel::CalcRxPower on the next model, right? 

If you do something like following:

YansWifiChannelHelper wifiChannel ;
wifiChannel.AddPropagationLoss ("ns3::SmallScaleFadingA");
wifiChannel.AddPropagationLoss ("ns3::ExtraLossB");

Current code will only return ns3::SmallScaleFadingA. But I think it should return ns3::LogDistancePropagationLossModel+ns3::SmallScaleFadingA+ns3::ExtraLossB. Is this right? Or this is not your original intent.
Comment 6 Gary Pei 2010-10-14 17:17:23 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > > problem I see is that only last model's value is returned 
> >                              ^^^^
> > I want to clarify. Current PropagationLossModel::CalcRxPower only returns the
> > 1st model result if there is only one model or the 2nd model result otherwise.
> 
> Yes, and the second result is expected to use the input value to return the
> correct value. i.e., if you get an incorrect value, it's because the model you
> are using is not correctly returning a value which uses the input value.

Sorry, I think I understand it now. This is only a problem for a loss model that ignores txpower as input. Thus, this is not a bug for general case.
Comment 7 Mathieu Lacage 2010-10-15 03:01:05 UTC
(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > > problem I see is that only last model's value is returned 
> > >                              ^^^^
> > > I want to clarify. Current PropagationLossModel::CalcRxPower only returns the
> > > 1st model result if there is only one model or the 2nd model result otherwise.
> > 
> > Yes, and the second result is expected to use the input value to return the
> > correct value. i.e., if you get an incorrect value, it's because the model you
> > are using is not correctly returning a value which uses the input value.
> 
> Sorry, I think I understand it now. This is only a problem for a loss model
> that ignores txpower as input. Thus, this is not a bug for general case.

Exactly. Maybe you could suggest a way to improve the API documentation for these methods to make this more clear.
Comment 8 Tom Henderson 2010-10-15 11:40:10 UTC
(In reply to comment #7)

> Exactly. Maybe you could suggest a way to improve the API documentation for
> these methods to make this more clear.

Here is what I would suggest if you just want to handle this via documentation.  If you want to programmatically enforce this, I would suggest to add a capability to check within SetNext().

OK to commit?


diff -r c6e03f378655 src/common/propagation-loss-model.h
--- a/src/common/propagation-loss-model.h       Fri Oct 08 10:01:00 2010 -0700
+++ b/src/common/propagation-loss-model.h       Fri Oct 15 08:37:57 2010 -0700
@@ -48,6 +48,14 @@
   PropagationLossModel ();
   virtual ~PropagationLossModel ();
 
+  /**
+   * \brief Enables a chain of loss models to act on the signal
+   * \param The next PropagationLossModel to add to the chain
+   *
+   * This method of chaining propagation loss models only works commutatively
+   * if the propagation loss of all models in the chain are independent
+   * of transmit power.
+   */
   void SetNext (Ptr<PropagationLossModel> next);
 
   /**
@@ -440,7 +448,15 @@
 };
 
 /**
- * \brief The propagation loss is fixed. The user can set received power level.
+ * \brief Return a constant received power level independent of the transmit 
+ *  power
+ *
+ * The received power is constant independent of the transmit power.  The user
+ * must set received power level through the Rss attribute or public 
+ * SetRss() method.  Note that if this loss model is chained to other loss
+ * models via SetNext() method, it can only be the first loss model in such
+ * a chain, or else it will disregard the losses computed by loss models
+ * that precede it in the chain. 
  */ 
 class FixedRssLossModel : public PropagationLossModel
 {
@@ -452,7 +468,7 @@
   /**
    * \param rss (dBm) the received signal strength
    *
-   * Set the RSS.
+   * Set the received signal strength (RSS) in dBm.
    */
   void SetRss (double rss);
Comment 9 Gary Pei 2010-10-15 12:37:53 UTC
(In reply to comment #8)
Tom,
Thanks, I think your doxygen changes make it very clear.
Comment 10 Mathieu Lacage 2010-10-15 14:45:29 UTC
(In reply to comment #8)

> OK to commit?

Yes.
Comment 11 Tom Henderson 2010-10-15 15:54:46 UTC
pushed(In reply to comment #10)
> (In reply to comment #8)
> 
> > OK to commit?
> 
> Yes.

pushed in changeset: ee39e2c196c3