Skip to content

Conversation

@esteve
Copy link
Contributor

@esteve esteve commented Jan 28, 2025

This PR moves the rosidl_runtime_rs folder from ros2-rust @ 9a845c17873cbdf49e8017d5f0af6d8f795589cc

nnmm and others added 30 commits March 18, 2022 12:11
Previously, only messages consisting of basic types and strings were supported. Now, all message types will work, including those that have fields of nested types, bounded types, or arrays.

Changes:
- The "rsext" library is deleted
- Unused messages in "rosidl_generator_rs" are deleted
- There is a new package, "rosidl_runtime_rs", see below
- The RMW-compatible messages from C, which do not require an extra conversion step, are exposed in addition to the "idiomatic" messages
- Publisher and subscription are changed to work with both idiomatic and rmw types, through the unifying `Message` trait

On `rosidl_runtime_rs`: This package is the successor of `rclrs_msg_utilities` package, but doesn't have much in common with it anymore.
It provides common types and functionality for messages. The `String` and `Sequence` types and their variants in that package essentially wrap C types from the `rosidl_runtime_c` package and C messages generated by the "rosidl_generator_c" package.
A number of functions and traits are implemented on these types, so that they feel as ergonomic as possible, for instance, a `seq!` macro for creating a sequence. There is also some documentation and doctests.

The memory for the (non-pretty) message types is managed by the C allocator.

Not yet implemented:
- long double
- constants
- Services/clients
- @verbatim comments
- ndarray for sequences/arrays of numeric types
- implementing `Eq`, `Ord` and `Hash` when a message contains no floats
Also remove unused dependency from rosidl_runtime_rs
* Removed support for no_std

* Removed more no_std dependencies

* Removed more no_std dependencies

* Removed more no_std dependencies

* Removed downcast

* Removed TryFrom, not needed with Rust 2021

* Fix visibility of modules
This is required to make multithreading work. For instance, calling publish() on a separate thread doesn't work without these impls.
* Copied tests from jhdcs' fork

* Ran cargo fmt

* Run clippy on tests as well

* Make Clippy happy

* Fix rustfmt warning

* Added std_msgs to package.xml

* Disable deref_nullptr warning for generated code

* Include Soya-Onishi's changes

* Fix Node::new call

* Fix spelling mistakes

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

* Fix spelling mistakes

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

* Fix spelling mistakes

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

* Fix formatting

Co-authored-by: jhdcs <48914066+jhdcs@users.noreply.github.com>
Previously, it was assumed by the $string_conversion_func, for instance, that the output of deref() would be an unsigned integer.
* Added support for clients and services
* Fixups for releasing to crates.io

* Removed std_msgs as test dependency. Fix rosidl_runtime_rs version

* Removed test

* Removed test
* Generalize callbacks to subscriptions

By implementing a trait (SubscriptionCallback) on valid function signatures,
and making subscriptions accept callbacks that implement this trait, we can now

* Receive both plain and boxed messages
* Optionally receive a MessageInfo along with the message, as the second argument
* Soon, receive a loaned message instead of an owned one

This corresponds to the functionality in any_subscription_callback.hpp in rclcpp.
These files will be shown by crates.io.
- Upgraded libloading to 0.8
- Added instructions to rerun build scripts if dependent files have changed
- Enabled bindgen to invalidate the built crate whenever any transitively included header files from the wrapper change
- Removed some default true options from our bindgen builder

Co-authored-by: Sam Privett <sam@privett.dev>
Signed-off-by: Esteve Fernandez <esteve@apache.org>
This reverts commit 8948409e0c6d9ced291b5cffda82036d067bd36d.
esteve and others added 10 commits November 24, 2023 16:13
* Allow rustdoc to run without a ROS distribution

* Extensions to get working

* Fail if generate_docs feature is not enabled and ROS_DISTRO is not set

* Fix formatting

* Fix formatting

* Make clippy slightly less unhappy

* Do not run clippy for all features if checking rclrs

* Clean up rcl_bindings.rs

* Fix comparison

* Avoid warnings for rosidl_runtime_rs

* Avoid running cargo test with all features for rclrs

* Ignore rosidl_runtime_rs for cargo test

* rosidl_runtime_rs does not have a dyn_msg feature

* Add comment about the generate_docs feature

---------

Co-authored-by: carter <carterjschultz@gmail.com>
* Update GitHub actions

* Downgrade rust-toolchain to the latest stable version (1.74.0) from 1.80.0

* Fix issues with latest clippy

* Fix rustdoc check issue

* Update Rust toolchain in Dockerfile
According to https://design.ros2.org/articles/wide_strings.html, the `string` and `wstring` types must use the UTF-8 and UTF-16 encodings (not necessarily enforced), cannot contain null bytes or words (enforced), and, when bounded, are measured in terms of bytes or words.

