Keep domain id if ROS_DOMAIN_ID is invalid.

[hidmic]

Precisely what the title says. This pull request prevents rcl_get_default_domain_id() from forcing 0 for a domain id on failure cases. Found while working on ros2/rmw_implementation#106.

[hidmic]

I'll run cross-platform CI once I've addressed the failure with rmw_cyclonedds_cpp.

[wjwwood]

lgtm, with green CI

[ivanpauno]

@hidmic Consider backporting to Dashing, Eloquent and Foxy if applicable.

[hidmic]

CI up to rcl and test_rmw_implementation against all RMW implementations:

• Linux
• Linux-aarch64
• macOS
• Windows

[ivanpauno]

can a else {*domain_id = 0u;} be added instead of all the PRs addressing rmw implementations?

[wjwwood]

What if they don't want to use 0 as the default?

[ivanpauno]

What if they don't want to use 0 as the default?

We used to force that in the past, and a lot of the rmw implementations aren't handling the default correctly (thus the other PRs are needed).
I don't mind if we want to change this.

[ivanpauno]

After this PR, rcl_node_get_domain_id started to return SIZE_MAX when the environment variable is unset.

The purpose of that API was to return the actual domain id that the node was using (see here), but the "actual" domain id is now erroneous.

IMO, the behavior before this PR was correct, i.e. using 0 as the default domain id if the environment variable is not set.
I really don't see any value in letting the rmw implementation define the actual domain id, and adding a layer in rmw to query the default domain id actually used seems unnecessary.

@hidmic @wjwwood what do you think?

[hidmic]

IMO, the behavior before this PR was correct, i.e. using 0 as the default domain id if the environment variable is not set.

As I've stated elsewhere (here?) I disagree.

I really don't see any value in letting the rmw implementation define the actual domain id, and adding a layer in rmw to query the default domain id actually used seems unnecessary.

I wonder what's the use case for rcl_get_node_domain_id.

After this PR, rcl_node_get_domain_id started to return 0 when the environment variable is unset.

Hmm, how's that possible? I would expect it to be RCL_DEFAULT_DOMAIN_ID w/o further changes.

[wjwwood]

For this point:

There's absolutely no guarantee that rmw_init_options_init() will use RMW_DEFAULT_DOMAIN_ID as opposed to 42 or 5. Which would then break environment checks.

I think it must use RMW_DEFAULT_DOMAIN_ID. When the domain id is in the init options, that is just a default value, the user may override it. If the user wants the middleware to select the domain id, then RMW_DEFAULT_DOMAIN_ID should be used.

The purpose of rmw_init_options_init() was not to allow the middleware to specify a custom default domain id. Though it could have been that or still could be, though I don't think that makes sense... The user needs to be able to explicitly ask the middleware to pick a domain ID regardless of what is in the init options initially, and that is what the RMW_DEFAULT_DOMAIN_ID is about, from my perspective.

---

I think that we should remove the domain id from the node (node options, actual_domain_id, etc) and move it into the rmw context object. I think we should keep the RMW_DEFAULT_DOMAIN_ID sentinel value, and it can be set in the rmw init options (we could require middlewares initialize it to that value) and then it will always be resolved in the resulting rmw context object.

The reason for keeping the RMW_DEFAULT_DOMAIN_ID, in my opinion, is to let the domain id being used be configured in the middleware, maybe based on some middleware specific configuration file (similar to our _DEFAULT QoS settings we have) or as @hidmic said based on some dynamic heuristic like a pool of domain id's.

I do not think we need a function to query what the default domain ID would be if we initialized with RMW_DEFAULT_DOMAIN_ID (e.g. rmw_get_default_domain_id()), but I could be convinced otherwise. I think it would be sufficient to say you need to initialize the system in order to find out what the default is.

In rcl, if the environment variable ROS_DOMAIN_ID is set, then it should be honored, always. The only time the middleware would choose a domain id, is if the ROS_DOMAIN_ID is not set and the RMW_DEFAULT_DOMAIN_ID value is used, or if ROS_DOMAIN_ID is somehow set to the RMW_DEFAULT_DOMAIN_ID value.

I think rcl_get_default_domain_id() should be renamed probably, since it does not get the default, instead it gets the value from the environment variable if it is set otherwise it does nothing to the given size_t *.

[ivanpauno]

I think that we should remove the domain id from the node (node options, actual_domain_id, etc) and move it into the rmw context object. I think we should keep the RMW_DEFAULT_DOMAIN_ID sentinel value, and it can be set in the rmw init options (we could require middlewares initialize it to that value) and then it will always be resolved in the resulting rmw context object.

👍

The reason for keeping the RMW_DEFAULT_DOMAIN_ID, in my opinion, is to let the domain id being used be configured in the middleware, maybe based on some middleware specific configuration file (similar to our _DEFAULT QoS settings we have) or as @hidmic said based on some dynamic heuristic like a pool of domain id's.

👍

I do not think we need a function to query what the default domain ID would be if we initialized with RMW_DEFAULT_DOMAIN_ID (e.g. rmw_get_default_domain_id()), but I could be convinced otherwise. I think it would be sufficient to say you need to initialize the system in order to find out what the default is.

We currently don't have any way to introspect the domain_id that the rmw implementation set, we would need either:

• rmw_context_get_domain_id(rmw_context_t * node)
• rmw_node_get_domain_id(rmw_node_t * node)

the first one makes more sense.
We can call that function after initing the rmw_context in rcl, and store it in the corresponding rcl_context_t structure.

Does that make sense?

[wjwwood]

Why do we need a function in rmw to get that field? Could it not just be a public member of the rmw_context_t object? I mean either would work, I'm just wondering if there's a reason to hide this member internally or not.

Otherwise, lgtm.

---

One lingering doubt in my mind, should we leave the domain id in the node just so middlewares that choose to do it that way can still map domain id to nodes rather than contexts? I'm not sure it makes sense to do that, but I'm just asking if anyone thinks we'll need that at some point?

[ivanpauno]

Why do we need a function in rmw to get that field? Could it not just be a public member of the rmw_context_t object? I mean either would work, I'm just wondering if there's a reason to hide this member internally or not.

Yeah, a member variable also sounds good.

One lingering doubt in my mind, should we leave the domain id in the node just so middlewares that choose to do it that way can still map domain id to nodes rather than contexts? I'm not sure it makes sense to do that, but I'm just asking if anyone thinks we'll need that at some point?

My mental idea is that you have to create two contexts if you need different domain ids, regardless if the implementation can have a different domain id for each node or not.
Multi domain id applications are strange (mainly domain bridges?), and support for setting the domain id with node granularity doesn't seem to add much value.

[hidmic]

It's settled then!

My mental idea is that you have to create two contexts if you need different domain ids, regardless if the implementation can have a different domain id for each node or not.
Multi domain id applications are strange (mainly domain bridges?), and support for setting the domain id with node granularity doesn't seem to add much value.

I agree. I had my doubts about this at some point in time, but I see now those were unfounded.