Option for controller-specific duration parameters #785

2scholz · 2018-03-01T14:56:16Z

allowed_execution_duration_scaling and allowed_goal_duration_margin were previously set globally and could not be specified for individual controllers.
This patch makes it possible to define the values for controllers in the controller_list parameter (they can be specified in the controllers.yaml).
A short example:

controller_list:
 - name: example_controller
   type: FollowJointTrajectory
   allowed_execution_duration_scaling: 1.2
   allowed_goal_duration_margin: 0.5

The global value is still used whenever no controller-specific value was defined.

@v4hn

v4hn · 2018-03-02T12:31:46Z

I supervised the development of this patch.

These values are controller-specific to begin with, but nobody complained before that they could not be set for individual controllers...
In our use-case the execution time of our robotic gripper can deviate quite a lot from the planned trajectory, but the robotic arm it is attached to is very reliable.

Should not be cherry-picked because it breaks ABI.

Please review @davetcoleman @rhaschke (sorry Robert, I know you're busy :))

davetcoleman

Great improvement! Just a few cleanup feedback. Also please add to tutorials this new option, in this tutorial i believe:
http://docs.ros.org/kinetic/api/moveit_tutorials/html/doc/fake_controller_manager_tutorial.html

davetcoleman · 2018-03-29T14:50:49Z

...execution_manager/include/moveit/trajectory_execution_manager/trajectory_execution_manager.h

@@ -343,6 +343,10 @@ class TrajectoryExecutionManager
  bool execution_duration_monitoring_;
  double allowed_execution_duration_scaling_;


add comment above that these are the 'global' variables, in contrast to the controller specific ones

davetcoleman · 2018-03-29T14:52:31Z

moveit_ros/planning/trajectory_execution_manager/src/trajectory_execution_manager.cpp

@@ -129,6 +129,25 @@ void TrajectoryExecutionManager::initialize()
  else
    allowed_goal_duration_margin_ = DEFAULT_CONTROLLER_GOAL_DURATION_MARGIN;

+  XmlRpc::XmlRpcValue controller_list;


add comment above saying what this section does

also lets just move this into a private function loadControllerParams()

davetcoleman · 2018-03-29T14:53:10Z

moveit_ros/planning/trajectory_execution_manager/src/trajectory_execution_manager.cpp

+      // expected duration is the duration of the longest part
+      expected_trajectory_duration =
+          std::max(d * current_scaling + ros::Duration(current_margin), expected_trajectory_duration);
+    }


rhaschke

I agree to Dave's comments. Otherwise LGTM.

rhaschke · 2018-04-01T16:06:03Z

moveit_ros/planning/trajectory_execution_manager/src/trajectory_execution_manager.cpp

-    expected_trajectory_duration = expected_trajectory_duration * allowed_execution_duration_scaling_ +
-                                   ros::Duration(allowed_goal_duration_margin_);
+      // use controller-specific values if defined
+      const double current_scaling = controller_allowed_execution_duration_scaling_.count(context.controllers_[i]) > 0 ?


This is inefficient, as the map is searched two times: once for count() and once for access.
Better use find() to search once and subsequently compare the retrieved iterator against end().

2scholz · 2018-04-05T12:08:03Z

Thanks for the feedback. I added the requested changes to the commit and made a pull request to add the option in the tutorials (see moveit/moveit_tutorials#161)

rhaschke

I just noticed that the global parameters actually moved into the namespace trajectory_execution as stated here and are handled by dynamic reconfigure.

It feels strange, that the controller-specific values are not handled by dynamic reconfigure... I'm not sure dynamic reconfigure can handle a dynamic number of parameters. However, it's worth to check!

2scholz · 2018-04-06T13:54:45Z

The parameters handled by dynamic reconfigure need to be declared in the .cfg file.
There is an old open issue where the possibility to add parameters at runtime was discussed, but nothing came of it yet.

rhaschke · 2018-05-26T11:57:06Z

@2scholz Could you please rebase? Also, discuss/document the relationship of the controller-specific variables and their dynamically reconfigurable global counterparts.
I guess, controller-specific variables override any globals.

allowed_execution_duration_scaling and allowed_goal_duration_margin were previously set globally for every controller. Now it is possible to define these value for each controller specifically in the controller_list parameter. The global value is still used, in case no controller-specific value was defined.

2scholz · 2018-05-30T13:06:53Z

I rebased the branch.
I changed the comment in the header file above the controller-specific variables a bit to make it clearer that these override the global ones.
Additionally a description of the difference between the global and controller-specific values was added in the pull request for the documentation: moveit/moveit_tutorials#161

rhaschke

Thanks for rebasing. Please comment on my concerns regarding dynamic reconfigurability.

rhaschke · 2018-05-30T21:37:39Z

moveit_ros/planning/trajectory_execution_manager/src/trajectory_execution_manager.cpp


-    expected_trajectory_duration = expected_trajectory_duration * allowed_execution_duration_scaling_ +
-                                   ros::Duration(allowed_goal_duration_margin_);
+      // use controller-specific values if defined


Please add a comment here, that the global parameters are dynamically reconfigurable (and thus might change from call to call). If there are controller-specific values defined, they override any global parameters - even if set through dynamic reconfigure.

That's the point, I'm feeling still uncomfortable with. If the user has changed the global parameters, they will not take effect and the user isn't notified about that fact...
Did you considered to use the minimum of global and specific?

@rhaschke We were discussing over here where exactly you wanted to have more documentation and @2scholz added more explanation to the tutorial already: moveit/moveit_tutorials#161 .

I see your concerns about dynamic_reconfigure not working, but the only way to get around this is to patch dynamic_reconfigure and allow runtime variables in there. I don't like that very much.

Neither minimum nor maximum operator make sense in practice.

I think, there should be a reminder in the source code (at indicated line 1422), that controller-specific values will override dynamically configurable globals. I don't yet have a solution for this as well, but there should be a reminder in the source code if we come up with a solution later...

Eventually there should also be a warning (once) if global parameters have changed through dynamic reconfigure, but are overridden by controller-specific ones. All you need to do for this, is to keep track of the dyn-reconfig-callbacks (I think there is always one to set the initial value).

I just added a comment there.

addressed.

Additionally to the global duration parameters `trajectory_execution/allowed_execution_duration_scaling` and `trajectory_execution/allowed_goal_duration_margin`, allow for controller-specific overrides, named `allowed_execution_duration_scaling` and `allowed_goal_duration_margin`. As opposed to the global values, the contoller-specific ones cannot be dynamically reconfigured at runtime. The global value is still used, when no controller-specific value was defined.

…tching (moveit#785) * Update controller_manager_plugin to fix MoveIt-managed controller switching * add documentation about mutex * add a comment about how interface names are modified in-place

2scholz mentioned this pull request Mar 1, 2018

Set mode on the fly depending on chosen target TAMS-Group/robotiq_s_model_action_server#1

Merged

davetcoleman previously requested changes Mar 29, 2018

View reviewed changes

rhaschke requested changes Apr 1, 2018

View reviewed changes

2scholz force-pushed the controller_params branch from 546fbad to 66e2e36 Compare April 5, 2018 11:36

2scholz mentioned this pull request Apr 5, 2018

Add description how to add duration parameters in controller manager tutorial moveit/moveit_tutorials#161

Merged

rhaschke reviewed Apr 5, 2018

View reviewed changes

2scholz force-pushed the controller_params branch from 66e2e36 to c69164e Compare May 30, 2018 13:00

rhaschke requested changes May 30, 2018

View reviewed changes

TEM: add todo on static controller-specific monitoring parameters

22b27a3

rhaschke approved these changes Jul 4, 2018

View reviewed changes

rhaschke merged commit 43dd845 into moveit:kinetic-devel Jul 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option for controller-specific duration parameters #785

Option for controller-specific duration parameters #785

2scholz commented Mar 1, 2018

v4hn commented Mar 2, 2018

davetcoleman left a comment

davetcoleman Mar 29, 2018

davetcoleman Mar 29, 2018

davetcoleman Mar 29, 2018

rhaschke left a comment

rhaschke Apr 1, 2018

2scholz commented Apr 5, 2018

rhaschke left a comment

2scholz commented Apr 6, 2018

rhaschke commented May 26, 2018

2scholz commented May 30, 2018

rhaschke left a comment

rhaschke May 30, 2018

v4hn May 31, 2018

rhaschke May 31, 2018

v4hn Jun 6, 2018

		@@ -343,6 +343,10 @@ class TrajectoryExecutionManager
		bool execution_duration_monitoring_;
		double allowed_execution_duration_scaling_;

Option for controller-specific duration parameters #785

Option for controller-specific duration parameters #785

Conversation

2scholz commented Mar 1, 2018

v4hn commented Mar 2, 2018

davetcoleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2scholz commented Apr 5, 2018

rhaschke left a comment

Choose a reason for hiding this comment

2scholz commented Apr 6, 2018

rhaschke commented May 26, 2018

2scholz commented May 30, 2018

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment