Add forwarding of parameters from controller_manager to controllers #234
Conversation
Codecov Report
@@ Coverage Diff @@
## master #234 +/- ##
==========================================
- Coverage 34.61% 34.40% -0.22%
==========================================
Files 52 52
Lines 2981 3075 +94
Branches 1855 1924 +69
==========================================
+ Hits 1032 1058 +26
- Misses 310 320 +10
- Partials 1639 1697 +58
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| // This sleep is needed to prevent a too fast test from ending before the | ||
| // executor has began to spin, which causes it to hang | ||
| std::this_thread::sleep_for(50ms); |
There was a problem hiding this comment.
this screams for flakyness. Any way we can work around this 50ms? Might be 100ms on my OSX machine.
There was a problem hiding this comment.
I know, I haven't found any simple one. I was generous with the sleep (I needed a fraction of those 50ms), but I don't like either.
To me it's a bug, because the destructor of the spin future hangs without this every now and then.
There's nothing that I see on the Executor API to check if it's spinning. A way would be to add a timer or something and wait until it is triggered by the executor, but it's not going to be pretty.
There was a problem hiding this comment.
FYI I reported ros2/rclcpp#1454
And attempted the workaround I suggested, which lead to a segfault, reported in ros2/rclcpp#1455
For now I'm gonna leave the sleep here and we can revisit this in the future if we manage to fix this upstream.
There was a problem hiding this comment.
Do you think we can accept this as it is, as the rclcpp might be a long way ahead?
I don't like the sleeps either, specially since I was pushing changes to address ros-controls/ros2_controllers#27 but I can't find a nice solution to this right now.
Co-authored-by: Karsten Knese <[email protected]>
There was a problem hiding this comment.
I think that's a valid approach and should work as far as I can see this (haven't tested it locally though.)
One thing I'd like to see in a test is the behavior of the on_set_parameters_callback within a controller. I think that should be supported, but essentially to verify that a callback is being triggered within the controller if the controller manager calls set_parameter on the controller node.
| rclcpp::Parameter new_param; | ||
| std::string prefix; | ||
| std::tie(prefix, new_param) = get_controller_param(parameter); | ||
|
|
|
|
||
| return result; | ||
| } | ||
|
|
There was a problem hiding this comment.
can we add another test here which checks for a a callback on a change of parameters?
There was a problem hiding this comment.
Yeah, need to finish some things tomorrow, probably will be done by Friday.
|
As discussed in the ROS Control WG meeting, we're not going to go with this solution. Instead, the load service will have one extra parameter, that will specify the target state of the lifecycle node of the controller. Probably a choice of unconfigured, configured and active (or whatever the right lifecycle states names are). This allows to load a unconfigured controller, load the parameters later, and configure and start it though the regular lifecylce interface. |
|
Can you also add here how an example YAML should then look like? My proposal can be found here. |
|
Agreed with @v-lopez to close this in a direct call on Dec, 14th |
* Remove from ci.yaml * Remove controller also from linters-CI
fixes #168