Play old bagfiles #69
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Martin-Idel-SI
force-pushed
the
feature/play_old_bagfiles
branch
from
3043a5f
to
1f8c50c
Compare
|
I updated the PR and changed the description. There is a major problem left to be sorted out. Why does the |
@dirk-thomas does that tell you anything? |
Karsten1987
reviewed
Dec 6, 2018
| + <build_depend>ros1_bridge</build_depend> | ||
| <build_depend>boost</build_depend> | ||
| <build_depend>bzip2</build_depend> | ||
| - <build_depend version_gte="0.3.17">cpp_common</build_depend> |
There was a problem hiding this comment.
why aren't these deps necessary?
There was a problem hiding this comment.
This is similar to what the ros1_bridge does:
From my understanding from the discussion with Dirk Thomas on the ros1_bridge PR:
- We only
find_package,ament_export_dependenciesand put into thepackage.xmlall dependencies to ros2 packages and non-ROS packages (like bzip2 and boost). - All ros1 packages are found by the
find_ros1_package. This ensures that the dependency tree does not get confused between ros2 and ros1 packages, which don't play nice together. - At the moment (no environment hooks present) this also means that all consuming downstream packages need to find those dependencies again. This is not nice but can be amended by registering the correct hooks to
ament_package(however, I don't understand the process well enough to do this with limited time)
Martin-Idel-SI
force-pushed
the
feature/play_old_bagfiles
branch
2 times, most recently
from
e82cbeb
to
0e2252e
Compare
|
Here is a longer description of the problems with this build: Short version:On some systems, the build tries to find On yet other systems, the build actually links against Long version:System 1: It works but I get CMake warnings.System specs:
CMake problems: CMakeLists output for package rosbag2_bag_v2_plugins: But: Everything works. The test that generates the warning (there are a few other tests with the same warning) run without a hitch. Analysis: The problem seems to be (justification following in part 2) the call to nodeletlib. Here is the beginning of the CMake output generating the warning above - I highlighted the nodeletlib search by inserting blank lines However, nodeletlib, albeit wanted, isn't actually linked. This is the So: No nodelets and everything works fine. Why do I think that those are the problem? System 2: It doesn't link (with test) or doesn't run (without tests)System specs:
CMake problems: CMakeLists outputs the same problems for package rosbag2_bag_v2_plugins as above. The paths are just a little more telling because the build is isolated: However, this time the tests don't link: Analysis: The error speaks for itself: nodeletlib can't be linked against class_loader. The path above indicates that the ros2 class loader will be used to link against and obviously, this has to fail. If we don't build the tests, everything gets build, because the final linking is only done at startup time. That's when the problem resurfaces with a very nice crash. So, what's the difference? The CMake output of the build is similar to the first system - we search and find the module nodeletlib. However, this time, the library For some reasons, this bridge is linked against nodeletlib - and that's creating an issue further down the line. But why is it linked against nodeletlib? System 3 Everything worksSystem specifications
What happens? in the CMake build output simply don't exist nor does the CMake warning. No linking against nodeletlib occurs. |
Karsten1987
reviewed
Dec 6, 2018
|
|
||
| find_ros1_package(roscpp) | ||
| if(NOT ros1_roscpp_FOUND) | ||
| message(WARNING "Failed to find ROS 1 roscpp, skipping...") |
There was a problem hiding this comment.
Suggested change
| message(WARNING "Failed to find ROS 1 roscpp, skipping...") | |
| message(WARNING "Failed to find ROS 1 roscpp, skipping...") | |
| message(WARNING "Make sure ros1_bridge is correctly built") |
Could it be that one of the message packages links again |
|
@dirk-thomas Thanks. I looked at all of them and they were clear. However, nodelets apparently has services (if you source ros1 and call rossrv list, you get nodel/NodeletList and a few others), so that's the problem. But that means I can't link against ros1_bridge if pluginlib is used... |
Yeah, that sounds reasonable. It should be straight forward to skip that specific library then since it shouldn't be necessary in order to use the services from that package. |
|
I still ran into the cyclic dependencies, however all libs and tests were building and running correctly. @Martin-Idel-SI @anhosi Could you see if this works for you as well with the ros1bridge patch? |
Martin-Idel-SI
force-pushed
the
feature/play_old_bagfiles
branch
from
0e2252e
to
a67e864
Compare
|
@Karsten1987
This is from the code generation and I get the same error message when building N.B: I tried to switch paths around and it turns out that this will always only work if you source the ROS 1 workspace before you source the ROS 2 workspace. When you switch paths, not even |
Martin-Idel-SI
force-pushed
the
feature/play_old_bagfiles
branch
from
96bb875
to
ecdba1d
Compare
Karsten1987
force-pushed
the
feature/play_old_bagfiles
branch
from
ecdba1d
to
a9ae225
Compare
- previously in rosbag2::Info - now in storage plugin
…en trying to read a rsbag v2 bag file
-This package will contain storage and converter plugins
- massive if/else between all message types - will be generated similar to ros1_bridge plugin
- Use files from ros1_bridge via PkgConfig
- move generation templates outside of plugin folders as both plugins need it - use ros::serialization routines to deserialize the ros message
- With the rosbag_v2_converter_plugin, we don't need to treat rosbag_v2 storage any different
- Only needed if we want to link against the library
- This is necessary as the information is used by rosbag2_transport - ros2 bag info still shows all topics and types - rosbag::View::getConnections() can return multiple connections corresponding to the same topic
- use a bagfile with messages not known to ros2
…ing for a topic
Co-Authored-By: Martin-Idel-SI <external.Martin.Idel@bosch-si.com>
- Improve toplevel CMakeLists - Put all patches into a resource subfolder
This switches the order of ros2 and ros1 directories resulting in build failures
This can lead to switching ros1 and ros2 include paths resulting in missing symbols as the wrong pluginlib gets included
Karsten1987
force-pushed
the
feature/play_old_bagfiles
branch
from
a9ae225
to
2f959a9
Compare
Karsten1987
approved these changes
Dec 15, 2018
sriramster
pushed a commit
to sriramster/rosbag2
that referenced
this pull request
Feb 28, 2019
* ros2GH-69 Read storage content in a separate thread For now the publishing starts only after the reading is completly done. This should change aufter ros2GH-68 is done and a thread-safe queue can be used instead of std::queue. * ros2GH-71 Add integration test for timing behavior * ros2GH-68 Introduce vendor package for shared queue - Download and install headers from moodycamel readerwriterqueue - Download and install headers from moodycamel concurrentqueue - Use readerwriterqueue in code to load and publish concurrently * ros2GH-71 Retain time difference of messages when playing a bag file - The main (play) thread sleeps until the time for publishing the message is reached. - Using std::chrono time_point and duration for type-safe time arithmetic instead of rcutils time types. * ros2GH-71 Improve stability of read test - Subscribers need to maintain a longer history if the messages are not consumed fast enough. * ros2GH-71 Fix Classloader instance lifetime The Classloader instance needs to outlive all objects created by it. * ros2GH-71 Extract playing code into a class of its own Reason: record and play have almost no common code but do the exact opposite with the storage and rclcpp. * ros2GH-70 Do not link explicitly against std_msgs - only required in tests - this decreases the amount of packages needed for a clean build without tests * ros2GH-70 Fix error message of storage * ros2GH-70 Fix pluginlib/storage issue for recording * ros2GH-71 Cleanup: variable naming * ros2GH-70 Load storage continuously instead of as fast as possible - Only load if queue contains less than 1000 messages - Wait a millisecond before loading again once the queue is long enough * ros2GH-70 Add options struct to allow specification of queue size * ros2GH-72 Wait for messages to fill up * ros2GH-74 Rename integration tests to play/record tests * ros2GH-74 Use test_msgs in integration tests - gets rid of string_msgs dependency * ros2GH-70 Rename is_not_ready to is_pending, use bulk reading to queue * ros2GH-70 Harmonize storage_loading_future variable * ros2GH-88 Read messages in order of their timestamps - Currently, we write sequentially in order of arrival time so reading in id order is fine - This may change at a later time and should not change the reading behaviour, i.e. we need to read in order of timestamps * Fix compiler error on Mac * ros2GH-8 Fix: use correct ros message type in test * ros2GH-8 Cleanup: minor code style fixes * ros2GH-8 Refactor future usage in player Make the future a class member of player to avoid having to hand it into several functions which is difficult with a move-only type. * ros2GH-8 Cleanup: remove verbose logging for every stored message * ros2GH-8 Refactor rosbag2 interface Add an explicit overload for record without a topic_names argument to record all topics. * fix: call vector.reserve instead of default initalization * fix record demo
james-rms
added a commit
to james-rms/rosbag2
that referenced
this pull request
Nov 17, 2022
Signed-off-by: James Smith <james@foxglove.dev>
james-rms
added a commit
to james-rms/rosbag2
that referenced
this pull request
Nov 17, 2022
Signed-off-by: James Smith <james@foxglove.dev>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a plugin to rosbag to play back rosbags of ros1. It relies on #70
Build instructions:
Technicalities:
ros2 bag infohas a new flag-sto specify the storage format (this is necessary as rosbags from ros1 don't have a yaml file).ros2 bag infoon a rosbag file from ros1, all contents will be shown. The user will however see a warning for all topics that cannot be played back.ros2 bag play, only those topics will be played back that can be converted to ros2 topics.rosbag_storage, the PR introduces a vendor package which rebuildsrosbag_storage(prefixed byros1_to avoid linker confusion).ros1_bridge, hence the same limitations apply as forros1_bridge.Known issues:
The problem is once again
libclass_loader.so. For some unknown (to me) reason, theros1_bridgeis linked againstlibnodeletlib.soon Linux, which in turn uses the pluginlib. This only happens on some systems, while others declare a dependency, but don't actually link it anywhere (this leads to a CMake error, but everything works smoothly).We currently don't understand why the
ros1_bridgesometimes links againstlibnodeletlib.soor at least finds the package. @Karsten1987 Do you have any idea?Not in this PR:
ros2 bagCLI can only handle one file/folder as an argument. In order to use split bagfiles, several arguments must be given. We don't support it for now.ros1_bridge, this won't happen.Decisions:
ros1_bridge). Is this the desired behaviour? Or should we only build if some cmake-args are set?