fix events-executor warm-up bug and add unit-tests by alsora · Pull Request #2591 · ros2/rclcpp

alsora · 2024-07-26T18:37:48Z

Wrote a test to verify the behavior of spin_all, that should collect work multiple times (if given enough duration) and this should allow to also get work from entities created after the node / cb group was added to the executor.

See #2589 for details

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

fujitatomoya

i think comments could be updated a bit, but this test itself makes sense to be added.

rclcpp/test/rclcpp/executors/test_executors.cpp

jmachowinski · 2024-08-02T13:04:36Z

This test is flawed. It builds on top of TestExecutors. The SetUp method of TestExecutors also instantiates a node and subscripers / publishers interfering with the test.

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

alsora · 2024-08-04T23:56:39Z

@jmachowinski I moved the tests to a new file with their own, minimal, test suite

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

rclcpp/test/rclcpp/executors/test_executors_minimal_init.cpp

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

alsora · 2024-08-10T17:05:21Z

Thanks for the early review.
Can you please take another look?

I updated the PR:

fixes the "warm up" bug for the events executor
adds unit-tests that exhibit the problem (the tests are currently enabled only for the executors that pass them)

CI:

Linux
Linux-aarch64
Linux-rhel
Windows

ros-discourse · 2024-08-15T03:17:28Z

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

https://discourse.ros.org/t/next-client-library-wg-meeting-friday-16th-august-2024/39130/1

MichaelOrlov · 2024-08-22T18:02:54Z

@jmachowinski @fujitatomoya A friendly ping for one more round of review to move forward with this PR.

SteveMacenski · 2024-08-29T16:16:16Z

Please :-)

sloretz · 2024-08-29T21:21:44Z

@Mergifyio update

mergify · 2024-08-29T21:21:57Z

update

✅ Branch has been successfully updated

sloretz · 2024-08-29T21:23:25Z

CI (build: --packages-above-and-dependencies rclcpp test: --packages-above rclcpp)

Linux
Linux-aarch64
Linux-rhel
Windows

ahcorde · 2024-08-30T07:59:36Z

rclcpp/test/rclcpp/executors/test_executors_warmup.cpp

@@ -0,0 +1,241 @@
+// Copyright 2017 Open Source Robotics Foundation, Inc.


Suggested change

// Copyright 2017 Open Source Robotics Foundation, Inc.

// Copyright 2024 Open Source Robotics Foundation, Inc.

ahcorde · 2024-08-30T08:00:18Z

rclcpp/test/rclcpp/executors/test_executors_warmup.cpp

+  // Create entities, this will produce a notifier waitable event, telling the executor to refresh
+  // the entities collection
+  auto publisher = node->create_publisher<test_msgs::msg::Empty>("test_topic", rclcpp::QoS(10));
+  size_t callback_count = 0;


include <cstddef>

ahcorde · 2024-08-30T08:00:41Z

rclcpp/test/rclcpp/executors/test_executors_warmup.cpp

+  auto callback = [&callback_count](test_msgs::msg::Empty::ConstSharedPtr) {callback_count++;};
+  auto subscription =
+    node->create_subscription<test_msgs::msg::Empty>(
+    "test_topic", rclcpp::QoS(10), std::move(callback));


include <utility>

ahcorde · 2024-08-30T08:01:15Z

rclcpp/test/rclcpp/executors/test_executors_warmup.cpp

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


include <type_traits>

alsora · 2024-09-06T12:50:05Z

Added missing include directives.
New CI:

Linux
Linux-aarch64
Linux-rhel
Windows

mjcarroll · 2024-09-06T14:23:29Z

RHEL is failing with out of disk space errors. Reported to buildfarmers.

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

…xecutor

alsora · 2024-09-10T08:16:12Z

Fixed DCO.

New CI:

Linux
Linux-aarch64
Linux-rhel
Windows

alsora · 2024-09-11T07:08:31Z

Merging this since the changes requests have been addressed and the RHEL failure is unrelated and known (ros2cli.pytest.missing_result)

SteveMacenski · 2024-09-11T18:09:19Z

Can we get this backported to Jazzy?

ahcorde · 2024-09-11T20:07:19Z

https://github.com/Mergifyio backport jazzy

mergify · 2024-09-11T20:07:28Z

backport jazzy

✅ Backports have been created

Details

#2625 fix events-executor warm-up bug and add unit-tests (backport #2591) has been created for branch jazzy but encountered conflicts

* add unit-test to verify that spin-all doesn't need warm-up Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> * improve comment and add callback group test Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> * move executor tests to a new file Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> * do not require warm up iteration with events executor spin_some Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> * add spin_some tests and cleanup Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> * add missing include directives Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> --------- Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit f7056c0) # Conflicts: # rclcpp/src/rclcpp/experimental/executors/events_executor/events_executor.cpp

