Added spin_node_until_future_complete #25

esteve · 2015-04-28T17:09:46Z

dirk-thomas · 2015-04-28T17:19:00Z

rclcpp/include/rclcpp/executors.hpp

+    executor.spin_node_some(node_ptr);
+    status = future.wait_for(std::chrono::seconds(0));
+  } while (status != std::future_status::ready && rclcpp::utilities::ok());
+  return future;


Does it need to return the passed in future? What would be a use case for that?

Doesn't need to, but it's convenient because one can write code like this:

auto value = spin_node_until_future_complete<int64_t>(executor, node, future).get();

For method chaining I find it pretty _un_intuitive that the function returns the future. Especially since it looks like that passing a temporary future does not work.

I don't know if others have an opinion on this. We could always defer this to an API review in the future.

The function already returns the future, what changes do you suggest?

Not returning anything.

For method chaining I find it pretty intuitive that the function returns the future

You meant intuitive or counter intuitive?

I think it is fine as is, the method chaining looks useful and I don't see that it needs to return anything else.

Sorry, important spelling error: I meant unintuitive.

The calling code has the future and it looks arbitrary and _un_intuitive to me why the method would return that specific argument for chaining.

Supporting chaining operators make usage much more compact. The other option I see is to have an additional or overridden get method which will do the background spinning. But I think this is cleaner and doesn't require modifying builtins.

wjwwood · 2015-04-28T18:29:16Z

Other than the comment, lgtm.

…plete inside a callback executed by an executor

dirk-thomas · 2015-04-28T18:36:07Z

lgtm

tfoote · 2015-04-28T22:08:14Z

lgtm

Added spin_node_until_future_complete

…ecutor Rename ExecutorEventCallback -> EventsExecutorCallback

Reenable rmw_connext_dynamic_cpp

* rosbag2_transport package with python interface * use cpp for python extension * use rosbag2_transport cpp API * use rosbag2_transport API in cli * linters * ros2GH-25 Rename target librosbag2 to rosbag2 CMake already prepends libraries with `lib`, so the old name resulted in `liblibrosbag2` * ros2GH-21 Initial call of rosbag2.record() from rosbag2_transport * ros2GH-21 Add missing copyright header * ros2GH-21 Cleanup clang tidy issues * ros2GH-21 Remove rclcpp dependency from rosbag2 * ros2GH-21 Wire rosbag play into CLI * ros2GH-21 Add missing test_depend in rosbag2_transport package.xml * ros2GH-21 Unify name of python import * ros2GH-21 Enable -a in CLI, show help on wrong args * ros2GH-85 Introduce topic and type struct for readability * ros2GH-85 Do not export sqlite3 as dependency from default plugins - not referenced in header, therefore unnecessary * ros2GH-85 Move rosbag2 except typesupport to rosbag2_transport * ros2GH-85 Add rosbag2 wrapper * ros2GH-85 Change signature of create_topic to take TopicWithType * ros2GH-85 Use rosbag2 in rosbag2_transport - Don't link against rosbag2_storage anymore * ros2GH-84 Cleanup package.xmls and CMakeLists everywhere * ros2GH-21 Add missing init() and shutdown() in record * ros2GH-85 Fix Windows build * ros2GH-85 Add visibility control to rosbag2 * ros2GH-85 Cleanup and documentation * ros2GH-87 Add test package rosbag2_tests * ros2GH-87 [WIP] Add first working prototype of an end-to-end test * ros2GH-87 Use test_msgs instead of std_msgs/String in end-to-end test * ros2GH-87 Use SIGTERM instead of SIGKILL and refactor test * ros2GH-87 Make end-to-end test work on Windows * ros2GH-87 Fix uncrustify * ros2GH-87 Refactor end-to-end test fixture * ros2GH-21 Extend transport python module interface The python interface should accept all options that can be passed to rosbag2_transport * ros2GH-87 Fix test fixture for Windows * ros2GH-87 Refactor test fixture * ros2GH-87 Separate record from play end-to-end test * ros2GH-87 Make record end-to-end test work * ros2GH-87 Publish before recording to create topic * ros2GH-87 Fix record all on Windows * ros2GH-87 Check for topics instead of all * ros2GH-87 Wait until rosbag record opened database * ros2GH-87 Delete directory recursively * ros2GH-87 Delete directories recursively on Linux * ros2GH-87 Reset ROS_DOMAIN_ID to protect against concurrent tests * ros2GH-89 Make rosbag2 interfaces virtual and add explicit open() method This allows downstream packages (e.g. rosbag2_transport) to mock these interfaces in tests. * ros2GH-87 Improve test and refactoring * ros2GH-87 Minor refactoring to increase test readability * ros2GH-87 Fix environmental variable behaviour on Mac * ros2GH-87 Fix Windows build * ros2GH-89 Use mock reader and writer in rosbag2_transport tests * ros2GH-87 Add play end_to_end test * ros2GH-87 Improvements of test * ros2GH-87 Fix Windows build * ros2GH-89 Cleanup: small documentation fixes. * ros2GH-89 [WIP] Test if Writer and Reader work with class visibility * ros2GH-87 Stabilize rosbag2_play test * ros2GH-87 Minor refactoring of tests * ros2GH-87 Rename end to end tests * add license agreement * ros2GH-89 Simplification of writing to in-memory storage * ros2GH-89 Stabilize transport tests * ros2GH-87 Refactoring of tests - Extract temporary file handling - Extract subscription management * ros2GH-87 Add pytest cache to gitignore * ros2GH-87 Refactoring of play test - Extract Publisher manager * ros2GH-87 Extract record test fixture for readability * ros2GH-89 Refactor transport tests - Use subscription and publisher manager just as e2e tests - Use options in recording * ros2GH-89 Use temporary directory fixture in sqlite tests * ros2GH-89 Conform to naming standard for tests * ros2GH-89 Prevent burst publishing of all messages - Improves test stability * ros2GH-89 Improve play stability - Sometimes the first message is lost (discovery) * ros2GH-25 Fix package.xmls * Consistently use project name in CMakeLists * Minor cleanup - make rosbag2_transport description more expressive - hide unnecessary methods in typesupport_helpers - fix incorrect logging in tests - minor cleanup * Change name of nodes in rosbag2_transport * Cleanup folder structure in rosbag2_storage and rosbag2_tests - use src/<package_name>/ and test/<package_name>/ folders everywhere - harmonises with all other packages - results in better header guards * Export sqlite3 dependency as package dependency * Create node in Rosbag2Transport always * Only hold one node in rosbag2_transport * Move all duplicate files to common package * Adapt namespacing in test commons package - use "using namespace" declaratives for tests - use package name as namespace * Replace "Waiting for messages..." message * ros2GH-25 rename rosbag2_test_commons -> rosbag2_test_common * ros2GH-25 Overwrite already existing test.bag when recording This is a temporary solution and will be handled properly once a file path can be passed via the cli. * ros2GH-25 Cleanups - Log every subscription - move all dependencies onside BUILD_TESTING for rosbag2_test_common * fix cmake typo for test_common * Remove superfluous loop in rosbag2 transport * Delete superfluous test_msgs dependency * Add rclcpp to test dependencies - Apparently ament_export_dependencies does not work in rosbag2_test_common * Fix rosbag2 node test - Clock topic is no longer present on all nodes - Remove assumptions on foreign ros topics * Fix dependencies by exporting them explicitly

esteve added the in progress Actively being worked on (Kanban column) label Apr 28, 2015

Added spin_node_until_future_complete

90ba163

esteve force-pushed the spin-node-until-future-complete branch from 19d9f67 to 90ba163 Compare April 28, 2015 17:11

esteve self-assigned this Apr 28, 2015

esteve added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 28, 2015

dirk-thomas reviewed Apr 28, 2015
View reviewed changes

Added comment about not being able to call spin_node_until_future_com…

6b6b94f

…plete inside a callback executed by an executor

esteve added a commit that referenced this pull request Apr 28, 2015

Merge pull request #25 from ros2/spin-node-until-future-complete

8ad1f1f

Added spin_node_until_future_complete

esteve merged commit 8ad1f1f into master Apr 28, 2015

esteve removed the in review Waiting for review (Kanban column) label Apr 28, 2015

esteve deleted the spin-node-until-future-complete branch April 28, 2015 22:09

hidmic mentioned this pull request Jun 26, 2020

Fix pub/sub count API tests. #1203

Merged

alsora pushed a commit to alsora/rclcpp that referenced this pull request Oct 21, 2020

Merge pull request ros2#25 from mauropasse/mauro-irobot/add-events-ex…

2259a67

…ecutor Rename ExecutorEventCallback -> EventsExecutorCallback

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

Merge pull request ros2#25 from ros2/introspection_cpp

0fe857c

Reenable rmw_connext_dynamic_cpp

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added spin_node_until_future_complete #25

Added spin_node_until_future_complete #25

esteve commented Apr 28, 2015

dirk-thomas Apr 28, 2015

esteve Apr 28, 2015

dirk-thomas Apr 28, 2015

esteve Apr 28, 2015

dirk-thomas Apr 28, 2015

esteve Apr 28, 2015

wjwwood Apr 28, 2015

dirk-thomas Apr 28, 2015

tfoote Apr 28, 2015

wjwwood commented Apr 28, 2015

dirk-thomas commented Apr 28, 2015

tfoote commented Apr 28, 2015

Added spin_node_until_future_complete #25

Added spin_node_until_future_complete #25

Conversation

esteve commented Apr 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood commented Apr 28, 2015

dirk-thomas commented Apr 28, 2015

tfoote commented Apr 28, 2015