Bug 962

Summary: list of paths to reach objects contains bogus entries
Product: ns-3 Reporter: Nicola Baldo <nicola>
Component: documentationAssignee: Tom Henderson <tomh>
Status: RESOLVED FIXED    
Severity: normal CC: mathieu.lacage, nicola, ns-bugs, watrous
Priority: P5    
Version: ns-3-dev   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 941    
Attachments: patch to fix
tentative patch: new GenericPhy Object aggregated to HalfDuplexIdealPhy
non-working patch: remove attributes of type Ptr<Object>

Description Nicola Baldo 2010-07-23 13:13:21 EDT
in the doxygen documentation, the list of paths to reach objects often contains bogus entries. For example, WifiMac is reported to be reachable through the following paths:

# /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::WifiMac
# /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::WifiNetDevice/Mac
# /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/BsIpcsPacketClassifier/$ns3::WifiMac
# /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/BsIpcsPacketClassifier/$ns3::WifiNetDevice/Mac
# /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/LinkManager/$ns3::WifiMac
# /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/LinkManager/$ns3::WifiNetDevice/Mac
# /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/SSManager/$ns3::WifiMac
# /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/SSManager/$ns3::WifiNetDevice/Mac
# /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/ServiceFlowManager/$ns3::WifiMac
# /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/ServiceFlowManager/$ns3::WifiNetDevice/Mac
# /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::WifiMac
# /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::WifiNetDevice/Mac
# /NodeList/[i]/DeviceList/[i]/$ns3::SubscriberStationNetDevice/Classifier/$ns3::WifiMac 
[snip]

Same holds for several other objects.
Comment 1 Tom Henderson 2010-07-23 18:04:21 EDT
Agree, bug 941 is related to this.  These paths are generated by the doxygen introspection program, which is not 100%.  I think this is important to fix but I have other priorities at the moment, so a patch would be welcome.
Comment 2 Mitch Watrous 2011-09-13 16:38:14 EDT
(In reply to comment #0)
> in the doxygen documentation, the list of paths to reach objects often contains
> bogus entries. For example, WifiMac is reported to be reachable through the
> following paths:
> 
> # /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::WifiMac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::WifiNetDevice/Mac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/BsIpcsPacketClassifier/$ns3::WifiMac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/BsIpcsPacketClassifier/$ns3::WifiNetDevice/Mac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/LinkManager/$ns3::WifiMac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/LinkManager/$ns3::WifiNetDevice/Mac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/SSManager/$ns3::WifiMac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/SSManager/$ns3::WifiNetDevice/Mac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/ServiceFlowManager/$ns3::WifiMac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/ServiceFlowManager/$ns3::WifiNetDevice/Mac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::WifiMac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::WifiNetDevice/Mac
> #
> /NodeList/[i]/DeviceList/[i]/$ns3::SubscriberStationNetDevice/Classifier/$ns3::WifiMac 
> [snip]
> 
> Same holds for several other objects.

What is wrong exactly with these paths?
Comment 3 Tom Henderson 2011-09-14 00:39:52 EDT
(In reply to comment #2)
> (In reply to comment #0)
> > in the doxygen documentation, the list of paths to reach objects often contains
> > bogus entries. For example, WifiMac is reported to be reachable through the
> > following paths:
> > 
> > # /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::WifiMac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::WifiNetDevice/Mac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/BsIpcsPacketClassifier/$ns3::WifiMac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/BsIpcsPacketClassifier/$ns3::WifiNetDevice/Mac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/LinkManager/$ns3::WifiMac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/LinkManager/$ns3::WifiNetDevice/Mac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/SSManager/$ns3::WifiMac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/SSManager/$ns3::WifiNetDevice/Mac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/ServiceFlowManager/$ns3::WifiMac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::BaseStationNetDevice/ServiceFlowManager/$ns3::WifiNetDevice/Mac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::WifiMac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::WifiNetDevice/Mac
> > #
> > /NodeList/[i]/DeviceList/[i]/$ns3::SubscriberStationNetDevice/Classifier/$ns3::WifiMac 
> > [snip]
> > 
> > Same holds for several other objects.
> 
> What is wrong exactly with these paths?

In this case, I think that there is only one valid path for this example (of WifiMac):
/NodeList/[i]/DeviceList/[i]/$ns3::WifiNetDevice/Mac 

and even if the others actually work, they should be suppressed, since they are polluting the output.

If you look at a bunch of these classes, you will see repeatedly that there are many lines of the form:

/NodeList/[i]/DeviceList/[i]/$ns3::SomeTypeOfNetDevice/..../$ns3::SomeOtherAggregatedObject/TypeId

such as (from the above):
/NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::WifiMac
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                             this is the part that is bogus

So I would look at, in this specific case, why AlohaNoackNetDevice is getting into this output, for starters.
Comment 4 Tom Henderson 2011-09-16 01:26:34 EDT
Here is another example where the documentation does not contain the correct succinct path:

http://www.nsnam.org/docs/release/3.12/doxygen/classns3_1_1_wifi_phy_state_helper.html

The correct working path is
/NodeList/[i]/DeviceList/[i]/Phy/State

This one that is listed will work:
/NodeList/[i]/DeviceList/[i]/$ns3::WifiNetDevice/Phy/$ns3::YansWifiPhy/State 

but is more verbose than needed (Phy is an attribute of WifiNetDevice, State is an attribute of YansWifiPhy).  The others may or may not work; they gave me runtime errors for my set of modules.

While this one will work:
/NodeList/[i]/DeviceList/[i]/$ns3::WifiNetDevice/Phy/$ns3::YansWifiPhy/State 
I'm wondering if we can, in general, trim these $ns3::... extra TypeIds when we generate the doxygen for the trace paths.
Comment 5 Mitch Watrous 2011-09-27 19:56:43 EDT
Most of the extra paths were coming from Objects with no GetTypeId() functions.  All of the missing GetTypeId() functions that could be determined were added to remove these spurious paths (changeset e8cd72402e69).

I believe that the the rest of the extra paths are coming from these 2 classes in the spectrum module:

    aloha-noack-net-device.cca
    non-communicating-net-device.cc

These 2 classes each have an attribute that is a pointer to an Object like this:

  static TypeId tid = TypeId ("ns3::AlohaNoackNetDevice")
    .SetParent<NetDevice> ()
                 ...
    .AddAttribute ("Phy", "The PHY layer attached to this device.",
                   PointerValue (),
                   MakePointerAccessor (&AlohaNoackNetDevice::GetPhy,
                                        &AlohaNoackNetDevice::SetPhy),
                   MakePointerChecker<Object> ())

and

  static TypeId tid = TypeId ("ns3::NonCommunicatingNetDevice")
    .SetParent<NetDevice> ()
                 ...
    .AddAttribute ("Phy", "The PHY layer attached to this device.",
                   PointerValue (),
                   MakePointerAccessor (&NonCommunicatingNetDevice::GetPhy,
                                        &NonCommunicatingNetDevice::SetPhy),
                   MakePointerChecker<Object> ())

These objects have many paths associated with them because any object can be in that part of the path.

Here is an example of these extra paths for DropTailQueue:

--------------------------------------------------
static TypeId ns3::DropTailQueue::GetTypeId(void)
--------------------------------------------------

This method returns the TypeId associated to ns3::DropTailQueue.

This object is accessible through the following paths with Config::Set and Config::Connect:

    * /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::CsmaNetDevice/TxQueue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::EmuNetDevice/TxQueue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::PointToPointNetDevice/TxQueue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Phy/$ns3::Queue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/Queue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::CsmaNetDevice/TxQueue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::EmuNetDevice/TxQueue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::AlohaNoackNetDevice/Queue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::CsmaNetDevice/TxQueue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::EmuNetDevice/TxQueue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::PointToPointNetDevice/TxQueue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/Phy/$ns3::Queue/$ns3::DropTailQueue
    * /NodeList/[i]/DeviceList/[i]/$ns3::PointToPointNetDevice/TxQueue/$ns3::DropTailQueue
Comment 6 Tom Henderson 2011-09-28 00:20:04 EDT
(In reply to comment #5)
> Most of the extra paths were coming from Objects with no GetTypeId() functions.
>  All of the missing GetTypeId() functions that could be determined were added
> to remove these spurious paths (changeset e8cd72402e69).

This is a big improvement already.

> 
> I believe that the the rest of the extra paths are coming from these 2 classes
> in the spectrum module:
> 
>     aloha-noack-net-device.cca
>     non-communicating-net-device.cc
> 
> These 2 classes each have an attribute that is a pointer to an Object like
> this:
> 
>   static TypeId tid = TypeId ("ns3::AlohaNoackNetDevice")
>     .SetParent<NetDevice> ()
>                  ...
>     .AddAttribute ("Phy", "The PHY layer attached to this device.",
>                    PointerValue (),
>                    MakePointerAccessor (&AlohaNoackNetDevice::GetPhy,
>                                         &AlohaNoackNetDevice::SetPhy),
>                    MakePointerChecker<Object> ())
> 
> and
> 
>   static TypeId tid = TypeId ("ns3::NonCommunicatingNetDevice")
>     .SetParent<NetDevice> ()
>                  ...
>     .AddAttribute ("Phy", "The PHY layer attached to this device.",
>                    PointerValue (),
>                    MakePointerAccessor (&NonCommunicatingNetDevice::GetPhy,
>                                         &NonCommunicatingNetDevice::SetPhy),
>                    MakePointerChecker<Object> ())
> 
> These objects have many paths associated with them because any object can be in
> that part of the path.

This is related to the bug 1271 I just filed.  I would suggest either more strongly enforcing type here (SpectrumPhy), or simply delete this attribute.  Having the attribute like this, as it is, doesn't seem to provide any utility since the type is opaque.
Comment 7 Nicola Baldo 2011-09-28 05:01:01 EDT

> This is related to the bug 1271 I just filed.  I would suggest either more
> strongly enforcing type here (SpectrumPhy), or simply delete this attribute. 
> Having the attribute like this, as it is, doesn't seem to provide any utility
> since the type is opaque.

AlohaNoackNetDevice does not need to work with SpectrumPhy, it could work as well with any other PHY that supports the GenericPhy API. Note that this API is just a set of callbacks, there is no Object type associated with it. Hence enforcing the type SpectrumPhy (or any other type) is not an option. 

I'll have a look if we can remove the pointer attribute: after all if there is no guarantee that the object is of a given type, then the attribute is maybe not so much useful.

Bug 1271 is slightly different so I'll reply there.
Comment 8 Nicola Baldo 2011-09-28 05:26:41 EDT
Thinking more about it, if there is no Phy attribute of type Ptr<Object> in AlohaNoackNetDevice, then there is no way to have a path in the attribute system that reaches the actual PHY object (whatever its type). So unfortunately I think that removing the attribute is not a good solution. 

Is there anything else we could do?
Comment 9 Tom Henderson 2011-09-28 10:53:38 EDT
(In reply to comment #8)
> Thinking more about it, if there is no Phy attribute of type Ptr<Object> in
> AlohaNoackNetDevice, then there is no way to have a path in the attribute
> system that reaches the actual PHY object (whatever its type). So unfortunately
> I think that removing the attribute is not a good solution. 
> 

I see your point that the spectrum phy concrete types are accessible through this attribute.

> Is there anything else we could do?

Some options seem to be:
1) rename the attribute to something unique in the system (such as GenericPhy or PhyObject)?
2) put some spectrum-specific code in the documentation generation to filter these paths

Any thoughts on these options?
Comment 10 Nicola Baldo 2011-09-28 12:55:53 EDT
(In reply to comment #9)
> Some options seem to be:
> 1) rename the attribute to something unique in the system (such as GenericPhy
> or PhyObject)?
> 2) put some spectrum-specific code in the documentation generation to filter
> these paths
> 
> Any thoughts on these options?

I would be fine with renaming the attribute to PhyObject.
Comment 11 Nicola Baldo 2011-09-28 12:57:10 EDT
(In reply to comment #10)
> I would be fine with renaming the attribute to PhyObject.

Well, actually GenericPhy that you proposed is a better option.
Comment 12 Tom Henderson 2011-09-29 00:57:23 EDT
(In reply to comment #11)
> (In reply to comment #10)
> > I would be fine with renaming the attribute to PhyObject.
> 
> Well, actually GenericPhy that you proposed is a better option.

That would be fine with me and I'd prefer it to putting some extra stuff in the documentation generation program.  

OK to patch it that way and close this issue (if it passes tests)?

Note, there is a bug 1213 open to cover the next step, which is to start testing that these documented paths work and that things do not regress.
Comment 13 Mitch Watrous 2011-10-05 13:34:44 EDT
The problem is not with the attriubte name.  I changed the Phy attribute to be called GenericPhy, but ended up with these new paths:

    * /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/GenericPhy/$ns3::AarfcdWifiManager
    * /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/GenericPhy/$ns3::WifiNetDevice/RemoteStationManager/$ns3::AarfcdWifiManager
    * /NodeList/[i]/DeviceList/[i]/$ns3::AlohaNoackNetDevice/GenericPhy/$ns3::WifiRemoteStationManager/$ns3::AarfcdWifiManager
    * /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/GenericPhy/$ns3::AarfcdWifiManager
    * /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/GenericPhy/$ns3::WifiNetDevice/RemoteStationManager/$ns3::AarfcdWifiManager
    * /NodeList/[i]/DeviceList/[i]/$ns3::NonCommunicatingNetDevice/GenericPhy/$ns3::WifiRemoteStationManager/$ns3::AarfcdWifiManager

The problem is that the attributes contain pointers to Objects, which have many paths associated with them:

    .AddAttribute ("Phy", "The PHY layer attached to this device.",
                   PointerValue (),
                   MakePointerAccessor (&AlohaNoackNetDevice::GetPhy,
                                        &AlohaNoackNetDevice::SetPhy),
                   MakePointerChecker<Object> ())

Here is a possible solution:

    .AddAttribute ("Phy", "The PHY layer attached to this device.",
                   PointerValue (),
                   MakePointerAccessor (&AlohaNoackNetDevice::GetGenericPhy,
                                        &AlohaNoackNetDevice::SetGenericPhy),
                   MakePointerChecker<GenericPhy> ())

This would require creation of a GenericPhy class:

    class GenericPhy : public Object
    {
    }
Comment 14 Tom Henderson 2011-10-05 15:42:01 EDT
(In reply to comment #13)
> Here is a possible solution:
> 
>     .AddAttribute ("Phy", "The PHY layer attached to this device.",
>                    PointerValue (),
>                    MakePointerAccessor (&AlohaNoackNetDevice::GetGenericPhy,
>                                         &AlohaNoackNetDevice::SetGenericPhy),
>                    MakePointerChecker<GenericPhy> ())
> 
> This would require creation of a GenericPhy class:
> 
>     class GenericPhy : public Object
>     {
>     }

Before committing to this workaround, I'd like to find out whether this is in fact a fundamental limitation in the attribute system or whether we should try to solve it another way.  I am not sure that the above really solves things long term because it seems like the problem could, in the future, manifest itself at the GenericPhy level rather than the Object level.
Comment 15 Mitch Watrous 2011-10-05 16:41:36 EDT
I think it would solve the problem as long as lots and lots of new classes don't inherit from GenericPhy like they inherit from Object now.

There is a loop in the introspection program that looks for all classes that are children of a particular Object in the path. 

The problem now is that almost everything inherits from Object, which means that almost everything is a match in that loop.

What do you mean about finding out "... whether this is in fact a fundamental limitation in the attribute system"?  Do you want me to look into something about the attribute system?
Comment 16 Tom Henderson 2011-10-05 18:03:31 EDT
(In reply to comment #15)
> I think it would solve the problem as long as lots and lots of new classes
> don't inherit from GenericPhy like they inherit from Object now.
> 
> There is a loop in the introspection program that looks for all classes that
> are children of a particular Object in the path. 
> 
> The problem now is that almost everything inherits from Object, which means
> that almost everything is a match in that loop.
> 
> What do you mean about finding out "... whether this is in fact a fundamental
> limitation in the attribute system"? 

Whether we need to ban attributes of type Object.

> Do you want me to look into something
> about the attribute system?

I see the problem that any Object could, in theory, be hooked to that attribute.  We want to avoid nonsensical paths there, which means making a judgment or rule that we really only consider putting certain types there in practice.  Which could either be enforced by stronger typing of Phy, or by adding intelligence in the introspection system to prune the paths we think are likely to be nonsensical.

(starting to think now that banning Object here may be cleanest...)
Comment 17 Nicola Baldo 2011-10-06 03:47:04 EDT
> This would require creation of a GenericPhy class:
> 
>     class GenericPhy : public Object
>     {
>     }

Do you mean that then HalfDuplexIdealPhy and LteSpectrumPhy would need to inherit from GenericPhy rather than from Object directly?


>> The problem now is that almost everything inherits from Object, which means
>> that almost everything is a match in that loop.
>> 
>> What do you mean about finding out "... whether this is in fact a fundamental
>> limitation in the attribute system"? 
> 
> Whether we need to ban attributes of type Object.
> 

do you mean:

1) adding a filter into the introspection program so that it does not generate further subpaths when the attribute is of type Ptr<Object>

or

2) forbidding developers to put attributes of type Ptr<Object> in the code

I think that 1) is an easy workaround, and it also makes sense (if the object is so generic, the user is intended to know what he plugged onto that attribute, and the corresponding path in the attribute system). On the other hand, 2) would be a bit too restrictive.
Comment 18 Mathieu Lacage 2011-10-07 03:12:55 EDT
(In reply to comment #17)
> > This would require creation of a GenericPhy class:
> > 
> >     class GenericPhy : public Object
> >     {
> >     }
> 
> Do you mean that then HalfDuplexIdealPhy and LteSpectrumPhy would need to
> inherit from GenericPhy rather than from Object directly?
> 
> 
> >> The problem now is that almost everything inherits from Object, which means
> >> that almost everything is a match in that loop.
> >> 
> >> What do you mean about finding out "... whether this is in fact a fundamental
> >> limitation in the attribute system"? 
> > 
> > Whether we need to ban attributes of type Object.
> > 
> 
> do you mean:
> 
> 1) adding a filter into the introspection program so that it does not generate
> further subpaths when the attribute is of type Ptr<Object>
> 
> or
> 
> 2) forbidding developers to put attributes of type Ptr<Object> in the code
> 
> I think that 1) is an easy workaround, and it also makes sense (if the object
> is so generic, the user is intended to know what he plugged onto that
> attribute, and the corresponding path in the attribute system). On the other
> hand, 2) would be a bit too restrictive.

My feeling is that 2) would be more productive long term because it is hard to find much use for a pointer of type Object.

I see that nicola is more and more leaning towards that kind of design where the C++ API does not enforce any kind of type checking (see his recent request for review at the spectrum phy layer) but I cannot make myself believe in it.
Comment 19 Nicola Baldo 2011-10-07 03:48:45 EDT
> 
> I see that nicola is more and more leaning towards that kind of design where
> the C++ API does not enforce any kind of type checking (see his recent request
> for review at the spectrum phy layer) but I cannot make myself believe in it.

yes it is true that when I first wrote the GenericPhy I liked very much the idea of interconnecting arbitrary PHYs and MACs without type checking, just using a set of callbacks. But actually, after using it quite a bit, I agree that this callback-only approach is somewhat wild and messy, so I would in principle be in favor of changing that for a more traditional C++ class-based API for the same use case (though I am not going to volunteer to do it now, sorry).

As for the spectrum API change that I recently proposed for review, I do not agree with your comment: there is a well defined parent class that provides  functionality that is common for all PHY technologies, and derived classes are used for specialization for each PHY technology. There is no such thing like a Ptr<Object> that by default you don't know what to do with it.
Comment 20 Mathieu Lacage 2011-10-07 04:20:07 EDT
(In reply to comment #19)

> As for the spectrum API change that I recently proposed for review, I do not
> agree with your comment: there is a well defined parent class that provides 
> functionality that is common for all PHY technologies, and derived classes are
> used for specialization for each PHY technology. There is no such thing like a
> Ptr<Object> that by default you don't know what to do with it.

Yes, my point was that your idea of generalizing the method arguments so that you never have to change it is an example of the kind of generalization process that led to the Ptr<Object> idea. I am fairly uncomfortable with both and I like the fact that I have to change a method signature if I need it to do more but I could not make myself care enough to actually write a decent review about it so, take this with a grain of salt.
Comment 21 Tom Henderson 2011-10-21 10:24:06 EDT
Created attachment 1263 [details]
patch to fix

can we close this bug by merging this patch?  If not, can you (Nicola) suggest an alternative patch?
Comment 22 Nicola Baldo 2011-11-19 06:37:40 EST
(In reply to comment #21)
> Created attachment 1263 [details]
> patch to fix
> 
> can we close this bug by merging this patch?  If not, can you (Nicola) suggest
> an alternative patch?

no I do not agree with merging this patch, sorry. I'll come up with an alternative patch.
Comment 23 Nicola Baldo 2011-11-28 18:08:04 EST
Created attachment 1277 [details]
tentative patch: new GenericPhy Object aggregated to HalfDuplexIdealPhy

Here is a tentative patch. It works this way:

1) a new GenericPhy Object is introduced
2) NetDevice objects such as AlohaNoackNetDevice and NonCommunicatingNetDevice have an attribute of type GenericPhy
3) PHY objects such as HalfDuplexIdealPhy, WaveformGenerator and SpectrumAnalyzer are aggregated to an instance of GenericPhy, so that this aggregated instance can be used as the attribute value for point 2) above


