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

Support loaned message for serialized data #1837

Closed
Barry-Xu-2018 opened this issue Dec 7, 2021 · 7 comments
Closed

Support loaned message for serialized data #1837

Barry-Xu-2018 opened this issue Dec 7, 2021 · 7 comments
Labels

Comments

@Barry-Xu-2018
Copy link
Collaborator

rosbag2 sends serialized data by rclcpp::GenericPublisher.
Now there is no interface to use loaned message for serialized data (DDS not support it).
In order to use current loaned message implementation, whether we can add new interface to rclcpp::GenericPublisher ?

e.g.

void GenericPublisher::try_publish_loaned_msg(const rclcpp::SerializedMessage & message)
{
    if (this->can_loan_messages()) {
      rcl_borrow_loaned_message(.., loaned_msg);
      rmw_deserialize(message.get_rcl_serialized_message(), .., loaned_msg);
      rcl_publish_loaned_message(..., loaned_msg, null);
    } else {
      publish(message); // Call original publish()
    }
}

Any thoughts?

@fujitatomoya
Copy link
Collaborator

@ivanpauno @wjwwood @jacobperron
CC: @emersonknapp

this is ros2 bag play to enable LoanedMessage for zero copy enhancement. would you share your thoughts about this?

@emersonknapp
Copy link
Collaborator

I agree with the approach you are taking. I think that this new interface would be very useful, and it's good to put it in rclcpp so that others could use it too. It will be important to document that deserialization happens within this function, so the user should be aware of that for performance.

@fujitatomoya
Copy link
Collaborator

@emersonknapp thanks for the comment.

It will be important to document that deserialization happens within this function, so the user should be aware of that for performance.

agree 👍

@fujitatomoya
Copy link
Collaborator

@ivanpauno @wjwwood friendly ping, what would you think about this?

@fujitatomoya
Copy link
Collaborator

@ivanpauno @wjwwood @jacobperron friendly ping.

@jacobperron
Copy link
Member

jacobperron commented Jan 5, 2022

IMO, I would just throw an error if the publisher doesn't support loaned messages. Otherwise, it's not clear exactly what the call will do unless the user calls can_loan_messages(). I imagine usage would look like this:

if (publisher->can_loan_messages()) {
    publisher->publish_loaned_message(message);
} else {
    publisher->publish(message);
}

It just seems more clear from the outside what is happening compared with having the can_loan_messages() check inside the publish method.

@wjwwood
Copy link
Member

wjwwood commented Feb 17, 2022

I agree with @jacobperron, have it throw and let callers do the try if they want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants