WIP: Adds isResetRequired to RobotHW and combinedRobotHW

[carlosjoserg]

Hi all,

I've found common for a driver to require a re-arm routine after a fault, protective stop, or emergency stop. And most implementations add a public method asking whether controllers should be reset, which it is advisable after calling for a re-arm of any kind. Couple of examples are:

HAL: https://github.com/zultron/hal_ros_control/blob/kinetic-devel/hal_hw_interface/src/hal_control_loop.cpp#L138

Universal Robots: https://github.com/UniversalRobots/Universal_Robots_ROS_Driver/blob/master/ur_robot_driver/src/ros/hardware_interface_node.cpp#L144

Problem becomes when using such implementations in a combined manner, then access to that method is somehow difficult, but quite necessary to avoid re-launching everything.

Thus, I was wondering if it'd be good to have it by default (i.e. in the base robotHW class, so it can be called by the combinedRobotHW class), since the controller manager has the ability to do it, but it is the hardware interface the one able to tell when to.

The way to trigger the reset is setting the protected member bool reset_once_; to true. Solution is inspired by the implementation int he UR package. This will make the controller manager to call a start/stop in all controllers.

I believe this PR is similar to #357 and #294 but no consensus so far. I used different naming though and simpler way to do it IMHO.

Problem I see so far is that, working in combined mode, one hardware requiring the reset will trigger the reset of all controllers, no matter if some of them might not be plugged into that hardware.

Already tested it in custom hardware, just proposing the change upstream for more discussion...

PS: I'll add test coverage and fix typos in comments after a merge is considered ;)

[carlosjoserg]

One more example...

Franka combined hw (wrapper of combinedRobotHW) which adds to the base interface the bool controllersNeedReset() method.

However, if this is combined with another hw interface that also have the need to reset controllers via a custom method, it will be missed here.

[bmagyar]

Thanks for this! I personally like the idea but let's hear some other opinions as well:
Adding @ipa-mdl @toliver @jordan-palacios @christoph-jaehne and @matthew-reynolds for their 2 cents

[christoph-jaehne]

I like the idea of having a generic interface for resetting in the base classes. That would definitely ease the combinability with other hardware that require different reset routines.

[toliver]

It sounds like a good idea, but as you point out, it would also reset controllers that are unrelated to the RobotHw requiring the reset. This is the main issue I see to it.

You mention the example of UniversalRobots and I find it curious as they seem to have implemented two mechanisms that do the same thing in a different way.

The one that I had seen is the combination of the robot_program_running topic (that responds to the same event that raises the flag to reset controllers) and the controller stopper, that stops all the controllers that are not in the consistent_controllers list when the robot_program_running is False and starts them when it becomes True.
I personally would prefer a whitelist approach using a managed_controllers list instead of the current consistent_controllers.
But this approach offers the advantage that it allows the user to decide what controllers should react to the readiness state of a particular RobotHW and which ones should be left alone.

The disadvantage is that it does that via a ROS topic and therefore the resetting of the controllers would not be so immediate.
Also, as it is, it doesn't strictly allow to trigger a reset (or restart of the controllers), but instead it triggers a stop (when something goes wrong in the RobotHW) and then a start (when things go back to normal).

[carlosjoserg]

Hi, I'm gonna close it for the lack of a clear proposal from my side. I think the need to account for which controllers are to be reset upon a particular hwiface reset request is not trivial.

In our projects, we inherit from the combined hwiface class, and add the reset from there to all of our hwifaces (most of the combined are encoders and sensors that are not critical for a general reset). It'd had been nice to have it from binaries though.

From our point of view, if any of the hardware calls for a reset (usually after a fault on drives or similar), we actually prefer to reset all controllers anyway.

Thanks for the rich discussion, I hope it also helps others.