Fix `start_type_description_service` param handling by roncapat · Pull Request #2897 · ros2/rclcpp

roncapat · 2025-07-09T09:24:41Z

Closes #2892 by using the same exact strategy for declaring use_sim_time.

rclcpp/rclcpp/src/rclcpp/time_source.cpp

Lines 254 to 274 in 7ebc9e4

    
           rclcpp::ParameterValue use_sim_time_param; 
        
           const std::string use_sim_time_name = "use_sim_time"; 
        
           if (!node_parameters_->has_parameter(use_sim_time_name)) { 
        
             use_sim_time_param = node_parameters_->declare_parameter( 
        
               use_sim_time_name, 
        
               rclcpp::ParameterValue(false)); 
        
           } else { 
        
             use_sim_time_param = node_parameters_->get_parameter(use_sim_time_name).get_parameter_value(); 
        
           } 
        
           if (use_sim_time_param.get_type() == rclcpp::PARAMETER_BOOL) { 
        
             if (use_sim_time_param.get<bool>()) { 
        
               parameter_state_ = SET_TRUE; 
        
               clocks_state_.enable_ros_time(); 
        
               create_clock_sub(); 
        
             } 
        
           } else { 
        
             RCLCPP_ERROR( 
        
               logger_, "Invalid type '%s' for parameter 'use_sim_time', should be 'bool'", 
        
               rclcpp::to_string(use_sim_time_param.get_type()).c_str()); 
        
             throw std::invalid_argument("Invalid type for parameter 'use_sim_time', should be 'bool'"); 
        
           }

Tested (you can try to define start_type_description_service like so, and pass it to Nav2 bt_navigator or diagnostic_aggregator):

