Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Specialize BagCallbackT for MessageInstance#1374

Merged
mikepurvis merged 2 commits intoros:melodic-develfrom
efernandez:support_bag_player_messageinstance_callback
Apr 25, 2018
Merged

Specialize BagCallbackT for MessageInstance#1374
mikepurvis merged 2 commits intoros:melodic-develfrom
efernandez:support_bag_player_messageinstance_callback

Conversation

@efernandez
Copy link
Copy Markdown
Contributor

Specialize BagCallbackT for MessageInstance so the overloaded call method calls the callback directly with the MessageInstance object.

This way the client callback can access the connection information, which is useful when processing a bag file to create a new one b/c then we can keep the message time and connection information, e.g. the latch property.

@efernandez
Copy link
Copy Markdown
Contributor Author

FYI @dirk-thomas

@efernandez efernandez force-pushed the support_bag_player_messageinstance_callback branch from 408ad60 to ec366a6 Compare April 23, 2018 00:17
@mikepurvis
Copy link
Copy Markdown
Member

Looks reasonable— related to #1372 in terms of overall objective, though that one is for the filter command whereas this is the C++ bag API.

👍

@mikepurvis
Copy link
Copy Markdown
Member

@ros-pull-request-builder retest this please

@mikepurvis
Copy link
Copy Markdown
Member

Since this is a header-only change, a CI pass isn't super meaningful. It's certainly not a complicated implementation, but I wonder if there might be some kind of trivial test which could be added to test_rosbag_storage?

And call the callback directly with the MessageInstance, so the client
callback can access the connection information.
This is useful when processing a bag file to create a new one b/c we can
keep the message time and connection information, e.g. the latch
property.
@efernandez efernandez force-pushed the support_bag_player_messageinstance_callback branch from ec366a6 to fbd5fe7 Compare April 23, 2018 22:27
@efernandez
Copy link
Copy Markdown
Contributor Author

@mikepurvis I agree, I'd add a test to test_rosbag_storage.
Do you know where the bag files used in those tests are?
For example, I see /tmp/swap1.bag, but I can't see where it's on the repo or any cmake rule to download it.

@mikepurvis
Copy link
Copy Markdown
Member

@efernandez It gets written as part of the test:

void writeBags(rosbag::CompressionType a, rosbag::CompressionType b) {
using std::swap;
rosbag::Bag bag1("/tmp/swap1.bag", rosbag::bagmode::Write);
rosbag::Bag bag2("/tmp/swap2.bag", rosbag::bagmode::Write);

@efernandez
Copy link
Copy Markdown
Contributor Author

efernandez commented Apr 24, 2018

Thanks.

I've just added a unit test (creating a simple test bag on runtime as the other tests in this package do).

@efernandez efernandez force-pushed the support_bag_player_messageinstance_callback branch 2 times, most recently from c64a0ef to 2dee8c6 Compare April 24, 2018 19:12
@efernandez efernandez force-pushed the support_bag_player_messageinstance_callback branch from 2dee8c6 to e9c4819 Compare April 24, 2018 19:12
@mikepurvis
Copy link
Copy Markdown
Member

@ros-pull-request-builder retest this please

@mikepurvis
Copy link
Copy Markdown
Member

Remaining failures are known-flaky. Thanks for this contribution!

@mikepurvis mikepurvis merged commit 2f7ccc5 into ros:melodic-devel Apr 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants