Created attachment 225 [details]
example implementation for csma

(In reply to comment #0)
> This is especially obvious when using the BridgeNetDevice/BridgeHelper.
> 
> I would like to propose adding both methods:
> std::pair<NetDeviceContainer,NetDeviceContainer> 
> CsmaHelper::InstallStar (Ptr<Node> center, const NodeContainer &terminals);
> 
> std::pair<NetDeviceContainer,NetDeviceContainer> 
> PointToPointHelper::InstallStar (Ptr<Node> center, const NodeContainer
> &terminals);
> 
> The biggest issue with this proposal is what should be the return value of
> InstallStar. As an alternative to the above proposal, we could create a
> NetDeviceContainerPair class to mirror our use of std:: containers in the
> helper API.

Personally, I would rather have two separate return values.  And of course the way to have two output parameters in C++ is to have output reference parameters...  So:

  void CsmaHelper::InstallStar (Ptr<Node> center, const NodeContainer &terminals, NetDeviceContainer &outCenterDevices, NetDeviceContainer &outTerminalDevices);

In Python, with 'direction' parameter annotations this can become:

  csma = ns3.CmsmaHelper()
  centerDevices, outerDevices = csma.InstallStar(center, terminals)

(In reply to comment #2)

> > std::pair<NetDeviceContainer,NetDeviceContainer> 
> > PointToPointHelper::InstallStar (Ptr<Node> center, const NodeContainer
> > &terminals);
> > 
> > The biggest issue with this proposal is what should be the return value of
> > InstallStar. As an alternative to the above proposal, we could create a
> > NetDeviceContainerPair class to mirror our use of std:: containers in the
> > helper API.
> 
> Personally, I would rather have two separate return values.  And of course the
> way to have two output parameters in C++ is to have output reference
> parameters...  So:
> 
>   void CsmaHelper::InstallStar (Ptr<Node> center, const NodeContainer
> &terminals, NetDeviceContainer &outCenterDevices, NetDeviceContainer
> &outTerminalDevices);
> 
> In Python, with 'direction' parameter annotations this can become:
> 
>   csma = ns3.CmsmaHelper()
>   centerDevices, outerDevices = csma.InstallStar(center, terminals)


naively, I would have thought that this could be obtained in python with the std::pair<> version. At worst, you could write a simple C++ wrapper which converts the std::pair return value to a pair of arguments and is wrapped itself to InstallStar in python. I understand that this is annoying for the automatic wrapper generation but using input arguments in the c++ version would force users to unconditionally create variables to hold the result while they can conceivably ignore the return value of InstallStar when they don't need it.

(In reply to comment #3)
> (In reply to comment #2)
> 
> > > std::pair<NetDeviceContainer,NetDeviceContainer> 
> > > PointToPointHelper::InstallStar (Ptr<Node> center, const NodeContainer
> > > &terminals);
> > > 
> > > The biggest issue with this proposal is what should be the return value of
> > > InstallStar. As an alternative to the above proposal, we could create a
> > > NetDeviceContainerPair class to mirror our use of std:: containers in the
> > > helper API.
> > 
> > Personally, I would rather have two separate return values.  And of course the
> > way to have two output parameters in C++ is to have output reference
> > parameters...  So:
> > 
> >   void CsmaHelper::InstallStar (Ptr<Node> center, const NodeContainer
> > &terminals, NetDeviceContainer &outCenterDevices, NetDeviceContainer
> > &outTerminalDevices);
> > 
> > In Python, with 'direction' parameter annotations this can become:
> > 
> >   csma = ns3.CmsmaHelper()
> >   centerDevices, outerDevices = csma.InstallStar(center, terminals)
> 
> 
> naively, I would have thought that this could be obtained in python with the
> std::pair<> version. At worst, you could write a simple C++ wrapper which
> converts the std::pair return value to a pair of arguments and is wrapped
> itself to InstallStar in python. I understand that this is annoying for the
> automatic wrapper generation but using input arguments in the c++ version would
> force users to unconditionally create variables to hold the result while they
> can conceivably ignore the return value of InstallStar when they don't need it.
> 

On the other hand, in case they _do_ need it (which I think is the more common scenario), they will be forced to decode the meaning of the std::pair.  What does 'first' hold, what does 'second' hold?  At least with output parameters, the parameter names make it clearer.

(In reply to comment #4)

> On the other hand, in case they _do_ need it (which I think is the more common
> scenario), they will be forced to decode the meaning of the std::pair.  What
> does 'first' hold, what does 'second' hold?  At least with output parameters,
> the parameter names make it clearer.

Hence my earlier suggestion to use a non-std::container: NetDeviceContainerPair or something. Maybe StarContainer.

(In reply to comment #5)
> (In reply to comment #4)
> 
> > On the other hand, in case they _do_ need it (which I think is the more common
> > scenario), they will be forced to decode the meaning of the std::pair.  What
> > does 'first' hold, what does 'second' hold?  At least with output parameters,
> > the parameter names make it clearer.
> 
> Hence my earlier suggestion to use a non-std::container: NetDeviceContainerPair
> or something. Maybe StarContainer.
> 

I don't care strongly; I probably lean towards Gustavo's suggestion of output parameters since I think it is most likely that users will want handles to these containers later.

I saw the python construct,

  csma.InstallStar(...)

What does that even mean?  A typo?

It seems odd (and relatively inflexible) to me to start putting methods for creating high-level topologies in helpers.  It seems more natural to me to have a Star helper that takes a number of nodes and wires them together using point-to-point helpers.

On another note:

Is someone going to file a bug that says, "ring topologies are painful to create"?  Will we respond by adding an InstallRing method to the point-to-point device helper?

Is someone going to file a bug that says, "mesh topologies are painful to create"?  Will we respond by adding an InstallMesh method to the point-to-point device helper?

Is someone going to file a bug that says, "tree topologies are painful to create"?  Will we respond by adding an InstallTree method to the csma device helper that installs stars on csma nodes?

Hmmm.

(In reply to comment #7)
> I saw the python construct,
> 
>   csma.InstallStar(...)
> 
> What does that even mean?  A typo?

I think gustavo was just lazy to show the full code :)

> 
> It seems odd (and relatively inflexible) to me to start putting methods for
> creating high-level topologies in helpers.  It seems more natural to me to have
> a Star helper that takes a number of nodes and wires them together using
> point-to-point helpers.

Part of the reason why I wanted to do that is because there are some devices for which "star" topologies make zero sense. Wireless networks for example. But, anyway, I don't care much either way. 

So, are you suggesting this ?

template <typename T>
class StarHelper
{
public:
  void SetDeviceHelper (T helper);
  void Install (Ptr<Node> center>, NodeContainer periphery, 
                NetDeviceContainer &centerDevices, 
                NetDeviceContainer &peripheryDevices);
private:
  T m_helper;
};

CsmaHelper csma;
csma. ... // setup
StarHelper<CsmaHelper> starHelper;
starHelper.SetDeviceHelper (csma);
starHelper.Install (center, periphery, centerDevices, perpheryDevices);

Another option would be, of course, to make all device helpers derive from a single base class to avoid the template.


> 
> On another note:
> 
> Is someone going to file a bug that says, "ring topologies are painful to
> create"?  Will we respond by adding an InstallRing method to the point-to-point
> device helper?
> 
> Is someone going to file a bug that says, "mesh topologies are painful to
> create"?  Will we respond by adding an InstallMesh method to the point-to-point
> device helper?
> 
> Is someone going to file a bug that says, "tree topologies are painful to
> create"?  Will we respond by adding an InstallTree method to the csma device
> helper that installs stars on csma nodes?

why not ? I would not strike me as utterly crazy. We are really talking about small amounts of pretty trivial code repeated in a couple of places so, I don't feel especially bad about not reusing a single instance of that logic.

If any amount of code can be reused easily without copy/paste across multiple source files, it should be done.  Way back at the Atlanta meeting, we discussed eventually having topology "helpers"; right now seems like a natural time to tackle them.

Since device helpers have common API, and now we have a use case to enforce these APIs, I say +1 to Mathieu's suggestion of a device-helper base class.  It is important to conceptually separate device-helpers from topology-helpers, so StarTopology or StarTopologyHelper could enforce this distinction.  Our src directory could additionally enforce the distinction with e.g. src/topology seperate from src/helpers, or perhaps src/helpers/devices and src/helpers/topolgy.  I lean towards an std::pair like custom container for the output of StarHelper::Install instead of pass-by-reference output parameters.  This gets rid of the ambiguity of what is "first" and what is "second".

struct StarDeviceContainer
{
  NetDeviceContainer center;
  NetDeviceContainer terminals;
}

class StarTopology
{
public:
  StarTopology (DeviceHelperBase helper) : m_helper(helper) {}
  StarDeviceContainer Install (Ptr<Node> center, NodeContainer periphery);
private:
  DeviceHelperBase m_helper;
};

The open question in my mind then becomes how do we handle when topology helpers are being used with incompatible device/link types?

(In reply to comment #9)
> If any amount of code can be reused easily without copy/paste across multiple
> source files, it should be done.

There is, of course, no such black-or-white answer to the problem and your comment shows it particularly well. Code reuse has costs. One of them is the one you outline below: its does not deal particularly well with variant behaviors, hence leading the system to behave in weird ways in some cases. So, reusing 5 lines of a trivial for loop must be weighted against allowing weird/invalid object/code combinations. I personnally could live with either solution but I want to make sure that the consequences of that code reuse are clear to everyone.

> The open question in my mind then becomes how do we handle when topology
> helpers are being used with incompatible device/link types?

That is clearly one of the issues which must be addressed.

(In reply to comment #10)
> (In reply to comment #9)
> > If any amount of code can be reused easily without copy/paste across multiple
> > source files, it should be done.
> 
> There is, of course, no such black-or-white answer to the problem and your
> comment shows it particularly well. Code reuse has costs. One of them is the
> one you outline below: its does not deal particularly well with variant
> behaviors, hence leading the system to behave in weird ways in some cases. So,
> reusing 5 lines of a trivial for loop must be weighted against allowing
> weird/invalid object/code combinations. I personnally could live with either
> solution but I want to make sure that the consequences of that code reuse are
> clear to everyone.

Yes, there are no black and white answers, only consequences of our design choices.  The usual line in my defense is that future changes will have to be made in 3-4 different places IF we change something like behavior or API of these topology helpers under the copy/paste approach, possibly leading to subtle inconsitencies.  That and Craig's layering idea under which these stock topologies are another independent layer on top of the helpers are enough reason for me to prefer the one solution over the other.

> 
> > The open question in my mind then becomes how do we handle when topology
> > helpers are being used with incompatible device/link types?
> 
> That is clearly one of the issues which must be addressed.
> 

One solution is to make the device and topology helpers into Object subclasses each with a TypeId, and use aggregation to attach a device helper to a topology helper.  We then do the inelegant thing: the topology helper iterates GetObject calls over a list of device helper TypeIds for which it is valid, and if it gets null after trying all of the "good" types, it gives up and FATAL_ERROR/ASSERTS out.  The pain of this inelegance is that when new device types are added, the model author (or people who marshal in the code) have to add a GetObject check to all of the valid topology helpers.  This seems somewhat less prone to subtle, non-crashing bugs than the copy/paste approach.  In the worst case of forgetting to add this check to the topology helpers with a new device type, the topology helper simply will FATAL_ERROR/ASSERT because you didn't add the device TypeId as a valid type.

(In reply to comment #10)
> (In reply to comment #9)
> > If any amount of code can be reused easily without copy/paste across multiple
> > source files, it should be done.
> 
> There is, of course, no such black-or-white answer to the problem and your
> comment shows it particularly well. Code reuse has costs. 

FWIW, my inclination is to resist trying to get a lot of reuse from these topology helpers, because I think they will end up being mainly device specific, so adding templates or hierarchy here seems like overkill to me.  If we keep them simple they will be easier to customize by users later.

given how close we are from trying to release, I would vote against fixing this bug now. Can we wait _after_ the release ?

+1

Time is running out and this is a "nice to have" API addition.