Action message support

[nwn]

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.

[jhdcs]

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?

[mxgrey]

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.

I'm not so sure this is true. We know three important things:

• Every message definition is a struct composed of some combination of other messages or of ROS message primitives (e.g. f32, f64, u32, i32, [T], ...).
• Every ROS message primitive can implement Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, and Hash (the only one missing is Copy).
• Every struct composed of things that implement those traits can also derive those traits

So I believe we could trivially derive all the popular traits besides Copy automatically for all message definitions.

[mxgrey]

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

I'm strongly in favor of providing the implementations for the sake of developer quality of life.

For better or worse, ROS message data structures play a big role in the internal mechanics of most ROS nodes.

• Being able to easily clone when needed can be vital to writing reasonable control flows.
• Debug would help people inspect their nodes and write useful log messages
• Hash and Eq help with caching and recognizing redundant messages
• PartialOrd and Ord can help with organizing or sorting data, but these are probably the least important of the traits, so I wouldn't feel strongly about including this or not.

[mxgrey]

All that said, I might suggest we save traits for a follow up PR.

[mxgrey]

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.

It would definitely be nice if we can clean up that situation a bit. I don't love what I'm about to suggest, but here's a stab at having something a bit more pleasant:

• Define newtype structs in the rosidl_runtime_rs such as Timestamp and Uuid to wrap this data
• Add a special condition to rosidl_generator_rs to check if we are generating bindings for builtin_interfaces::Time or unique_identifier_msgs::Uuid
• If we are generating one of the special cases, then include an impl From<Newtype> for Message for each. Perhaps also give each message struct a method to convert it into the relevant rosidl_runtime_rs newtype.

Let me know if anyone has thoughts on that. I can take a stab at drafting it if there are no strong objections.

[Guelakais]

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?

In my opinion, this makes handling the structs feel more idiomatic. When I program other things with rust, I expect these standard traits from the imported structs. It also makes the handling of the structs simpler, as certain things like comparisons or the output of the structs in the standard output then already work. You get a better feel for what the struct is doing. It's also a bit awkward every time you have to implement it yourself. In many cases it works with the newtype pattern. In some cases you have to dig deeper into the material. It doesn't matter if it doesn't work. I thought it would be a nice idea. On the other hand: Ros messages are defined by design so that they can only consist of primitive data types. This goes so far that it is basically not possible to transmit enum symbols with Ros. As far as I know, strings in Ros are equivalent to rusts String.

[nwn]

@Guelakais I assume we're only talking about the message types, since the top-level action type is an empty struct that's only useful as a generic type parameter. It's not useful to create an actual instance of this type, so I don't see a need for any trait derivations there.

For the message types, I agree that it's useful to have certain common traits implemented on them. Regarding the list you gave:

• Clone, Debug, PartialEq, and PartialOrd are already derived for today's message types.
• Default is manually implemented for message types, using the *__init() functions generated by rosidl_generator_c.
• Copy is not universally possible since some messages may contain non-Copy fields like strings and sequences. Since we can't always determine whether a non-primitive field is Copy or not, we would only be able to implement this for messages containing only primitive Copy fields.
• Eq, Ord, and Hash are not universally applicable since the primitives f32 and f64 don't implement these traits. This could be worked around using something like ordered-float to wrap the fields in the implementations, but I'm not sure if that is always correct.

So the traits that are trivially derivable are already done.

In any case, I'd say this should be discussed in a separate issue since the message generation isn't actually touched in this PR.

[nwn]

@mxgrey

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.

It would definitely be nice if we can clean up that situation a bit. I don't love what I'm about to suggest, but here's a stab at having something a bit more pleasant:

* Define newtype structs in the `rosidl_runtime_rs` such as `Timestamp` and `Uuid` to wrap this data

* Add a special condition to `rosidl_generator_rs` to check if we are generating bindings for `builtin_interfaces::Time` or `unique_identifier_msgs::Uuid`

* If we are generating one of the special cases, then include an `impl From<Newtype> for Message` for each. Perhaps also give each message struct a method to convert it into the relevant `rosidl_runtime_rs` newtype.

