-
Notifications
You must be signed in to change notification settings - Fork 248
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
(API) Generate bagfile metadata in Writer #184
(API) Generate bagfile metadata in Writer #184
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.
looks good to me except one comment.
Will reiterate once the dependent PRs are addressed.
e1e4bf9
to
0534b50
Compare
0534b50
to
2dd8ada
Compare
@Karsten1987 I merged #184 into this PR and rebased onto master. |
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.
So I think I stick to my point that having a metadata class instance makes things easier. In the generate_metadata()
function, we can then tackle remaining things like the duration, but in general I think it's easier to update the metadata on the fly.
With that, are you going to remove the function get_metadata()
from the storage interfaces here as well? I feel like that should be part of that PR as well.
@Karsten1987 ok I reworked it so that it can build as much of |
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.
really only a few nitpicks. But generally looks good.
rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_info_interface.hpp
Show resolved
Hide resolved
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.
Left a few comments, I am worried that we may not have proper testing here around the implicit FSM declared in this class (ctor / open / create_topic / remote_topic / read / write / close / dtor being states and not all transitions being allowed).
edit: ^ this is a general concern, probably too big to be addressed in this PR.
The dtor also is doing more than calling close, which makes me nervous (storage factory reset being put aside, which anyway is not necessary here IIUC).
We should also add test to this PR
4b98107
to
7567751
Compare
Opened Issue #195 regarding tests on FSM since the logic in |
7567751
to
7b44f79
Compare
The problem in Windows builds was due to Windows including Edit: if(WIN32)
add_definitions(-DNOMINMAX)
endif() |
Signed-off-by: Zachary Michaels <[email protected]>
c5240e4
to
8754a0d
Compare
Signed-off-by: Zachary Michaels <[email protected]>
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.
The code LGTM, we should just resolve Karsten's comment on returning dummy paths and testing for those.
Co-Authored-By: Thomas Moulard <[email protected]> Signed-off-by: Zachary Michaels <[email protected]>
Co-Authored-By: Thomas Moulard <[email protected]> Signed-off-by: Zachary Michaels <[email protected]>
Signed-off-by: Zachary Michaels <[email protected]>
9653160
to
0614d30
Compare
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.
LGTM 🚀
@piraka9011 one last time, but with more unit tests |
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 looks good to me (with green CI).
Just a nitpick with renaming the constants header file.
Signed-off-by: Zachary Michaels <[email protected]>
|
Please run a new round of CI on this. Also make sure you build packages up to ros2bag and rosbag2_tests and test with packages-select-regex rosbag2 |
Gist: https://gist.githubusercontent.com/piraka9011/d15a12ab0ff8cd4c3f20c7c1f14180b8/raw/7cda0198d9bee189ba8f61a3da0f8801c1ee378e/ros2.repos |
Given that this PR is touching more than one package, I think we should test all rosbag2 related packages. |
Sorry I missed that. Here's the new build: Gist: https://gist.githubusercontent.com/piraka9011/d15a12ab0ff8cd4c3f20c7c1f14180b8/raw/7cda0198d9bee189ba8f61a3da0f8801c1ee378e/ros2.repos |
Signed-off-by: Zachary Michaels <[email protected]>
Gist: https://gist.githubusercontent.com/piraka9011/21c04ad8e74d0f75a3f6bfae01264072/raw/7cda0198d9bee189ba8f61a3da0f8801c1ee378e/ros2.repos |
@Karsten1987 we're green! |
This is part of an effort to rework PR #158 into multiple, smaller PRs.
Changes
Writer
can now createBagMetadata
, this enables splitting of bagfiles without having to merge intermediate bagfile metadata.Dependent PRs
Issues