Moreover, though the rosidl_runtime_c `U16String` type uses `uint_least16_t`, Rust guarantees the existence of a precise `u16` type, so we should use that instead of `ushort`, which isn't guaranteed to be the same as `uint_least16_t` either. (Rust doesn't support platforms where `uint_least16_t != uint16_t`.)
* Use nightly for style check

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Install nightly for cargo +nightly fmt

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Fix style in examples

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Update style for rosidl_runtime_rs

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Add a comment indicating that nightly release is needed for formatting

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

---------

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
* WIP Adding describe paramater service

* Implement parameter setting services

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

* Restructure and cleanup

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

* Implement list_parameters with prefixes

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

* Minor cleanups

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

* Fix tests, cleanups

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

* Fix order of drop calls

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

* Add first bunch of unit tests for list and get / set parameters

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

* Clear warnings in rclrs

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

* Fix clippy, add set atomically tests

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

* Add describe parameter and get parameter types tests

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

* Minor cleanups, remove several unwraps

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

* Remove commented code

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

* Address first round of feedback

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

* Allow undeclared parameters in parameter getting services

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

* Clippy

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

* Run rustfmt

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Update rclrs/src/parameter/service.rs

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

* Change behavior to return NOT_SET for non existing parameters

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

* Make use_sim_time parameter read only

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

* Format

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

* Add a comment to denote why unwrap is safe

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

* Use main fmt

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

* Add a builder parameter to start parameter services

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

