Inconsistent handling of registered InterfaceManagers in InterfaceManager

[RobertWilbrandt]

Currently, the hardware_interface::InterfaceManager class allows to either register either interfaces directly or sub-InterfaceManagers (afaik only used for combined_robot_hw in this repo). But while the get<InterfaceType>() uses the registered InterfaceManagers in order to find a valid interface/Resourcemanager, the getNames() and getInterfaceResources() ignore them completely. Because of this, when you use a RobotHW which registered another InterfaceManager, you cannot use it in a combined_robot_hw (the filter-routines don't know about the 'transitively-registered' interfaces).

While it is probably not too tragic that you cannot use a combined_robot_hw inside another combined_robot_hw, i had some siuations where using registerInterfaceManager() would have been quite handy (mostly wrapping existing ros-control drivers to be loadable in a combined_robot_hw). The current state also seems pretty inconsistent to me, which is why i want to create a PR to handle all transitively registered interfaces consistently.

Because i don't know too much about other ros-control parts, i created this issue first before providing a PR myself. As far as i can tell this shouldn't break things inside ros_control, but please tell if this doesn't work for some reason.

[bmagyar]

Hi, Have you tried implementing this, do you have an estimate of the size of the changeset? Also, could you please elaborate a little more on potential use cases? Ideally we'd want changes to be binary compatible as well.

…

On Fri, 8 May 2020, 14:42 RobertWilbrandt, ***@***.***> wrote: Currently, the hardware_interface::InterfaceManager <https://github.com/ros-controls/ros_control/blob/melodic-devel/hardware_interface/include/hardware_interface/internal/interface_manager.h> class allows to either register either interfaces directly or sub- InterfaceManagers (afaik only used for combined_robot_hw in this repo). But while the get<InterfaceType>() uses the registered InterfaceManagers in order to find a valid interface/Resourcemanager, the getNames() and getInterfaceResources() ignore them completely. Because of this, when you use a RobotHW which registered another InterfaceManager, you cannot use it in a combined_robot_hw (the filter-routines don't know about the 'transitively-registered' interfaces). While it is probably not too tragic that you cannot use a combined_robot_hw inside another combined_robot_hw, i had some siuations where using registerInterfaceManager() would have been quite handy (mostly wrapping existing ros-control drivers to be loadable in a combined_robot_hw). The current state also seems pretty inconsistent to me, which is why i want to create a PR to handle all transitively registered interfaces consistently. Because i don't know too much about other ros-control parts, i created this issue first before providing a PR myself. As far as i can tell this shouldn't break things inside ros_control, but please tell if this doesn't work for some reason. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#452>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA24PYKP3QJPW2BSAY6GEWLRQQD3XANCNFSM4M4FX62A> .

[RobertWilbrandt]

Hey, the change should be pretty small, as it would basically would just go through the interface_managers in both InterfaceManager::getNames() and InterfaceManager::getInterfaceResources() (+ maybe some error handling for duplicates). That should be possible in a binary compatible way.

For the use cases: I worked on integrating some ros_control hardware interfaces into combined_robot_hws (enabling e.g. using a cartesian controller over multiple pieces of hardware), which can be quite challenging without directly forking them (bringing them into a form loadable with parameterless constructor etc.). As an example, this was possible for ros_canopen, but only by creating a loadable RobotHW which created all required structures in its init() and forwards all calls to the RobotHW burried far inside some Layers of ros_canopen. It seemed reasonable to me that registering the inner hw as an interface manager should just "forward" all the interfaces and resources, but this doesn't happen right now (as an alternative, you can get the interfaces from the inner hw manually and register them yourself).

Probably more important then that, the current behavior doesn't seem intuitive and like the expected behavior (e.g. this function doesn't produce the correct result for a combined_robot_hw). Changing the behavior would also make things in the direction of #346 easier to implement (e.g. listing each interface with its resources).

I'll have time to provide a patch in the next days, for now i am still looking at which error behaviors there currently are (example: what happens when multiple interfaces try to provide the same resource). It also seems like the doxygen doc for InterfaceManager and some classes around it are a little bit out of date.

[ivan140]

@RobertWilbrandt I've stumbled also on the issue of the lacking compatibility of the ros_canopen package with the combine_hw required class_loader plugin. Do you, by any chance, have a branch somewhere with your solution?

This is the problematic part from the wiki where the problems start:
Note that init, read and write has parameters, they are strictly and has to be written like this. Note that constructor and destructor are parameterless.
Writing-CombinedRobotHW

[bmagyar]

@ivan140 you can find a branch with the solution here: #456 please let us know how you find it, I would like to merge it for noetic and perhaps melodic

[RobertWilbrandt]

@ivan140 Currently our ros_canopen adapter is only internal, i'll ask if i can share it. Basically it consists of a new RobotHW which does all of the startup code and then either uses registerInterface for all the interfaces of the ros_canopen-enclosed RobotHW (the RobotLayer class) or (after #456) just registerInterfaceManagers it. It is possible to implement this without a change to ros_canopen, but this requires a small code duplication. It is also still not a good solution (the internal control still runs at its own frequency etc), but good enough for what we needed it for.

[ivan140]

@RobertWilbrandt I created an issue/feature request in ros_canopen. Maybe you can add a comment there, since you're basically neighbours with the maintainers of that package.
@bmagyar I'd really like to test it, but I'm missing the canopen part for it, sadly.
My usecase is a tranport robot with a free rotating diff drive base in the middle and an encoder to detect the rotation of the base vs the body. The base itself uses canopen servo drives, whereas the encoder just uses a simple can protocol.

[RobertWilbrandt]

@bmagyar Is there something holding this back? It would be nice if registerInterfaceManagercould be used in ros_control_boilerplate for the scenario "combined_robot_hw call registerInterfaceManager on adapter calls registerInterfaceManager on SimHWInterface" (here it would be even better if this were in both melodic and noetic). Just let me know if there is some problem with the pr, otherwise forget i asked.

[RobertWilbrandt]

Resolved with merge of #456

[bmagyar]

FYI I also put releases out to Melodic and Noetic ;) Now we only need to wait for the next sync