Message generation refactoring by nnmm · Pull Request #80 · ros2-rust/ros2_rust

nnmm · 2022-03-05T12:25:31Z

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

How to review

I know that this is a really large PR. If desired, I can split it up for reviewing. Here are some steps I suggest to get familiar with the changes:

Run the message_demo and read the code
Generate cargo docs for the rosidl_runtime_rs crate and read them
Dive into msg.rs.em and __init__.py files

nnmm · 2022-03-05T12:27:18Z

There is currently a limitaiton where I'm using copy functions (ros2/rosidl#650) that are not yet in Foxy and Galactic. Jacob wanted to take a look at backporting those. However, I could also try to do a polyfill for them if we don't want to wait that long.

jhdcs · 2022-03-07T12:27:48Z

There is currently a limitaiton where I'm using copy functions (ros2/rosidl#650) that are not yet in Foxy and Galactic. Jacob wanted to take a look at backporting those. However, I could also try to do a polyfill for them if we don't want to wait that long.

What would be involved in the creation of a "polyfill"? I'm not familiar with the process.

nnmm · 2022-03-07T12:33:08Z

There is currently a limitaiton where I'm using copy functions (ros2/rosidl#650) that are not yet in Foxy and Galactic. Jacob wanted to take a look at backporting those. However, I could also try to do a polyfill for them if we don't want to wait that long.

What would be involved in the creation of a "polyfill"? I'm not familiar with the process.

I'd try implementing the functions from that PR in Rust, using rcutils_get_default_allocator(). So rosidl_runtime_rs would need to link against rcutils for the time being.

esteve · 2022-03-07T12:41:51Z

@nnmm awesome work, thanks! CI seems to be failing because of a missing message, the file that's causing the problem is NestedType.msg, any reference to it should be removed if we're no longer using it.

nnmm · 2022-03-07T12:46:22Z

@nnmm awesome work, thanks! CI seems to be failing because of a missing message, the file that's causing the problem is NestedType.msg, any reference to it should be removed if we're no longer using it.

Oh, forgot to add it, fixed now. CI will still fail because of the issue mentioned above (the copy functions being Rolling-only for now). I still opened this PR for review to parallelize reviewing + work on that issue.

jhdcs · 2022-03-07T13:16:32Z

There is currently a limitaiton where I'm using copy functions (ros2/rosidl#650) that are not yet in Foxy and Galactic. Jacob wanted to take a look at backporting those. However, I could also try to do a polyfill for them if we don't want to wait that long.

What would be involved in the creation of a "polyfill"? I'm not familiar with the process.

I'd try implementing the functions from that PR in Rust, using rcutils_get_default_allocator(). So rosidl_runtime_rs would need to link against rcutils for the time being.

I think implementing the functions in Rust for now might be good, provided the work isn't too difficult. That way, if I get pulled away to work on other projects you won't be stuck waiting on me for this to get merged/working.

esteve · 2022-03-07T13:42:55Z

rclrs/src/node/mod.rs

        qos: QoSProfile,
    ) -> Result<Publisher<T>, RclReturnCode>
    where
-        T: MessageDefinition<T>,


Why was the generic type parameter dropped?

I just didn't see any reason why it should be there.

esteve · 2022-03-07T13:44:52Z

rclrs_example_msgs/CMakeLists.txt

@@ -0,0 +1,30 @@
+cmake_minimum_required(VERSION 3.5)


Instead of adding rclrs_example_msgs, I'd prefer using the example_interfaces or test_interface_files packages.

The test_interface_files looks good, thanks for the tip.

We need ros2/test_interface_files#18 to make that work, I believe

I'd propose to merge the version with the rclrs_example_msgs if the PR I linked isn't approved by the time the rest of the review is finished, and then later remove it once it is.

Seems like bringing in test_interface_files is more complicated than we thought. Would you be fine with leaving in rclrs_example_msgs for now?

rosidl_generator_rs/cmake/rosidl_generator_rs_generate_interfaces.cmake

esteve · 2022-03-07T13:51:45Z

@nnmm I just did a first pass. This is really awesome, you truly outdid yourself dude! I'll do a more extensive review later, but this looks great.

rclrs_examples/src/minimal_publisher.rs

rclrs_example_msgs/msg/VariousTypes.msg

jhdcs

Mostly a few comments/clarifications that I'd like to know about. Looks excellent, though!

rclrs_example_msgs/msg/VariousTypes.msg

rosidl_generator_rs/resource/msg.rs.em

rosidl_runtime_rs/build.rs

jhdcs · 2022-03-07T17:12:19Z

rosidl_runtime_rs/src/sequence.rs

+            // If there is no more capacity for the next element, resize to the
+            // next power of two.
+            //
+            // A pedantic implementation would check for usize overflow here, but