---------

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Co-authored-by: Michael X. Grey <mxgrey@intrinsic.ai>
Co-authored-by: jhdcs <48914066+jhdcs@users.noreply.github.com>
…` (#416)

* Add in missing nullptr check when calling `std::slice::from_raw_parts`

* Added missing testcase
* Added action template

* Added action generation

* Added basic create_action_client function

* dded action generation

* checkin

* Fix missing exported pre_field_serde field

* Removed extra code

* Sketch out action server construction and destruction

This follows generally the same pattern as the service server. It
required adding a typesupport function to the Action trait and pulling
in some more rcl_action bindings.

* Fix action typesupport function

* Add ActionImpl trait with internal messages and services

This is incomplete, since the service types aren't yet being generated.

* Split srv.rs.em into idiomatic and rmw template files

This results in the exact same file being produced for services,
except for some whitespace changes. However, it enables actions to
invoke the respective service template for its generation, similar to
how the it works for services and their underlying messages.

* Generate underlying service definitions for actions

Not tested

* Add runtime trait to get the UUID from a goal request

C++ uses duck typing for this, knowing that for any `Action`, the type
`Action::Impl::SendGoalService::Request` will always have a `goal_id`
field of type `unique_identifier_msgs::msg::UUID` without having to
prove this to the compiler. Rust's generics are more strict, requiring
that this be proven using type bounds.

The `Request` type is also action-specific as it contains a `goal` field
containing the `Goal` message type of the action. We therefore cannot
enforce that all `Request`s are a specific type in `rclrs`.

This seems most easily represented using associated type bounds on the
`SendGoalService` associated type within `ActionImpl`. To avoid
introducing to `rosidl_runtime_rs` a circular dependency on message
packages like `unique_identifier_msgs`, the `ExtractUuid` trait only
operates on a byte array rather than a more nicely typed `UUID` message
type.

I'll likely revisit this as we introduce more similar bounds on the
generated types.

* Integrate RMW message methods into ActionImpl

Rather than having a bunch of standalone traits implementing various
message functions like `ExtractUuid` and `SetAccepted`, with the
trait bounds on each associated type in `ActionImpl`, we'll instead add
these functions directly to the `ActionImpl` trait. This is simpler on
both the rosidl_runtime_rs and the rclrs side.

* Add rosidl_runtime_rs::ActionImpl::create_feedback_message()

Adds a trait method to create a feedback message given the goal ID and
the user-facing feedback message type. Depending on how many times we do
this, it may end up valuable to define a GoalUuid type in
rosidl_runtime_rs itself. We wouldn't be able to utilize the
`RCL_ACTION_UUID_SIZE` constant imported from `rcl_action`, but this is
pretty much guaranteed to be 16 forever.

Defining this method signature also required inverting the super-trait
relationship between Action and ActionImpl. Now ActionImpl is the
sub-trait as this gives it access to all of Action's associated types.
Action doesn't need to care about anything from ActionImpl (hopefully).

* Add GetResultService methods to ActionImpl

* Implement ActionImpl trait methods in generator

These still don't build without errors, but it's close.

* Replace set_result_response_status with create_result_response

rclrs needs to be able to generically construct result responses,
including the user-defined result field.

* Implement client-side trait methods for action messages

This adds methods to ActionImpl for creating and accessing
action-specific message types. These are needed by the rclrs
ActionClient to generically read and write RMW messages.

Due to issues with qualified paths in certain places
(rust-lang/rust#86935), the generator now
refers directly to its service and message types rather than going
through associated types of the various traits. This also makes the
generated code a little easier to read, with the trait method signatures
from rosidl_runtime_rs still enforcing type-safety.

* Format the rosidl_runtime_rs::ActionImpl trait

* Wrap longs lines in rosidl_generator_rs action.rs

This at least makes the template easier to read, but also helps with the
generated code. In general, the generated code could actually fit on one
line for the function signatures, but it's not a big deal to split it
across multiple.

* Use idiomatic message types in Action trait

This is user-facing and so should use the friendly message types.

* Cleanup ActionImpl using type aliases

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

* Formatting

* Switch from std::os::raw::c_void to std::ffi::c_void

While these are aliases of each other, we might as well use the more
appropriate std::ffi version, as requested by reviewers.

* Clean up rosidl_generator_rs's cmake files

Some of the variables are present but no longer used. Others were not
updated with the action changes.

* Add a short doc page on the message generation pipeline

This should help newcomers orient themselves around the rosidl_*_rs
packages.

---------

Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Co-authored-by: Esteve Fernandez <esteve@apache.org>
Co-authored-by: Michael X. Grey <grey@openrobotics.org>
* Update for clippy 1.83

Signed-off-by: Michael X. Grey <greyxmike@gmail.com>

* Update clippy for cfg(test) as well

Signed-off-by: Michael X. Grey <greyxmike@gmail.com>

* Exclude dependencies from clippy test

Signed-off-by: Michael X. Grey <greyxmike@gmail.com>

* Fix clippy for rosidl_runtime_rs

Signed-off-by: Michael X. Grey <greyxmike@gmail.com>

---------

Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
Copy link
Contributor

@nwn nwn left a comment

Choose a reason for hiding this comment

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

I know this was discussed in the working group meeting, but it might be worth recording here the reasoning for why we're splitting rosidl_runtime_rs into a separate repo from the generator in rosidl_rust. I believe we chose to model this after how it's done for Python with the rosidl_python/rosidl_runtime_py split, rather than how it's done for C++, where everything is in rosidl. However, I don't recall the reasoning behind that.

Copy link
Contributor

@maspe36 maspe36 left a comment

Choose a reason for hiding this comment

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

LGTM, but yeah good question @nwn. Do we really gain anything by having the generator and the runtime being separate repos? Do we ever actually want to version these separately?

@mxgrey
Copy link
Contributor

mxgrey commented Feb 4, 2025

Do we really gain anything by having the generator and the runtime being separate repos?

Yes, the first thing to be aware of is that the ROS buildfarm currently requires all packages within the same repo to have the same version number. I can't speak to the reason for that requirement, but I've been told it's baked in right now and inviolable.

The next thing to be aware of is that any time a version gets bumped for any package on the buildfarm, it will have to rebuild all packages that (transitively) depend on the one whose version was bumped.

Our goal is for rosidl_generator_rs to become a default IDL generator, which means all message packages on the buildfarm will depend on it. Any version bump on rosidl_generator_rs will force a version bump for all message packages.

In contrast, rosidl_runtime_rs will only be a dependency for rclrs and any nodes built with rclrs. That's a much smaller pool of dependencies than rosidl_generator_rs will have.

By keeping them in separate repos, we can make changes to rosidl_runtime_rs and as long as those changes are compatible with the current version of rosidl_generator_rs, we will not need to trigger a rebuild for all message packages on the buildfarm.

Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Retaining all the commits is really great. The loss of timestamps is unfortunate, but at least we can see the order of all the commits, so we can still retrace how the implementation evolved over time.

Very clean migration 👍

@esteve esteve force-pushed the add-rosidl_runtime_rs branch 2 times, most recently from 2018cf2 to 4993476 Compare February 5, 2025 12:51
@esteve
Copy link
Contributor Author

esteve commented Feb 5, 2025

@maspe36 @nwn @mxgrey thanks for your reviews. I gave it another try and I've managed to keep the timestamp thanks to this answer on Stackoverflow:

https://stackoverflow.com/a/5520650

For future reference the command I used was:

$ git filter-branch --commit-filter 'export GIT_COMMITTER_NAME="$GIT_AUTHOR_NAME"; export GIT_COMMITTER_EMAIL="$GIT_AUTHOR_EMAIL"; export GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE"; git commit-tree "$@"' -- origin/main..HEAD

I was also able to keep the original author for each commit without adding myself as a committer.

@esteve esteve merged commit 69411d3 into ros2-rust:main Feb 5, 2025
@esteve
Copy link
Contributor Author

esteve commented Feb 5, 2025

Ugh, turns out that when merging this onto main, GitHub decided to do a rebase and undo the timestamp trick, now all the commits have the current timestamp. I'll do a force push onto main to correct that.

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.

9 participants