doc: readme: fix example conversion configs: all -> all_topics #1661

ottojo · 2024-05-19T17:23:31Z

this was changed in #1480, see issue #1660

Signed-off-by: Jonas Otto <[email protected]>

fujitatomoya

good eye! lgtm.

@MichaelOrlov @Barry-Xu-2018 this actually breaks the user configuration as reported. i will go through the documentation again.

Barry-Xu-2018 · 2024-05-20T02:08:03Z

The RecordOptions has already been changed quite a bit.
Refer to the code below.

rosbag2/rosbag2_transport/src/rosbag2_transport/record_options.cpp

Lines 58 to 103 in 5a06430

    
           bool convert<rosbag2_transport::RecordOptions>::decode( 
        
             const Node & node, rosbag2_transport::RecordOptions & record_options, int version) 
        
           { 
        
             optional_assign<bool>(node, "all_topics", record_options.all_topics); 
        
             optional_assign<bool>(node, "all_services", record_options.all_services); 
        
             optional_assign<bool>(node, "is_discovery_disabled", record_options.is_discovery_disabled); 
        
             optional_assign<std::vector<std::string>>(node, "topics", record_options.topics); 
        
             optional_assign<std::vector<std::string>>(node, "topic_types", record_options.topic_types); 
        
             optional_assign<std::vector<std::string>>(node, "services", record_options.services); 
        
             optional_assign<std::string>( 
        
               node, "rmw_serialization_format", record_options.rmw_serialization_format); 
        
             optional_assign<std::chrono::milliseconds>( 
        
               node, "topic_polling_interval", record_options.topic_polling_interval); 
        
             optional_assign<std::string>(node, "regex", record_options.regex); 
        
             optional_assign<std::string>(node, "exclude_regex", record_options.exclude_regex); 
        
             optional_assign<std::vector<std::string>>(node, "exclude_topics", record_options.exclude_topics); 
        
             optional_assign<std::vector<std::string>>( 
        
               node, "exclude_topic_types", 
        
               record_options.exclude_topic_types); 
        
             optional_assign<std::vector<std::string>>( 
        
               node, "exclude_services", record_options.exclude_service_events); 
        
             optional_assign<std::string>(node, "node_prefix", record_options.node_prefix); 
        
             optional_assign<std::string>(node, "compression_mode", record_options.compression_mode); 
        
             optional_assign<std::string>(node, "compression_format", record_options.compression_format); 
        
             optional_assign<uint64_t>(node, "compression_queue_size", record_options.compression_queue_size); 
        
             optional_assign<uint64_t>(node, "compression_threads", record_options.compression_threads); 
        
             optional_assign<int32_t>( 
        
               node, "compression_threads_priority", 
        
               record_options.compression_threads_priority); 
        
             std::unordered_map<std::string, rclcpp::QoS> qos_overrides; 
        
             if (node["topic_qos_profile_overrides"]) { 
        
               qos_overrides = YAML::decode_for_version<std::unordered_map<std::string, rclcpp::QoS>>( 
        
                 node["topic_qos_profile_overrides"], version); 
        
             } 
        
             record_options.topic_qos_profile_overrides = qos_overrides; 
        
             optional_assign<bool>(node, "include_hidden_topics", record_options.include_hidden_topics); 
        
             optional_assign<bool>( 
        
               node, "include_unpublished_topics", 
        
               record_options.include_unpublished_topics); 
        
             optional_assign<bool>( 
        
               node, "disable_keyboard_controls", 
        
               record_options.disable_keyboard_controls);

The current README.md only mentions part of it. Is it necessary to point to the relevant code, so users know what can be configured ? @fujitatomoya @MichaelOrlov

rosbag2/README.md

Lines 221 to 240 in 5a06430

    
           ``` 
        
           output_bags: 
        
           - uri: /output/bag1  # required 
        
             storage_id: ""  # will use the default storage plugin, if unspecified 
        
             max_bagfile_size: 0 
        
             max_bagfile_duration: 0 
        
             storage_preset_profile: "" 
        
             storage_config_uri: "" 
        
             all: false 
        
             topics: [] 
        
             rmw_serialization_format: ""  # defaults to using the format of the input topic 
        
             regex: "" 
        
             exclude: "" 
        
             compression_mode: "" 
        
             compression_format: "" 
        
             compression_queue_size: 1 
        
             compression_threads: 0 
        
             include_hidden_topics: false 
        
             include_unpublished_topics: false 
        
           ```

Barry-Xu-2018 · 2024-05-20T02:13:43Z

@ottojo

Additionally. exclude should be changed to exclude_regex.
Please refer to

rosbag2/rosbag2_transport/src/rosbag2_transport/record_options.cpp

Lines 58 to 103 in 5a06430

    
           bool convert<rosbag2_transport::RecordOptions>::decode( 
        
             const Node & node, rosbag2_transport::RecordOptions & record_options, int version) 
        
           { 
        
             optional_assign<bool>(node, "all_topics", record_options.all_topics); 
        
             optional_assign<bool>(node, "all_services", record_options.all_services); 
        
             optional_assign<bool>(node, "is_discovery_disabled", record_options.is_discovery_disabled); 
        
             optional_assign<std::vector<std::string>>(node, "topics", record_options.topics); 
        
             optional_assign<std::vector<std::string>>(node, "topic_types", record_options.topic_types); 
        
             optional_assign<std::vector<std::string>>(node, "services", record_options.services); 
        
             optional_assign<std::string>( 
        
               node, "rmw_serialization_format", record_options.rmw_serialization_format); 
        
             optional_assign<std::chrono::milliseconds>( 
        
               node, "topic_polling_interval", record_options.topic_polling_interval); 
        
             optional_assign<std::string>(node, "regex", record_options.regex); 
        
             optional_assign<std::string>(node, "exclude_regex", record_options.exclude_regex); 
        
             optional_assign<std::vector<std::string>>(node, "exclude_topics", record_options.exclude_topics); 
        
             optional_assign<std::vector<std::string>>( 
        
               node, "exclude_topic_types", 
        
               record_options.exclude_topic_types); 
        
             optional_assign<std::vector<std::string>>( 
        
               node, "exclude_services", record_options.exclude_service_events); 
        
             optional_assign<std::string>(node, "node_prefix", record_options.node_prefix); 
        
             optional_assign<std::string>(node, "compression_mode", record_options.compression_mode); 
        
             optional_assign<std::string>(node, "compression_format", record_options.compression_format); 
        
             optional_assign<uint64_t>(node, "compression_queue_size", record_options.compression_queue_size); 
        
             optional_assign<uint64_t>(node, "compression_threads", record_options.compression_threads); 
        
             optional_assign<int32_t>( 
        
               node, "compression_threads_priority", 
        
               record_options.compression_threads_priority); 
        
             std::unordered_map<std::string, rclcpp::QoS> qos_overrides; 
        
             if (node["topic_qos_profile_overrides"]) { 
        
               qos_overrides = YAML::decode_for_version<std::unordered_map<std::string, rclcpp::QoS>>( 
        
                 node["topic_qos_profile_overrides"], version); 
        
             } 
        
             record_options.topic_qos_profile_overrides = qos_overrides; 
        
             optional_assign<bool>(node, "include_hidden_topics", record_options.include_hidden_topics); 
        
             optional_assign<bool>( 
        
               node, "include_unpublished_topics", 
        
               record_options.include_unpublished_topics); 
        
             optional_assign<bool>( 
        
               node, "disable_keyboard_controls", 
        
               record_options.disable_keyboard_controls);

…readme Signed-off-by: Jonas Otto <[email protected]>

ottojo · 2024-05-20T11:09:34Z

@Barry-Xu-2018 thanks for pointing me to the corresponding parts in the source. I updated the exclude key and added the other keys relevant for topic and service selection. I did not add all keys found in the source, i think some may already be excluded on purpose, because they are not relevant for bag conversion (such as for example disable_keyboard_controls, compression_threads_priority and topic_polling_interval)

ottojo · 2024-05-20T11:15:06Z

oh, i also just notice that it's not only the RecordingOptions but also StorageOptions, where the actual options are that i wanted to use prior to hitting this issue:

rosbag2/rosbag2_storage/src/rosbag2_storage/storage_options.cpp

Lines 44 to 56 in 5a06430

    
           storage_options.uri = node["uri"].as<std::string>(); 
        
           optional_assign<std::string>(node, "storage_id", storage_options.storage_id); 
        
           optional_assign<uint64_t>(node, "max_bagfile_size", storage_options.max_bagfile_size); 
        
           optional_assign<uint64_t>(node, "max_bagfile_duration", storage_options.max_bagfile_duration); 
        
           optional_assign<uint64_t>(node, "max_cache_size", storage_options.max_cache_size); 
        
           optional_assign<std::string>( 
        
             node, "storage_preset_profile", storage_options.storage_preset_profile); 
        
           optional_assign<std::string>(node, "storage_config_uri", storage_options.storage_config_uri); 
        
           optional_assign<bool>(node, "snapshot_mode", storage_options.snapshot_mode); 
        
           optional_assign<int64_t>(node, "start_time_ns", storage_options.start_time_ns); 
        
           optional_assign<int64_t>(node, "end_time_ns", storage_options.end_time_ns); 
        
           using KEY_VALUE_MAP = std::unordered_map<std::string, std::string>; 
        
           optional_assign<KEY_VALUE_MAP>(node, "custom_data", storage_options.custom_data);

fujitatomoya

lgtm.

i do not think we need to introduce all options here, but all options need to be aligned with the implementation.

MichaelOrlov

I think it will be valuable to add all_services: true for examples with bag compression and bag split.
To avoid other potential questions in the future, like "Why services were lost during conversion?"

README.md

Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Jonas Otto <[email protected]>

MichaelOrlov

LGTM. Thanks for the PR!

clalancette

Looks good to me as well.

Given that this is a change to documentation, we can merge this without CI, but @MichaelOrlov I'll wait for you to do that. I think we should also backport this to the jazzy branch.

MichaelOrlov · 2024-05-21T17:53:41Z

Merging without CI run since changes only in the readme.md file.

MichaelOrlov · 2024-05-21T17:54:10Z

https://github.com/Mergifyio backport jazzy

mergify · 2024-05-21T17:54:23Z

backport jazzy

✅ Backports have been created

#1668 doc: readme: fix example conversion configs: all -> all_topics (backport #1661) has been created for branch jazzy

* fix example conversion configs: all -> all_topics Signed-off-by: Jonas Otto <[email protected]> * update topic and service selection arguments in conversion config in readme Signed-off-by: Jonas Otto <[email protected]> * add all_services: true to first example Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Jonas Otto <[email protected]> * add all_services: true to second example Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Jonas Otto <[email protected]> --------- Signed-off-by: Jonas Otto <[email protected]> Co-authored-by: Michael Orlov <[email protected]> (cherry picked from commit 938fafa)

#1668) * fix example conversion configs: all -> all_topics Signed-off-by: Jonas Otto <[email protected]> * update topic and service selection arguments in conversion config in readme Signed-off-by: Jonas Otto <[email protected]> * add all_services: true to first example Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Jonas Otto <[email protected]> * add all_services: true to second example Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Jonas Otto <[email protected]> --------- Signed-off-by: Jonas Otto <[email protected]> Co-authored-by: Michael Orlov <[email protected]> (cherry picked from commit 938fafa) Co-authored-by: Jonas Otto <[email protected]>

ottojo requested a review from a team as a code owner May 19, 2024 17:23

ottojo requested review from gbiggs and jhdcs and removed request for a team May 19, 2024 17:23

fix example conversion configs: all -> all_topics

2fc2605

Signed-off-by: Jonas Otto <[email protected]>

ottojo force-pushed the fix-example-convert-configs branch from 442781e to 2fc2605 Compare May 19, 2024 17:24

fujitatomoya approved these changes May 19, 2024

View reviewed changes

update topic and service selection arguments in conversion config in …

4930def

…readme Signed-off-by: Jonas Otto <[email protected]>

fujitatomoya approved these changes May 20, 2024

View reviewed changes

MichaelOrlov reviewed May 21, 2024

View reviewed changes

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

ottojo and others added 2 commits May 21, 2024 16:08

add all_services: true to first example

6b6fc77

Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Jonas Otto <[email protected]>

add all_services: true to second example

9b4d10d

Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Jonas Otto <[email protected]>

MichaelOrlov approved these changes May 21, 2024

View reviewed changes

clalancette approved these changes May 21, 2024

View reviewed changes

MichaelOrlov merged commit 938fafa into ros2:rolling May 21, 2024

mergify bot mentioned this pull request May 21, 2024

[jazzy] doc: readme: fix example conversion configs: all -> all_topics (backport #1661) #1668

Merged

ottojo deleted the fix-example-convert-configs branch May 21, 2024 19:11

doc: readme: fix example conversion configs: all -> all_topics #1661

doc: readme: fix example conversion configs: all -> all_topics #1661

Uh oh!

Conversation

ottojo commented May 19, 2024

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Barry-Xu-2018 commented May 20, 2024

Uh oh!

Barry-Xu-2018 commented May 20, 2024

Uh oh!

ottojo commented May 20, 2024

Uh oh!

ottojo commented May 20, 2024

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelOrlov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichaelOrlov left a comment

Choose a reason for hiding this comment

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelOrlov commented May 21, 2024

Uh oh!

MichaelOrlov commented May 21, 2024

Uh oh!

mergify bot commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mergify bot commented May 21, 2024 •

edited

Loading