From 7923b807cd5c1aaef809700c63b52273264e45f4 Mon Sep 17 00:00:00 2001 From: Old-Ding <35417409+Old-Ding@users.noreply.github.com> Date: Sat, 11 Jul 2026 14:47:49 +0800 Subject: [PATCH 1/2] Fix PlayOptions YAML round trips The encoder writes action filters, loop, and remapping options, but the decoder ignored those keys. Decode every existing encoded key and add complete text YAML round-trip coverage for the current converter contract. Signed-off-by: Old-Ding <35417409+Old-Ding@users.noreply.github.com> --- rosbag2_transport/CMakeLists.txt | 4 + .../src/rosbag2_transport/play_options.cpp | 8 ++ .../rosbag2_transport/test_play_options.cpp | 89 +++++++++++++++++++ 3 files changed, 101 insertions(+) create mode 100644 rosbag2_transport/test/rosbag2_transport/test_play_options.cpp diff --git a/rosbag2_transport/CMakeLists.txt b/rosbag2_transport/CMakeLists.txt index b330049f8c..039c9e631a 100644 --- a/rosbag2_transport/CMakeLists.txt +++ b/rosbag2_transport/CMakeLists.txt @@ -559,6 +559,10 @@ if(BUILD_TESTING) test/rosbag2_transport/test_record_options.cpp) target_link_libraries(test_record_options ${PROJECT_NAME}) + ament_add_gmock(test_play_options + test/rosbag2_transport/test_play_options.cpp) + target_link_libraries(test_play_options ${PROJECT_NAME}) + ament_add_gmock(test_type_description_hash test/rosbag2_transport/test_type_description_hash.cpp) target_link_libraries(test_type_description_hash ${PROJECT_NAME}) diff --git a/rosbag2_transport/src/rosbag2_transport/play_options.cpp b/rosbag2_transport/src/rosbag2_transport/play_options.cpp index ccdc767354..8c9b47c433 100644 --- a/rosbag2_transport/src/rosbag2_transport/play_options.cpp +++ b/rosbag2_transport/src/rosbag2_transport/play_options.cpp @@ -77,6 +77,8 @@ bool convert::decode( node, "topics_to_filter", play_options.topics_to_filter); optional_assign>( node, "services_to_filter", play_options.services_to_filter); + optional_assign>( + node, "actions_to_filter", play_options.actions_to_filter); optional_assign(node, "regex_to_filter", play_options.regex_to_filter); optional_assign( node, "exclude_regex_to_filter", @@ -85,10 +87,16 @@ bool convert::decode( node, "exclude_topics", play_options.exclude_topics_to_filter); optional_assign>( node, "exclude_services", play_options.exclude_services_to_filter); + optional_assign>( + node, "exclude_actions", play_options.exclude_actions_to_filter); optional_assign>( node, "topic_qos_profile_overrides", play_options.topic_qos_profile_overrides); + optional_assign(node, "loop", play_options.loop); + optional_assign>( + node, "topic_remapping_options", play_options.topic_remapping_options); + optional_assign(node, "clock_publish_frequency", play_options.clock_publish_frequency); optional_assign( diff --git a/rosbag2_transport/test/rosbag2_transport/test_play_options.cpp b/rosbag2_transport/test/rosbag2_transport/test_play_options.cpp new file mode 100644 index 0000000000..5b46e713aa --- /dev/null +++ b/rosbag2_transport/test/rosbag2_transport/test_play_options.cpp @@ -0,0 +1,89 @@ +// Copyright 2026 Old-Ding +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include + +#include +#include +#include + +#include "rosbag2_transport/play_options.hpp" + +TEST(play_options, yaml_round_trip_preserves_encoded_options) +{ + rosbag2_transport::PlayOptions original; + original.read_ahead_queue_size = 42; + original.node_prefix = "prefix"; + original.rate = 2.5f; + original.topics_to_filter = {"topic", "other_topic"}; + original.services_to_filter = {"service", "other_service"}; + original.actions_to_filter = {"action", "other_action"}; + original.regex_to_filter = "include.*"; + original.exclude_regex_to_filter = "exclude.*"; + original.exclude_topics_to_filter = {"excluded_topic"}; + original.exclude_services_to_filter = {"excluded_service"}; + original.exclude_actions_to_filter = {"excluded_action", "other_excluded_action"}; + original.topic_qos_profile_overrides.emplace( + "topic", rclcpp::QoS(10).transient_local()); + original.loop = true; + original.topic_remapping_options = {"--ros-args", "-r", "input:=output"}; + original.clock_publish_frequency = 10.0; + original.clock_publish_on_topic_publish = true; + original.clock_trigger_topics = {"trigger_topic"}; + original.delay = rclcpp::Duration(1, 2); + original.playback_duration = rclcpp::Duration(3, 4); + original.playback_until_timestamp = 5; + original.start_paused = true; + original.start_offset = 6; + original.disable_keyboard_controls = true; + original.wait_acked_timeout = 7; + original.disable_loan_message = true; + original.progress_bar_update_rate = 8; + original.progress_bar_separation_lines = 9; + + auto node = YAML::convert::encode(original); + std::stringstream serializer; + serializer << node; + auto reconstructed = YAML::Load(serializer.str()).as(); + + #define CHECK(field) EXPECT_EQ(original.field, reconstructed.field) + CHECK(read_ahead_queue_size); + CHECK(node_prefix); + CHECK(rate); + CHECK(topics_to_filter); + CHECK(services_to_filter); + CHECK(actions_to_filter); + CHECK(regex_to_filter); + CHECK(exclude_regex_to_filter); + CHECK(exclude_topics_to_filter); + CHECK(exclude_services_to_filter); + CHECK(exclude_actions_to_filter); + CHECK(topic_qos_profile_overrides); + CHECK(loop); + CHECK(topic_remapping_options); + CHECK(clock_publish_frequency); + CHECK(clock_publish_on_topic_publish); + CHECK(clock_trigger_topics); + CHECK(delay); + CHECK(playback_duration); + CHECK(playback_until_timestamp); + CHECK(start_paused); + CHECK(start_offset); + CHECK(disable_keyboard_controls); + CHECK(wait_acked_timeout); + CHECK(disable_loan_message); + CHECK(progress_bar_update_rate); + CHECK(progress_bar_separation_lines); + #undef CHECK +} From 42a0544c527dab175419e23bc9623645b3a775b7 Mon Sep 17 00:00:00 2001 From: Old-Ding <35417409+Old-Ding@users.noreply.github.com> Date: Mon, 13 Jul 2026 07:05:13 +0800 Subject: [PATCH 2/2] Fix negative Duration YAML encoding Use rclcpp's standard Duration message conversion so negative nanoseconds are normalized to a non-negative nanosecond field. This keeps the default PlayOptions acknowledgement timeout round-trippable through text YAML. Signed-off-by: Old-Ding <35417409+Old-Ding@users.noreply.github.com> --- rosbag2_storage/include/rosbag2_storage/yaml.hpp | 5 +++-- .../test/rosbag2_transport/test_play_options.cpp | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/rosbag2_storage/include/rosbag2_storage/yaml.hpp b/rosbag2_storage/include/rosbag2_storage/yaml.hpp index 0f27160686..ea7df925e3 100644 --- a/rosbag2_storage/include/rosbag2_storage/yaml.hpp +++ b/rosbag2_storage/include/rosbag2_storage/yaml.hpp @@ -104,9 +104,10 @@ struct convert { static Node encode(const rclcpp::Duration & duration) { + const builtin_interfaces::msg::Duration duration_msg = duration; Node node; - node["sec"] = duration.nanoseconds() / 1000000000; - node["nsec"] = duration.nanoseconds() % 1000000000; + node["sec"] = duration_msg.sec; + node["nsec"] = duration_msg.nanosec; return node; } diff --git a/rosbag2_transport/test/rosbag2_transport/test_play_options.cpp b/rosbag2_transport/test/rosbag2_transport/test_play_options.cpp index 5b46e713aa..19201ff876 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_play_options.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_play_options.cpp @@ -87,3 +87,18 @@ TEST(play_options, yaml_round_trip_preserves_encoded_options) CHECK(progress_bar_separation_lines); #undef CHECK } + +TEST(play_options, yaml_round_trip_preserves_default_negative_ack_timeout) +{ + rosbag2_transport::PlayOptions original; + + auto node = YAML::convert::encode(original); + EXPECT_EQ(-1, node["wait_acked_timeout"]["sec"].as()); + EXPECT_EQ(999999999U, node["wait_acked_timeout"]["nsec"].as()); + + std::stringstream serializer; + serializer << node; + auto reconstructed = YAML::Load(serializer.str()).as(); + + EXPECT_EQ(original.wait_acked_timeout, reconstructed.wait_acked_timeout); +}