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(Executor): Fixed entities not beeing executed after just beeing added #2724

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

jmachowinski
Copy link
Contributor

Fixes #2589

@alsora
Copy link
Collaborator

alsora commented Jan 10, 2025

CI

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

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, but one question

Is it important for just_waited to be set to false as we are doing, or could we just change the one place it is read to be if (!entities_need_rebuild_ && just_waited) {? From a purely semantic point of view, setting just_waited = false is kind not "true" because we did actually just wait, we just have another condition to consider now, which is what lead me to wonder why we don't just change the condition logic where we check for whether we just waited or not (line 401).

rclcpp/src/rclcpp/executor.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/executor.cpp Outdated Show resolved Hide resolved
@jmachowinski
Copy link
Contributor Author

I agree the name just_waited is now odd, I'll try to adopt it a bit...

@SteveMacenski
Copy link
Collaborator

@jmachowinski have you had a chance to work on the review comments? ❤️

@jmachowinski
Copy link
Contributor Author

@jmachowinski have you had a chance to work on the review comments? ❤️

Friday is my "Ros Day". I would still need a confirmation that this really fixes your bug though...

@jmachowinski
Copy link
Contributor Author

@wjwwood I renamed the just_wait bool to entity_states_fully_polled, so that the semantic should be clearer now.

@jmachowinski
Copy link
Contributor Author

Pulls: #2724
Gist: https://gist.githubusercontent.com/jmachowinski/c67ed34a1751f9eeb89029c25a289a3a/raw/61d3fc5dd6bf750db9a6ffe3b99ecbe7701e7d00/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15092

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

@jmachowinski
Copy link
Contributor Author

I would say the windows tests are unrelated, as they are mypy something.
@alsora @ahcorde @fujitatomoya can one of you merge ?

@fujitatomoya fujitatomoya merged commit e7afaa6 into ros2:rolling Jan 27, 2025
3 checks passed
@fujitatomoya
Copy link
Collaborator

@Mergifyio backport jazzy

Copy link
Contributor

mergify bot commented Jan 27, 2025

backport jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 27, 2025
…dded (#2724)

Fixes #2589

Signed-off-by: Janosch Machowinski <[email protected]>
Co-authored-by: Janosch Machowinski <[email protected]>
(cherry picked from commit e7afaa6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double spin required since 28.2.0
6 participants