HW interface switch feature with unit tests #200

mathias-luedtke · 2015-03-18T14:26:50Z

As promised in #197 I have implemented a new version of the HW interface switch.

The RobotHW class got new members as discussed beforehand:

virtual bool canSwitch(const std::list<ControllerInfo> &start_list,
                       const std::list<ControllerInfo> &stop_list) const;

virtual void doSwitch(const std::list<ControllerInfo> &start_list,
                      const std::list<ControllerInfo> &stop_list);

These should check and execute the switches on the hardware.

The switchController filters the start and stop list as follows:

filter all restarts (make start_list and stop_list disjoint)
filter stops of stopped controllers (raise an error for strict checking)
filter starts (not restarts) of started controllers (raise an error for strict checking)

I have put a lot of effort into the unit tests, they should cover most cases, including the filters, resource and inter-controller mode conflicts.
The SwitchBot only allows direct (in one function call) switches if new_hwi >= old_hwi (string comparison).

@ipa-fxm will implement a gazebo plugin again, I will test it with https://github.com/ipa320/ros_canopen.

davetcoleman · 2015-03-18T15:34:30Z

+1 thanks for putting the effort into the unit tests. i'd really like to see this feature merged, its been nearly 2 years since i made the first implementation

adolfo-rt · 2015-03-20T14:00:23Z

controller_manager/src/controller_manager.cpp

@@ -440,6 +472,16 @@ bool ControllerManager::switchController(const std::vector<std::string>& start_c
    return false;
  }

+  if (!robot_hw_->canSwitch(start_list, stop_list))
+  {
+    ROS_ERROR("Could not switch controllers, due to mode conflicts");


I see that you try to maintain parallelism with the resource conflict message, but I find the error message not so clear. Now there's no explicit notion of modes, or what a mode conflict is. Here's some alternatives.

More explicit in blaming the robot for the failure:

"Could not switch controllers, requested controller state cannot be realized by robot"

Less explicit, more compact:

"Could not switch controllers, requested controller state is unfeasible"

Most verbose:

"Could not switch controllers. The resource combination for the requested controller state is unfeasible"

adolfo-rt · 2015-03-20T14:01:27Z

Please add your affiliation to the copyright statement of the affected files.

adolfo-rt · 2015-03-20T14:02:02Z

controller_manager/test/mode_switch_test.cpp

@@ -0,0 +1,380 @@
+#include <gtest/gtest.h>


Add license statement with copyright

adolfo-rt · 2015-03-20T14:40:13Z

All my comments are minor. A great +1 to this PR.

mathias-luedtke · 2015-03-20T15:07:14Z

Thanks for the review!
It will take some time to do all the beautifying (=find a suitable auto code styler ;)) and correct the c&p errors.

I will use "hardware interface switching" (as in the PR title) throughout the code and docs, because this is the view of the controller_manager.
"Mode switching" might be the necessary action for specific RobotHW implementations.

fmessmer · 2015-03-25T18:38:22Z

The gazebo plugin that uses the stricht_hwi_switch API proposed in this PR can be found here.
According documentation is included in the package README

mathias-luedtke · 2015-04-08T18:12:39Z

I have just updated the code based on your remarks.
I will squash the new commits after the final review; this way it is easier to track the changes.

adolfo-rt · 2015-04-17T15:25:42Z

This is looking good. Go ahead and squash the commits, and let's merge this one!

mathias-luedtke · 2015-04-17T15:41:42Z

Squashed :)

HW interface switch feature with unit tests

adolfo-rt · 2015-04-17T16:03:15Z

Merged, at last!

davetcoleman · 2015-04-17T18:54:11Z

I thought this would never happen!

fmessmer · 2015-04-18T10:25:31Z

Haha, awesome!

A Pull Request to ros_controls enforced STRICT controller switching to mean that already stopped controllers cannot be stopped again: ros-controls/ros_control#200 Passing in a value of STRICT to the controller manager didn't matter before because it was never properly implemented. According to the service Request definition: * STRICT means that switching will fail if anything goes wrong (an invalid controller name, a controller that failed to start, etc. ) * BEST_EFFORT means that even when something goes wrong with on controller, the service will still try to start/stop the remaining controllers We were relying on STRICT functionality not being fully implemented. Instead, we will use BEST_EFFORT for a more forgiving controller switching strategy. This design decision should be revisited in the development cycle.

As reported in Issue #49 **ros_control upgrade breaks baxter_gazebo simulation** A pull request to ros_controls enforced STRICT controller switching to mean that already stopped controllers cannot be stopped again: ros-controls/ros_control#200 This resulted in baxter_simulator being unable to start any controllers since it preemptively closes controllers whether or not they have been started. This was caused by the chosen strategy for switching controllers. Passing in a value of STRICT to the controller manager didn't matter before because it was never properly implemented. According to the service Request definition: * STRICT means that switching will fail if anything goes wrong (an invalid controller name, a controller that failed to start, etc. ) * BEST_EFFORT means that even when something goes wrong with on controller, the service will still try to start/stop the remaining controllers We were relying on STRICT functionality not being fully implemented. Instead, we will use BEST_EFFORT for a more forgiving controller switching strategy. This design decision should be revisited in the development cycle.

