[jazzy] Add optional '--topics' CLI argument for 'ros2 bag record' (b…

…ackport #1632) (#1640) * Add optional '--topics' CLI argument for 'ros2 bag record' (#1632) * Add optional "--topics" CLI parameter for recorder - Deprecate positional "topics" parameter. - Add test coverage for non-trivial cases in the recorder CLI parser. Signed-off-by: Michael Orlov <[email protected]> * Use "--topics" instead of positional argument in integration tests Signed-off-by: Michael Orlov <[email protected]> * Cleanup in the recorder CLI help section Signed-off-by: Michael Orlov <[email protected]> * Add empty line to the end of the pytest.ini Signed-off-by: Michael Orlov <[email protected]> * Add deprecation notice for positional topics argument in help section Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: Tomoya Fujita <[email protected]> (cherry picked from commit 9146878) * Remove warning about deprecation of the positional "topics" argument Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: Michael Orlov <[email protected]>
ros2 · May 23, 2024 · 8e7e450 · 8e7e450
1 parent 8c49903
commit 8e7e450
Show file tree

Hide file tree

Showing 4 changed files with 316 additions and 171 deletions.
diff --git a/ros2bag/pytest.ini b/ros2bag/pytest.ini
@@ -1,2 +1,4 @@
 [pytest]
 junit_family=xunit2
+# flake8 in Ubuntu 22.04 prints this warning; we ignore it for now
+filterwarnings=ignore:SelectableGroups:DeprecationWarning
diff --git a/ros2bag/ros2bag/verb/record.py b/ros2bag/ros2bag/verb/record.py
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from argparse import FileType
+from argparse import ArgumentParser, FileType
 import datetime
 import os
 
@@ -34,168 +34,178 @@
 import yaml
 
 
+def add_recorder_arguments(parser: ArgumentParser) -> None:
+    parser.formatter_class = SplitLineFormatter
+    writer_choices = get_registered_writers()
+    default_storage_id = get_default_storage_id()
+    default_writer = default_storage_id if default_storage_id in writer_choices else \
+        next(iter(writer_choices))
+
+    serialization_choices = get_registered_serializers()
+    converter_suffix = '_converter'
+    serialization_choices = {
+        f[:-len(converter_suffix)]
+        for f in serialization_choices
+        if f.endswith(converter_suffix)
+    }
+
+    # Base output
+    parser.add_argument(
+        '-o', '--output',
+        help='Destination of the bagfile to create, '
+             'defaults to a timestamped folder in the current directory.')
+    parser.add_argument(
+        '-s', '--storage', default=default_writer, choices=writer_choices,
+        help="Storage identifier to be used, defaults to '%(default)s'.")
+
+    # Topic filter arguments
+    topics_args_group = parser.add_mutually_exclusive_group()
+    topics_args_group.add_argument(
+        'topics_positional', type=str, default=[], metavar='[Topic ...]', nargs='*',
+        help='Space-delimited list of topics to record. (deprecated)')
+    topics_args_group.add_argument(
+        '--topics', type=str, default=[], metavar='Topic', nargs='+',
+        help='Space-delimited list of topics to record.')
+    parser.add_argument(
+        '--services', type=str, metavar='ServiceName', nargs='+',
+        help='Space-delimited list of services 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).')
+    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(
+        '-e', '--regex', default='',
+        help='Record only topics and services containing provided regular expression. '
+             'Overrides --all, --all-topics and --all-services, applies on top of '
+             'topics list and service list.')
+    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.')
+    parser.add_argument(
+        '--exclude-topic-types', type=str, default=[], metavar='ExcludeTopicTypes', nargs='+',
+        help='Space-delimited list of topic types not being recorded. '
+             'Works on top of --all, --all-topics, --topics or --regex.')
+    parser.add_argument(
+        '--exclude-topics', type=str, metavar='Topic', nargs='+',
+        help='Space-delimited list of topics not being recorded. '
+             'Works on top of --all, --all-topics, or --regex.')
+    parser.add_argument(
+        '--exclude-services', type=str, metavar='ServiceName', nargs='+',
+        help='Space-delimited list of services not being recorded. '
+             'Works on top of --all, --all-services, or --regex.')
+
+    # Discovery behavior
+    parser.add_argument(
+        '--include-unpublished-topics', action='store_true',
+        help='Discover and record topics which have no publisher. '
+             'Subscriptions on such topics will be made with default QoS unless otherwise '
+             'specified in a QoS overrides file.')
+    parser.add_argument(
+        '--include-hidden-topics', action='store_true',
+        help='Discover and record hidden topics as well. '
+             'These are topics used internally by ROS 2 implementation.')
+    parser.add_argument(
+        '--no-discovery', action='store_true',
+        help='Disables topic auto discovery during recording: only topics present at '
+             'startup will be recorded.')
+    parser.add_argument(
+        '-p', '--polling-interval', type=int, default=100,
+        help='Time in ms to wait between querying available topics for recording. '
+             'It has no effect if --no-discovery is enabled.')
+    parser.add_argument(
+        '--ignore-leaf-topics', action='store_true',
+        help='Ignore topics without a subscription.')
+    parser.add_argument(
+        '--qos-profile-overrides-path', type=FileType('r'),
+        help='Path to a yaml file defining overrides of the QoS profile for specific topics.')
+
+    # Core config
+    parser.add_argument(
+        '-f', '--serialization-format', default='', choices=serialization_choices,
+        help='The rmw serialization format in which the messages are saved, defaults to the '
+             'rmw currently in use.')
+    parser.add_argument(
+        '-b', '--max-bag-size', type=int, default=0,
+        help='Maximum size in bytes before the bagfile will be split. '
+             'Default: %(default)d, recording written in single bagfile and splitting '
+             'is disabled.')
+    parser.add_argument(
+        '-d', '--max-bag-duration', type=int, default=0,
+        help='Maximum duration in seconds before the bagfile will be split. '
+             'Default: %(default)d, recording written in single bagfile and splitting '
+             'is disabled. If both splitting by size and duration are enabled, '
+             'the bag will split at whichever threshold is reached first.')
+    parser.add_argument(
+        '--max-cache-size', type=int, default=100 * 1024 * 1024,
+        help='Maximum size (in bytes) of messages to hold in each buffer of cache. '
+             'Default: %(default)d. The cache is handled through double buffering, '
+             'which means that in pessimistic case up to twice the parameter value of memory '
+             'is needed. A rule of thumb is to cache an order of magnitude corresponding to '
+             'about one second of total recorded data volume. '
+             'If the value specified is 0, then every message is directly written to disk.')
+    parser.add_argument(
+        '--disable-keyboard-controls', action='store_true', default=False,
+        help='disables keyboard controls for recorder')
+    parser.add_argument(
+        '--start-paused', action='store_true', default=False,
+        help='Start the recorder in a paused state.')
+    parser.add_argument(
+        '--use-sim-time', action='store_true', default=False,
+        help='Use simulation time for message timestamps by subscribing to the /clock topic. '
+             'Until first /clock message is received, no messages will be written to bag.')
+    parser.add_argument(
+        '--node-name', type=str, default='rosbag2_recorder',
+        help='Specify the recorder node name. Default is %(default)s.')
+    parser.add_argument(
+        '--custom-data', type=str, metavar='KEY=VALUE', nargs='*',
+        help='Space-delimited list of key=value pairs. Store the custom data in metadata '
+             'under the "rosbag2_bagfile_information/custom_data". The key=value pair can '
+             'appear more than once. The last value will override the former ones.')
+    parser.add_argument(
+        '--snapshot-mode', action='store_true',
+        help='Enable snapshot mode. Messages will not be written to the bagfile until '
+             'the "/rosbag2_recorder/snapshot" service is called. e.g. \n '
+             'ros2 service call /rosbag2_recorder/snapshot rosbag2_interfaces/Snapshot')
+
+    # Storage configuration
+    add_writer_storage_plugin_extensions(parser)
+
+    # Core compression configuration
+    # TODO(emersonknapp) this configuration will be moved down to implementing plugins
+    parser.add_argument(
+        '--compression-queue-size', type=int, default=1,
+        help='Number of files or messages that may be queued for compression '
+             'before being dropped.  Default is %(default)d.')
+    parser.add_argument(
+        '--compression-threads', type=int, default=0,
+        help='Number of files or messages that may be compressed in parallel. '
+             'Default is %(default)d, which will be interpreted as the number of CPU cores.')
+    parser.add_argument(
+        '--compression-mode', type=str, default='none',
+        choices=['none', 'file', 'message'],
+        help='Choose mode of compression for the storage. Default: %(default)s.')
+    parser.add_argument(
+        '--compression-format', type=str, default='',
+        choices=get_registered_compressors(),
+        help='Choose the compression format/algorithm. '
+             'Has no effect if no compression mode is chosen. Default: %(default)s.')
+
+
 class RecordVerb(VerbExtension):
     """Record ROS data to a bag."""
 
     def add_arguments(self, parser, cli_name):  # noqa: D102
-        parser.formatter_class = SplitLineFormatter
-        writer_choices = get_registered_writers()
-        default_storage_id = get_default_storage_id()
-        default_writer = default_storage_id if default_storage_id in writer_choices else \
-            next(iter(writer_choices))
-
-        serialization_choices = get_registered_serializers()
-        converter_suffix = '_converter'
-        serialization_choices = {
-            f[:-len(converter_suffix)]
-            for f in serialization_choices
-            if f.endswith(converter_suffix)
-        }
-
-        # Base output
-        parser.add_argument(
-            '-o', '--output',
-            help='Destination of the bagfile to create, '
-                 'defaults to a timestamped folder in the current directory.')
-        parser.add_argument(
-            '-s', '--storage', default=default_writer, choices=writer_choices,
-            help="Storage identifier to be used, defaults to '%(default)s'.")
-
-        # Topic filter arguments
-        parser.add_argument(
-            'topics', nargs='*', default=None, help='List of topics to record.')
-        parser.add_argument(
-            '--topic-types', nargs='+', default=[], help='List of topic types to record.')
-        parser.add_argument(
-            '-a', '--all', action='store_true',
-            help='Record all topics and services (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(
-            '-e', '--regex', default='',
-            help='Record only topics and services containing provided regular expression. '
-                 'Overrides --all, --all-topics and --all-services, applies on top of '
-                 'topics list and service list.')
-        parser.add_argument(
-            '--exclude-regex', default='',
-            help='Exclude topics and services containing provided regular expression. '
-                 'Works on top of --all, --all-topics, or --regex.')
-        parser.add_argument(
-            '--exclude-topic-types', type=str, default=[], metavar='ExcludeTypes', nargs='+',
-            help='List of topic types not being recorded. '
-                 'Works on top of --all, --all-topics, or --regex.')
-        parser.add_argument(
-            '--exclude-topics', type=str, metavar='Topic', nargs='+',
-            help='List of topics not being recorded. '
-                 'Works on top of --all, --all-topics, or --regex.')
-        parser.add_argument(
-            '--exclude-services', type=str, metavar='ServiceName', nargs='+',
-            help='List of services not being recorded. '
-                 'Works on top of --all, --all-services, or --regex.')
-
-        # Enable to record service
-        parser.add_argument(
-            '--services', type=str, metavar='ServiceName', nargs='+',
-            help='List of services to record.')
-
-        # Discovery behavior
-        parser.add_argument(
-            '--include-unpublished-topics', action='store_true',
-            help='Discover and record topics which have no publisher. '
-                 'Subscriptions on such topics will be made with default QoS unless otherwise '
-                 'specified in a QoS overrides file.')
-        parser.add_argument(
-            '--include-hidden-topics', action='store_true',
-            help='Discover and record hidden topics as well. '
-                 'These are topics used internally by ROS 2 implementation.')
-        parser.add_argument(
-            '--no-discovery', action='store_true',
-            help='Disables topic auto discovery during recording: only topics present at '
-                 'startup will be recorded.')
-        parser.add_argument(
-            '-p', '--polling-interval', type=int, default=100,
-            help='Time in ms to wait between querying available topics for recording. '
-                 'It has no effect if --no-discovery is enabled.')
-        parser.add_argument(
-            '--ignore-leaf-topics', action='store_true',
-            help='Ignore topics without a subscription.')
-        parser.add_argument(
-            '--qos-profile-overrides-path', type=FileType('r'),
-            help='Path to a yaml file defining overrides of the QoS profile for specific topics.')
-
-        # Core config
-        parser.add_argument(
-            '-f', '--serialization-format', default='', choices=serialization_choices,
-            help='The rmw serialization format in which the messages are saved, defaults to the '
-                 'rmw currently in use.')
-        parser.add_argument(
-            '-b', '--max-bag-size', type=int, default=0,
-            help='Maximum size in bytes before the bagfile will be split. '
-                 'Default: %(default)d, recording written in single bagfile and splitting '
-                 'is disabled.')
-        parser.add_argument(
-            '-d', '--max-bag-duration', type=int, default=0,
-            help='Maximum duration in seconds before the bagfile will be split. '
-                 'Default: %(default)d, recording written in single bagfile and splitting '
-                 'is disabled. If both splitting by size and duration are enabled, '
-                 'the bag will split at whichever threshold is reached first.')
-        parser.add_argument(
-            '--max-cache-size', type=int, default=100*1024*1024,
-            help='Maximum size (in bytes) of messages to hold in each buffer of cache. '
-                 'Default: %(default)d. The cache is handled through double buffering, '
-                 'which means that in pessimistic case up to twice the parameter value of memory '
-                 'is needed. A rule of thumb is to cache an order of magnitude corresponding to '
-                 'about one second of total recorded data volume. '
-                 'If the value specified is 0, then every message is directly written to disk.')
-        parser.add_argument(
-            '--disable-keyboard-controls', action='store_true', default=False,
-            help='disables keyboard controls for recorder')
-        parser.add_argument(
-            '--start-paused', action='store_true', default=False,
-            help='Start the recorder in a paused state.')
-        parser.add_argument(
-            '--use-sim-time', action='store_true', default=False,
-            help='Use simulation time for message timestamps by subscribing to the /clock topic. '
-                 'Until first /clock message is received, no messages will be written to bag.')
-        parser.add_argument(
-            '--node-name', type=str, default='rosbag2_recorder',
-            help='Specify the recorder node name. Default is %(default)s.')
-        parser.add_argument(
-            '--custom-data', type=str, metavar='KEY=VALUE', nargs='*',
-            help='Store the custom data in metadata.yaml '
-                 'under "rosbag2_bagfile_information/custom_data". The key=value pair can '
-                 'appear more than once. The last value will override the former ones.')
-        parser.add_argument(
-            '--snapshot-mode', action='store_true',
-            help='Enable snapshot mode. Messages will not be written to the bagfile until '
-                 'the "/rosbag2_recorder/snapshot" service is called.')
-
-        # Storage configuration
-        add_writer_storage_plugin_extensions(parser)
-
-        # Core compression configuration
-        # TODO(emersonknapp) this configuration will be moved down to implementing plugins
-        parser.add_argument(
-            '--compression-queue-size', type=int, default=1,
-            help='Number of files or messages that may be queued for compression '
-                 'before being dropped.  Default is %(default)d.')
-        parser.add_argument(
-            '--compression-threads', type=int, default=0,
-            help='Number of files or messages that may be compressed in parallel. '
-                 'Default is %(default)d, which will be interpreted as the number of CPU cores.')
-        parser.add_argument(
-            '--compression-mode', type=str, default='none',
-            choices=['none', 'file', 'message'],
-            help='Choose mode of compression for the storage. Default: %(default)s.')
-        parser.add_argument(
-            '--compression-format', type=str, default='',
-            choices=get_registered_compressors(),
-            help='Choose the compression format/algorithm. '
-                 'Has no effect if no compression mode is chosen. Default: %(default)s.')
+        add_recorder_arguments(parser)
 
     def _check_necessary_argument(self, args):
         # At least one options out of --all, --all-topics, --all-services, --services, --topics,
@@ -208,6 +218,9 @@ def _check_necessary_argument(self, args):
 
     def main(self, *, args):  # noqa: D102
 
+        if args.topics_positional:
+            args.topics = args.topics_positional
+
         if not self._check_necessary_argument(args):
             return print_error('Need to specify one option out of --all, --all-topics, '
                                '--all-services, --services, --topics, --topic-types and --regex')