doisyg · 2024-09-12T16:54:09Z

Can we merge the backport to Jazzy and trigger a release plz ?

ahcorde · 2024-09-12T17:01:13Z

Ey @alsora I created the backport but there are some conflicts. do you mind to fix it ?

I can take care of the release once the backport PR is merged

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

…2628) Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

add unit-test to verify that spin-all doesn't need warm-up

43847f8

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

alsora requested review from hidmic, ivanpauno and wjwwood as code owners July 26, 2024 18:37

fujitatomoya approved these changes Jul 28, 2024

View reviewed changes

rclcpp/test/rclcpp/executors/test_executors.cpp Outdated Show resolved Hide resolved

improve comment and add callback group test

8591223

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

move executor tests to a new file

77ddcd1

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

alsora force-pushed the asoragna/warm-up-executor branch from d2df4b0 to 77ddcd1 Compare August 4, 2024 23:57

alsora commented Aug 4, 2024

View reviewed changes

rclcpp/test/rclcpp/executors/test_executors_minimal_init.cpp Outdated Show resolved Hide resolved

alsora mentioned this pull request Aug 5, 2024

Double spin required since 28.2.0 #2589

Closed

alsora added 2 commits August 10, 2024 09:27

do not require warm up iteration with events executor spin_some

7a5d7e3

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

add spin_some tests and cleanup

cf2280c

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

alsora mentioned this pull request Aug 10, 2024

[DRAFT] do not require warm up iteration with events executor spin_some #2597

Closed

alsora changed the title ~~add unit-test to verify that spin-all doesn't need warm-up~~ fix events-executor warm-up bug and add unit-tests Aug 10, 2024

Merge branch 'rolling' into asoragna/warm-up-executor

1b5de29

sloretz assigned mjcarroll Aug 29, 2024

ahcorde requested changes Aug 30, 2024

View reviewed changes

mjcarroll approved these changes Sep 6, 2024

View reviewed changes

add missing include directives

4b7c434

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

Merge remote-tracking branch 'origin/rolling' into asoragna/warm-up-e…

d3ed8a6

…xecutor

alsora force-pushed the asoragna/warm-up-executor branch from 067b721 to d3ed8a6 Compare September 10, 2024 08:15

alsora merged commit f7056c0 into rolling Sep 11, 2024

alsora deleted the asoragna/warm-up-executor branch September 11, 2024 07:08

alsora mentioned this pull request Sep 11, 2024

DRAFT: Fix: Double spin required since 28.2.0 #2595

Closed

mergify bot mentioned this pull request Sep 11, 2024

fix events-executor warm-up bug and add unit-tests (backport #2591) #2625

Closed

alsora added a commit that referenced this pull request Sep 13, 2024

backport fix events-executor warm-up bug and add unit-tests (#2591)

21de352

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

alsora mentioned this pull request Sep 13, 2024

backport fix events-executor warm-up bug and add unit-tests (#2591) #2628

Merged

ahcorde pushed a commit that referenced this pull request Sep 17, 2024

backport fix events-executor warm-up bug and add unit-tests (#2591) (#…

38ed9ed

…2628) Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

tonynajjar mentioned this pull request Jan 10, 2025

Revert Double Spin in #4558 once rclcpp #2589 fixed ros-navigation/navigation2#4567

Closed

		@@ -0,0 +1,241 @@
		// Copyright 2017 Open Source Robotics Foundation, Inc.

	// Copyright 2017 Open Source Robotics Foundation, Inc.
	// Copyright 2024 Open Source Robotics Foundation, Inc.

Conversation

alsora commented Jul 26, 2024

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmachowinski commented Aug 2, 2024

Uh oh!

alsora commented Aug 4, 2024

Uh oh!

Uh oh!

alsora commented Aug 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ros-discourse commented Aug 15, 2024

Uh oh!

MichaelOrlov commented Aug 22, 2024

Uh oh!

SteveMacenski commented Aug 29, 2024

Uh oh!

sloretz commented Aug 29, 2024

Uh oh!

mergify bot commented Aug 29, 2024

✅ Branch has been successfully updated

Uh oh!

sloretz commented Aug 29, 2024

Uh oh!

ahcorde Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

ahcorde Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

ahcorde Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

ahcorde Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

alsora commented Sep 6, 2024

Uh oh!

mjcarroll commented Sep 6, 2024

Uh oh!

alsora commented Sep 10, 2024

Uh oh!

alsora commented Sep 11, 2024

Uh oh!

SteveMacenski commented Sep 11, 2024

Uh oh!

ahcorde commented Sep 11, 2024

Uh oh!

mergify bot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

doisyg commented Sep 12, 2024

Uh oh!

ahcorde commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

alsora commented Aug 10, 2024 •

edited

Loading

mergify bot commented Sep 11, 2024 •

edited

Loading