Remove thread and guard condition from Timer #69

jacquelinekay · 2015-07-29T00:25:58Z

Connects to #6

Depends on /pull/66 and related PRs

Removes the 1 thread, 1 guard condition per timer scheme. Instead, the executor checks if a timer callback is eligible to be executed in get_next_timer based on the timer period and the last time the callback was triggered.

Initial CI run builds for all, and tests pass for all but Windows:

http://ci.ros2.org/job/ros2_batch_ci_linux/104/
http://ci.ros2.org/job/ros2_batch_ci_osx/45/
http://ci.ros2.org/job/ros2_batch_ci_windows/78/

I'm open to feedback regarding any tests that should be added to check timing correctness.

tfoote · 2015-07-30T22:22:45Z

+1

jacquelinekay · 2015-07-31T16:14:01Z

more CI after rebasing:

http://ci.ros2.org/job/ros2_batch_ci_linux/120/
http://ci.ros2.org/job/ros2_batch_ci_osx/62
http://ci.ros2.org/job/ros2_batch_ci_windows/89/

OSX has one test failure, which perhaps will be fixed by: ros2/system_tests#23

esteve · 2015-07-31T16:17:50Z

+1

jacquelinekay · 2015-07-31T20:36:07Z

I'd like @wjwwood to comment on this before I consider merging since he suggested these changes originally, so I'll wait until he's back on the grid.

wjwwood · 2015-07-31T20:59:13Z

rclcpp/include/rclcpp/executor.hpp