I don't like much this patch because:

- the GenericPhy object is not really used for anything beyond defining a type other than Object that does not fool the automatic attribute path generation

- none of WaveformGenerator, SpectrumAnalyzer and NonCommunicatingNetDevice actually supports the GenericPhy interface (the PHY<-->MAC API) hence the new object is misnomed

-the attribute path to reach the PHY is uglier (see for instance the diff in spectrum-ideal-phy-test.cc)
Comment 24 Nicola Baldo 2011-11-28 18:10:40 EST
Created attachment 1278 [details]
non-working patch: remove attributes of type Ptr<Object>

for the record, I also tried removing the Ptr<Object> attributes completely (see other attached patch). It results in the failure of the test spectrum-ideal-phy, because this test relies on the reachability of the HalfDuplexIdealPhy via the attribute system,
Comment 25 Nicola Baldo 2011-11-28 18:19:42 EST
the relevant piece is line 195 in spectrum-ideal-phy-test.cc:

  Config::Connect ("/NodeList/*/DeviceList/*/Phy/RxEndOk", MakeCallback (&PhyRxEndOkTrace));

I am citing this because earlier in this thread it was said:

> Having the attribute like this, as it is, doesn't seem to provide any utility
since the type is opaque.

and the above code shows that in fact the opaque type is usable.
Ok, you need to know what type of Phy it is.
But I would argue that the attribute system already relies a lot on knowing the correct type. For example, to use a path like "DeviceList/*/Phy", I have to know the type of NetDevice.
Comment 26 Tom Henderson 2011-11-29 01:01:24 EST
(In reply to comment #25)
> the relevant piece is line 195 in spectrum-ideal-phy-test.cc:
> 
>   Config::Connect ("/NodeList/*/DeviceList/*/Phy/RxEndOk", MakeCallback
> (&PhyRxEndOkTrace));
> 
> I am citing this because earlier in this thread it was said:
> 
> > Having the attribute like this, as it is, doesn't seem to provide any utility
> since the type is opaque.
> 
> and the above code shows that in fact the opaque type is usable.
> Ok, you need to know what type of Phy it is.
> But I would argue that the attribute system already relies a lot on knowing the
> correct type. For example, to use a path like "DeviceList/*/Phy", I have to
> know the type of NetDevice.

I should have perhaps rephrased my comment that I didn't see any utility over making it type-specific (vs. of type Object).  

I wasn't sure why you nacked my previous SpectrumPhy patch because it seemed like you were leaning this way based on comment #19, and since you don't seem happy with the alternative.
Comment 27 Nicola Baldo 2011-11-29 07:39:23 EST
(In reply to comment #26)
> 
> I wasn't sure why you nacked my previous SpectrumPhy patch 

my apologies for this, I should have given more arguments. The reason that I nacked this is because it forces AlohaNoackNetDevice to be used with SpectrumPhy only, whereas it was intended to be used with other PHYs as well (that's the purpose of the GenericPhy interface). For example, I think I had already suggested some time ago on ns-developers to use AlohaNoackNetDevice with the simple-wireless model. 


> because it seemed
> like you were leaning this way based on comment #19, and since you don't seem
> happy with the alternative.

my comment #19 referred to *how* to implement the GenericPhy interface, i.e., using callbacks vs using a C++ class. I actually did some progress on this activity and have some draft patch that replaces callbacks with a SAP definition based on two abstract base classes (one for the SAP "provider" and one for the SAP "user", following an approach we have already been using for LENA).

But this problem (callbacks vs c++ classes) only affects the run-time interaction between PHY and above layer, not the usage of the attribute system which is what the Ptr<Object> attribute is being used for. I think the two issues are orthogonal.
Comment 28 Nicola Baldo 2012-03-24 11:10:35 EDT
wrapping up the discussion at the developers meeting...

I recommend as the preferred solution to patch the program generating the list of paths so that it does not consider attributes of type Ptr<Object>.

If this is not feasible, the first alternative in the list would be the one of comment #23 (which I still do not like, for the reasons previously mentioned).
Comment 29 Mitch Watrous 2012-05-01 13:21:04 EDT
Bug closed.

Changeset:  059b1b83e29b