Skip to content

Comments

feat: Added check for double usage of entities in rcl_waitset#1206

Open
jmachowinski wants to merge 1 commit intoros2:rollingfrom
cellumation:check_for_duplicated_entities
Open

feat: Added check for double usage of entities in rcl_waitset#1206
jmachowinski wants to merge 1 commit intoros2:rollingfrom
cellumation:check_for_duplicated_entities

Conversation

@jmachowinski
Copy link
Contributor

@jmachowinski jmachowinski commented Jan 17, 2025

Adding entities twice to a waitset is not allowed, and will introduce subtle bugs. Therefore the rcl_wait function will not explicitly check for duplicated entries in the waitset.

@wjwwood @mjcarroll @alsora This commit adds the check that we agreed on in the last client workgroup meeting

@jmachowinski jmachowinski force-pushed the check_for_duplicated_entities branch 3 times, most recently from f2041ac to 13ad17c Compare January 17, 2025 19:08
@jmachowinski
Copy link
Contributor Author

jmachowinski commented Feb 14, 2025

Pulls: #1206
Gist: https://gist.githubusercontent.com/jmachowinski/75562e3c9dd95204212810eff089a23e/raw/9813cf1dddd5efd5fb3ad11d7b432b8880f45ebd/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15195

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski jmachowinski force-pushed the check_for_duplicated_entities branch from 13ad17c to 53a0d40 Compare February 17, 2025 09:23
@jmachowinski
Copy link
Contributor Author

jmachowinski commented Feb 17, 2025

Pulls: #1206
Gist: https://gist.githubusercontent.com/jmachowinski/a1610420370b149cc98a22cbf6f19c8c/raw/9813cf1dddd5efd5fb3ad11d7b432b8880f45ebd/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15203

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski jmachowinski force-pushed the check_for_duplicated_entities branch from 53a0d40 to 1cdb729 Compare February 17, 2025 12:47
@jmachowinski
Copy link
Contributor Author

jmachowinski commented Feb 17, 2025

Pulls: #1206
Gist: https://gist.githubusercontent.com/jmachowinski/84c64ce2e17e4c2926602a6b879b4654/raw/9813cf1dddd5efd5fb3ad11d7b432b8880f45ebd/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15208

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski
Copy link
Contributor Author

jmachowinski commented Feb 18, 2025

This need to be merged after ros2/rclcpp#2714, as this contains the fix for the two identical guard conditions added by the executor.

@jmachowinski
Copy link
Contributor Author

jmachowinski commented Feb 19, 2025

Pulls: #1206
Gist: https://gist.githubusercontent.com/jmachowinski/9ba939d7e6f462f79c660af54e56ce56/raw/9813cf1dddd5efd5fb3ad11d7b432b8880f45ebd/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15225

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

fujitatomoya
fujitatomoya approved these changes Feb 26, 2025
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with green CI.

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Feb 26, 2025

https://ci.ros2.org/job/ci_windows/23361/testReport/junit/(root)/test_launch_ros/pytest_missing_result/ is unrelated.

@jmachowinski if you want to wait for more reviews on this, please do that. i will leave this to you.

alsora
alsora reviewed Mar 1, 2025
View reviewed changes
@alsora
Copy link
Contributor

alsora commented Mar 1, 2025 •
edited
Loading

Besides the minor request to add a comment, could we have a unit-test for this?
Then the PR would be ready to merge IMO

@jmachowinski jmachowinski force-pushed the check_for_duplicated_entities branch from 1cdb729 to d1f6983 Compare March 11, 2025 10:37
@jmachowinski
Copy link
Contributor Author

jmachowinski commented Mar 11, 2025

Added a test case for duplicate timer and duplicate guard condition. I didn't add tests for subscribers and services, as the boilerplate for setup would be a bit much and it would check the identical code path.

mjcarroll
mjcarroll approved these changes Mar 11, 2025
View reviewed changes
Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

LGTM and addresses @alsora's request.

One more CI and we are good to go here?

@alsora
Copy link
Contributor

alsora commented Mar 13, 2025

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@jmachowinski jmachowinski force-pushed the check_for_duplicated_entities branch from d1f6983 to ae7e9a3 Compare March 13, 2025 14:43
@jmachowinski
Copy link
Contributor Author

jmachowinski commented Mar 13, 2025

Pulls: #1206
Gist: https://gist.githubusercontent.com/jmachowinski/313bac2cf0368d362a8f4c204f8eaac4/raw/9813cf1dddd5efd5fb3ad11d7b432b8880f45ebd/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15365

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

ahcorde
ahcorde requested changes Oct 1, 2025
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Do you mind to merge with rolling?

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Oct 2, 2025

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Oct 2, 2025

rebase

❌ Pull request can't be updated with latest base branch changes

Details

Mergify needs the author permission to update the base branch of the pull request.
@jmachowinski needs to authorize modification on its head branch.

@ahcorde
Copy link
Contributor

ahcorde commented Nov 6, 2025

@jmachowinski Do you mind to merge with rolling?

feat: Added check for double usage of entities in rcl_waitset
e5479e2 
Adding entities twice to a waitset is not allowed, and will introduce
subtle bugs. Therefore the rcl_wait function will not explicitly check
for duplicated entries in the waitset.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
@jmachowinski jmachowinski force-pushed the check_for_duplicated_entities branch from ae7e9a3 to e5479e2 Compare November 24, 2025 15:19
@jmachowinski
Copy link
Contributor Author

jmachowinski commented Nov 24, 2025

Pulls: #1206
Gist: https://gist.githubusercontent.com/jmachowinski/cefd44fd73da75d97b83ffcbbf310e78/raw/5e77b5f58a8799938ececf247ecd30dc74004d2e/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17589

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahcorde ahcorde ahcorde requested changes

@mjcarroll mjcarroll mjcarroll approved these changes

@alsora alsora alsora approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

@mjcarroll mjcarroll

Labels

None yet

Projects

Kilted Kaiju
Status: Needs Review

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jmachowinski @fujitatomoya @alsora @ahcorde @mjcarroll