Fix start_type_description_service param handling

[roncapat]

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

[jmachowinski]

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]

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

[jmachowinski]

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

[fujitatomoya]

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]

@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]

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.

[jmachowinski]

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]

@jmachowinski are those unrelated failures, right?

[roncapat]

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

[fujitatomoya]

@roncapat thanks for letting me know, lgtm.

[fujitatomoya]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[fujitatomoya]

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]

security related failures are unrelated to this PR.

[fujitatomoya]

@Mergifyio backport kilted jazzy

[mergify]

backport kilted jazzy

✅ Backports have been created

• #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