Skip to content

fix(Executor): Fixed entities not beeing executed after just beeing added#2724

Merged
fujitatomoya merged 1 commit intoros2:rollingfrom
cellumation:double_spin
Jan 27, 2025
Merged

fix(Executor): Fixed entities not beeing executed after just beeing added#2724
fujitatomoya merged 1 commit intoros2:rollingfrom
cellumation:double_spin

Conversation

@jmachowinski
Copy link
Collaborator

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).

@jmachowinski
Copy link
Collaborator 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
Collaborator 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...

…dded

Fixes ros2#2589

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
@jmachowinski
Copy link
Collaborator Author

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

@jmachowinski
Copy link
Collaborator 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
Collaborator 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
@fujitatomoya
Copy link
Collaborator

@Mergifyio backport jazzy

@mergify
Copy link
Contributor

mergify bot commented Jan 27, 2025

backport jazzy

✅ Backports have been created

Details

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

Fixes #2589

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>
(cherry picked from commit e7afaa6)
fujitatomoya pushed a commit that referenced this pull request Jan 28, 2025
…dded (#2724) (#2729)

Fixes #2589

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>
(cherry picked from commit e7afaa6)

Co-authored-by: Janosch Machowinski <jmachowinski@users.noreply.github.com>
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