-
Notifications
You must be signed in to change notification settings - Fork 194
feat: add serialized pub/sub APIs #592
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Re-exports the generated low-level rcl_bindings module from rclrs so downstream crates can use raw rcl/rmw APIs without duplicating bindings.
Changes:
- Makes the internal
rcl_bindingsmodule public (pub mod rcl_bindings;).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -204,7 +204,7 @@ mod worker; | |||
| #[cfg(test)] | |||
| mod test_helpers; | |||
|
|
|||
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib.rs has #![warn(missing_docs)], and CI runs cargo rustdoc -- -D warnings / cargo clippy ... -D warnings. Making rcl_bindings public without a doc comment will emit a missing_docs warning for this module item and fail CI. Add a /// doc comment for pub mod rcl_bindings; (or annotate this item with #[allow(missing_docs)] / #[doc(hidden)] if you don't want it documented).
| /// Low-level bindings to the underlying ROS 2 `rcl` C API. |
|
Is this possibly a duplicate of the feature introduced by #492 ? |
luca-della-vedova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't done Rust in a few months (life!) but I have a few comments
| pub fn create_serialized_subscription<'a>( | ||
| &self, | ||
| topic_type: MessageTypeName, | ||
| options: impl Into<SubscriptionOptions<'a>>, | ||
| ) -> Result<SerializedSubscription, RclrsError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is a bit odd for a subscription, instead of specifying a callback we return a subscription and then leave it to the user to call SerializedSubscription::take whenever they can to see whether a message is available or not.
If I understand correctly, this would be the equivalent of rclcpp create_generic_subscription? In that case should we try to get a consistent API with other subscriptions that are callback based except this callback will have a serialized message as a parameter?
Following up on @mxgrey's note, I also wonder if we can merge the implementation of generic + dynamic pub/sub. It seems to me that both need to look for the message types at runtime and the only difference is that the serializes pub/sub have a callback with raw bytes, while the dynamic pub/sub has a callback with (DynamicMessage, MessageInfo).
If my understanding is correct, how about reusing the dynamic subscription API and just make the callback generic so we can get raw bytes if needed?
| pub fn create_serialized_publisher<'a>( | ||
| &self, | ||
| topic_type: MessageTypeName, | ||
| options: impl Into<crate::PublisherOptions<'a>>, | ||
| ) -> Result<SerializedPublisher, RclrsError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment to the dynamic subscription on whether we can streamline this with the dynamic message implementation.
From my understanding, the only difference here is that the publish API takes in a &SerializedMessage rather than a DynamicMessage. What if we shared the implementation by doing something like:
- Remove this function.
- Have the
DynamicPublisher::publishfunction take in a generic argument for the message that implements a trait with a function to "call rcl and publish this item". - We implement the trait for both
DynamicMessagefor dynamic message, with the current implementation. - We also implement the trait for
SerializedMessage, where it just callsrcl_publish_serialized_messageas you did. - Users can now publish either type of message because the publish signature is some sort of
msg: impl GenericPublishableor whatever bikeshedding.
Does this make sense?
What
rcl_bindings(pub mod rcl_bindings) to enable low-level extensions.SerializedMessageSerializedSubscriptionSerializedPublisherNode::create_serialized_subscription(...)andNode::create_serialized_publisher(...).Why
Enables native tools (e.g. rosbag MCAP record/play) to work with raw serialized CDR bytes without needing generated Rust message crates.
Notes
DynamicMessageMetadata) so it works for any installed message type.