As reported in Issue #49 **ros_control upgrade breaks baxter_gazebo simulation** A pull request to ros_controls enforced STRICT controller switching to mean that already stopped controllers cannot be stopped again: ros-controls/ros_control#200 This resulted in baxter_simulator being unable to start any controllers since it preemptively closes controllers whether or not they have been started. This was caused by the chosen strategy for switching controllers. Passing in a value of STRICT to the controller manager didn't matter before because it was never properly implemented. According to the service Request definition: * STRICT means that switching will fail if anything goes wrong (an invalid controller name, a controller that failed to start, etc. ) * BEST_EFFORT means that even when something goes wrong with on controller, the service will still try to start/stop the remaining controllers We were relying on STRICT functionality not being fully implemented. Instead, we will use BEST_EFFORT for a more forgiving controller switching strategy. This design decision should be revisited in the development cycle. (cherry picked from commit 535cbb1)

adolfo-rt reviewed Mar 20, 2015
View reviewed changes

controller_manager/test/mode_switch_test.cpp

@@ -0,0 +1,380 @@

#include <gtest/gtest.h>

Copy link

Member

adolfo-rt Mar 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add license statement with copyright

mathias-luedtke mentioned this pull request Apr 17, 2015

Implementation of new HWI switching ros-industrial/ros_canopen#89

Merged

added HW interface switch feature with unit tests

e5b9581

mathias-luedtke force-pushed the strict_hwi_switch branch from c248255 to e5b9581 Compare April 17, 2015 15:39

adolfo-rt pushed a commit that referenced this pull request Apr 17, 2015

Merge pull request #200 from ipa-mdl/strict_hwi_switch

0f647a0

HW interface switch feature with unit tests

adolfo-rt merged commit 0f647a0 into ros-controls:indigo-devel Apr 17, 2015

fmessmer mentioned this pull request Apr 18, 2015

[WIP] HW interface switching revisited #197

Closed

fmessmer mentioned this pull request Apr 18, 2015

Use strict hwi switch API ipa320/cob_gazebo_plugins#6

Merged

fmessmer mentioned this pull request Apr 28, 2015

Move gazebo_ros_control into separate git repository? ros-simulation/gazebo_ros_pkgs#179

Closed

rethink-imcmahon mentioned this pull request May 21, 2015

ros_control upgrade breaks baxter_gazebo simulation RethinkRobotics/baxter_simulator#49

Closed

rethink-imcmahon mentioned this pull request May 21, 2015

Resolves #49 Use BEST_EFFORT to switch controller RethinkRobotics/baxter_simulator#50

Merged

This was referenced Jun 3, 2015

Update the xacro model to pass which interface to use CentroEPiaggio/kuka-lwr#19

Closed

[ENHANCEMENTS] multi-robot-test -> master CentroEPiaggio/kuka-lwr#18

Merged

adolfo-rt mentioned this pull request Aug 13, 2015

transmission_interface: Add new loading method #206

Merged

This was referenced Sep 4, 2015

[controller_manager] Fix execution point of doSwitch #211

Closed

Velocity Position Controller ros-controls/ros_controllers#131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HW interface switch feature with unit tests #200

HW interface switch feature with unit tests #200

mathias-luedtke commented Mar 18, 2015

davetcoleman commented Mar 18, 2015

adolfo-rt Mar 20, 2015

adolfo-rt commented Mar 20, 2015

adolfo-rt Mar 20, 2015

adolfo-rt commented Mar 20, 2015

mathias-luedtke commented Mar 20, 2015

fmessmer commented Mar 25, 2015

mathias-luedtke commented Apr 8, 2015

adolfo-rt commented Apr 17, 2015

mathias-luedtke commented Apr 17, 2015

adolfo-rt commented Apr 17, 2015

davetcoleman commented Apr 17, 2015

fmessmer commented Apr 18, 2015

HW interface switch feature with unit tests #200

HW interface switch feature with unit tests #200

Conversation

mathias-luedtke commented Mar 18, 2015

davetcoleman commented Mar 18, 2015

adolfo-rt Mar 20, 2015

Choose a reason for hiding this comment

adolfo-rt commented Mar 20, 2015

adolfo-rt Mar 20, 2015

Choose a reason for hiding this comment

adolfo-rt commented Mar 20, 2015

mathias-luedtke commented Mar 20, 2015

fmessmer commented Mar 25, 2015

mathias-luedtke commented Apr 8, 2015

adolfo-rt commented Apr 17, 2015

mathias-luedtke commented Apr 17, 2015

adolfo-rt commented Apr 17, 2015

davetcoleman commented Apr 17, 2015

fmessmer commented Apr 18, 2015