Created an attachment (id=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.