Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Handle RMW_DEFAULT_DOMAIN_ID. #427

Merged
merged 1 commit into from Jun 22, 2020
Merged

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jun 19, 2020

Precisely what the title says. Connected to ros2/rcl#689.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic requested review from wjwwood and ivanpauno June 19, 2020 20:24
@ivanpauno
Copy link
Member

why is this needed?
there's similar logic in rcl to avoid passing the default domain id IIRC

@ivanpauno
Copy link
Member

ivanpauno commented Jun 19, 2020

See ros2/rcl#689 (comment)

participant = dpf_->create_participant(
static_cast<DDS::DomainId_t>(domain_id), participant_qos, NULL,
static_cast<DDS::DomainId_t>(
domain_id != RMW_DEFAULT_DOMAIN_ID ? domain_id : 0u),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: line wrap looks unnecessary.

@hidmic
Copy link
Contributor Author

hidmic commented Jun 19, 2020

why is this needed? there's similar logic in rcl to avoid passing the default domain id IIRC

No there isn't. Or there shouldn't be. The bug that ros2/rcl#689 fixed was forcing a 0 when ROS_DOMAIN_ID=. The point of having an invalid domain ID as default is for rmw implementations to decide what to do with it. We can discuss whether we want to have such sentinel or not.

@hidmic
Copy link
Contributor Author

hidmic commented Jun 19, 2020

CI up to rcl and test_rmw_implementation against all RMW implementations:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

ivanpauno commented Jun 19, 2020

why is this needed? there's similar logic in rcl to avoid passing the default domain id IIRC

No there isn't. Or there shouldn't be. The bug that ros2/rcl#689 fixed was forcing a 0 when ROS_DOMAIN_ID=. The point of having an invalid domain ID as default is for rmw implementations to decide what to do with it. We can discuss whether we want to have such sentinel or not.

It has historically been used to determine in rcl if ROS_DOMAIN_ID variable has to be check or not (see https://github.com/ros2/rcl/blob/72965a0e3fa9ad0af6d0fe301a1d02edc746a1fb/rcl/src/rcl/init.c#L146-L147).
It has never been used for letting the rmw implementation decide what to do with the default domain id (same in foxy, eloquent, dashing, etc).

Not addressing ros2/rcl#689 (comment) will break rmw implementations not maintained by us.

@hidmic
Copy link
Contributor Author

hidmic commented Jun 19, 2020

It has historically been used to determine in rcl if ROS_DOMAIN_ID variable has to be check or not

True. In rcl, where it happens to be the same number. This patch is simply making sure that rmw_init() with default options as provided by the implementation does not fail.

Not addressing ros2/rcl#689 (comment) will break rmw implementations not maintained by us.

If an implementation does not handle RMW_DEFAULT_DOMAIN_ID, it is broken already.


IMHO the issue is that both domain ID defaults are conflated. rcl has a default to decide whether it has to check the environment or not, and rmw has a default with a sentinel value. If no ID is passed to rcl and the environment does no provide one either, the rmw default should be honored.

We can discuss the rmw default though. It could be 0, sure, if we think no rmw can benefit from knowing if that ID is the default and not a user-defined one that happens to have the same value.

@ivanpauno
Copy link
Member

If an implementation does not handle RMW_DEFAULT_DOMAIN_ID, it is broken already.

Currently, rcl never passes RMW_DEFAULT_DOMAIN_ID, so it's not already broken.

@hidmic
Copy link
Contributor Author

hidmic commented Jun 22, 2020

Currently, rcl never passes RMW_DEFAULT_DOMAIN_ID, so it's not already broken.

It is if the rmw can't handle it. And that's regardless of whether it's most common downstream package guards it from that situation or not.

@hidmic hidmic merged commit 802c7d2 into master Jun 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/handle-default-domain-id branch June 22, 2020 15:08
hidmic added a commit that referenced this pull request Jun 22, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
hidmic added a commit that referenced this pull request Jun 24, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants