Skip to content

Implement action recording and display info about recorded action #1939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Apr 2, 2025
Merged

Implement action recording and display info about recorded action #1939

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
92b5beb
Implement action recording and display info about recorded action
Barry-Xu-2018 Mar 14, 2025
1bc1f9a
Address review comment of first round
Barry-Xu-2018 Mar 26, 2025
6c437c0
Address review comments from fujitatomoya
Barry-Xu-2018 Mar 26, 2025
7a29fcc
genstub
Barry-Xu-2018 Mar 27, 2025
cb175c2
Optimize some parts of the code
Barry-Xu-2018 Mar 27, 2025
0e23198
Fix an issue with the display when there is no service data
Barry-Xu-2018 Mar 28, 2025
a4f2190
Address review comments
Barry-Xu-2018 Mar 28, 2025
a56f295
Change a variable name to make the code clearer and more readable
Barry-Xu-2018 Mar 29, 2025
fb1286d
Address review comments
Barry-Xu-2018 Mar 29, 2025
e4fd9ef
Simplify if-else block with a shorter name service_info
morlov-apexai Mar 29, 2025
63723f5
Throw exception by default in the get_action_type_for_info(topic_type)
morlov-apexai Mar 29, 2025
8fcfbde
Remove type conversion in sequential_writer
morlov-apexai Mar 29, 2025
99103f8
Calculate actions status and feedback statistics in read_service_and_…
morlov-apexai Mar 29, 2025
c85ed3c
Address some other small style changes and nitpicks
morlov-apexai Mar 29, 2025
5c31b04
Add default section with exception to sorting functions
MichaelOrlov Mar 29, 2025
8e69169
Use structural bindings in the test_action_utils.cpp
MichaelOrlov Mar 29, 2025
6bb2be1
Add Doxygen comments to the action_utils.hpp
MichaelOrlov Mar 29, 2025
e196b67
Avoid using ContainsRegex on Windows to handle the '|' character
Barry-Xu-2018 Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ros2bag/ros2bag/verb/info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ def add_arguments(self, parser, cli_name): # noqa: D102
)
parser.add_argument(
'-v', '--verbose', action='store_true',
help='Display request/response information for services'
help='Display verbose information about request/response services, actions and'
' print messages size contribution on per topic bases.'
)
available_sorting_methods = Info().get_sorting_methods()
parser.add_argument(
Expand Down
46 changes: 36 additions & 10 deletions ros2bag/ros2bag/verb/record.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,27 +70,34 @@ def add_recorder_arguments(parser: ArgumentParser) -> None:
parser.add_argument(
'--services', type=str, metavar='ServiceName', nargs='+',
help='Space-delimited list of services to record.')
parser.add_argument(
'--actions', type=str, metavar='ActionName', nargs='+',
help='Space-delimited list of actions to record.')
parser.add_argument(
'--topic-types', nargs='+', default=[], metavar='TopicType',
help='Space-delimited list of topic types to record.')
parser.add_argument(
'-a', '--all', action='store_true',
help='Record all topics and services (Exclude hidden topic).')
help='Record all topics, services and actions (Exclude hidden topic).')
parser.add_argument(
'--all-topics', action='store_true',
help='Record all topics (Exclude hidden topic).')
parser.add_argument(
'--all-services', action='store_true',
help='Record all services via service event topics.')
parser.add_argument(
'--all-actions', action='store_true',
help='Record all actions via internal topics and service event topics.')
parser.add_argument(
'-e', '--regex', default='',
help='Record only topics and services containing provided regular expression. '
'Note: --all, --all-topics or --all-services will override --regex.')
'Note: --all, --all-topics, --all-services or --all-actions will override --regex.')
parser.add_argument(
'--exclude-regex', default='',
help='Exclude topics and services containing provided regular expression. '
'Works on top of '
'--all, --all-topics, --all-services, --topics, --services or --regex.')
'--all, --all-topics, --all-services, --all-actions, --topics, --services, --actions '
'or --regex.')
parser.add_argument(
'--exclude-topic-types', type=str, default=[], metavar='ExcludeTopicTypes', nargs='+',
help='Space-delimited list of topic types not being recorded. '
Expand All @@ -103,6 +110,10 @@ def add_recorder_arguments(parser: ArgumentParser) -> None:
'--exclude-services', type=str, metavar='ServiceName', nargs='+',
help='Space-delimited list of services not being recorded. '
'Works on top of --all, --all-services, --services or --regex.')
parser.add_argument(
'--exclude-actions', type=str, metavar='ActionName', nargs='+',
help='Space-delimited list of actions not being recorded. '
'Works on top of --all, --all-actions, --actions or --regex.')

