RobotHWGroup with aliased interfaces

[kphawkins]

This fixes the issue of RobotHWGroup not allowing the same interface to be used in different registered RobotHW.

[adolfo-rt]

I think this implementation will cause problems. This method is not only used for logging purposes, but also for checking resource conflicts. This line inside the controller manager populates a ControlerInfo instance that ends up being used by RobotHW::checkForConflict. The dedault implementation does not use the hardware_interface field, but robot-specific overloads might (I can already think of valid usecases).

I still think that to support multi-interface controllers, the changes will go deeper than this class. At the very least the ControllerInfo struct must be modified to allow discriminating which hardware_interfaces are being claimed by each resource, otherwise proper resource confict checks will be impossible.

[kphawkins]

If all you are concerned about is having all the interface types enumerated, I believe that's an easy fix, though more involved than this. Changing message types would require modifying Python code, as well as several debugging/logging statements. Other repos would need updating to accommodate a list of hardware interfaces.

[kphawkins]

Addressed in commit 841123a

[adolfo-rt]

I wanted to add a comment on the memory leak caused by this new statement, which was already discussed in #75, in particular here. There's a solution I think would work well, but haven't had the time to implement. The key idea is that you can do this with shared pointers:

class Foo {/* stuff*/};

int main()
{
  shared_ptr<void> ptr(new Foo());
}

Note that although the template parameter of ptr is void, when it gets out of scope the proper (Foo's) destructor will be invoked. Explanation here. I also set up a more complete example in this gist, where a standard container is filled with different types stored as shared void pointers.

Now, if in hardware_interface::internal::InterfaceManager we store a map<string, shared_ptr<void> > instead of a map<string, void*>, then the memory leak issue should be taken care of.

What do you think, @kphawkins?.

[kphawkins]

@adolfo-rt std::shared_ptr is just one of the many features from C++11 I would have loved to use in this pull. Unfortunately, ROS is still on C++03 by default, see REP 3. If we'd like to permanently switch ros-control to C++11, I'd support that switch. REP 3 makes enabling it permissible, but some machines may not support compilation.

[adolfo-rt]

I omitted namespaces for clarity, but I'm using boost::shared_ptr to avoid C++11 dependencies, as is done in most of ROS. The test gist does explicitly reference the boost namespace. I understand that there are other C++11 features that could be useful and don't yet have a straightforward replacement as shared_ptr does, for for the particular problem of getting rid of the said memory leak, C++03 and Boost should be enough.

[kphawkins]

For some reason I avoided Boost libraries for my commits for this PR.

However, without changing the T* get() definition, to shared_ptr<T> get(), we cannot convert new T to shared_ptr<T>(new T). The reason is that there is no way to check whether the raw pointer retrieved in earlier calls is ever dereferenced. If you decide, for instance, to destroy the object on a subsequent get call for the same interface, then the user might be holding a corrupted pointer. If you keep all the pointers around, then nothing really changes. I suppose I could delete the objects in the destructor, but I don't see why the InterfaceManager should not stick around until the main method terminates.

At the moment the functionality is that it will be holding a reference to an earlier version of the object. Honestly, the use case where this becomes a problem is really unusual. Basically you'd have to alternate registering and adding interfaces. At most, you'll have O(N) interfaces, where N is the number of register* calls.

@adolfo-rt I'd agree that shared_ptr is the best way to go, but is it worth changing the existing definition? It doesn't matter to me, though I'd like to get this merged into the main branches at some point.

[kphawkins]

Commit 841123a addresses the earlier issue about dealing with multiple hardware interfaces. Although we should eventually deprecate getHardwareInterfaceType() in favor of getHardwareInterfaceTypes(), I kept a legacy version in so that I wouldn't have to do massive changes to ros-controllers code.

[bmagyar]

This I believe is now safe to close as #231 addressed this feature. Please open a new one if you have suggestions to improve that implementation.