Should we be pedantic, though? If ros2_rust is being used on safety-critical systems, they may want this sort of check. Or is there a better value to test against? What would the performance impact be?

Yeah, good question. I'm not worried about performance impact, but about code complexity/overengineering.

I think ROS 2 messages will never reach this limit – certainly not on 64-bit machines, and I don't know if anyone is building ROS 2 for 32-bit systems. There is also a 1 GB message size limit, so even if we have a 32-bit system, there won't be any sequences with more than usize/2 elements. Besides, there are probably plenty of unchecked additions in rcl and rmw that would cause trouble too.

So I propose to not be pedantic about usize overflow, but to just leave comments like this one when such an issue is encountered. If anyone ever really wants to use ros2_rust in a safety-critical context, they'll need to go through the code and refactor a whole lot of stuff anyway, and then at least there's a comment to point to potential issues.

rosidl_runtime_rs/src/traits.rs

jhdcs

Just another comment, mostly a stylistic thing. Otherwise, this looks good to me!

Thank you very much for your work!

jhdcs · 2022-03-08T13:28:15Z

rosidl_runtime_rs/src/string.rs

+        impl Clone for $string {
+            fn clone(&self) -> Self {
+                let mut msg = Self::default();
+                // SAFETY: This is doing the same thing as rosidl_runtime_c__String__copy.


I'm not sure this comment addresses why this is safe. I feel like these safety comments should be written so that someone won't need to dive into an external C library to verify, unless they really wanted to.

Something like: "This is safe because neither self.data nor self.size are mutated, and self.data is guaranteed to be pointing to valid memory by this point, and self.size is guaranteed to be correct"?

I rephrased it to be a little clearer. The safety argument is basically: The rosidl_runtime_c__String__copy function is implemented exactly the same way as this Clone instance: https://github.com/ros2/rosidl/blob/caef9b8f820dfcb16e6a2df9118c5669cc816642/rosidl_runtime_c/src/string_functions.c#L135-L143

We may assume that the libraries we're binding to are correct, so when we do the same thing as that library, it's safe.

esteve · 2022-03-08T15:01:49Z

@nnmm given RFC344 is there a need for BoundedSequence and BoundedString? Checking for length is done in setters in other ROS2 bindings instead of using a separate structure.

nnmm · 2022-03-08T20:42:11Z

@nnmm given RFC344 is there a need for BoundedSequence and BoundedString? Checking for length is done in setters in other ROS2 bindings instead of using a separate structure.

I'd say that direct access to a field is the most flexible and ergonomic option, and only when that for some reason is not allowed (e.g. because it would allow violating some invariant) are getters and setters needed.

This PR also has TryFrom<&str> for BoundedString and I will also add TryFrom<&[T]> for bounded sequences. With that, the pattern of building up a string/sequence outside and checking the length when it's inserted into the message is also covered.

nnmm · 2022-03-08T20:44:08Z

@esteve Do you know how to turn off these cpplint_rosidl_generated_rs, cppcheck_rosidl_generated_rs, uncrustify_rosidl_generated_rs tests in CI?

esteve · 2022-03-10T09:59:50Z

rclrs/src/node/publisher.rs

        })
    }