# Discovery behavior
parser.add_argument(
Expand Down Expand Up @@ -217,10 +228,11 @@ def add_recorder_arguments(parser: ArgumentParser) -> None:


def check_necessary_argument(args):
# At least one options out of --all, --all-topics, --all-services, --services, --topics,
# --topic-types or --regex must be used
if not (args.all or args.all_topics or args.all_services or
# At least one options out of --all, --all-topics, --all-services, --all-actions, --services,
# --actions --topics, --topic-types or --regex must be used
if not (args.all or args.all_topics or args.all_services or args.all_actions or
(args.services and len(args.services) > 0) or
(args.actions and len(args.actions) > 0) or
(args.topics and len(args.topics) > 0) or
(args.topic_types and len(args.topic_types) > 0) or args.regex):
return False
Expand All @@ -239,9 +251,9 @@ def validate_parsed_arguments(args, uri) -> str:

if args.exclude_regex and not \
(args.all or args.all_topics or args.topic_types or args.all_services or
args.regex):
args.all_actions or args.regex):
return print_error('--exclude-regex argument requires either --all, '
'--all-topics, --topic-types, --all-services or --regex')
'--all-topics, --topic-types, --all-services, --all-actions or --regex')

if args.exclude_topics and not \
(args.all or args.all_topics or args.topic_types or args.regex):
Expand All @@ -257,14 +269,22 @@ def validate_parsed_arguments(args, uri) -> str:
return print_error('--exclude-services argument requires either --all, --all-services '
'or --regex')

if args.exclude_actions and not (args.all or args.all_actions or args.regex):
return print_error('--exclude-actions argument requires either --all, --all-actions '
'or --regex')

if (args.all or args.all_services) and args.services:
print(print_warn('--all or --all-services will override --services'))

if (args.all or args.all_topics) and args.topics:
print(print_warn('--all or --all-topics will override --topics'))

if (args.all or args.all_topics or args.all_services) and args.regex:
print(print_warn('--all, --all-topics or --all-services will override --regex'))
if (args.all or args.all_actions) and args.actions:
print(print_warn('--all or --all-actions will override --actions'))

if (args.all or args.all_topics or args.all_services or args.all_actions) and args.regex:
print(print_warn('--all, --all-topics --all-services or --all-actions will override '
'--regex'))

if os.path.isdir(uri):
return print_error("Output folder '{}' already exists.".format(uri))
Expand All @@ -281,6 +301,8 @@ def validate_parsed_arguments(args, uri) -> str:
if args.compression_queue_size < 0:
return print_error('Compression queue size must be at least 0.')

return None


class RecordVerb(VerbExtension):
"""Record ROS data to a bag."""
Expand Down Expand Up @@ -330,11 +352,14 @@ def main(self, *, args): # noqa: D102
record_options = RecordOptions()
record_options.all_topics = args.all_topics or args.all
record_options.all_services = args.all_services or args.all
record_options.all_actions = args.all_actions or args.all
record_options.is_discovery_disabled = args.no_discovery
record_options.topics = args.topics
record_options.topic_types = args.topic_types
# Convert service name to service event topic name
record_options.services = convert_service_to_service_event_topic(args.services)
record_options.actions = args.actions if args.actions else []
Copy link
Contributor

@MichaelOrlov MichaelOrlov Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to explicitly assign an empty list []?
It seems it could be simplified as a simple assignment

record_options.actions = args.actions

The same as topics

Copy link
Contributor Author

@Barry-Xu-2018 Barry-Xu-2018 Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I change to this, running the ros2bag test will report this error.

[ros2bag-cli-7]   File "/root/ros2_src_rolling/build/ros2bag/ros2bag/verb/record.py", line 361, in main
[ros2bag-cli-7]     record_options.actions = args.actions
[ros2bag-cli-7]     ^^^^^^^^^^^^^^^^^^^^^^
[ros2bag-cli-7] TypeError: (): incompatible function arguments. The following argument types are supported:
[ros2bag-cli-7]     1. (self: rosbag2_py._transport.RecordOptions, arg0: List[str]) -> None