Let me know if anyone has thoughts on that. I can take a stab at drafting it if there are no strong objections.

An alternative we could consider would be this:

• Redefine ActionImpl as rosidl_runtime_rs::ActionImpl<TimeMsg, UuidMsg>, taking two generic type parameters that it uses in all the relevant spots in its method signatures.
• In rosidl_generator_rs, where the generated code already depends on the concrete builtin_interfaces and unique_identifier_msgs packages, have the action struct impl rosidl_runtime_rs::ActionImpl<builtin_interfaces::msg::rmw::Time, unique_identifier_msgs::msg::rmw::UUID>. The method implementations would then use the concrete message types.
• In rclrs, use rosidl_runtime_rs::ActionImpl<builtin_interfaces::msg::rmw::Time, unique_identifier_msgs::msg::rmw::UUID> as a trait bound on actions so that we can access the concrete methods.

Without trait aliases, this might be a bit cumbersome to work with on the rclrs side, but it avoids special casing in the generator.

[Guelakais]

@Guelakais I assume we're only talking about the message types, since the top-level action type is an empty struct that's only useful as a generic type parameter. It's not useful to create an actual instance of this type, so I don't see a need for any trait derivations there.

For the message types, I agree that it's useful to have certain common traits implemented on them. Regarding the list you gave:

* `Clone`, `Debug`, `PartialEq`, and `PartialOrd` are already derived for today's message types.

* `Default` is manually implemented for message types, using the `*__init()` functions generated by `rosidl_generator_c`.

* `Copy` is not universally possible since some messages may contain non-`Copy` fields like strings and sequences. Since we can't always determine whether a non-primitive field is `Copy` or not, we would only be able to implement this for messages containing only primitive `Copy` fields.

* `Eq`, `Ord`, and `Hash` are not universally applicable since the primitives `f32` and `f64` don't implement these traits. This could be worked around using something like [ordered-float](https://docs.rs/ordered-float/latest/ordered_float/struct.OrderedFloat.html) to wrap the fields in the implementations, but I'm not sure if that is always correct.

So the traits that are trivially derivable are already done.

In any case, I'd say this should be discussed in a separate issue since the message generation isn't actually touched in this PR.

OH, I didn't know that. If I didn't know that - I should make a pull request to improve the documentation of the codegenerator. :)

I didn't want to cause so much confusion. all in all, the integration of actions is great.

A small note: The communication system of ros2 has strong dependencies on the DDS interface. Depending on which DDS is used here, this can lead to strange effects. How does the rust api handle this?

[mxgrey]

In rclrs, use rosidl_runtime_rs::ActionImpl<builtin_interfaces::msg::rmw::Time, unique_identifier_msgs::msg::rmw::UUID> as a trait bound on actions so that we can access the concrete methods.

This is a very interesting idea for how to break the circular dependency. One nitpick I might have is that if a trait has generic parameters, that implies that users should expect it to be possible for the same data structure to implement the same trait with different generic arguments. I think a slightly better arrangement might be to use associated types:

pub trait ActionImpl {
    type GoalUuid;
    type Timestamp;
    ...
}

then if rosidl_generator_rs can just always use

impl ActionTrait for <T> {
    type GoalUuid = unique_identifier_msgs::msg::rmw::UUID;
    type Timestamp = builtin_interfaces::msg::rmw::Time;
}

Then rclrs can use

Action: ActionImpl<GoalUuid = unique_identifier_msgs::msg::rmw::UUID, Timestamp = builtin_interfaces::msg::rmw::Time>

as a trait bound when needed.

That being said, I don't see anything wrong with special-casing builtin_interfaces and unique_identifier_msgs since those are foundational message packages that play a special role in the internal mechanics of actions. I suspect that having these two special cases in the message generation would be a small price to pay to avoid juggling lots of generics that aren't otherwise necessary.

[nwn]

I know this may be asking a lot for such complex message generation, but can you add some high level docs?

@maspe36 I've added a short doc page under docs/message-generation.md. Let me know what you think.

[nwn]

@esteve & @jhdcs, I think this is ready to merge, in case you would like to take a look.

[maspe36]

This LGTM, we can address anything more in follow up PRs. Thanks @nwn!