SW-4837: replace the use of ros service to retrieve sensor metadata with latched topics foxy #103
Conversation
| @@ -30,7 +30,9 @@ class OusterReplay : public OusterSensorNodeBase { | |||
|
|
|||
| try { | |||
| auto meta_file = parse_parameters(); | |||
| populate_metadata_from_json(meta_file); | |||
| create_metadata_publisher(); | |||
| load_metadata_from_file(meta_file); | |||
There was a problem hiding this comment.
So would user be able to replay the BAGs and omit the metadata_file path altogether if the metadata topic present in the replayed bag?
Also how would the 2 metadata sources will be handled if one metadata is present in .bag and it's also specified as a param metadata? :
ros2 launch ouster_ros replay.launch.xml \
metadata:=<json file name> \
bag_file:=<path to rosbag file>
There was a problem hiding this comment.
Yes, the user wouldn't need to supply the metadata path (I have marked it as an optional parameter and noted that in the README.md)
If the user still provides a metadata path along with a bag file that contains the metadata topic then both messages will be emitted, the last one to be published will remain in effect.
There was a problem hiding this comment.
maybe it makes sense to treat the metadata from the file as the only valid source and not make the two metadata compete? My thought is that if someone is specifying metadata para they either know that metadata topic is not present in the bag file or they want to explicitly overwrite the metadata that is recorded in the bag file with smth new.
There was a problem hiding this comment.
It does make sense, my issue with we can't selectively discard a specific topic.
The only way to achieve that is to explicitly set the topics to be replayed, which for us would be lidar_packets and imu_packets. This would in discard any other topics the user may have included in the bag (since the driver wouldn't know what is in the file).
There was a problem hiding this comment.
In a sense if the user provides metadata file we will have to exclude other topics that existed in the file. If the user doesn't then we can keep all topics.
There was a problem hiding this comment.
yeah, excluding all topics if metadata param is present maybe not as clear to the user ... hard choices what is right, maybe we can add yet another param for this? like "exclude_other_topics" that will be "on" if the metadata is present, and if it all set in a launch file it's at least somewhat visible to the user and they can overwrite the params as they like if they want different behavior.
There was a problem hiding this comment.
I would consider this option as part of node merging effort. The reason is when this is due, raw packets won't be emitted by default by the driver. They will only be specified when the user chooses to. During bag record and replay we want to have the raw packets. I am considering to rename record.launch to record_raw.launch and replay.launch to replay_raw.launch. In this case, only we are only concerned with raw packets and we would select raw packets only along with metadata (whether as a topic or being overriden by a file). This way it is clear that this record/replay functionality is only for sensor data.
Samahu
deleted the
SW-4837-replace-the-use-of-ros-service-to-retrieve-sensor-metadata-with-latched-topics-foxy
branch
Related Issues & PRs
Summary of Changes
/ouster/metadatatopic that is consumed by os_cloud and os_image nodes and save it to the bag file on recordValidation
all functionality maintained