I compared the code with the record_options.topics and did not find any differences in the types. Do you have any suggestion about this issue ?

Copy link
Contributor

@MichaelOrlov MichaelOrlov Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting.. I will take a look


record_options.exclude_topic_types = args.exclude_topic_types
record_options.rmw_serialization_format = args.serialization_format
record_options.topic_polling_interval = datetime.timedelta(
Expand All @@ -344,6 +369,7 @@ def main(self, *, args): # noqa: D102
record_options.exclude_topics = args.exclude_topics if args.exclude_topics else []
record_options.exclude_service_events = \
convert_service_to_service_event_topic(args.exclude_services)
record_options.exclude_actions = args.exclude_actions if args.exclude_actions else []
Copy link
Contributor

@MichaelOrlov MichaelOrlov Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here it seems we don;t need conversion here.

Copy link
Contributor Author

@Barry-Xu-2018 Barry-Xu-2018 Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have the same problem mentioned in #1939 (comment)

If I change record_options.exclude_topics, it also has the same problem.

record_options.node_prefix = NODE_NAME_PREFIX
record_options.compression_mode = args.compression_mode
record_options.compression_format = args.compression_format
Expand Down
98 changes: 95 additions & 3 deletions ros2bag/test/test_recorder_args_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ def test_recorder_services_list_argument(test_arguments_parser):
assert output_path.as_posix() == args.output


def test_recorder_actions_list_argument(test_arguments_parser):
"""Test recorder --actions list argument parser."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
args = test_arguments_parser.parse_args(
['--actions', 'action1', 'action2', '--output', output_path.as_posix()]
)
assert ['action1', 'action2'] == args.actions
assert output_path.as_posix() == args.output


def test_recorder_services_and_positional_topics_list_arguments(test_arguments_parser):
"""Test recorder --services list and positional topics list arguments parser."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
Expand All @@ -74,6 +84,17 @@ def test_recorder_services_and_positional_topics_list_arguments(test_arguments_p
assert output_path.as_posix() == args.output


def test_recorder_actions_and_positional_topics_list_arguments(test_arguments_parser):
"""Test recorder --actions list and positional topics list arguments parser."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
args = test_arguments_parser.parse_args(
['--output', output_path.as_posix(),
'--actions', 'action1', 'action2', '--', 'topic1', 'topic2'])
assert ['action1', 'action2'] == args.actions
assert ['topic1', 'topic2'] == args.topics_positional
assert output_path.as_posix() == args.output


def test_recorder_services_and_optional_topics_list_arguments(test_arguments_parser):
"""Test recorder --services list and optional --topics list arguments parser."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
Expand All @@ -85,6 +106,41 @@ def test_recorder_services_and_optional_topics_list_arguments(test_arguments_par
assert output_path.as_posix() == args.output


def test_recorder_actions_and_optional_topics_list_arguments(test_arguments_parser):
"""Test recorder --actions list and optional --topics list arguments parser."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
args = test_arguments_parser.parse_args(
['--output', output_path.as_posix(),
'--actions', 'action1', 'action2', '--topics', 'topic1', 'topic2'])
assert ['action1', 'action2'] == args.actions
assert ['topic1', 'topic2'] == args.topics
assert output_path.as_posix() == args.output


def test_recorder_services_and_actions_list_arguments(test_arguments_parser):
"""Test recorder --services list and --actions list arguments parser."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
args = test_arguments_parser.parse_args(
['--output', output_path.as_posix(),
'--services', 'service1', 'service2', '--actions', 'action1', 'action2'])
assert ['service1', 'service2'] == args.services
assert ['action1', 'action2'] == args.actions
assert output_path.as_posix() == args.output


def test_recorder_services_actions_and_topics_list_arguments(test_arguments_parser):
"""Test recorder --services list, --actions list and --topics list arguments parser."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
args = test_arguments_parser.parse_args(
['--output', output_path.as_posix(),
'--services', 'service1', 'service2', '--actions', 'action1', 'action2',
'--topics', 'topic1', 'topic2'])
assert ['service1', 'service2'] == args.services
assert ['action1', 'action2'] == args.actions
assert ['topic1', 'topic2'] == args.topics
assert output_path.as_posix() == args.output


def test_recorder_topic_types_list_argument(test_arguments_parser):
"""Test recorder --topic-types list argument parser."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
Expand Down Expand Up @@ -124,6 +180,16 @@ def test_recorder_exclude_services_list_argument(test_arguments_parser):
assert output_path.as_posix() == args.output


def test_recorder_exclude_actions_list_argument(test_arguments_parser):
"""Test recorder --exclude-actions list argument parser."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
args = test_arguments_parser.parse_args(
['--exclude-actions', 'action1', 'action2', '--output', output_path.as_posix()]
)
assert ['action1', 'action2'] == args.exclude_actions
assert output_path.as_posix() == args.output


def test_recorder_custom_data_list_argument(test_arguments_parser):
"""Test recorder --custom-data list argument parser."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
Expand All @@ -138,21 +204,22 @@ def test_recorder_validate_exclude_regex_needs_inclusive_args(test_arguments_par
"""Test that --exclude-regex needs inclusive arguments."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
args = test_arguments_parser.parse_args(
['--exclude-regex', '%s-%s', '--services', 'service1', '--topics', 'topic1',
'--output', output_path.as_posix()]
['--exclude-regex', '%s-%s', '--services', 'service1', '--topics', 'topic1', '--actions',
'action1', '--output', output_path.as_posix()]
)
assert '%s-%s' == args.exclude_regex
assert args.all is False
assert args.all_topics is False
assert [] == args.topic_types
assert args.all_services is False
assert args.all_actions is False
assert '' == args.regex

uri = args.output or datetime.datetime.now().strftime('rosbag2_%Y_%m_%d-%H_%M_%S')
error_str = validate_parsed_arguments(args, uri)
assert error_str is not None
expected_output = '--exclude-regex argument requires either --all, ' \
'--all-topics, --topic-types, --all-services or --regex'
'--all-topics, --topic-types, --all-services, --all-actions or --regex'
matches = expected_output in error_str
assert matches, ERROR_STRING_MSG.format(expected_output, error_str)

Expand Down Expand Up @@ -227,3 +294,28 @@ def test_recorder_validate_exclude_services_needs_inclusive_args(test_arguments_
'or --regex'
matches = expected_output in error_str
assert matches, ERROR_STRING_MSG.format(expected_output, error_str)


def test_recorder_validate_exclude_actions_needs_inclusive_args(test_arguments_parser):
"""Test that --exclude-actions needs at least --all, --all-actions or --regex arguments."""
output_path = RESOURCES_PATH / 'ros2bag_tmp_file'
args = test_arguments_parser.parse_args(
['--exclude-actions', 'action1', '--actions', 'action1', '--all-topics',
'--output', output_path.as_posix()]
)
assert ['action1'] == args.exclude_actions
assert args.all is False
assert args.all_topics is True
assert [] == args.topic_types
assert args.all_services is False
assert args.all_actions is False
assert ['action1'] == args.actions
assert '' == args.regex
assert '' == args.exclude_regex

uri = args.output or datetime.datetime.now().strftime('rosbag2_%Y_%m_%d-%H_%M_%S')
error_str = validate_parsed_arguments(args, uri)
assert error_str is not None
expected_output = '--exclude-actions argument requires either --all, --all-actions or --regex'
matches = expected_output in error_str
assert matches, ERROR_STRING_MSG.format(expected_output, error_str)
13 changes: 12 additions & 1 deletion rosbag2_cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ add_library(${PROJECT_NAME} SHARED
src/rosbag2_cpp/writer.cpp
src/rosbag2_cpp/writers/sequential_writer.cpp
src/rosbag2_cpp/reindexer.cpp
src/rosbag2_cpp/service_utils.cpp)
src/rosbag2_cpp/service_utils.cpp
src/rosbag2_cpp/action_utils.cpp)

target_link_libraries(${PROJECT_NAME} PUBLIC
pluginlib::pluginlib
Expand Down Expand Up @@ -312,6 +313,16 @@ if(BUILD_TESTING)
${test_msgs_TARGETS}
)
endif()

ament_add_gmock(test_action_utils
test/rosbag2_cpp/test_action_utils.cpp)
if(TARGET test_action_utils)
target_link_libraries(test_action_utils
${PROJECT_NAME}
rosbag2_test_common::rosbag2_test_common
${test_msgs_TARGETS}
)
endif()
endif()

ament_package()
Loading
Loading