ros2 / rclcpp

rclcpp (ROS Client Library for C++)
Apache License 2.0
559 stars 422 forks source link

Parameter Descriptor simplification for generic types #1660

Open gbalke opened 3 years ago

gbalke commented 3 years ago

Feature request

Feature description

Simplify creation of parameter descriptors for generic types or add documentation to rosdocs (or both).

When trying to mimic behavior in ROS1 for dynamic reconfigure, I noticed there was no documentation on how to set up the parameter descriptors. The interface also ended up being quite strange/esoteric. After trying to find ROS documentation for some time I gave up and looked for sample code which I eventually found here: https://git.3dvisionlabs.com/3dvisionlabs/software/hemistereo/apps/stereo/ros2/-/blob/master/src/hemistereo/src/ros2_wrapper.cpp

I wanted to define a single floating point value with fixed step sizes and min/max values. This ended up with the descriptor declaration:

rcl_interfaces::msg::ParameterDescriptor param_descriptor = {};
param_descriptor.type = rcl_interfaces::msg::ParameterType::PARAMETER_DOUBLE;
// Generalized for lists?
param_descriptor.floating_point_range.resize(1);
param_descriptor.floating_point_range.at(0).from_value = 0.0f;
param_descriptor.floating_point_range.at(0).to_value = 1.0f;
param_descriptor.floating_point_range.at(0).step = 0.001f;
this->declare_parameter<double>("value", 0.5, param_descriptor);

This is quite strange and there is a lot of excess code.

Implementation considerations

While the reason for this being complicated likely comes from the generated messages, that doesn't mean the implementation needs to be messy. It may be reasonable to create convenient tools which allow for the definition of such a variable in much fewer steps:

rcl_interfaces::msg::FloatingPointDescription(args_for_default_description, min, max, step);

This could be a class structure that simply wraps the default constructor and mimics the message types? Regardless of if this is a good idea or not, I think documentation for generic type descriptors should be more available as dynamic_reconfigure's GUI interface is nowhere near as valuable without it.

wjwwood commented 3 years ago

I think a convenience API for this makes total sense.

It is generalized for lists and convoluted mostly to make it fit into generated message data, and not storing list and non-list separately (wire efficiency).

I'd recommend a builder pattern (https://cpppatterns.com/patterns/builder.html) for the API, so that if we add more to the descriptor in the future we won't have to add/subtract/reorder arguments to a function.

It could live in either rcl_interfaces as an additional header, or it could live in rclcpp, since that is where it is used in this fashion.

CursedRock17 commented 1 year ago

I opened a pull request to get a general idea of the layout of these classes, but there are some clarifications I would like to address.

It could live in either rcl_interfaces as an additional header, or it could live in rclcpp

First, I just put this as separate files in the rclcpp repository because they seemed to adequately fit with the other files in the main library. Additionally, the request of a convenience API was to create a function opposed to a large constructor correct? Otherwise a large constructor would deviate from the intensions of the builder class; to extend on this the builder class is meant for the earlier initialization of the param_descriptor object, with those default values like the type and name correct?