-    pub fn publish(&self, message: &T) -> Result<(), RclReturnCode> {


Re-add reference to message here

Done, in a sense.

What do you mean by "in a sense", are there any caveats?

It's not message: &T now, it's message: impl MessageCow, which accepts T and &T (aka theConvert trait from the Matrix chat).

Suggestions for a better name welcome. In case you dislike this design, we could also split it up into two functions, publish_owned() and publish_ref().

I don't get it, you seemed to be very keen on not having a separate create_subscription function for RMW-compatible types, but publish_owned and publish_ref are ok. Could you elaborate?

Let's keep it as is now, though, and we could refine the APIs later if needed, though it'll be more painful.

Sure. The distinction between publish_owned() and publish_ref() is of course orthogonal to the question of which message type is published. But you're right that both are a "have two functions for doing the thing vs. having one more general function" question.

In this case here, I'm more on the fence between splitting vs unifying, since the more general function comes at the cost of a more complicated signature: The MessageCow trait. In the case of the extra create_subscription function, the create_subscription function would look the same whether or not we also have a create_rmw_subscription, so there's no extra complexity in the unified version. Being simpler is not the only argument in favor of one subscription function, but I'll keep that discussion in the other thread.

README.md

esteve · 2022-03-10T10:11:30Z

rclrs/src/node/mod.rs

@@ -109,7 +110,7 @@ impl Node {
        callback: F,


Given that the RmwMessage API is specific to ros2-rust, I prefer if there are two separate functions here. One for the expected normal ROS2 API (i.e. idiomatic), and another (e.g. called create_subscription_rmw_type) for RmwMessage. That way it's more explicit and the latter can be marked as an optimization for Rust.

Why would we have a redundant function like this? The current Subscription API can receive Messages, and both idiomatic and RMW-compatible messages are instances of that. Adding a second API that only supports the latter type is redundant and confuses only, imo. Uniformity is nice.

Precisely for uniformity with the other bindings, no other bindings accept these so-call rmw types, all of them support idiomatic types only. Adding a second API clears that confusion by explicitly signaling that it's specific to ros2-rust. Rust, very much like Python, and unlike C++, encourages explicitness vs implicitness. Explicitness is nice too.

I don't want the more efficient message type to become a second-class citizen by only being accessible through less-than-canonical APIs. There shouldn't be any confusion with the two message types, since (1) I tried to document it really well and (2) even if someone skips the documentation, they will not be confused, they simply won't know that there's also the RMW-compatible message type. For such a user, the API will arguably appear more uniform with other languages than if there was an create_subscription_rmw_type function they need to look up.

Besides, there is also lots of stuff that is specific to rclcpp, and none of it is marked as such (topic statistics, generic pub/sub, intraprocess, etc.).

through less-than-canonical APIs

The RMW-compatible message API is already non-canonical, because it does not have any counterpart in the other bindings. APIs are not only about names, but also semantics, the arguments they accept and the values they return. Having a distinctly different name emphasizes that it behaves differently from the other bindings, and that users should expect differences.

Besides, there is also lots of stuff that is specific to rclcpp, and none of it is marked as such (topic statistics, generic pub/sub, intraprocess, etc.).

Because the goal is that eventually that functionality might be incorporated into other bindings. For example, rclc could potentially get support for intraprocess.

Anyway, I rather not waste more time on this, but it's important to get APIs right early on, because changing them later is far more difficult

As for counterparts in other bindings, here's my perspective for posterity:

Python is slow anyway, and usability of a raw C message (= our "RMW-compatible" message) would be absymal from a Python perspective, so of course the messages in that language are an idiomatic version that are converted to/from the C message.

C and C++ have the privilege of being natively RMW-compatible, so they can be both idiomatic and maximally efficient. They are the counterpart to the RMW-compatible and the idiomatic messages.

I assume other language bindings also use their own idiomatic message types that are converted to/from C messages. However, I believe none of the other languages are quite as performance-focused as Rust/C/C++.

Hence, Rust is kind of unique, in that its users expect maximum performance but it can't achieve that (yet) by serializing/deserializing its standard library types to/from CDR directly. So you could argue that it would be canonical to use RMW-compatible messages because that's what the other performance-focused language bindings do. Or you could argue that it would be canonical to use idiomatic messages because that's what all language bindings do.

I think the solution in this PR, to offer both, isn't too bad. And I placed the idiomatic messages in the <pkg>::msg namespace and the RMW-compatible ones in <pkg>::msg::rmw, because they can't be both in the same namespace, but I believe we shouldn't hide the RMW-compatible messages more than that if it's not technically necessary.

I don't think the time spent discussing on this is wasted. Hope I'm not too stubborn! If you insist, I'll add the separate function. Or I can also poll the ROS + Rust people I know to get more input. But since you approved, I take it that it's okay to merge as it is now and potentially discuss it again later.

I assume other language bindings also use their own idiomatic message types that are converted to/from C messages. However, I believe none of the other languages are quite as performance-focused as Rust/C/C++.

Ada reuses the C data layout in memory, so there is no overhead involved. The price is somewhat un-idiomatic use when sequences are involved that I hope to alleviate at some point in the future.

rclrs_examples/src/minimal_publisher.rs

rclrs_example_msgs/msg/VariousTypes.msg

esteve · 2022-03-10T11:30:13Z

@esteve Do you know how to turn off these cpplint_rosidl_generated_rs, cppcheck_rosidl_generated_rs, uncrustify_rosidl_generated_rs tests in CI?

I'll look into it, neither of those tests should run on Rust files and there are no more C files to be checked.

nnmm · 2022-03-10T20:29:02Z

Thanks for fixing the linter @esteve. Now this PR is ready to merge from my side: I added the workaround for the copy_sequence functions in Foxy and Galactic until the backports are shipped, and changed the publishing + conversion logic to allow publishing of both borrowed and owned messages, for maximum efficiency.

esteve · 2022-03-11T12:12:23Z

Thanks for fixing the linter @esteve. Now this PR is ready to merge from my side: I added the workaround for the copy_sequence functions in Foxy and Galactic until the backports are shipped, and changed the publishing + conversion logic to allow publishing of both borrowed and owned messages, for maximum efficiency.

Thanks, there is still some unanswered feedback, could you address it? CI is not passing either, do you know why?

esteve · 2022-03-17T16:09:01Z

@nnmm awesome job! I've approved these changes, but there are conflicts with main because of #81 I think removing rclrs_msg_utilities should suffice.

@verbatim

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

This requires a cfg for conditionally compiling the code depending on the ROS distro. I'm setting that cfg in the build script instead of colcon-ros-cargo so that building with cargo doesn't become too annoying.

nnmm · 2022-03-18T11:13:10Z

Huge thanks for reviewing @esteve and also @jhdcs! 🎉

nnmm requested a review from esteve March 5, 2022 12:27

nnmm mentioned this pull request Mar 6, 2022

Building/testing rosidl_runtime_rs with pure cargo produces linker errors #79

Closed

nnmm force-pushed the no_c_glue branch 2 times, most recently from 9dcea47 to a91ab7c Compare March 6, 2022 14:12

nnmm force-pushed the no_c_glue branch from a91ab7c to f7e1371 Compare March 7, 2022 12:45

esteve requested changes Mar 7, 2022

View reviewed changes

rclrs_examples/src/minimal_publisher.rs Outdated Show resolved Hide resolved

nnmm commented Mar 7, 2022

View reviewed changes

rclrs_example_msgs/msg/VariousTypes.msg Show resolved Hide resolved

jhdcs reviewed Mar 7, 2022

View reviewed changes

nnmm force-pushed the no_c_glue branch 2 times, most recently from 54e662c to f26454c Compare March 8, 2022 10:58

jhdcs previously approved these changes Mar 8, 2022

View reviewed changes

nnmm dismissed jhdcs’s stale review via 855a93f March 8, 2022 21:17

nnmm force-pushed the no_c_glue branch from f26454c to 855a93f Compare March 8, 2022 21:17

esteve requested changes Mar 10, 2022

View reviewed changes

nnmm force-pushed the no_c_glue branch from 3dcebef to c7024ab Compare March 10, 2022 20:20

nnmm force-pushed the no_c_glue branch from c7024ab to 4c0f6ae Compare March 11, 2022 12:53

nnmm force-pushed the no_c_glue branch 4 times, most recently from 4c0f6ae to 3aecf1f Compare March 13, 2022 15:39

nnmm mentioned this pull request Mar 14, 2022

Update package.xml ros2/test_interface_files#18

Merged

esteve previously approved these changes Mar 17, 2022

View reviewed changes

nnmm and others added 7 commits March 18, 2022 10:54

Remove C++ linters for generated Rust files

0d5b92a

Reimplement sequence_copy on Foxy and Galactic

33077d0

This requires a cfg for conditionally compiling the code depending on the ROS distro. I'm setting that cfg in the build script instead of colcon-ros-cargo so that building with cargo doesn't become too annoying.

Allow publishing messages as references

715f0db

Unify publish and publish_ref

c523531

Remove usage of the sequence_copy functions until they are released

f08c3c9

Clean up a little bit

a3bb51d

nnmm dismissed esteve’s stale review via a3bb51d March 18, 2022 09:57

nnmm force-pushed the no_c_glue branch from 8da496d to a3bb51d Compare March 18, 2022 09:57

Cargo fmt

1214355

nnmm force-pushed the no_c_glue branch from dcd154d to 1214355 Compare March 18, 2022 10:44

esteve approved these changes Mar 18, 2022

View reviewed changes

nnmm merged commit df971f1 into master Mar 18, 2022

esteve deleted the no_c_glue branch March 18, 2022 11:21

This was referenced Mar 18, 2022

Twist geometry_msg always read as all zeroes #37

Closed

Array content not published #27

Closed

IDL Support (Dashing+ Support) #21

Closed

esteve mentioned this pull request Apr 3, 2022

spin() does not check which subscription has received messages #93

Closed

Conversation

nnmm commented Mar 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Not yet implemented

How to review

Uh oh!

nnmm commented Mar 5, 2022

Uh oh!

jhdcs commented Mar 7, 2022

Uh oh!

nnmm commented Mar 7, 2022

Uh oh!

esteve commented Mar 7, 2022

Uh oh!

nnmm commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhdcs commented Mar 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

esteve commented Mar 7, 2022

Uh oh!

Uh oh!

Uh oh!

jhdcs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhdcs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esteve commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nnmm commented Mar 8, 2022

Uh oh!

nnmm commented Mar 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnmm Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nnmm commented Mar 5, 2022 •

edited

Loading

nnmm commented Mar 7, 2022 •

edited

Loading

esteve commented Mar 8, 2022 •

edited

Loading

nnmm Mar 11, 2022 •

edited

Loading

esteve Mar 11, 2022 •

edited

Loading

esteve commented Mar 17, 2022 •

edited

Loading