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

use rclcpp serialized messages to write data #457

Merged
merged 4 commits into from
Mar 24, 2021

Conversation

Karsten1987
Copy link
Collaborator

@Karsten1987 Karsten1987 commented Jul 10, 2020

Next on the list towards a user-friendly C++ API. Sits on top of "defaults" branch.

The implementation here is pretty straightforward even though we have to iterate over the existing data structures used here at some point, namely the rosbag2_storage::SerializedBagMessage. We're dealing with shared pointers here in order to ease the cleanup of the underlying uint8 buffer. However, this prevents any stack allocations and makes it pretty cumbersome to integrate with the existing rclcpp::SerializedMessage API.

As a second point, I personally don't see a way around the rclcpp dependency at this point. Even though I'd like to keep the number of dependencies here fairly small, in order to eventually come up with a template<class MsgT> void write(MsgT & msg) API, we'd need to have that dependency.

I'll open this PR as a draft to start a discussion.

@emersonknapp
Copy link
Collaborator

Maybe I'm missing some context - now that rclcpp::SerializedMessage exists (and if we pull a dependency on rclcpp) - do we still need the rosbag2 SerializedBagMessage structure? Or do they provide the same functionality

@Karsten1987
Copy link
Collaborator Author

fair point. I am a big fan of making the serialized bag message a POD, essentially removing the shared pointer and only having the actual buffer pointer.

We could get around rclcpp if the rosbag2 api only introduces API which takes a serialized data buffer (uint8_t*), but yes, if we introduce rclcpp, I think it makes sense to remove the serialized bag message altogether.

@Karsten1987
Copy link
Collaborator Author

oh, also to be clear. The rclcpp dependency will only be introduced at rosbag2_cpp, not below. So we still need to keep the SerializedBagMessage struct around. It should really only be a container for the data buffer IMO.

@emersonknapp
Copy link
Collaborator

Makes sense. I think it's fine to add the rclcpp dependency. We were trying to limit it to just rosbag2_transport, right? However, it does seem reasonable to say "you're using a ROS2-related C++ API, it requires the C++ client library"

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-07-16/15468/1

@emersonknapp
Copy link
Collaborator

@Karsten1987 did this already get implemented in a different PR? Any need to keep this draft open?

@Karsten1987 Karsten1987 changed the base branch from defaults to master March 16, 2021 02:34
@Karsten1987 Karsten1987 marked this pull request as ready for review March 16, 2021 02:35
@Karsten1987 Karsten1987 requested a review from a team as a code owner March 16, 2021 02:35
@Karsten1987 Karsten1987 requested review from emersonknapp and mjeronimo and removed request for a team March 16, 2021 02:35
@Karsten1987
Copy link
Collaborator Author

@emersonknapp please have a look at this. I've just finished the reader and writer API in conjunction with the rclcpp addition. The API should now look relatively close to the ROS1 cookbook snippets.

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

This LGTM - it is a clean interface

rosbag2_cpp/package.xml Show resolved Hide resolved
rosbag2_cpp/include/rosbag2_cpp/writer.hpp Outdated Show resolved Hide resolved
@Karsten1987
Copy link
Collaborator Author

I never run CI on this:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Karsten1987 and others added 3 commits March 23, 2021 15:48
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987 Karsten1987 merged commit d09cdaa into master Mar 24, 2021
@delete-merged-branch delete-merged-branch bot deleted the rclcpp_serialized_messages branch March 24, 2021 04:19
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.

3 participants