/**:
  ros__parameters:
    use_sim_time: false
    start_type_description_service: false

whithout the patch, it crashes like ros/diagnostics#519.

Add test

Did you use Generative AI?

No

rclcpp/src/rclcpp/node_interfaces/node_type_descriptions.cpp

jmachowinski · 2025-07-09T14:19:33Z

can you fix the DCO ? afterwards I will start the CI run.

@fujitatomoya can you have a second look, I seem to miss a lot currently ;-)

roncapat · 2025-07-09T14:38:01Z

@jmachowinski linted, squashed and signed. Thank you for the review :)

jmachowinski · 2025-07-09T14:49:41Z

Pulls: #2897
Gist: https://gist.githubusercontent.com/jmachowinski/11e9cd91cc3356d1b1025dbd5477f75e/raw/5329df1d35568b558b8e03f68247f146b458d39f/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16441

Linux
Linux-aarch64
Linux-rhel
Windows

rclcpp/test/rclcpp/node_interfaces/test_node_type_descriptions.cpp

fujitatomoya · 2025-07-10T07:12:08Z

Pulls: #2897
Gist: https://gist.githubusercontent.com/fujitatomoya/36059be8eac06e935cd0e300025b59b1/raw/5329df1d35568b558b8e03f68247f146b458d39f/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/16446

Linux
Linux-aarch64
Linux-rhel
Windows

roncapat · 2025-07-10T07:21:10Z

@fujitatomoya please wait, I noticed one thing I still want to fix.
Please have a look at the updated test case.

We get two different exceptions depending on whether we use automatically_declare_parameters_from_overrides or not. IMHO we should always throw InvalidParameterTypeException. What do you think?

What happens now:

ASSERT_THROW(
{
  auto node = std::make_shared<rclcpp::Node>("node", "ns", node_options);
  (void) node;
}, rclcpp::exceptions::InvalidParameterTypeException);

ASSERT_THROW(
{
  node_options.automatically_declare_parameters_from_overrides(true);
  auto node = std::make_shared<rclcpp::Node>("node", "ns", node_options);
  (void) node;
}, std::invalid_argument);

roncapat · 2025-07-10T13:26:37Z

With c2ca00d, I made both cases automatically_declare_parameters_from_overrides (true/false) throw the same exception, and moreover the same exact exception message:

parameter 'start_type_description_service' has invalid type: Wrong parameter type,
parameter {start_type_description_service} is of type {bool}, setting it to {string} is not allowed.

This message, in one of the two codepaths, is generated by:

rclcpp/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp

Lines 299 to 311 in c2ca00d

    
           static 
        
           std::string 
        
           format_type_reason( 
        
             const std::string & name, const std::string & old_type, const std::string & new_type) 
        
           { 
        
             std::ostringstream ss; 
        
             // WARN: A condition later depends on this message starting with "Wrong parameter type", 
        
             // check `declare_parameter` if you modify this! 
        
             ss << "Wrong parameter type, parameter {" << name << "} is of type {" << old_type << 
        
               "}, setting it to {" << new_type << "} is not allowed."; 
        
             return ss.str(); 
        
           }

but unfortunately this method is static, so I copied the snipped here inline, so that when I manually throw, the same exact text is generated.

rclcpp/src/rclcpp/node_interfaces/node_type_descriptions.cpp

jmachowinski · 2025-07-11T08:36:41Z

Pulls: #2897
Gist: https://gist.githubusercontent.com/jmachowinski/5f8b63ffd74b1bd31a130924c950ba83/raw/5329df1d35568b558b8e03f68247f146b458d39f/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16467

Linux
Linux-aarch64
Linux-rhel
Windows

roncapat · 2025-07-14T07:42:13Z

@jmachowinski are those unrelated failures, right?

roncapat · 2025-07-16T11:28:44Z

@fujitatomoya @jmachowinski friendly ping on this, is it ok for merge?

fujitatomoya

@roncapat thanks for letting me know, lgtm.

fujitatomoya · 2025-07-16T16:14:13Z

@Mergifyio rebase

Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>

mergify · 2025-07-16T16:14:24Z

rebase

✅ Branch has been successfully rebased

fujitatomoya · 2025-07-16T16:15:00Z

Pulls: #2897
Gist: https://gist.githubusercontent.com/fujitatomoya/3cb75d1938440af27724a32d32c1fa32/raw/5329df1d35568b558b8e03f68247f146b458d39f/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/16500

Linux
Linux-aarch64
Linux-rhel
Windows

fujitatomoya · 2025-07-16T22:45:36Z

security related failures are unrelated to this PR.

fujitatomoya · 2025-07-16T22:48:30Z

@Mergifyio backport kilted jazzy

mergify · 2025-07-16T22:48:38Z

backport kilted jazzy

✅ Backports have been created

Details

#2908 Fix start_type_description_service param handling (backport #2897) has been created for branch kilted
#2909 Fix start_type_description_service param handling (backport #2897) has been created for branch jazzy

* Fix `start_type_description_service` param handling Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> * Add test Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> * Demonstrate different exceptions depending on node options Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> * Same exact exception and `what()` message in both cases Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> * Uncrustify Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> --------- Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> (cherry picked from commit 4fb558a)

* Fix `start_type_description_service` param handling * Add test * Demonstrate different exceptions depending on node options * Same exact exception and `what()` message in both cases * Uncrustify --------- (cherry picked from commit 4fb558a) Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> Co-authored-by: Patrick Roncagliolo <ronca.pat@gmail.com>

…#2909) * Fix `start_type_description_service` param handling * Add test * Demonstrate different exceptions depending on node options * Same exact exception and `what()` message in both cases * Uncrustify --------- (cherry picked from commit 4fb558a) Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> Co-authored-by: Patrick Roncagliolo <ronca.pat@gmail.com>

* QoSInitialization::from_rmw does not validate invalid history policy values, leading to silent failures (ros2#2841) (ros2#2845) (cherry picked from commit 73e9bfb) Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com> Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com> * get_all_data_impl() does not handle null pointers properly, causing segmentation fault (backport ros2#2840) (ros2#2851) Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com> Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com> * Added missing chrono includes (ros2#2854) (ros2#2856) (cherry picked from commit 373a63c) Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com> Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com> * Fix for memory leaks in rclcpp::SerializedMessage (ros2#2861) (ros2#2864) (cherry picked from commit 8d44b95) Signed-off-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Michael Orlov <morlovmr@gmail.com> Co-authored-by: kylemarcey <marcey.kyle@gmail.com> * Replace std::default_random_engine with std::mt19937 (humble) (ros2#2847) (ros2#2867) (cherry picked from commit a0e2240) Signed-off-by: keeponoiro <keeeeeeep@gmail.com> Co-authored-by: keeponoiro <keeeeeeep@gmail.com> * Changelog Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com> * 28.1.10 * fix test_publisher_with_system_default_qos. (ros2#2881) (ros2#2883) (cherry picked from commit e6577c6) Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> * Shutdown deadlock fix jazzy (ros2#2887) * fix: Don't deadlock if removing shutdown callbacks in a shutdown callback Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * refactor: Made fix API compatible Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> --------- Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com> * Event exec timer fix for ros2#2889 (ros2#2890) Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Signed-off-by: Janosch Machowinski <jmachowinski@users.noreply.github.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com> * Add overload of `append_parameter_override` (ros2#2891) (ros2#2895) (cherry picked from commit fa0cf2d) Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> Co-authored-by: Patrick Roncagliolo <ronca.pat@gmail.com> * Fujitatomoya/test append parameter override (ros2#2896) (ros2#2900) (cherry picked from commit 84c6fb1) Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> * [jazzy] Expose `typesupport_helpers` API needed for the Rosbag2 (backport ros2#2858) (ros2#2902) * Expose `typesupport_helpers` API needed for the Rosbag2 (ros2#2858) * Expose extract_type_identifier and get_typesupport_library_path API - Rationale: We need to use this API in the Rosbag2 - Reference PR ros2/rosbag2#2017 in the Rosbag2 Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Use C++ style in doxygen documentation Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai> (cherry picked from commit 448287b) # Conflicts: # rclcpp/include/rclcpp/typesupport_helpers.hpp * Address merge conflicts Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Michael Orlov <morlovmr@gmail.com> * Add qos parameter for wait_for_message function (ros2#2903) (ros2#2906) (cherry picked from commit 2fcef70) Signed-off-by: Sriharsha Ghanta <ghanta1996@gmail.com> Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com> Co-authored-by: Sriharsha Ghanta <ghanta_sriharsha@mymail.sutd.edu.sg> Co-authored-by: Alejandro Hernandez Cordero <ahcorde@gmail.com> * Fix `start_type_description_service` param handling (ros2#2897) (ros2#2909) * Fix `start_type_description_service` param handling * Add test * Demonstrate different exceptions depending on node options * Same exact exception and `what()` message in both cases * Uncrustify --------- (cherry picked from commit 4fb558a) Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> Co-authored-by: Patrick Roncagliolo <ronca.pat@gmail.com> --------- Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com> Signed-off-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Signed-off-by: keeponoiro <keeeeeeep@gmail.com> Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Signed-off-by: Janosch Machowinski <jmachowinski@users.noreply.github.com> Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com> Signed-off-by: Sriharsha Ghanta <ghanta1996@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com> Co-authored-by: Michael Orlov <morlovmr@gmail.com> Co-authored-by: kylemarcey <marcey.kyle@gmail.com> Co-authored-by: keeponoiro <keeeeeeep@gmail.com> Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Co-authored-by: Janosch Machowinski <jmachowinski@users.noreply.github.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Patrick Roncagliolo <ronca.pat@gmail.com> Co-authored-by: Sriharsha Ghanta <ghanta_sriharsha@mymail.sutd.edu.sg>

jmachowinski requested changes Jul 9, 2025

View reviewed changes

rclcpp/src/rclcpp/node_interfaces/node_type_descriptions.cpp Show resolved Hide resolved

roncapat commented Jul 9, 2025

View reviewed changes

rclcpp/src/rclcpp/node_interfaces/node_type_descriptions.cpp Outdated Show resolved Hide resolved

roncapat commented Jul 9, 2025

View reviewed changes

rclcpp/src/rclcpp/node_interfaces/node_type_descriptions.cpp Outdated Show resolved Hide resolved

roncapat requested a review from jmachowinski July 9, 2025 13:43

jmachowinski approved these changes Jul 9, 2025

View reviewed changes

roncapat force-pushed the patch-4 branch from bf9eabc to 4f8d10f Compare July 9, 2025 14:37

fujitatomoya reviewed Jul 9, 2025

View reviewed changes

rclcpp/test/rclcpp/node_interfaces/test_node_type_descriptions.cpp Show resolved Hide resolved

fujitatomoya approved these changes Jul 10, 2025

View reviewed changes

roncapat requested review from fujitatomoya and jmachowinski July 10, 2025 13:27

jmachowinski approved these changes Jul 10, 2025

View reviewed changes

jmachowinski reviewed Jul 10, 2025

View reviewed changes

rclcpp/src/rclcpp/node_interfaces/node_type_descriptions.cpp Outdated Show resolved Hide resolved

roncapat force-pushed the patch-4 branch from ac1a294 to afec770 Compare July 10, 2025 14:27

jmachowinski approved these changes Jul 11, 2025

View reviewed changes

fujitatomoya approved these changes Jul 16, 2025

View reviewed changes

roncapat added 5 commits July 16, 2025 16:14

Fix start_type_description_service param handling

2f95463

Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>

Add test

677d9a1

Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>

Demonstrate different exceptions depending on node options

c7d8192

Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>

Same exact exception and what() message in both cases

8c7b84a

Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>

Uncrustify

fbacdc3

Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>

fujitatomoya force-pushed the patch-4 branch from 7180206 to fbacdc3 Compare July 16, 2025 16:14

fujitatomoya merged commit 4fb558a into ros2:rolling Jul 16, 2025
3 checks passed

mergify bot mentioned this pull request Jul 16, 2025

Fix start_type_description_service param handling (backport #2897) #2908

Merged

1 task

mergify bot mentioned this pull request Jul 16, 2025

Fix start_type_description_service param handling (backport #2897) #2909

Merged

1 task

	rclcpp::ParameterValue use_sim_time_param;
	const std::string use_sim_time_name = "use_sim_time";
	if (!node_parameters_->has_parameter(use_sim_time_name)) {
	use_sim_time_param = node_parameters_->declare_parameter(
	use_sim_time_name,
	rclcpp::ParameterValue(false));
	} else {
	use_sim_time_param = node_parameters_->get_parameter(use_sim_time_name).get_parameter_value();
	}
	if (use_sim_time_param.get_type() == rclcpp::PARAMETER_BOOL) {
	if (use_sim_time_param.get<bool>()) {
	parameter_state_ = SET_TRUE;
	clocks_state_.enable_ros_time();
	create_clock_sub();
	}
	} else {
	RCLCPP_ERROR(
	logger_, "Invalid type '%s' for parameter 'use_sim_time', should be 'bool'",
	rclcpp::to_string(use_sim_time_param.get_type()).c_str());
	throw std::invalid_argument("Invalid type for parameter 'use_sim_time', should be 'bool'");
	}

Conversation

roncapat commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Did you use Generative AI?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmachowinski commented Jul 9, 2025

Uh oh!

roncapat commented Jul 9, 2025

Uh oh!

jmachowinski commented Jul 9, 2025

Uh oh!

Uh oh!

fujitatomoya commented Jul 10, 2025

Uh oh!

roncapat commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roncapat commented Jul 10, 2025

Uh oh!

Uh oh!

jmachowinski commented Jul 11, 2025

Uh oh!

roncapat commented Jul 14, 2025

Uh oh!

roncapat commented Jul 16, 2025

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

fujitatomoya commented Jul 16, 2025

Uh oh!

mergify bot commented Jul 16, 2025

✅ Branch has been successfully rebased

Uh oh!

fujitatomoya commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujitatomoya commented Jul 16, 2025

Uh oh!

Uh oh!

fujitatomoya commented Jul 16, 2025

Uh oh!

mergify bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

roncapat commented Jul 9, 2025 •

edited

Loading

roncapat commented Jul 10, 2025 •

edited

Loading

fujitatomoya commented Jul 16, 2025 •

edited

Loading

mergify bot commented Jul 16, 2025 •

edited

Loading