-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Controller interface now accepts yaml as node parameters #64
Conversation
Hm not sure if the ament_uncrustify errors are some kind of false-positive? Is it not happy with how the TODOs are only in one if-clause? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you envision this to be used? Do you see one yaml file per controller?
Do you envision that there is one yaml file per controller instance? How do you pass these parameters to a controller when being dynamically loaded?
Then further, why not using the ROS2 inbuilt functionality to pass parameters of a YAML file to a ros node? https://index.ros.org/doc/ros2/Tutorials/Node-arguments/#setting-parameters-from-yaml-files
But I generally agree that the parameter server has to go.
controller_interface/CMakeLists.txt
Outdated
|
||
add_library(controller_interface SHARED src/controller_interface.cpp) | ||
target_include_directories( | ||
controller_interface | ||
PRIVATE | ||
include) | ||
include | ||
"yaml_cpp_vendor") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is superfluous when also being specified as ament_target_dependencies
as seen below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 1256483
controller_interface/include/controller_interface/controller_interface.hpp
Outdated
Show resolved
Hide resolved
controller_interface/include/controller_interface/controller_interface.hpp
Show resolved
Hide resolved
controller_interface/include/controller_interface/controller_interface.hpp
Outdated
Show resolved
Hide resolved
Co-Authored-By: Karsten Knese <Karsten1987@users.noreply.github.com>
I wouldn't enforce a one yaml file per controller rule but it's a lazy option.
Again, no. I'm testing on a yaml file that has parameters for 2 controllers. Ideally, the user would do YAML::LoadFile to load the appropriate file, go deeper into the correct depth, and call the function with the YAML::Node argument to do the rest.
I spent a couple of hours on this and I wasn't able to get my parameter files to automatically supply parameters to the nodes. I suppose one would be able to grab yaml filenames from the arguments somehow? But if anyone wants to tackle this feel free to go on ahead. |
I may be able to help with the yaml file issue using inbuilt ROS2 parameters. I've done a lot with ROS2 yaml config and parameter parsing. I've run into some issues too including out-dated documentation.
|
…into control_interface_yaml
…into control_interface_yaml
@guru-florida
This is the file I'm using.
|
Looks like you are not specifying the node name perhaps? Try this, I added _-r _node:=node-name for the coffeebot_arm_effort_controller yaml desc:
Usually the node_name is added automatically by the python launch facility. |
I am still unable to get the correct parameter values. For more context, my c++ code makes a rclcpp::Node with the same name, then does node->has_parameter("type"), and it always returns false. I tried with these options:
And it still doesnt work. |
Do you declare the parameters as well? Also for reference, there might be a mismatch between |
I dont use the option you mention. Like you, I have had issues with Parameter getters. Here's some of my code with a bool value. The secret sauce seems to be supplying a default value in the declare and the get_value() part of get_parameter.
You say your c++ code "makes a Node with the same name", so is the node name coffeebot_custom_controller or coffeebot_arm_effort_controller and coffeebot_gripper_effort_controller? Since the ROS2 yaml files support config of multiple nodes the line before ros__parameters: much match the node name. |
FYI. I am not sure about your { .. } values on some of your yaml values. I think I tried this and it wouldnt transfer into Parameter objects since Parameters only work with primitive types. Take a look at the get_value() methods and you'll see whats supported. For example, arrays are supported. You can do parameter hierarchies using the parameter dotted name format. I.e. Here is my yaml file with the channel and gains.gravity. The { ... } syntax should be the same as what I have here, so possibly the dotted syntax works with your { ... } format too.
My python launch file:
|
Ok, it seems like the way to get my yaml parameters in is to use launch files via
I have 2 nodes named /coffeebot_arm_effort_controller. This link and this link are the problems I'm facing, we should fix them in another PR i think? Thanks @guru-florida ! |
You're welcome. I reviewed those links and I see the shortcoming there. :( I didnt encounter that but only because of luck I guess. I have a joint controller that spans 3 serial buses. I made 1 overall node in the process, but it instantiates 3 JointChannel classes that are basically Lifecycle nodes themselves, I even used the same method prototypes. Figured later I might refactor to be multiple nodes in a process. You could probably refactor a bit and have an initial ControlManager node that reads from yaml config params what nodes to create. It then creates the Node classes with the node name specified which will each also read their yaml config but with the right node-names. This means the launch file only specified the main controller node and the yaml does the rest. The launch can still manage lifecycle of all nodes though so nothing is really lost, just bringup is a bit different. You should still be able to use the ros2 run command. I am able to here. I did a |
@ddengster can you update on what's the status with this PR? Once more, I feel we should rely on From what I am reading here in this conversation, I have the impression that you have solved it that way, is that right? In that case, can we close this PR? |
Yup, let's close it. Anyway, we should cook up an example with controller_manager. |
* Removed empty test fixture class * Added trailing underscore to member variables * Common thresholds moved to global anonymous const variable * Moved redundant code to test functions * Split into smaller tests * Fix compilation after rebase -- needs more refactoring for new tests! * Setting parameters in declaration * Typo * New test now also use the common initialization function * Added const to lots of parts of the tests * Removed unncessary auxiliary vector * Replaced occurrences of EXPECT_EQ(true, ...) with EXPECT_TRUE(...) Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
2 new(rather, moved) functions that allow yaml to be parsed and fed into the lifecycle node of the controller; allowing our controllers to use them. One just takes in a filename, the other takes in an already custom-parsed yaml node.
If this goes in there shouldn't be any more reason to have controller_parameter_server as a seperate node, nor library nor class. We could use the testing code around though. Let me know if it goes against any of your philosophies.
Thanks Karsten for the initial yaml parsing code.