Domain ID not changable on rclcpp::NodeOptions

[rotu]

rclcpp::NodeOptions has no way to set the Domain ID on a per-node basis, and it's kludgy to get the domain ID node_options->get_rcl_node_options()->domain_id. This functionality is exposed on rcl_node_options_t, so it's just a deficiency in the c++ API.

[wjwwood]

This makes sense to me, we'd appreciate anyone who has time to contribute this addition to the NodeOptions. We should consider doing it in rclpy at the same time, if it's not already there.

[suab321321]

@wjwwood sir I would like to contribute

[ivanpauno]

Note: When we finish with ros2/rmw#183, one domain id per node will have no more sense, and it will be one domain id per context.

[wjwwood]

That's a good point @ivanpauno. In that case maybe we should hold off on this until we have a little more clear direction on that set of changes.

@suab321321 it sounds like we'd like to hold off on this for now. Our intention is to follow through with ros2/rmw#183, in which case this issue will no longer make sense. There's a chance we change our mind, in which case this issue would make sense again, but that is unlikely in my opinion.

I added a note to the issue description of ros2/rmw#183 that we should follow up on this issue once we've committed to doing that work or not.

[suab321321]

@wjwwood okay

[fujitatomoya]

@wjwwood @ivanpauno

kinda off topic from this thread, but the same goes to rclpy ?

actually rclpy does not have support for ROS_DOMAIN_ID (ros2/rclpy#484). i think that ROS_DOMAIN_ID should be supported via rcl and frontend(rclxxx) can ask the domain id to rcl. (just to reduce redundancy of implementation.)

could you share your thoughts on this?

[ivanpauno]

actually rclpy does not have support for ROS_DOMAIN_ID (ros2/rclpy#484). i think that ROS_DOMAIN_ID should be supported via rcl and frontend(rclxxx) can ask the domain id to rcl. (just to reduce redundancy of implementation.)

The ROS_DOMAIN_ID logic is actually implemented in rcl, as you suggested.
If it's not working, it must be an error in how rclpy is wrapping rcl stuff.

[fujitatomoya]

@ivanpauno

The ROS_DOMAIN_ID logic is actually implemented in rcl, as you suggested.

sorry my bad, i should have checked.

https://github.com/ros2/rcl/blob/024c3ee50d9b79a85665efe1c191cc1225026519/rcl/src/rcl/node.c#L262-L281

and the question is why rclcpp has the same code to get the ROS_DOMAIN_ID?

rclcpp/rclcpp/src/rclcpp/node_options.cpp

Lines 325 to 355 in 0c66d0c

	// TODO(wjwwood): reuse rcutils_get_env() to avoid code duplication.
	// See also: https://github.com/ros2/rcl/issues/119
	size_t
	NodeOptions::get_domain_id_from_env() const
	{
	// Determine the domain id based on the options and the ROS_DOMAIN_ID env variable.
	size_t domain_id = std::numeric_limits<size_t>::max();
	char * ros_domain_id = nullptr;
	const char * env_var = "ROS_DOMAIN_ID";
	#ifndef _WIN32
	ros_domain_id = getenv(env_var);
	#else
	size_t ros_domain_id_size;
	_dupenv_s(&ros_domain_id, &ros_domain_id_size, env_var);
	#endif
	if (ros_domain_id) {
	uint32_t number = strtoul(ros_domain_id, NULL, 0);
	if (number == (std::numeric_limits<uint32_t>::max)()) {
	#ifdef _WIN32
	// free the ros_domain_id before throwing, if getenv was used on Windows
	free(ros_domain_id);
	#endif
	throw std::runtime_error("failed to interpret ROS_DOMAIN_ID as integral number");
	}
	domain_id = static_cast<size_t>(number);
	#ifdef _WIN32
	free(ros_domain_id);
	#endif
	}
	return domain_id;
	}

i believe that we could delete the redundant code in rclcpp?
if so, i will make the issue and PR against it.

[ivanpauno]

and the question is why rclcpp has the same code to get the ROS_DOMAIN_ID?

It's needed just for better introspection. Currently, the default node options have a domain id value of SIZEMAX. When that value is detected in the node creation, it's updated to the value indicated by the env variable. To be able to introspect correctly the domain id if you call get_rcl_node_options before creating a node, the value is also get from the environment in that case.

i believe that we could delete the redundant code in rclcpp?
if so, i will make the issue and PR against it.

Yes, the redundant code can be avoided.
As part of ros2/rcl#515, I'm creating an rcl_domain_id function, that allows you to get the default domain_id from the environment.
Code can easily be reused after that gets merged.

[fujitatomoya]

@ivanpauno

got it, i will get it done after your PR is merged.

thanks