rcl icon indicating copy to clipboard operation
rcl copied to clipboard

Published 20 hours ago • verified publisher icon ros2

feat: Added check for double usage of entities in rcl_waitset

Open jmachowinski opened this issue 10 months ago • 3 comments

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 avatar Jan 17 '25 18:01 jmachowinski

Pulls: ros2/rcl#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 avatar Feb 14 '25 15:02 jmachowinski

Pulls: ros2/rcl#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 avatar Feb 17 '25 09:02 jmachowinski

Pulls: ros2/rcl#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 avatar Feb 17 '25 12:02 jmachowinski

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

jmachowinski avatar Feb 18 '25 12:02 jmachowinski

Pulls: ros2/rcl#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

jmachowinski avatar Feb 19 '25 13:02 jmachowinski

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.

fujitatomoya avatar Feb 26 '25 05:02 fujitatomoya

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

alsora avatar Mar 01 '25 11:03 alsora

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.

jmachowinski avatar Mar 11 '25 10:03 jmachowinski

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

alsora avatar Mar 13 '25 07:03 alsora

Pulls: ros2/rcl#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

jmachowinski avatar Mar 13 '25 14:03 jmachowinski

@Mergifyio rebase

fujitatomoya avatar Oct 02 '25 08:10 fujitatomoya

rebase

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

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

mergify[bot] avatar Oct 02 '25 08:10 mergify[bot]

@jmachowinski Do you mind to merge with rolling?

ahcorde avatar Nov 06 '25 20:11 ahcorde

Pulls: ros2/rcl#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

jmachowinski avatar Nov 24 '25 15:11 jmachowinski