Open RoboticsYY opened 4 years ago
henningkayser
commented
4 years ago So, for now we are using this approach https://github.com/ros-planning/moveit2/pull/148#discussion_r360469027. It seems pretty straight forward to apply this for arbitrary maps. However, this would mean we would have to update all config files accordingly.
v4hn
commented
4 years ago @wjwwood please briefly confirm.
sensors:
sensors: ["sensor1", "sensor2"]
sensor1:
sensor_plugin: occupancy_map_monitor/PointCloudOctomapUpdater
...
....Listing the unknown parameters in a central parameter again is error-prone and not extendable (the user has to modify sensors, if they also want to include another sensor from a separate yaml file, breaking overloading and commit diffs*). It's not viable.
If I understand this answer by William right, we could require automatically_declare_parameters_from_overrides (which is already needed anyway), and use list_parameters({"sensors"},1) to retrieve the dynamic parameter names? I cannot test this locally.
Of course, even if this works, it would be much better to declare unknown parameters in this namespace and possibly with this substructure for a node, instead of declaring anything someone puts in a yaml file. How hard would it be to add something like that to the rclcpp implementation?
*can --params-file be used multiple times or is there some other way to set parameters from multiple yaml files? I do hope so...
henningkayser
commented
4 years ago @v4hn I totally feel your pain with this. It seems like the overall changes with the parameter interface is inteded to make everything more explicit (which is actually great imo) but it breaks a lot of use cases that were common with ROS1. Merging (or changing) multiple yaml files is definitely possible with launch files. However, I'm not sure if auto-declaration is really the behavior that we want here. I would prefer just referencing namespaces and plugin types and then let the plugins care about loading their declared parameters, possibly from a separate yaml file.
v4hn
commented
4 years ago is inteded to make everything more explicit (which is actually great imo)
I feel a bit like Norman (guess you know what I mean), but I'll still play the grumpy one here. It's not great, it's close to unusable in complex extensible systems like MoveIt. It forces you to use the "dirty and not recommended" approach of auto-registering everything the user copies somewhere. Personally I consider this the only reasonable way, but I understand that users might expect the framework to "do thinks correctly".
Think of things like ompl_planning.yaml.
The file is structured in the (quite intuitive) way of listing and configure any number of planner configurations. Or ros_controllers.yaml which specifies any number of external control interfaces.
If we do not want to be forced to use automatic declaration, we need to provide a patch to ros2 core that enable us to lookup and declare parameters from overloads that fulfill a certain structure.
Or at least top elements in a path /ns/* from overloads without declaring them yet (so that you can read the list, extract the controller names, and generate parameters for each of them).
I would expect such a feature to be required by others as well and to be welcome upstream.
wjwwood
commented
4 years ago If I understand this answer by William right, we could require
automatically_declare_parameters_from_overrides(which is already needed anyway), and uselist_parameters({"sensors"},1)to retrieve the dynamic parameter names? I cannot test this locally.
That's correct. I modified the parameter_blackboard demo like this:
diff --git a/demo_nodes_cpp/src/parameters/parameter_blackboard.cpp b/demo_nodes_cpp/src/parameters/parameter_blackboard.cpp
index 9170e92..39187e6 100644
--- a/demo_nodes_cpp/src/parameters/parameter_blackboard.cpp
+++ b/demo_nodes_cpp/src/parameters/parameter_blackboard.cpp
@@ -36,11 +36,31 @@ public:
options.allow_undeclared_parameters(true).
automatically_declare_parameters_from_overrides(true))
{
+ auto parameters_list = this->list_parameters(
+ {},
+ rcl_interfaces::srv::ListParameters::Request::DEPTH_RECURSIVE);
RCLCPP_INFO(
this->get_logger(),
"Parameter blackboard node named '%s' ready, and serving '%zu' parameters already!",
- this->get_fully_qualified_name(), this->list_parameters(
- {}, rcl_interfaces::srv::ListParameters::Request::DEPTH_RECURSIVE).names.size());
+ this->get_fully_qualified_name(), parameters_list.names.size());
+ RCLCPP_INFO(
+ this->get_logger(),
+ "Serving these parameters:");
+ for (const auto & parameter_name : parameters_list.names) {
+ RCLCPP_INFO(
+ this->get_logger(),
+ "- %s", parameter_name.c_str());
+ }
+ std::map<std::string, rclcpp::Parameter> sensor_parameters;
+ bool has_sensor_parameters = this->get_parameters("sensors", sensor_parameters);
+ if (has_sensor_parameters) {
+ RCLCPP_INFO(this->get_logger(), "Parameters in the 'sensors' namespace:");
+ for (const auto & parameter_pair : sensor_parameters) {
+ RCLCPP_INFO(this->get_logger(), "- %s", std::to_string(parameter_pair.second).c_str());
+ }
+ } else {
+ RCLCPP_INFO(this->get_logger(), "No parameters found in the 'sensors' namespace.");
+ }
}
};
And then it works like this:
william@kisra ~/ros2_ws
% cat input1.yaml
/**:
ros__parameters:
sensors:
sensor1:
key: value
sensor2:
other_key: other_value
new_key: new_value%
william@kisra ~/ros2_ws
% cat input2.yaml
/**:
ros__parameters:
sensors:
sensor3:
key: value
sensor2:
other_key: other_value%
william@kisra ~/ros2_ws
% ./install/demo_nodes_cpp/lib/demo_nodes_cpp/parameter_blackboard --ros-args --params-file ./input1.yaml --params-file ./input2.yaml
[INFO] [1590634805.125776959] [parameter_blackboard]: Parameter blackboard node named '/parameter_blackboard' ready, and serving '5' parameters already!
[INFO] [1590634805.126340025] [parameter_blackboard]: Serving these parameters:
[INFO] [1590634805.126376937] [parameter_blackboard]: - sensors.sensor1.key
[INFO] [1590634805.126398594] [parameter_blackboard]: - sensors.sensor2.new_key
[INFO] [1590634805.126416913] [parameter_blackboard]: - sensors.sensor2.other_key
[INFO] [1590634805.126435287] [parameter_blackboard]: - sensors.sensor3.key
[INFO] [1590634805.126453502] [parameter_blackboard]: - use_sim_time
[INFO] [1590634805.126535976] [parameter_blackboard]: Parameters in the 'sensors' namespace:
[INFO] [1590634805.126598206] [parameter_blackboard]: - {"name": "sensors.sensor1.key", "type": "string", "value": "value"}
[INFO] [1590634805.126629736] [parameter_blackboard]: - {"name": "sensors.sensor2.new_key", "type": "string", "value": "new_value"}
[INFO] [1590634805.126656159] [parameter_blackboard]: - {"name": "sensors.sensor2.other_key", "type": "string", "value": "other_value"}
[INFO] [1590634805.126682003] [parameter_blackboard]: - {"name": "sensors.sensor3.key", "type": "string", "value": "value"}
^C[INFO] [1590634807.372316619] [rclcpp]: signal_handler(signal_value=2)As I said in my ROS answer, you can also use the overrides directly if you'd prefer to parse them yourself, I mean you can see how the automatically_declare_parameters_from_overrides option works here:
It's not using any private methods or anything to do it.
Of course, even if this works, it would be much better to declare unknown parameters in this namespace and possibly with this substructure for a node, instead of declaring anything someone puts in a yaml file. How hard would it be to add something like that to the rclcpp implementation?
That would be harder I think, basically saying one namespace is automatically declared and allowed to be undefined, just from the interface you'd need to specify that programmatically. You could instead parse the overrides yourself, take the ones you want, declare them, and ignore the rest or pass the rest to the node or whatever you wanted to do.
*can --params-file be used multiple times or is there some other way to set parameters from multiple yaml files? I do hope so...
Yes, I did this in my example above as a demonstration.
but it breaks a lot of use cases that were common with ROS1.
We need those cases to be clearly reported on ros2/rclcpp so we can discuss them and adjust the API as needed. Sorry if you've already done that and we just haven't gotten to them yet, we're pretty far behind at this point. I think the parameter interface needs more work, e.g. the listing of parameters and checking for unexpected parameters in YAML files both need more work too.
It's not great, it's close to unusable in complex extensible systems like MoveIt. It forces you to use the "dirty and not recommended" approach of auto-registering everything the user copies somewhere.
Well, I don't consider it dirty to use those options, and those options exist exactly for dynamic or extensible arguments, so in those cases I think it's totally fine to use them. What's recommended is that if you know your parameters upfront, you should declare them first so they can be documented, checked for, and later so we can check for unexpected arguments, like typos or out of date names, etc...
Also, isn't this exactly what ROS 1 does? You'd take a YAML file from the user, load all of it indiscriminately into the parameter server and then query them... I don't see how this is different, except that they are local by default.
If you really want the ROS 1 way, then you can use the parameter blackboard demo I modified above, load all parameters to it, and then query them remotely from all of the nodes.
In each case I'm sure there are UX edges that need smoothing, but there's nothing technically infeasible here that I can see.
If we do not want to be forced to use automatic declaration, we need to provide a patch to ros2 core that enable us to lookup and declare parameters from overloads that fulfill a certain structure. Or at least top elements in a path /ns/* from overloads without declaring them yet (so that you can read the list, extract the controller names, and generate parameters for each of them). I would expect such a feature to be required by others as well and to be welcome upstream.
I mean, the easiest thing I could see is a "transfer" function of sorts that is given the overrides, allowed to declare parameters based on them, and then give the rest back for other transfer functions or finally giving the rest to the node for actual overrides.
You can already do that now by accessing the parameter overrides in the NodeOptions, and use them to declare parameters that you want and ignore the rest.
If there's a useful way to structure that in the rclcpp API that would be fine.
v4hn
commented
4 years ago Thanks for the elaborate response @wjwwood! Here you have some feedback from an excessive ROS1 user and one more followup question.
I modified the parameter_blackboard demo like this:
Looks like a way to implement it. Is there already a shortcut that yields only the list
{"sensor1", "sensor2", "sensor3"} from parameters (or better from overrides which you might want to declare as parameters) to explicitly iterate over each sensor namespace?
you can see how the automatically_declare_parameters_from_overrides option works here. It's not using any private methods or anything to do it.
That looks much better than exploiting the global automatically_declare flag and is probably the way to go about it for MoveIt. The only thing I would really expect in the upstream API to support this usage is the method I ask about above.
If there's a useful way to structure that in the rclcpp API that would be fine.
How about a method in the spirit of
vector<string> declare_aggregates("ns", {'required_key1", "required_key2"})
that would act on
/**:
ros__parameters:
ns:
sensor1:
required_key1: value
required_key2: value
other_key: other_value
sensor2:
required_key1: value
other_key: other_value(the /**: ros__parameters: syntax is way too bulky and looks like multiple typos in a row... Aside from also looking like a doxygen c++ comment...)
, declare `{"sensors.sensor1.required_key1", "sensors.sensor1.required_key2"}, and return {"sensor1"}?
Well, I don't consider it dirty to use those options, and those options exist exactly for dynamic or extensible arguments, so in those cases I think it's totally fine to use them
Sorry, I was under the impression automatically_declare_parameters_from_overrides requires allow_undeclared_parameters and you explicitly discouraged people using allow_undeclared_parameters in the past.
But apparently allow_undeclared_parameters could be rephrased as allow_get_parameter_for_undeclared_parameters instead of allow_undeclared_parameters_in_overrides. The latter was my interpretation and I guess
[...] and checking for unexpected parameters in YAML files
means that you do not support this yet. :smile:
Assuming declare_parameter were (is?) idempotent, having two distinct functions doing the same thing (at least if the parameter is set) is a rather redundant API...
Also, isn't this exactly what ROS 1 does?
Yes, with the huge UX difference that you did not have to set a parameter that is almost 50 characters long to a non-default value. If you do that in a framework context some user will obviously complain about it.
Sorry if you've already done that and we just haven't gotten to them yet
Give me a well-funded project and a PhD and I would actually be involved in these discussions a lot, writing patches. The way things are, I'm happy to have time to look at some of these issues in my spare time... I'm sure you've heard that answer numerous times over the last years. :slightly_smiling_face:
wjwwood
commented
4 years ago Is there already a shortcut that yields only the list {"sensor1", "sensor2", "sensor3"} from parameters (or better from overrides which you might want to declare as parameters) to explicitly iterate over each sensor namespace?
No, but it is just a bit of string manipulation. If there are good tools to do this, upstream could take them in a pr.
How about a method in the spirit of vector
declare_aggregates("ns", {'required_key1", "required_key2"})
Well there is already these functions:
I'm not 100% sure how the above works differently, maybe it throws if it isn't in the override? I'm not sure, so I'd need to see a pr with a new signature and a doc block covering the particulars to know if it makes sense or if it is redundant or something.
(the /**: ros__parameters: syntax is way too bulky and looks like multiple typos in a row... Aside from also looking like a doxygen c++ comment...)
Well, it was a compromise to handle all the issues that came up from supporting more than one node per process. You can avoid the /** if you put the actual node name in there, e.g. /my_node: ros__parameters: ....
means that you do not support this yet. 😄
No, unfortunately not, I have a branch with a prototype, but I never got to finish it. The problem is we don't know when the user is done declaring parameters and so we don't know when to start checking to see if any of the overrides were not used. The user would need to have a tail call to start that checking. So it will likely be an opt-in thing when it is supported.
Assuming declare_parameter were (is?) idempotent, having two distinct functions doing the same thing (at least if the parameter is set) is a rather redundant API...
I don't follow exactly? declare parameter is not idempotent, in the sense that it mutates the parameter state in the node, and calling it twice with the same arguments will result in it throwing an exception that you declared it already.
Yes, with the huge UX difference that you did not have to set a parameter that is almost 50 characters long to a non-default value. If you do that in a framework context some user will obviously complain about it.
I also don't follow this, sorry. 😅
v4hn
commented
4 years ago You can avoid the /** if you put the actual node name in there, e.g. /my_node
that still leaves you with two consecutive underscores and you can't do that if you want to pass these parameters to multiple nodes I guess (which is a damn common situation)...
But let's try not to extend this thread too much with general chatter. I believe this could go on for longer. :sweat_smile:
Looks like the solution for this thread is to
(1) fetch all sub-namespaces (substrings without .) that appear after a namespace (e.g., sensors.) from the parameter_overloads
and
(2) iterate over them declaring their expected sub-parameters (e.g., sensor_plugin) with the expected datatype and look up overload values.
while
(3) possibly failing if any of the required parameters are not found in the overloads.
And possibly come up with a nice new API for rclcpp that simplifies doing that.
For example:
https://github.com/ros-planning/moveit2/blob/056c5ac03cd9ea27198f8d8ce24a54b8178ccf01/moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor.cpp#L91-L169