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.

Sign up for GitHub

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 (backport #2724) #2729

Merged
merged 1 commit into from
Jan 28, 2025
+19 −12
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions rclcpp/src/rclcpp/executor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,14 @@ Executor::spin_some_impl(std::chrono::nanoseconds max_duration, bool exhaustive)
// both spin_some and spin_all wait for work at the beginning
wait_result_.reset();
wait_for_work(std::chrono::milliseconds(0));
bool just_waited = true;
bool entity_states_fully_polled = true;

if (entities_need_rebuild_) {
// if the last wait triggered a collection rebuild, we need to call
// wait_for_work once more, in order to do a collection rebuild and collect
// events from the just added entities
entity_states_fully_polled = false;
}

// The logic of this while loop is as follows:
//
Expand All @@ -393,12 +400,14 @@ Executor::spin_some_impl(std::chrono::nanoseconds max_duration, bool exhaustive)
AnyExecutable any_exec;
if (get_next_ready_executable(any_exec)) {
execute_any_executable(any_exec);
just_waited = false;
// during the execution some entity might got ready therefore we need
// to repoll the states of all entities
entity_states_fully_polled = false;
} else {
// if nothing is ready, reset the result to clear it
wait_result_.reset();

if (just_waited) {
if (entity_states_fully_polled) {
// there was no work after just waiting, always exit in this case
// before the exhaustive condition can be checked
break;
Expand All @@ -408,7 +417,13 @@ Executor::spin_some_impl(std::chrono::nanoseconds max_duration, bool exhaustive)
// if exhaustive, wait for work again
// this only happens for spin_all; spin_some only waits at the start
wait_for_work(std::chrono::milliseconds(0));
just_waited = true;
entity_states_fully_polled = true;
if (entities_need_rebuild_) {
// if the last wait triggered a collection rebuild, we need to call
// wait_for_work once more, in order to do a collection rebuild and
// collect events from the just added entities
entity_states_fully_polled = false;
}
} else {
break;
}
Expand Down
8 changes: 0 additions & 8 deletions rclcpp/test/rclcpp/executors/test_executors_warmup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,6 @@ TYPED_TEST(TestExecutorsWarmup, spin_all_doesnt_require_warmup_with_cbgroup)
{
using ExecutorType = TypeParam;

// TODO(alsora): Enable when https://github.com/ros2/rclcpp/pull/2595 gets merged
if (
std::is_same<ExecutorType, rclcpp::executors::SingleThreadedExecutor>() ||
std::is_same<ExecutorType, rclcpp::executors::MultiThreadedExecutor>())
{
GTEST_SKIP();
}

ExecutorType executor;

// Enable intra-process to guarantee deterministic and synchronous delivery of the message / event
Expand Down