Bug 1271

Created attachment 1247 [details]
suggested patch

Hi Tom,

I am fine with this patch, and also with applying a similar one for m_mobility.

BTW, I did not invent this idea of using opaque pointers by myself, I just got inspiration from YansWifiPhy, which has the following methods:

  void SetDevice (Ptr<Object> device);
  void SetMobility (Ptr<Object> mobility);

and the corresponding member variables:

  Ptr<Object> m_device;
  Ptr<Object> m_mobility;

Do you suggest that we change these as well?

(In reply to comment #2)
> Hi Tom,
> 
> I am fine with this patch, and also with applying a similar one for m_mobility.
> 
> BTW, I did not invent this idea of using opaque pointers by myself, I just got
> inspiration from YansWifiPhy, which has the following methods:
> 
>   void SetDevice (Ptr<Object> device);
>   void SetMobility (Ptr<Object> mobility);
> 
> and the corresponding member variables:
> 
>   Ptr<Object> m_device;
>   Ptr<Object> m_mobility;
> 
> Do you suggest that we change these as well?

I did not realize that it was cloned from somewhere else.  In working with the code, it seems to require an unnecessary downcast or GetObject when the actual underlying object is always going to be a NetDevice or MobilityModel (i.e. I am not sure what it buys anyone to keep these ones generic to Object, unlike your other example of GenericPhy).  What would be your preference, to leave as is?

One other comment I have relating to m_mobility.  I understand why you have per-device mobility if you need per-device positioning, but it may be the case that the node instead has a mobility model, so I might suggest a patch like this:

@@ -138,7 +138,16 @@
 HalfDuplexIdealPhy::GetMobility ()
 {
   NS_LOG_FUNCTION (this);
-  return m_mobility;
+  if (m_mobility != 0)
+    {
+      return m_mobility;
+    }
+  else
+    {
+      // Try to see if the node itself has a mobility model aggregated
+      Ptr<NetDevice> nd = DynamicCast<NetDevice> (m_device);
+      return nd->GetNode ()->GetObject<MobilityModel> ();
+    }
 }

(In reply to comment #3)
> I did not realize that it was cloned from somewhere else.  In working with the
> code, it seems to require an unnecessary downcast or GetObject when the actual
> underlying object is always going to be a NetDevice or MobilityModel (i.e. I am
> not sure what it buys anyone to keep these ones generic to Object, unlike your
> other example of GenericPhy).  What would be your preference, to leave as is?


I agree with your arguments, so I am fine with the change to Ptr<NetDevice> and Ptr<MobilityModel>. I'll give it a try, if it's not too time consuming I'll post a patch.



> One other comment I have relating to m_mobility.  I understand why you have
> per-device mobility if you need per-device positioning, but it may be the case
> that the node instead has a mobility model, so I might suggest a patch like
> this:
> 
> @@ -138,7 +138,16 @@
>  HalfDuplexIdealPhy::GetMobility ()
>  {
>    NS_LOG_FUNCTION (this);
> -  return m_mobility;
> +  if (m_mobility != 0)
> +    {
> +      return m_mobility;
> +    }
> +  else
> +    {
> +      // Try to see if the node itself has a mobility model aggregated
> +      Ptr<NetDevice> nd = DynamicCast<NetDevice> (m_device);
> +      return nd->GetNode ()->GetObject<MobilityModel> ();
> +    }
>  }


I don't see much added value in that, considering that the helpers (at least those that I know of) are already getting the pointer from MobilityModel:


AdhocAlohaNoackIdealPhyHelper::Install (NodeContainer c) const
{
 ...
 phy->SetMobility (node);
 ...
}

YansWifiPhyHelper::Create (Ptr<Node> node, Ptr<WifiNetDevice> device) const
{
  ...
  phy->SetMobility (node);
  ...
}


So your patch would not change anything for users writing simulation programs.

We could discuss whether per-device positioning is a realistic use case (I have some doubts). But since the existing PHY classes already support this, I don't see why we should change.

Created attachment 1248 [details]
working patch based on the previous proposal

Here's a patch with the proposed modifications. It passes tests with spectrum and lte enabled. If you think it's satisfactory, I can push it.

(In reply to comment #4)


> 
> So your patch would not change anything for users writing simulation programs.
> 
> We could discuss whether per-device positioning is a realistic use case (I have
> some doubts). But since the existing PHY classes already support this, I don't
> see why we should change.


Hmm, I thought that I had raised the question before as to why there was per-device pointers to mobility models in spectrum, and that the answer was that there may be some MIMO use cases in which this granularity might be useful.

I had a look at what SingleModelSpectrumChannel does with this method, and found
         Ptr<MobilityModel> receiverMobility = (*rxPhyIterator)->GetMobility ()->GetObject<MobilityModel> ();

This kind of usage means that the spectrum-phy.h doxygen for Set/GetMobility() is not very clear.  That is, it should be instead something like this:

   /**
-   * get the associated MobilityModel instance
+   * get a pointer to an object to which a MobilityModel has been aggregated
+   * or to a MobilityModel itself.  User of this method should call
+   * GetObject<MobilityModel> () on the returned pointer.
    *
-   * @return a Ptr to the associated NetDevice instance
+   * @return a Ptr to an Object to which a MobilityModel has been aggregated
    */
   virtual Ptr<Object> GetMobility () = 0;

So, it seems at this point that there are two main options forward:

1) disregard my previous suggestion to make GetMobility return explicitly a MobilityModel, but fix the Doxygen along the lines above (also fix SetMobility()).  

2) adopt your patch

in either case, the presence of the per-device mobility pointer allows Spectrum to support either per-device or per-node mobility models; it is just a matter of how to set the pointer.  

Also, I believe that either case wouldn't break existing API for GetMobility (), since this will work in either case: 

          Ptr<MobilityModel> receiverMobility = (*rxPhyIterator)->GetMobility ()->GetObject<MobilityModel> ();

But adopting your patch 2) would preclude passing in e.g. a Node pointer to SetMobility() so that would be the main API breakage for users doing that sort of thing.

I don't care strongly either way.   As for the NetDevice side of this patch, I don't really see much harm in adopting the change and I don't think it will necessarily break anyone's existing code to do so (probably most people pass in Ptr<NetDevice> to that method already).

 (In reply to comment #6)
> Hmm, I thought that I had raised the question before as to why there was
> per-device pointers to mobility models in spectrum, and that the answer was
> that there may be some MIMO use cases in which this granularity might be
> useful.

I agree in theory, I just have some doubts about whether this is practical to do. Simulating all the multiple paths of a MIMO system implies having a symbol-level PHY model, of the kind of PhySim for wifi. These model have a huge computational complexity, hence their practical usability is limited. I think that a link-layer model of MIMO performance working on top of a single-path propagation model is more appropriate for a network simulator. 


> in either case, the presence of the per-device mobility pointer allows 
> Spectrum to support either per-device or per-node mobility models; it is 
> just a matter of how to set the pointer.  

Yes I agree it's good for the developer to have the possibility of choosing either modeling strategy, considering that it is already supported by the current code.


> So, it seems at this point that there are two main options forward:
> 
> 1) disregard my previous suggestion to make GetMobility return explicitly a
> MobilityModel, but fix the Doxygen along the lines above (also fix
> SetMobility()).  
> 
> 2) adopt your patch


I prefer 2). I still value more your previous argument, i.e., that if those pointers won't ever be something different than MobilityModel and NetDevice, then it's more transparent to make their type explicit.


> But adopting your patch 2) would preclude passing in e.g. a Node pointer to
> SetMobility() so that would be the main API breakage for users doing that sort
> of thing.

I think this API change would be for good, in the sense that the API becomes more transparent. These "phy->AddMobility (node)" look a bit like black magic.

BTW, the patch already documents the API change in CHANGES.html

(In reply to comment #7)
> 
> I think this API change would be for good, in the sense that the API becomes
> more transparent. These "phy->AddMobility (node)" look a bit like black magic.
> 
> BTW, the patch already documents the API change in CHANGES.html

It is fine with me to push your patch to close this issue.

changeset:   7553:2b93d333dea6
tag:         tip
user:        Nicola Baldo <nbaldo@cttc.es>
date:        Tue Oct 04 18:40:50 2011 +0200
summary:     Bug 1271 - stronger type checking in SpectrumPhy