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.

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

Refactor spin and spin_once to mimic rclcpp's Executor API #324

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

esteve
Copy link
Collaborator

@esteve esteve commented Jul 26, 2023

Moving spin and spin_once to a similar API as rclcpp's and rclpy's Executor API allows us to run more than one node in a single program and makes it easier to test node behaviors.

@esteve esteve force-pushed the executor-refactor branch 7 times, most recently from 55fc519 to 0b3f447 Compare July 30, 2023 18:18
@esteve esteve requested review from maspe36 and jhdcs July 30, 2023 18:24
@esteve esteve marked this pull request as ready for review July 30, 2023 18:24
@esteve
Copy link
Collaborator Author

esteve commented Jul 30, 2023

@jhdcs @maspe36 these changes are functionally equivalent to what we alredy have now, but add the extra feature of being able to run more than one node in a program (see the example I've added where there's a publisher and two subscriptions). I plan on expanding this to enable multiple implementations of an executor following rclcpp/rclpy's approach, but for now I just wanted to get a concrete implementation and then I'll derive traits and other generalizations in a follow-up PR.

This would also allow us to implement composable nodes in the future, but for now the spin method in SingleThreadedExecutor just takes an &Arc<Node>

@esteve esteve force-pushed the executor-refactor branch 2 times, most recently from 9396971 to 057481f Compare July 30, 2023 21:00
Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work!

I only have a couple very-small issues with the comments for spin in lib.rs. Of the two, I think the potentially-unneeded SAFETY comment is the more important to take a look at, but I still wanted to call out a potential inaccuracy.

But all in all, this is beautifully done, thank you!

rclrs/src/lib.rs Outdated Show resolved Hide resolved
rclrs/src/lib.rs Outdated Show resolved Hide resolved
@esteve
Copy link
Collaborator Author

esteve commented Aug 1, 2023

@jhdcs thanks for the review!

You're right about the comments, I'll update/remove them.

But all in all, this is beautifully done, thank you!

These are very kind words, I really appreciate it, thanks 🙂

@esteve esteve requested a review from jhdcs August 2, 2023 02:54
@esteve
Copy link
Collaborator Author

esteve commented Aug 3, 2023

@jhdcs I've addressed your feedback, could you have another look when you have a moment? Thanks!

Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thank you again for this!

@esteve esteve merged commit 8694d41 into main Aug 7, 2023
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the executor-refactor branch August 7, 2023 17:25
luca-della-vedova pushed a commit to luca-della-vedova/ros2_rust that referenced this pull request Sep 20, 2023
esteve added a commit that referenced this pull request Oct 27, 2023
* WIP Add minimal TimeSource implementation

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Minor cleanup and code reorganization

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Minor cleanups, add debug code

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Minor cleanup

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Change safety note to reflect get_now safety

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Fix comment spelling

Co-authored-by: jhdcs <48914066+jhdcs@users.noreply.github.com>

* Cleanup and add some docs / tests

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Avoid duplicated clocks in TimeSource

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Change _rcl_clock to just Mutex

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Remove Mutex from node clock

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Change Mutex<bool> to AtomicBool

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Avoid cloning message

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Minor cleanup, add dependency

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Remove hardcoded use_sim_time

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Cleanup remaining warnings / clippy

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Fix tests

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Refactor API to use ClockSource

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Fix Drop implementation, finish builder and add comments

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Minor cleanups

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Fix graph_tests

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Remove Arc from time source

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Remove Sync trait, format

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add comparison function for times, use reference to clock

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Implement Add<Duration> and Sub<Duration> for Time

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Make node pointer Weak to avoid memory leaks

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Cleanups

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* WIP change clock type when use_sim_time parameter is changed

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Remove need for mutex in node TimeSource

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Change get_clock() to return cloned Clock object

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Minor cleanup

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Refactor spin and spin_once to mimic rclcpp's Executor API (#324)

* Added rclcpp/rclpy-like executor

* Fix comments

* add serde big array support (fixed #327) (#328)

* add serde big array support

* Fixed an issue where cpu usage remained at 100% (#330)

* Make get_parameter pub(crate)

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Address review feedback

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Make time_source pub(crate), remove unused APIs

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* WIP parameter interface implementation

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Migrate to proposed API

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add API for mandatory and optional parameters

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* First round of unit tests for parameter API

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Make complex parameter types Arc

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* First attempt at undeclared parameter API

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* First attempt at automatic parameter undeclaring

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Change undeclared_parameter API to be more explicit

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Minor cleanup, use atomic for undeclared params

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add docs

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add more tests

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Update setter for optional parameters

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add unset api for optional parameters

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Cleanup and add helper functions for arrays

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add type alias

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Reduce number of generics, add more tests

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Refactor to include parameter ranges and read_only

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Refactor ranges

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Revert API change for range builder

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add ParameterValueError variant

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* First implementation of tentative API

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Rework API for unified declaration syntax

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add missing APIs

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add error variant for default out of range

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Refactor and remove duplicated code

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Revert change to parameter service file

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Remove time related changes

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Clarify doc for unset API

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Fix comments / TODOs

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Remove Arc<RwLock> wrapper from undeclared params

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add short description of deviations from other client libraries

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Address minor feedback

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Remove duplicated range

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Change maybe_from to try_from

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* WIP converting to default value builder argument

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add type for test

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Infer arrays and allow custom discriminators

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

* Fix CI

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Fix some documentation

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

* Fix doc link

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Minor cleanups and optimizations

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Fix outdated docstrings

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

* Add links in documentation / address clippy warnings

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>

---------

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Co-authored-by: jhdcs <48914066+jhdcs@users.noreply.github.com>
Co-authored-by: Esteve Fernandez <33620+esteve@users.noreply.github.com>
Co-authored-by: Fawdlstty <fawdlstty@users.noreply.github.com>
Co-authored-by: Michael X. Grey <grey@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants