Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the triggering of guard conditions. #504

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

clalancette
Copy link
Contributor

When a guard condition goes active, we have to remember to increase the trig_idx so we look at the next trigger. Otherwise, we can get into situations where we skip a triggered member.

This is a regression from #482 . This fixes #494 and should also fix ros2/rclcpp#2502 . @Crola1702 FYI.

This should also be backported to Jazzy.

When a guard condition goes active, we have to remember
to increase the trig_idx so we look at the next trigger.
Otherwise, we can get into situations where we skip a
triggered member.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

Here's regular CI on this:

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

But here is also a run with all RMWs except for CycloneDDS disabled:

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

@clalancette
Copy link
Contributor Author

clalancette commented Jul 10, 2024

Here's another try of CycloneDDS-only, with a fix in the CI scripts:

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

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/preparing-for-rolling-sync-2024-07-11/38526/2

@clalancette
Copy link
Contributor Author

clalancette commented Jul 11, 2024

Since we haven't run CI on just CycloneDDS in a long time, here is a run of it on rolling (i.e. without this PR), just to see if it matches what we saw above:

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

@clalancette
Copy link
Contributor Author

All right. Regular CI went swimmingly. CI on CycloneDDS only wasn't perfect, but it definitely showed less failures than without this PR. After poking at some of the other issues locally, I'm convinced that those are for other reasons.

So with all of that, and the approval, I'm going to go ahead and merge this, and also backport to Jazzy.

@clalancette clalancette merged commit 899bbdf into rolling Jul 11, 2024
2 checks passed
@clalancette clalancette deleted the clalancette/fix-triggering-guard-condition branch July 11, 2024 20:33
@clalancette
Copy link
Contributor Author

@Mergifyio backport jazzy

Copy link

mergify bot commented Jul 11, 2024

backport jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 11, 2024
When a guard condition goes active, we have to remember
to increase the trig_idx so we look at the next trigger.
Otherwise, we can get into situations where we skip a
triggered member.

Signed-off-by: Chris Lalancette <[email protected]>
(cherry picked from commit 899bbdf)
ahcorde pushed a commit that referenced this pull request Jul 12, 2024
When a guard condition goes active, we have to remember
to increase the trig_idx so we look at the next trigger.
Otherwise, we can get into situations where we skip a
triggered member.

Signed-off-by: Chris Lalancette <[email protected]>
(cherry picked from commit 899bbdf)

Co-authored-by: Chris Lalancette <[email protected]>
@claraberendsen
Copy link

@Mergifyio backport iron

Copy link

mergify bot commented Jul 19, 2024

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 19, 2024
When a guard condition goes active, we have to remember
to increase the trig_idx so we look at the next trigger.
Otherwise, we can get into situations where we skip a
triggered member.

Signed-off-by: Chris Lalancette <[email protected]>
(cherry picked from commit 899bbdf)

# Conflicts:
#	rmw_cyclonedds_cpp/src/rmw_node.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants