CLI: Get storage-specific values from plugin (ros2#1209)

* Get storage-specific values from plugin-registered modules, instead of hardcoding in the CLI

Signed-off-by: Emerson Knapp <[email protected]>

Author: emersonknapp

.github/workflows/lint.yml

@@ -102,3 +102,5 @@ jobs:
         package-name: |
             ros2bag
             rosbag2_py
+            rosbag2_storage_sqlite3
+            rosbag2_storage_mcap

.github/workflows/test.yml

@@ -13,7 +13,7 @@ jobs:
   build_and_test:
     runs-on: ubuntu-latest
     container:
-      image: rostooling/setup-ros-docker:ubuntu-focal-latest
+      image: rostooling/setup-ros-docker:ubuntu-jammy-latest
     steps:
     - name: Build and run tests
       id: action-ros-ci

README.md

@@ -332,6 +332,8 @@ $ ros2 bag record --storage <storage_id>
 
 Bag reading commands can detect the storage plugin automatically, but if for any reason you want to force a specific plugin to read a bag, you can use the `--storage` option on any `ros2 bag` verb.
 
+To write your own Rosbag2 storage implementation, refer to [this document describing that process](docs/storage_plugin_development.md)
+
 
 ## Serialization format plugin architecture
 

docs/storage_plugin_development.md

@@ -1,55 +1,146 @@
-# Writing storage plugins
+# Writing storage plugins for Rosbag2
 
-There are different interfaces for storage plugins depending on your need: The general `ReadWriteStorage` and the more specific `ReadableStorage`.
+Storage plugins provide the actual underlying storage format for Rosbag2.
 
-## Writing a general plugin
+Plugins can implement the following APIs to provide a storage plugin, specified in `rosbag2_storage::storage_interfaces`:
+* `ReadOnlyInterface` which covers only reading files
+* `ReadWriteInterface` can both write new files and read existing ones. It is a superset of ReadOnly
 
-Assume you write a plugin `MyStorage` which can both save messages and read messages.
-Its header file could be `my_storage.hpp` and `MyStorage` will derive from `rosbag2_storage::storage_interfaces::ReadWriteInterface`.
-**Important:** While implementing the interface provided by `rosbag2_storage::storage_interfaces::ReadWriteInterface`, make sure that all resources such as file handles or database connections are closed or destroyed in the destructor, no additional `close` call should be necessary.
+## Creating a ReadWrite plugin
+
+Goal: Create a plugin named `my_storage`, in package `rosbag2_storage_my_storage`, implemented by class `my_namespace::MyStorage`.
+
+The following code snippets shows the necessary pieces to provide this plugin.
 
-In order to find the plugin at runtime, it needs to be exported to the pluginlib.
-Add the following lines to `my_storage.cpp`:
 
 ```
+// my_storage.cpp
+#include "rosbag2_storage/storage_interfaces/read_write_interface.hpp"
+
+namespace my_namespace {
+
+class MyStorage : public rosbag2_storage::storage_interfaces::ReadWriteInterface
+{
+public:
+  MyStorage();
+  ~MyStorage() override;  // IMPORTANT: All cleanup must happen in the destructor, such as closing  file handles or database connections
+
+  // ReadWriteInterface's virtual overrides here
+};
+
+// Implementations
+
+}  // namespace my_namespace
+
+// The following block exposes our class to pluginlib so that it can be discovered at runtime.
 #include "pluginlib/class_list_macros.hpp"
-PLUGINLIB_EXPORT_CLASS(MyStorage, rosbag2_storage::storage_interfaces::ReadWriteInterface)
+PLUGINLIB_EXPORT_CLASS(my_namespace::MyStorage,
+                       rosbag2_storage::storage_interfaces::ReadWriteInterface)
 ```
 
-Furthermore, we need some meta-information in the form of a `plugin_description.xml` file.
+Next, our package must provide a file named `plugin_description.xml`.
 Here, it contains
 
 ```
-<library path="my_storage_lib">
-  <class name="my_storage" type="MyStorage" base_class_type="rosbag2_storage::storage_interfaces::ReadWriteInterface">
+<library path="rosbag2_storage_my_storage">
+  <class
+    name="my_storage"
+    type="MyStorage"
+    base_class_type="rosbag2_storage::storage_interfaces::ReadWriteInterface"
+  >
+    <description>Rosbag2 storage plugin providing the MyStorage file format.</description>
   </class>
 </library>
 ```
-`my_storage_lib` is the name of the library (ament package) while `my_storage` is an identifier used by the pluginlib to load it.
 
-In addition, in the `CMakeLists.txt` the `plugin_description.xml` file needs to be added to the index to be found at runtime:
+`rosbag2_storage_my_storage` is the name of the library from `package.xml` while `my_storage` is an identifier used by the pluginlib to refer to the plugin.
 
-`pluginlib_export_plugin_description_file(rosbag2_storage plugin_description.xml)`
+Finally, the `CMakeLists.txt` must add our `plugin_description.xml` file to the ament index to be found at runtime:
 
-The first argument `rosbag2_storage` denotes the library we add our plugin to (this will always be `rosbag2_storage`), while the second argument is the path to the plugin description file.
+```
+pluginlib_export_plugin_description_file(rosbag2_storage plugin_description.xml)
+```
 
-## Writing a plugin for reading only
+The first argument `rosbag2_storage` denotes the library we add our plugin to (this will always be `rosbag2_storage` for this plugin type), while the second argument is the path to the plugin description file.
 
-When writing plugins to only provide functionality for reading, derive from `rosbag2_storage::storage_interfaces::ReadOnlyInterface`.
+## Creating a ReadOnly plugin
 
-If the read-only plugin is called `my_readonly_storage` in a library `my_storage_lib`, it will be registered using
+When writing plugins to only provide functionality for reading, derive your implementation class from `rosbag2_storage::storage_interfaces::ReadOnlyInterface` instead.
+This is the only functional difference, it will require only a subset of the interface overrides.
 
 ```
+// my_readonly_storage.cpp
+#include "rosbag2_storage/storage_interfaces/read_only_interface.hpp"
+
+namespace my_namespace {
+
+class MyReadOnlyStorage : public rosbag2_storage::storage_interfaces::ReadOnlyInterface
+{
+public:
+  MyReadOnlyStorage();
+  ~MyReadOnlyStorage() override;  // IMPORTANT: All cleanup must happen in the destructor, such as closing  file handles or database connections
+
+  // ReadOnlyInterface's virtual overrides here
+};
+
+// Implementations
+
+}  // namespace my_namespace
+
+// The following block exposes our class to pluginlib so that it can be discovered at runtime.
 #include "pluginlib/class_list_macros.hpp"
-PLUGINLIB_EXPORT_CLASS(MyReadonlyStorage, rosbag2_storage::storage_interfaces::ReadOnlyInterface)
+PLUGINLIB_EXPORT_CLASS(my_namespace::MyReadOnlyStorage,
+                       rosbag2_storage::storage_interfaces::ReadOnlynterface)
 ```
-with the plugin description
+
 ```
-<library path="my_storage_lib">
-  <class name="my_readonly_storage" type="MyReadonlyStorage" base_class_type="rosbag2_storage::storage_interfaces::ReadOnlyInterface">
+<!-- plugin_description.xml -->
+<library path="rosbag2_storage_my_storage">
+  <class
+    name="my_readonly_storage"
+    type="my_namespace::MyReadOnlyStorage"
+    base_class_type="rosbag2_storage::storage_interfaces::ReadOnlyInterface"
+  >
+    <description>Rosbag2 storage plugin providing read functionality for MyStorage file format.</description>
   </class>
 </library>
 ```
+
 and the usual pluginlib export in the CMakeLists:
 
-`pluginlib_export_plugin_description_file(rosbag2_storage plugin_description.xml)`
+```
+# CMakeLists.txt
+pluginlib_export_plugin_description_file(rosbag2_storage plugin_description.xml)
+```
+
+## Providing plugin-specific configuration
+
+Some storage plugins may have configuration parameters unique to the format that you'd like to allow users to provide from the command line.
+Rosbag2 provides a CLI argument `--storage-config-file` which allows users to pass the path to a file.
+This file can contain anything, its format is specified by the storage implementation, it is passed as a path all the way to the plugin, where it may be used however desired.
+Plugins are recommended to document the expected format of this file so that users can write well-formatted configurations.
+
+### Extending CLI from a storage plugin
+
+Commandline arguments can be a much more convenient way to expose configuration to users than writing out a file.
+The `ros2bag` package, which creates the `ros2 bag` command, provides an entrypoint for plugins to extend the CLI.
+
+All a package needs to do is expose a Python setuptools entrypoint to the group `ros2bag.storage_plugin_cli_extension`, with an entrypoint keyed by the name of the storage plugin. For example, here is `setup.cfg` from `rosbag2_storage_mcap`:
+
+```
+[options.entry_points]
+ros2bag.storage_plugin_cli_extension =
+  mcap = ros2bag_mcap_cli
+```
+
+This registers an entrypoint in group `ros2bag.storage_plugin_cli_extension`, for the plugin named `mcap`, that is implemented by a Python module called `rosbag2_mcap_cli`.
+
+The exposed entrypoint can be installed as a Python module by any method, for example via `ament_cmake_python`'s `ament_python_install_package` macro, or by having a pure-python `ament_python` package with a `setup.py`.
+
+The functions this entry point may provide:
+
+* `get_preset_profiles(): List[Tuple[str, str]]` - provide a list of string pairs containing (name, description) of _preset profiles_, or predefined configurations, for writing storage files. The first item will be used as default. Consider returning 'none' as the first profile.
+
+NOTE: For each of these lists, the string literal 'none' will be used to indicate the feature is disable/not used.
+
+NOTE: Any entry point may exclude any of the extension functions, and a warning will be printed for each extention point omitted. When the function for a list of values is not provided, or returns `None`, by default `'none'` will be provided as the only option.

ros2bag/ros2bag/api/__init__.py

@@ -12,7 +12,12 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from argparse import ArgumentParser, ArgumentTypeError
+from argparse import (
+    ArgumentParser,
+    ArgumentTypeError,
+    FileType,
+    HelpFormatter,
+)
 import os
 from typing import Any
 from typing import Dict
@@ -24,6 +29,7 @@
 from rclpy.qos import QoSLivelinessPolicy
 from rclpy.qos import QoSProfile
 from rclpy.qos import QoSReliabilityPolicy
+from ros2cli.entry_points import get_entry_points
 import rosbag2_py
 
 # This map needs to be updated when new policies are introduced
@@ -37,6 +43,17 @@
 _VALUE_KEYS = ['depth', 'avoid_ros_namespace_conventions']
 
 
+class SplitLineFormatter(HelpFormatter):
+    """Extend argparse HelpFormatter to allow for explicit newlines in help string."""
+
+    def _split_lines(self, text, width):
+        lines = text.splitlines()
+        result_lines = []
+        for line in lines:
+            result_lines.extend(HelpFormatter._split_lines(self, line, width))
+        return result_lines
+
+
 def print_error(string: str) -> str:
     return '[ERROR] [ros2bag]: {}'.format(string)
 
@@ -129,3 +146,57 @@ def add_standard_reader_args(parser: ArgumentParser) -> None:
         '-s', '--storage', default='', choices=reader_choices,
         help='Storage implementation of bag. '
              'By default attempts to detect automatically - use this argument to override.')
+
+
+def _parse_cli_storage_plugin():
+    plugin_choices = set(rosbag2_py.get_registered_writers())
+    default_storage = rosbag2_py.get_default_storage_id()
+    if default_storage not in plugin_choices:
+        default_storage = next(iter(plugin_choices))
+
+    storage_parser = ArgumentParser(add_help=False)
+    storage_parser.add_argument(
+        '-s', '--storage',
+        default=default_storage,
+        choices=plugin_choices,
+        help='Storage implementation of bag. '
+             'By default attempts to detect automatically - use this argument to override.')
+    storage_parsed_args, _ = storage_parser.parse_known_args()
+    plugin_id = storage_parsed_args.storage
+
+    if plugin_id not in plugin_choices:
+        raise ValueError(f'No storage plugin found with ID "{plugin_id}". Found {plugin_choices}.')
+    return plugin_id
+
+
+def add_writer_storage_plugin_extensions(parser: ArgumentParser) -> None:
+
+    plugin_id = _parse_cli_storage_plugin()
+    try:
+        extension = get_entry_points('ros2bag.storage_plugin_cli_extension')[plugin_id].load()
+    except KeyError:
+        print(f'No CLI extension module found for plugin name {plugin_id} '
+              'in entry_point group "ros2bag.storage_plugin_cli_extension".')
+        # Commandline arguments should still be added when no extension present
+        # None will throw AttributeError for all method calls
+        extension = None
+
+    parser.add_argument(
+        '--storage-config-file', type=FileType('r'),
+        help='Path to a yaml file defining storage specific configurations. '
+             f'See {plugin_id} plugin documentation for the format of this file.')
+
+    try:
+        preset_profiles = extension.get_preset_profiles() or \
+            [('none', 'Default writer configuration.')]
+    except AttributeError:
+        print(f'Storage plugin {plugin_id} does not provide function "get_preset_profiles".')
+        preset_profiles = ['none']
+    default_preset_profile = preset_profiles[0][0]
+    parser.add_argument(
+        '--storage-preset-profile', type=str, default=default_preset_profile,
+        choices=[preset[0] for preset in preset_profiles],
+        help=f'R|Select a preset configuration for storage plugin "{plugin_id}". '
+             'Settings in this profile can still be overriden by other explicit options '
+             'and --storage-config-file. Profiles:\n' +
+             '\n'.join([f'{preset[0]}: {preset[1]}' for preset in preset_profiles]))

ros2bag/ros2bag/verb/burst.py

@@ -53,11 +53,7 @@ def add_arguments(self, parser, cli_name):  # noqa: D102
         parser.add_argument(
             '--storage-config-file', type=FileType('r'),
             help='Path to a yaml file defining storage specific configurations. '
-                 'For the default storage plugin settings are specified through syntax:'
-                 'read:'
-                 '  pragmas: [\"<setting_name>\" = <setting_value>]'
-                 'Note that applicable settings are limited to read-only for ros2 bag play.'
-                 'For a list of sqlite3 settings, refer to sqlite3 documentation')
+                 'See storage plugin documentation for the format of this file.')
         parser.add_argument(
             '--start-offset', type=check_positive_float, default=0.0,
             help='Start the playback player this many seconds into the bag file.')

ros2bag/ros2bag/verb/play.py

@@ -74,11 +74,7 @@ def add_arguments(self, parser, cli_name):  # noqa: D102
         parser.add_argument(
             '--storage-config-file', type=FileType('r'),
             help='Path to a yaml file defining storage specific configurations. '
-                 'For the default storage plugin settings are specified through syntax:'
-                 'read:'
-                 '  pragmas: [\"<setting_name>\" = <setting_value>]'
-                 'Note that applicable settings are limited to read-only for ros2 bag play.'
-                 'For a list of sqlite3 settings, refer to sqlite3 documentation')
+                 'See storage plugin documentation for the format of this file.')
         clock_args_group = parser.add_mutually_exclusive_group()
         clock_args_group.add_argument(
             '--clock', type=positive_float, nargs='?', const=40, default=0,
@@ -99,29 +95,29 @@ def add_arguments(self, parser, cli_name):  # noqa: D102
         parser.add_argument(
             '--playback-duration', type=float, default=-1.0,
             help='Playback duration, in seconds. Negative durations mark an infinite playback. '
-                 'Default is -1.0. When positive, the maximum of `playback-until-*` and the one '
-                 'that this attribute yields will be used to determine which one stops playback '
-                 'execution.')
+                 'Default is %(default)d. '
+                 'When positive, the maximum effective time between `playback-until-*` '
+                 'and this argument will determine when playback stops.')
 
         playback_until_arg_group = parser.add_mutually_exclusive_group()
         playback_until_arg_group.add_argument(
             '--playback-until-sec', type=float, default=-1.,
             help='Playback until timestamp, expressed in seconds since epoch. '
                  'Mutually exclusive argument with `--playback-until-nsec`. '
-                 'Use this argument when floating point to integer conversion error is not a '
-                 'problem for your application. Negative stamps disable this feature. Default is '
-                 '-1.0. When positive, the maximum of the effective time that '
-                 '`--playback-duration` yields and this attribute will be used to determine which '
-                 'one stops playback execution.')
+                 'Use when floating point to integer conversion error is not a concern. '
+                 'A negative value disables this feature. '
+                 'Default is %(default)f. '
+                 'When positive, the maximum effective time between `--playback-duration` '
+                 'and this argument will determine when playback stops.')
         playback_until_arg_group.add_argument(
             '--playback-until-nsec', type=int, default=-1,
             help='Playback until timestamp, expressed in nanoseconds since epoch.  '
                  'Mutually exclusive argument with `--playback-until-sec`. '
-                 'Use this argument when floating point to integer conversion error matters for '
-                 'your application. Negative stamps disable this feature. Default is -1. When '
-                 'positive, the maximum of the effective time that `--playback-duration` yields '
-                 'and this attribute will be used to determine which one stops playback '
-                 'execution.')
+                 'Use when floating point to integer conversion error matters for your use case. '
+                 'A negative value disables this feature. '
+                 'Default is %(default)s. '
+                 'When positive, the maximum effective time between `--playback-duration` '
+                 'and this argument will determine when playback stops.')
 
         parser.add_argument(
             '--disable-keyboard-controls', action='store_true',