+          if (timer->check_and_trigger()) {
+            any_exec->timer = timer;
+            any_exec->callback_group = group;
+            any_exec->node = get_node_by_group(group);


Can't you just use node here?

I didn't write this originally, but sure, should work.

wjwwood · 2015-07-31T21:07:02Z

Don't you need to make use of the timeout to wait before merging this?

dirk-thomas · 2015-07-31T21:28:50Z

LGTM.

I do have a test locally for timers. I will update it after the merge and make a PR to add it to test_rclcpp.

jacquelinekay · 2015-07-31T22:27:20Z

You mean the changes made in the wait_timeout branches? I'm not sure how
to incorporate those changes, maybe you can clarify the expected behavior
for me. Should the timeout pased to rmw_wait limit the size of the
timeouts of the node timers?

On Fri, Jul 31, 2015 at 2:07 PM, William Woodall notifications@github.com
wrote:

Don't you need to make use of the timeout to wait before merging this?

—
Reply to this email directly or view it on GitHub
#69 (comment).

jacquelinekay · 2015-07-31T22:40:18Z

@dirk-thomas thanks, a timers test would be great.

wjwwood · 2015-08-01T17:52:56Z

As far as I can tell, this code as is will never wake up from a blocking call to spin in order to process timers. Since there is no longer a guard condition which wakes up rmw_wait then calls to rmw_wait must be setup to timeout in time for the next timer. In order to do that you need the ability to do a timed call to rmw_wait.

dirk-thomas · 2015-08-01T18:09:17Z

Can we cover such a case with a test then - to ensure it works after the patch?

wjwwood · 2015-08-02T21:15:35Z

I can look at your test and try to modify it. The important part is to test it without using subscriptions (so that the wait is only interrupted by signals and timers). And make sure that the timer is fired multiple times and that a blocking spin is used. I think the "simplest" way to test this is to:

Create a timer with a callback that increments a global variable.
Call spin in a separate thread.
Wait enough time for several timer callbacks to have fired.
Call rclcpp::shutdown.
Join the spin thread.
Check the number of times the timer callback fired and assert that it fired a few times.

I can try to put together a test as described, but I need to get my other pull request open first, so someone else might want to take a stab it as to not slow this pr down.

dirk-thomas · 2015-08-03T04:29:04Z

I have created ros2/system_tests#25 which adds a simple test for timers. I does roughly what @wjwwood described just without an extra thread and shutdown call. @jacquelinekay Can you try the test with your branch?

jacquelinekay · 2015-08-03T16:58:42Z

To fix the problem @wjwwood pointed out a few days ago, I merged this with the wait_timeout branch to get an rmw_wait call with a timeout, which has a few major API changes that cause the test to not compile. I'll make a branch of system_tests with the changes and test this branch with it. actually, I can just make a branch that's merged with ros2/system_tests#25 and test it with that

To simplify things, we should probably focus on the review process for the wait_timeout pull requests before progressing with this one.

jacquelinekay · 2015-08-05T23:49:36Z

The timer gtest passes locally on my machine. Let's see how Jenkins goes. Will run Windows when it's fixed.

http://ci.ros2.org/job/ros2_batch_ci_linux/158/
http://ci.ros2.org/job/ros2_batch_ci_osx/86/

jacquelinekay · 2015-08-06T17:11:33Z

The same 4 tests fail in both jobs (missing result) and I cannot reproduce the failures locally, either by running them individually or "make test" for test_rclcpp.

dirk-thomas · 2015-08-06T17:25:17Z

The console output will show what the problem for these four tests is. They all timeout.

It could be that the timeout is just very close to the actual runtime of the test and therefore results in a flaky test. But looking at the log it seems more likely that your changes are affecting the behavior:

Expected: (nonblocking_diff) < (tolerance), actual: 8-byte object <0C-CB 03-00 01-00 00-00> vs 8-byte object <0F-00 00-00 00-00 00-00>

jacquelinekay · 2015-08-06T17:56:59Z

I corrected some boolean logic that should fix the tests.
http://ci.ros2.org/job/ros2_batch_ci_linux/159/
http://ci.ros2.org/job/ros2_batch_ci_osx/87/

trying again now that the build is fixed:
http://ci.ros2.org/job/ros2_batch_ci_linux/162/
http://ci.ros2.org/job/ros2_batch_ci_osx/89

update: both jobs are green

tfoote · 2015-08-07T22:19:14Z

+1

esteve · 2015-08-07T22:21:08Z

+1

wjwwood · 2015-08-07T22:22:28Z

rclcpp/include/rclcpp/executor.hpp

+  {
+    std::chrono::nanoseconds latest = std::chrono::nanoseconds::max();
+    bool timers_empty = true;
+    for (auto weak_node : weak_nodes_) {


Might want to use auto & weak_node. The compiler might be smart enough to avoid extra work here, but we can help it out.

wjwwood · 2015-08-07T22:23:38Z

rclcpp/include/rclcpp/executor.hpp

-        any_exec->node = get_node_by_group(group);
-        guard_condition_handles_.erase(it++);
-        return;
+        for (auto timer_ref : group->timer_ptrs_) {


Same here, and anywhere else you're using for-range loop.

there are several instances in executor where references are not being used
in range-based for loops, should I go ahead and correct them in one fell
swoop?

On Fri, Aug 7, 2015 at 3:23 PM, William Woodall notifications@github.com
wrote:

In rclcpp/include/rclcpp/executor.hpp
#69 (comment):

continue; }

// Otherwise it is safe to set and return the any_exec

any_exec->timer = timer;

any_exec->callback_group = group;

any_exec->node = get_node_by_group(group);

guard_condition_handles_.erase(it++);

return;

for (auto timer_ref : group->timer_ptrs_) {

Same here, and anywhere else you're using for-range loop.

—
Reply to this email directly or view it on GitHub
https://github.com/ros2/rclcpp/pull/69/files#r36566699.

Well, you need to consider what the implications are, but yes usually the auto reference is a safe optimization to do. I'd say just update the ones you're adding and anything related to your changes. If you want to audit the rest, I'd do that in a separate pr so we can more easily review each case.

dirk-thomas · 2015-08-07T22:29:47Z

I am not sure which commits are different since the last review but looks still good to me.

wjwwood · 2015-08-07T22:29:55Z

rclcpp/include/rclcpp/executor.hpp

@@ -351,8 +344,7 @@ class Executor

    // Use the number of guard conditions to allocate memory in the handles
    // Add 2 to the number for the ctrl-c guard cond and the executor's
-    size_t start_of_timer_guard_conds = 2;
-    size_t number_of_guard_conds = timers.size() + start_of_timer_guard_conds;
+    size_t number_of_guard_conds = 2;


Can you update the comment to reflect this new value?

Bug fix: remove and add again node to executor

See: https://github.com/ros2/rclcpp/pull/207/files#r73983227

* ros2GH-69 Read storage content in a separate thread For now the publishing starts only after the reading is completly done. This should change aufter ros2GH-68 is done and a thread-safe queue can be used instead of std::queue. * ros2GH-71 Add integration test for timing behavior * ros2GH-68 Introduce vendor package for shared queue - Download and install headers from moodycamel readerwriterqueue - Download and install headers from moodycamel concurrentqueue - Use readerwriterqueue in code to load and publish concurrently * ros2GH-71 Retain time difference of messages when playing a bag file - The main (play) thread sleeps until the time for publishing the message is reached. - Using std::chrono time_point and duration for type-safe time arithmetic instead of rcutils time types. * ros2GH-71 Improve stability of read test - Subscribers need to maintain a longer history if the messages are not consumed fast enough. * ros2GH-71 Fix Classloader instance lifetime The Classloader instance needs to outlive all objects created by it. * ros2GH-71 Extract playing code into a class of its own Reason: record and play have almost no common code but do the exact opposite with the storage and rclcpp. * ros2GH-70 Do not link explicitly against std_msgs - only required in tests - this decreases the amount of packages needed for a clean build without tests * ros2GH-70 Fix error message of storage * ros2GH-70 Fix pluginlib/storage issue for recording * ros2GH-71 Cleanup: variable naming * ros2GH-70 Load storage continuously instead of as fast as possible - Only load if queue contains less than 1000 messages - Wait a millisecond before loading again once the queue is long enough * ros2GH-70 Add options struct to allow specification of queue size * ros2GH-72 Wait for messages to fill up * ros2GH-74 Rename integration tests to play/record tests * ros2GH-74 Use test_msgs in integration tests - gets rid of string_msgs dependency * ros2GH-70 Rename is_not_ready to is_pending, use bulk reading to queue * ros2GH-70 Harmonize storage_loading_future variable * ros2GH-88 Read messages in order of their timestamps - Currently, we write sequentially in order of arrival time so reading in id order is fine - This may change at a later time and should not change the reading behaviour, i.e. we need to read in order of timestamps * Fix compiler error on Mac * ros2GH-8 Fix: use correct ros message type in test * ros2GH-8 Cleanup: minor code style fixes * ros2GH-8 Refactor future usage in player Make the future a class member of player to avoid having to hand it into several functions which is difficult with a move-only type. * ros2GH-8 Cleanup: remove verbose logging for every stored message * ros2GH-8 Refactor rosbag2 interface Add an explicit overload for record without a topic_names argument to record all topics. * fix: call vector.reserve instead of default initalization * fix record demo

* ros2GH-138 Move calculation of bag size - previously in rosbag2::Info - now in storage plugin * ros2GH-130 Add rosbag2_bag_v2_plugins package -This package will contain storage and converter plugins * ros2GH-131 don't build plugins on Windows * ros2GH-129 Add function to be generated - massive if/else between all message types - will be generated similar to ros1_bridge plugin * ros2GH-138 Write storage plugin for rosbag v2 bags * ros2GH-138 Make sure that no attempt to create a converter is made when trying to read a rsbag v2 bag file * ros2GH-138 Add play end-to-end test for rosbag v2 plugin * ros2GH-138 Use cmake files to find ros1 packages - Use files from ros1_bridge via PkgConfig * ros2GH-138 Add generator code * ros2GH-141 Add initial version of vendor package * ros2GH-141 Improve vendor package to build on Mac * ros2GH-138 Cleanup CMakeLists * ros2GH-141 Use unmanaged Instance of class-loader - managed instance somehow isn't available for gcc 6.3 * ros2GH-141 Reduce patch and copy new toplevle CMakeLists by hand * ros2GH-141 Fix Shared Instance usage * ros2GH-141 Improve maintainability of vendor package - Document what patches do and why changes are necessary - Load ros1 packages through cmake macro - Do not export ros1 packages via ament - use commit hash of current master which is more stable than using melodic-devel * ros2GH-138 Link against rclcpp - necessary for ros1_bridge * ros2GH-138 Avoid crash when trying to play v2 bags which contain unknown message types * ros2GH-138 Add CLI -s <storage_id> option to ros2 bag info and use it in rosbag2::info - this allows ros2 bag info to work also when the yaml metadata file does not exsist - this is always the case for rosbag1 bagfiles - it could also happen for sqlite or other storage based bagfiles * ros2GH-138 Add end-to-end info test for rosbag v2 files * ros2GH-138 Add unit tests to rosbag_storage * ros2GH-138 Add method to extract filename from path to FilesystemHelpers * ros2GH-138 Add proper logging for topics which cannot be converted * ros2GH-138 Improve finding dependencies of ros1 * ros2GH-141 Explicitly import transitive dependencies of vendor package * ros2GH-138 Skip tests via ament if ros1 is not available * ros2GH-133 First split of plugins * ros2GH-133 Write serialized rosbag message * ros2GH-133 Improve converter plugin - move generation templates outside of plugin folders as both plugins need it - use ros::serialization routines to deserialize the ros message * ros2GH-133 Add plugin to be found by pluginlib * ros2GH-133 Remove empty check in converter - With the rosbag_v2_converter_plugin, we don't need to treat rosbag_v2 storage any different * ros2GH-133 Assert serialization format in unit tests for storage * ros2GH-133 Delete superfluous include folder - Only needed if we want to link against the library * ros2GH-133 get_all_topics_and_types returns only valid ros2 types - This is necessary as the information is used by rosbag2_transport - ros2 bag info still shows all topics and types - rosbag::View::getConnections() can return multiple connections corresponding to the same topic * ros2GH-133 Improve end to end test - use a bagfile with messages not known to ros2 * ros2GH-133 Reformulate info message in case of missing ros1-ros2 mapping for a topic * ros2GH-14 Find messages first * Explicitly print message when on Windows Co-Authored-By: Martin-Idel-SI <external.Martin.Idel@bosch-si.com> * ros2GH-14 Refactor rosbag_storage vendor package - Improve toplevel CMakeLists - Put all patches into a resource subfolder * ros2GH-14 Reflect renames of converter interfaces * ros2GH-156 Workaround for path problems * ros2GH-156 Add documentation for plugin * ros2GH-156 Fix the pluginlib version to greater 2 * ros2GH-156 Prohibit CMake from declaring paths as system paths This switches the order of ros2 and ros1 directories resulting in build failures * ros2GH-156 Prohibit system include paths for rosbag plugins This can lead to switching ros1 and ros2 include paths resulting in missing symbols as the wrong pluginlib gets included * ros2GH-14 Split patches * make README more verbose * add plugin specific readme * more readme for bag_v2 plugin

jacquelinekay added the in progress Actively being worked on (Kanban column) label Jul 29, 2015

jacquelinekay self-assigned this Jul 29, 2015

jacquelinekay force-pushed the wait_thread branch from 03eff6a to e1484ac Compare July 29, 2015 19:59

wjwwood reviewed Jul 31, 2015
View reviewed changes

jacquelinekay force-pushed the wait_thread branch from e1484ac to a16b64a Compare August 3, 2015 16:36

jacquelinekay force-pushed the wait_thread branch 2 times, most recently from c411467 to b5bfe6f Compare August 5, 2015 23:41

jacquelinekay force-pushed the wait_thread branch from f5ac979 to c9eb295 Compare August 6, 2015 17:46

jacquelinekay force-pushed the wait_thread branch from 12e51d0 to 26f1a94 Compare August 7, 2015 16:29

wjwwood reviewed Aug 7, 2015
View reviewed changes

Remove thread from Timer, remove guard conditions for timers

cd81802

jacquelinekay force-pushed the wait_thread branch from 7a8c90d to cd81802 Compare August 7, 2015 23:29

jacquelinekay merged this pull request into master Aug 7, 2015

jacquelinekay removed the in progress Actively being worked on (Kanban column) label Aug 7, 2015

jacquelinekay deleted the wait_thread branch August 7, 2015 23:29

jacquelinekay restored the wait_thread branch August 8, 2015 00:21

jacquelinekay deleted the wait_thread branch August 10, 2015 22:11

mauropasse pushed a commit to mauropasse/rclcpp that referenced this pull request Jun 21, 2021

Merge pull request ros2#69 from irobot-ros/asoragna/fix-remove-node

c3ab2fe

Bug fix: remove and add again node to executor

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

call to rmw_init, despite the lack of rmw_shutdown (ros2#69)

03900d6

See: https://github.com/ros2/rclcpp/pull/207/files#r73983227

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove thread and guard condition from Timer #69

Remove thread and guard condition from Timer #69

jacquelinekay commented Jul 29, 2015

tfoote commented Jul 30, 2015

jacquelinekay commented Jul 31, 2015

esteve commented Jul 31, 2015

jacquelinekay commented Jul 31, 2015

wjwwood Jul 31, 2015

jacquelinekay Jul 31, 2015

wjwwood commented Jul 31, 2015

dirk-thomas commented Jul 31, 2015

jacquelinekay commented Jul 31, 2015

jacquelinekay commented Jul 31, 2015

wjwwood commented Aug 1, 2015

dirk-thomas commented Aug 1, 2015

wjwwood commented Aug 2, 2015

dirk-thomas commented Aug 3, 2015

jacquelinekay commented Aug 3, 2015

jacquelinekay commented Aug 5, 2015

jacquelinekay commented Aug 6, 2015

dirk-thomas commented Aug 6, 2015

jacquelinekay commented Aug 6, 2015

tfoote commented Aug 7, 2015

esteve commented Aug 7, 2015

wjwwood Aug 7, 2015

wjwwood Aug 7, 2015

jacquelinekay Aug 7, 2015

wjwwood Aug 7, 2015

dirk-thomas commented Aug 7, 2015

wjwwood Aug 7, 2015

Remove thread and guard condition from Timer #69

Remove thread and guard condition from Timer #69

Conversation

jacquelinekay commented Jul 29, 2015

tfoote commented Jul 30, 2015

jacquelinekay commented Jul 31, 2015

esteve commented Jul 31, 2015

jacquelinekay commented Jul 31, 2015

wjwwood Jul 31, 2015

Choose a reason for hiding this comment

jacquelinekay Jul 31, 2015

Choose a reason for hiding this comment

wjwwood commented Jul 31, 2015

dirk-thomas commented Jul 31, 2015

jacquelinekay commented Jul 31, 2015

jacquelinekay commented Jul 31, 2015

wjwwood commented Aug 1, 2015

dirk-thomas commented Aug 1, 2015

wjwwood commented Aug 2, 2015

dirk-thomas commented Aug 3, 2015

jacquelinekay commented Aug 3, 2015

jacquelinekay commented Aug 5, 2015

jacquelinekay commented Aug 6, 2015

dirk-thomas commented Aug 6, 2015

jacquelinekay commented Aug 6, 2015

tfoote commented Aug 7, 2015

esteve commented Aug 7, 2015

wjwwood Aug 7, 2015

Choose a reason for hiding this comment

wjwwood Aug 7, 2015

Choose a reason for hiding this comment

jacquelinekay Aug 7, 2015

Choose a reason for hiding this comment

wjwwood Aug 7, 2015

Choose a reason for hiding this comment

dirk-thomas commented Aug 7, 2015

wjwwood Aug 7, 2015

Choose a reason for hiding this comment