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

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

rclcpp/test/rclcpp/executors/test_executors_minimal_init.cpp

Signed-off-by: Alberto Soragna <[email protected]>

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 <[email protected]>

…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

#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 <[email protected]> * improve comment and add callback group test Signed-off-by: Alberto Soragna <[email protected]> * move executor tests to a new file Signed-off-by: Alberto Soragna <[email protected]> * do not require warm up iteration with events executor spin_some Signed-off-by: Alberto Soragna <[email protected]> * add spin_some tests and cleanup Signed-off-by: Alberto Soragna <[email protected]> * add missing include directives Signed-off-by: Alberto Soragna <[email protected]> --------- Signed-off-by: Alberto Soragna <[email protected]> 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 <[email protected]>

…2628) Signed-off-by: Alberto Soragna <[email protected]>

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

43847f8

Signed-off-by: Alberto Soragna <[email protected]>

alsora requested review from ivanpauno, hidmic 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 <[email protected]>

move executor tests to a new file

77ddcd1

Signed-off-by: Alberto Soragna <[email protected]>

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

Open

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 <[email protected]>

add spin_some tests and cleanup

cf2280c

Signed-off-by: Alberto Soragna <[email protected]>

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 <[email protected]>

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
3 checks passed

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

Open

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 <[email protected]>

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 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alsora commented Jul 26, 2024

fujitatomoya left a comment

jmachowinski commented Aug 2, 2024

alsora commented Aug 4, 2024

alsora commented Aug 10, 2024 •

edited

Loading

ros-discourse commented Aug 15, 2024

MichaelOrlov commented Aug 22, 2024

SteveMacenski commented Aug 29, 2024

sloretz commented Aug 29, 2024

mergify bot commented Aug 29, 2024

sloretz commented Aug 29, 2024

ahcorde Aug 30, 2024

ahcorde Aug 30, 2024

ahcorde Aug 30, 2024

ahcorde Aug 30, 2024

alsora commented Sep 6, 2024

mjcarroll commented Sep 6, 2024

alsora commented Sep 10, 2024

alsora commented Sep 11, 2024

SteveMacenski commented Sep 11, 2024

ahcorde commented Sep 11, 2024

mergify bot commented Sep 11, 2024 •

edited

Loading

doisyg commented Sep 12, 2024

ahcorde commented Sep 12, 2024

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

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

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

Conversation

alsora commented Jul 26, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

jmachowinski commented Aug 2, 2024

alsora commented Aug 4, 2024

alsora commented Aug 10, 2024 • edited Loading

ros-discourse commented Aug 15, 2024

MichaelOrlov commented Aug 22, 2024

SteveMacenski commented Aug 29, 2024

sloretz commented Aug 29, 2024

mergify bot commented Aug 29, 2024

✅ Branch has been successfully updated

sloretz commented Aug 29, 2024

ahcorde Aug 30, 2024

Choose a reason for hiding this comment

ahcorde Aug 30, 2024

Choose a reason for hiding this comment

ahcorde Aug 30, 2024

Choose a reason for hiding this comment

ahcorde Aug 30, 2024

Choose a reason for hiding this comment

alsora commented Sep 6, 2024

mjcarroll commented Sep 6, 2024

alsora commented Sep 10, 2024

alsora commented Sep 11, 2024

SteveMacenski commented Sep 11, 2024

ahcorde commented Sep 11, 2024

mergify bot commented Sep 11, 2024 • edited Loading

✅ Backports have been created

doisyg commented Sep 12, 2024

ahcorde commented Sep 12, 2024

alsora commented Aug 10, 2024 •

edited

Loading

mergify bot commented Sep 11, 2024 •

edited

Loading