Skip to content

Parameter Descriptor Simplification #2179

Merged
ahcorde merged 16 commits intoros2:rollingfrom
CursedRock17:parameter_descriptor
Dec 5, 2025
Merged

Parameter Descriptor Simplification #2179
ahcorde merged 16 commits intoros2:rollingfrom
CursedRock17:parameter_descriptor

Conversation

@CursedRock17
Copy link
Copy Markdown
Contributor

These changes are a direct response to issue #1660 which calls for the simplification of common parameter_descriptor objects.

  • Firstly, this first commit attempted to initialize a builder class for the ParameterDescription class. The references were mainly cpppatterns and refactoringguru to establish a correct definition of the builder class.
  • Target Intension: to solve redundancy in the creation of param_descriptor objects, specifically the float version which comes with a counter. The ParameterDescription class is meant to hold all of the standard information in ParameterDescriptor and then build up classes around it that can alter that information.

It may be reasonable to create convenient tools which allow for the definition of such a variable in much fewer steps:

and

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.

were the main tasks assigned in the creation in this so, to follow builder protocol I created a general first class, then secondary classes that contain the details for a counter such as: min, max, step. Then I created a function to handle the setting of these functions, where there had to be an overloaded version meant to hold templates given in the example of this->declare_parameter<double>("value", 0.5, param_descriptor); I assumed this had to be done because a Node must be initiated through this method, or a general version with a rclcpp::ParameterValue. Additionally, this was a function because the builder design patterns aims to stray away from massive constructors, which originally concerned me, because this PR was beginning to turn into that.

There are definetly improvements to be made to this PR, especially in the addition of more subclasses - at least an int version - but I wanted to get some early feedback.

namespace rclcpp
{

// Implments ParameterDesription class with builder design pattern
Copy link
Copy Markdown
Collaborator

@alsora alsora May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Implments ParameterDesription class with builder design pattern
// Implements ParameterDescription class with builder design pattern


//Builder Method - Mains
FloatingPointDescription& SetName(std::string name);
FloatingPointDescription& SetType(std::uint8_t type);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that many of these methods should be in a base class

//Builder Method - Mains
FloatingPointDescription& SetName(std::string name);
FloatingPointDescription& SetType(std::uint8_t type);
FloatingPointDescription& SetDescription(std::string description);
Copy link
Copy Markdown
Collaborator

@alsora alsora May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be something like "set description text", because the whole object is a description

Moreover use const reference to pass the string

ParameterDescription build() const;

//Builder Method - Mains
FloatingPointDescription& SetName(std::string name);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Const reference to pass the string


// Need the current node in order to begin the configuraiton state forit via the declare_parameter function which setups up the Node
template<typename ParameterType>
FloatingPointDescription& DeclareParameter<ParameterType>(ParameterType default_value, std::shared_ptr<rclcpp::Node> required_node)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't take a node, but rather the required node interfaces

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any implemented class inherits from the rclcpp::Node class shouldn't that class be able to fall into the rclcpp::Node parameter, or does this engage a bad practice we don't want to follow. The goal is to use the Node as a form of this, where the original issue suggests this code on something like line 48. Where this acts as the encasing class which inherits from rclcpp::Node seen on line 20 of that file.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not everything that creates a parameter is derived from rclcpp::Node; for example rclcpp_lifecycle::LifecycleNode doesn't.

The correct approach is to pass the list of node interfaces that you are going to need

Copy link
Copy Markdown
Contributor Author

@CursedRock17 CursedRock17 Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not everything that creates a parameter is derived from rclcpp::Node; for example rclcpp_lifecycle::LifecycleNode doesn't.

Currently in this pull request I have set up two implementations for the DeclareParameter function for both of those sited nodes, after digging around the codebase I saw an implementation here. Would it be better to follow in the form of template based programming like you had made in that pull request in order to increase viability of the function?

In our case an example would be :

template<typename ParameterType, typename NodeT> 

DeclareParameter(ParameterType default_value, std::shared_ptr<NodeT> required_node){
    // Implementation
}

FloatingPointDescription& SetMax(float max);
FloatingPointDescription& SetStep(float step);

// Simplification Method - Outside of the base clss that the user should be able to set up easily, they should then be able to call a function which setups up the specifics for this class (The floating point parameter description)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Simplification Method - Outside of the base clss that the user should be able to set up easily, they should then be able to call a function which setups up the specifics for this class (The floating point parameter description)
// Simplification Method - Outside of the base class that the user should be able to set up easily, they should then be able to call a function which setups the specifics for this class (The floating point parameter description)

// The main descriptor object we're meant to adjust
rcl_interfaces::msg::ParameterDescriptor parameter_descriptor = {};

// Copies all the information in ParameterDescriptor.msg - https://github.com/ros2/rcl_interfaces/blob/rolling/rcl_interfaces/msg/ParameterDescriptor.msg
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we copy all the information? Shouldn't we just store an internal parameter description message and return it as output of a "build" method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I interpret this as just returning the entirety of the parameter_descriptor message that we are currently using to store all the values, which makes the build method appropriate.

@alsora
Copy link
Copy Markdown
Collaborator

alsora commented May 2, 2023

I'm not convinced by the need for creating different classes for each type of constraints we may eventually have on the parameters.

Given that we only have 1 parameter description object, having 1 builder class should be sufficient.
We could have some alias/utilities to simplify the creation of some commonly used descriptions, but that's a different story

@CursedRock17
Copy link
Copy Markdown
Contributor Author

I may have failed to grasp the objective the original issue was trying to fix, I was just confused on the direction they were trying to take. Though, now that you've defined it a bit better I may understand, because beforehand it seemed convoluted to use a builder - which takes zero arguments but uses multiple method calls - to create a convience API. From what you've suggested, we should make a builder API with a bunch of small method calls and a large build that returns all the details, for just the ParameterDescription obejct. Then, create two smaller function calls for the Integers ranges highlighed in both the PR and ParameterDescriptor that work as a convience API.

FloatingPointDescription& SetMin(float min);
FloatingPointDescription& SetMax(float max);
FloatingPointDescription& SetStep(float step);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I roll out the next set of changes how would we like these three functions: SetMin, SetMax, and SetStep to act? There are multiple aspects to consider when deciding between simplicity and modularity.

Currently these three functions have no use, because the values they set are unused. In order to make them have purpose we would not only set their corresponding value, but also resize the type of range it's trying to target, if that range had not already been set. After we would then call parameter_descriptor.floating_point_range.at(0).corresponding_function. On the other hand we can opt for simplicity by removing the set functions and the values they alter all together, then in the functions that Set the Description Range we can use default values to remove possible arguments because the .msg field specifies bounds that we can have. On top of this discussion, should we also create Set functions for the int range, but with different names i.e SetMinInteger or should we aim to use overloading like SetMin(float/int min)

@CursedRock17
Copy link
Copy Markdown
Contributor Author

Now with the issue #2184, should both ranges in the functions SetFloatingPointDescriptionRange and SetIntegerDescriptionRange be available in the main class. Or would it be wise to extend the DeclareParameter function to check, during runtime, for the type and determine the validity of a range. Similarly we could use the parameter_descriptor.type to check as well.

@CursedRock17 CursedRock17 force-pushed the parameter_descriptor branch from d322451 to c703c9e Compare May 17, 2023 04:08
@CursedRock17
Copy link
Copy Markdown
Contributor Author

How important are tests to a simple builder class object? I was debating on adding tests but didn't know what they were going to look like.

Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CursedRock17 i think this is good enhancement, sorry we did not allocate time for the review. Do you plan to open another PR or find something missing in here? just checking.

@CursedRock17
Copy link
Copy Markdown
Contributor Author

I was planning on opening this up as an extension in the rcl_interfaces library as rclpy wouldn't be able to use this, but I think my closing of the branch was a little too quick. Should I instead reopen this one and do something similar for rclpy?

@CursedRock17 CursedRock17 reopened this Mar 25, 2024
ParameterDescription & SetDynamicTyping(bool dynamic_typing);
ParameterDescription & SetFloatingPointDescriptionRange(float min = 0.0f, float max = 1.0f,
float step = 0.0f);
ParameterDescription & SetIntegerDescriptionRange(int min = 0, int max = 1, int step = 0);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: I don't like having default values for these SetFloatingPointDescriptionRange and SetIntegerDescriptionRange APIs.
I would expect users to always set them all

return *this;
}

ParameterDescription & ParameterDescription::SetIntegerDescriptionRange(int min, int max, int step)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this method and SetFloatingPointDescriptionRange check the parameter type?
e.g. if the parameter is a string you shouldn't try to set these

@alsora
Copy link
Copy Markdown
Collaborator

alsora commented Mar 26, 2024

Overall looks good, left some minor comments.
We need unit-tests before proceeding

Copy link
Copy Markdown
Collaborator

@alsora alsora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with green CI

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before checking the implementation, i have a question. CC: @CursedRock17


TEST_F(TestParameterService, parameter_descriptor) {
{
rclcpp::ParameterDescription param_description;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting we were going to have the class ParameterDescription templated with ParameterType or something else to support syntactic sugar like following? So that it would be easier to create descriptor for the user application? current code almost provides the same experience with rcl_interfaces::msg::ParameterDescriptor?

Suggested change
rclcpp::ParameterDescription param_description;
rclcpp::ParameterDescription<PARAMETER_INTEGER>
param_description(
"int_parameter",
"description",
"constraints",
false,
start, end, step
);
node->declare_parameter(rclcpp::ParameterValue{1}, param_description.build());

above is just a pseudo cod. i might be mistaken on this, probably we want to get feedback from @wjwwood @gbalke .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this was an option, I went with the advice in the original issue, specifically:

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.

The original goal was to make the floating_point_description easier to use, that's why that part is set up in the form of a general function/constructor with all the arguments declared at once.

@alsora
Copy link
Copy Markdown
Collaborator

alsora commented Apr 2, 2024

@CursedRock17 can you rebase your branch on top of Rolling? I think that it's too old and it's causing CI failures

@CursedRock17 CursedRock17 force-pushed the parameter_descriptor branch from 13cb99e to f510db1 Compare April 2, 2024 04:15
@CursedRock17 CursedRock17 reopened this Apr 2, 2024
@CursedRock17 CursedRock17 force-pushed the parameter_descriptor branch 2 times, most recently from cd3a2cf to d6482f2 Compare April 2, 2024 04:29
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
This reverts commit 6431630.

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
This reverts commit 4bbcbe9.

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@CursedRock17 CursedRock17 force-pushed the parameter_descriptor branch from d6482f2 to 1482c88 Compare April 2, 2024 04:34
@alsora
Copy link
Copy Markdown
Collaborator

alsora commented Apr 3, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@alsora
Copy link
Copy Markdown
Collaborator

alsora commented Apr 6, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@CursedRock17
Copy link
Copy Markdown
Contributor Author

I should've commented this earlier, but I didn't expect the changes to pass as the includes don't work. I needed to include the header file in rclcpp/rclcpp.hpp since this feature isn't included in any other file.

These are the steps I followed to get the includes working, I don't think there's anything wrong now:

@ahcorde
Copy link
Copy Markdown
Contributor

ahcorde commented Nov 28, 2025

Pulls: #2179
Gist: https://gist.githubusercontent.com/ahcorde/bed35067b11ed1d48fc7b8153b5cfcbe/raw/c4da2bcd15992d6d51d192a2e8490cd208ac1fc3/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17648

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde ahcorde requested a review from fujitatomoya December 1, 2025 12:36
@ahcorde
Copy link
Copy Markdown
Contributor

ahcorde commented Dec 1, 2025

@fujitatomoya do you mind to take a look ?

@ahcorde ahcorde merged commit 41c7f68 into ros2:rolling Dec 5, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants