Rclcpp: 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 https://github.com/ros2/rmw/issues/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 (https://github.com/ros2/rclpy/issues/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?

https://github.com/ros2/rclcpp/blob/0c66d0c725ad1f977e2b907bc0619d6ceb6de238/rclcpp/src/rclcpp/node_options.cpp#L325-L355

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 https://github.com/ros2/rcl/pull/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

[fujitatomoya]

@ivanpauno

i got a question, since Context is introduced and mapped to dds participant, I think that domain id also should be mapped to Context (same as security) not only for NodeOptions.

i am considering the following,

• expose domain_id to NodeOptions
  default std::numeric_limits<size_t>::max() and ROS_DOMAIN_ID variable is set via get_domain_id_from_env(), but user can overwrite the domain_id if needed. actually rmw_fastrtps/cyclonedds does not use this domain_id via rmw_node_create, so this is for implementation that can support domain_id with node level.
• expose domain_id to InitOptions
  this is actually effective with dds implementation such as fastrtps or cyclonedds. default std::numeric_limits<size_t>::max() and ROS_DOMAIN_ID variable is set, and user can overwrite the domain_id if needed.

do you have any other thoughts?

[ivanpauno]

We shouldn't expose nothing in NodeOptions, as that will not continue to be used.
Exposing it in InitOptions is ok, but I would prefer to have a cleaner API (i.e.: not exposing that size_t max means "default").

class InitOptions
{
...
void use_default_domain_id();  // this will take the domain id from the env
void set_domain_id(size_t domain_id); 
size_t get_domain_id() const; 
...
}

[fujitatomoya]

I do agree with you, so maybe it is better to clean anything related to ROS_DOMAIN_ID stuff from NodeOptions. and we also update docs (if exist) explicitly about domain id with Context for user.

[fujitatomoya]

@suab321321

sorry i missed it, are you up for this?

[ivanpauno]

I do agree with you, so maybe it is better to clean anything related to ROS_DOMAIN_ID stuff from NodeOptions. and we also update docs (if exist) explicitly about domain id with Context for user.

If you want to clean that up, it's welcomed too! The only reference too domain_id in node related API is in rmw IIRC.
Exposing the domain id here is not related with that change, so it can be done separately.

[fujitatomoya]

@ivanpauno

either me or someone else from our team can work on this, you can leave this on us. though, we would require your review for sure.