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

Action message support #417

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Action message support #417

wants to merge 20 commits into from

Conversation

nwn
Copy link
Contributor

@nwn nwn commented Sep 28, 2024

This implements action support in rosidl_generator_rs and rosidl_runtime_rs. It's a selective rebase of the original changes made in #295 and #410, applying only changes to the two rosidl packages, without any modification to rclrs. The intention here is to merge these changes sooner so that we can split the message support packages into a separate repo and add them to the ROS buildfarm.

The general approach follows the same pattern as the existing message and service generation. However, the additional message wrapping and introspection done on actions means that we need generic access to certain message internals. In rclcpp, this is done using duck typing on the Action and Action::Impl template parameters. However, Rust is stricter about this and requires trait bounds to ensure that generic type parameters can be accessed in a given way. As a result, a series of trait methods are defined in ActionImpl that enable a client library like rclrs to generically create and access the various message types.

Certain of these methods involve accessing timestamps and UUIDs in messages. To avoid adding a dependency in rosidl_runtime_rs on builtin_interfaces and unique_identifier_msgs, these are represented in a more primitive form as (i32, u32) and [u8; 16] in the signatures, respectively. This is rather ugly, so ideas are welcome.

The existing srv.rs.em file is split into separate "idiomatic" and "RMW" implementations, similarly to how messages are already handled. This makes embedding the service template into the action.rs.em easier.

esteve and others added 20 commits September 28, 2024 15:41
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.
This is incomplete, since the service types aren't yet being generated.
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.
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.
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.
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).
These still don't build without errors, but it's close.
rclrs needs to be able to generically construct result responses,
including the user-defined result field.
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.
@nwn nwn mentioned this pull request Sep 30, 2024
@nwn nwn marked this pull request as ready for review September 30, 2024 12:33
Copy link
Contributor

@Guelakais Guelakais left a comment

Choose a reason for hiding this comment

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

I think it would be a good idea to create Rust packages on a test basis and check whether they generate warnings. I would also like to see the generated rust structures include a few derivable traits. I have attached a list. It all looks great in itself.

Common Derivable Rust Traits

In Rust, the derive attribute allows for automatic implementation of certain traits for structs and enums, simplifying the process of writing boilerplate code. Here are some of the most common derivable traits:

  1. Clone: This trait allows for creating a duplicate of a value. When you derive Clone, it enables the use of the clone method to create a copy of the instance.

  2. Copy: This trait provides "copy semantics" instead of "move semantics." Types that implement Copy can be duplicated simply by copying bits, which is useful for simple data types.

  3. Debug: This trait enables formatting a value using the {:?} formatter, which is particularly useful for debugging purposes.

  4. Default: The Default trait allows you to create a default value for a type. When derived, it implements the default function, which calls the default function on each field of the type.

  5. PartialEq: This trait allows for equality comparisons between instances of a type. Deriving PartialEq enables the use of the == and != operators.

  6. Eq: This trait is a marker trait that indicates that a type's equality comparison is reflexive, symmetric, and transitive. It is typically derived alongside PartialEq.

  7. PartialOrd: This trait allows for partial ordering of instances, enabling the use of comparison operators like <, >, <=, and >=.

  8. Ord: This trait provides a total ordering for instances of a type. It is derived from PartialOrd and allows for the use of the sort method.

  9. Hash: This trait allows for computing a hash from an instance, which is essential for types that will be used in hash-based collections like HashMap.

@jhdcs
Copy link
Collaborator

jhdcs commented Sep 30, 2024

I think one of the difficulties of deriving traits for message types is that we have very little information about the individual messages before compile time. We cannot be sure that any of these traits are derivable before the generators create the code.

Additionally, these messages are interacted with through the FFI boundary, which can potentially cause some weirdness at the best of times. I would be cautious about the automatic derivation of any traits unless we are 100% certain that we aren't going to have a problem introduced due to the FFI interactions. Admittedly this is a rare issue, but I feel it's best to be cautious when dealing with FFI.

Is there a specific reason that you would like to see these traits automatically derived?

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.

4 participants