Public API for remap_rule_t / node_option_t::arguments::remap_rules?

[gavanderhoorn]

Feature request

Feature description

tl;dr: provide API to CRUD rcl_remap_t types and access/populate node_options_t::arguments::remap_rules without having to go through rcl_parse_arguments(..) (with a 'fake' argv).

---

longer:

The current implementation of node_options_t and related functions completely hide all CRUD operations on rcl_remap_t types. Additionally, node_options_t::rcl_arguments_t contains a list of rcl_remap_t instances which is also inaccessible (due to the use of rcl_arguments_impl_t).

For 'normal' implementations of client libraries, this seems to make sense: all they have to do is call rcl_parse_arguments(argc, argv, ...) and they'll get a correctly initialised and populated node_options_t which they can then pass to functions like rclc_node_init_with_options(..).

On systems that do not have command lines, or don't allow or can't give access to command lines, this implementation seems to make things more complex than it appears to have to be.

An example of this is rclcpp::ComponentManager, which as part of ComponentManager::on_load_node(..) calls ComponentManager::create_node_options(..) which essentially constructs a faux argv -- based on the incoming service request fields -- which it then passes to rclcpp::NodeOptions(..) which basically uses rcl_parse_arguments(..) again.

Similar to rclcpp::ComponentManager, my system also already has remap tuples in a list, which I could use to populate node_options_t::arguments::remap_rules instead of doing what ComponentManager::create_node_options(..) does.

The current implementation doesn't appear to support that however.

Allowing access to this could simplify code in rclcpp as well as make it much more straightforward for systems which do not get their 'arguments' from a command line to initialise a node_options_t structure.

Implementation considerations

My assumption is that this is currently not supported (and rcl_parse_arguments(..) the only entry-point) to guarantee correct parsing and initialisation of remap rules and other parameters.

A potential implementation which would allow access to node_options_t::arguments::remap_rules could still use the current code -- and thus keep all checks and safe-guards in place -- it would just not be completely hidden behind rcl_parse_arguments(..). Doing-the-right-thing(s) (ie: in the right order) would then be the responsibility of callers of the new API. That would not be different from how other APIs in RCL are designed.

There is already rcl/remap.h, but this appears to provide access to the actual remapping functionality (ie: it consumes the results of rcl_parse_arguments(..)).

Perhaps some additional functions could be added which allow CRUD operations on node_options_t::arguments::remap_rules in an instance of node_options_t::arguments?

---

Edit: initially I thought/hoped #254 made already did this, but it looks like that's changing the consuming side (ie: in rcl_remap.h).

[gavanderhoorn]

Friendly ping.

Would a PR which adds the described access/functionality be considered for inclusion into rcl?

[fujitatomoya]

@gavanderhoorn IMO, i am thinking why not?. i may be missing something.

My assumption is that this is currently not supported (and rcl_parse_arguments(..) the only entry-point) to guarantee correct parsing and initialisation of remap rules and other parameters.

besides, we do not need to break the userspace to modify the interfaces if that is hidden in the implementation?

CC: @ros2/team

[gavanderhoorn]

we do not need to break the userspace to modify the interfaces if that is hidden in the implementation?

I don't believe so, no.

And it looks like at least initially #254 seemed to be going in the direction of what I wrote in the OP, but only the "rcl_remap_t needs to use PIMPL pattern" part got implemented AFAICT.

[wjwwood]

I think this is totally reasonable. Though if we're worried about someone using CRUD operations to get the rcl_remap_t into a inconsistent state, then we could instead just provide an alternative to rcl_parse_arguments() which takes a data structure (or set of data structures) instead of the arguments as a string. In fact, long term I think it would make sense to completely separate the representation of remapping (rcl_remap_t) and the parsing of command line arguments, i.e. something like rcl_parse_arguments() -> intermediate data structure like rcl_parsed_arguments_t -> rcl_remap_init(rcl_parsed_arguments_t * ...). Allowing us to create that intermediate data structure by other means, e.g. from a config file or programmatically.

That's all to say, I think adding CRUD is fine and I would review such a pr, but I'd also be fine with a single new function that allows us to initialize the data structure once programmatically. I agree that creating fake arguments just to initialize the structure is not necessary.

[wjwwood]

For what it's worth, I always wanted this separation up to the nodes too, e.g. I wanted the rclcpp::NodeOptions to not require command line style arguments as a string, instead having a separate function to parse them into a data structure and pass that to the options instead. But that's a bigger change.

[gavanderhoorn]

Coming back to this after (quite) some time, I've investigated what exactly I would need for my application (a bit selfish, I know, but that's how it is unfortunately).

Context: I have a list of remap rules in a .yaml file, strings, of the form from:=to. So essentially what you'd have after the --remap on a typical ros2 run command line.

Currently, I iterate over those and construct the faux argv I describe in my OP, then pass it to rcl_parse_arguments(..). After that the (partially initialised) rcl_node_options_t instance gets passed to rclc_node_init_with_options(..) and everything works (tm).

To do this without the rcl_parse_arguments(..) detour, I believe I'd only need to add either something like:

rcl_ret_t
rcl_arguments_add_remap_rule(
  rcl_arguments_t * arguments,
  rcl_remap_t * remap_rule,
  rcl_allocator_t allocator);

and make rcl_parse_remap_rule(..) a public function (still in arguments.c), or a variant of this could be added, which instead directly takes the remap rule in string form, parses it and then adds it to arguments->impl->remap_rules:

rcl_ret_t
rcl_arguments_add_remap_rule(
  rcl_arguments_t * arguments,
  const char * remap_str,
  rcl_allocator_t allocator);

This would not require making rcl_parse_remap_rule(..) public.

Both of these options would allow 'injecting' rcl_remap_t into an rcl_arguments instance, with the latter requiring the least amount of changes IIUC and it'd keep rcl_parse_remap_rule(..) responsible for all the sanity and consistency checks.

I fully admit this is less well-rounded than what I believe @wjwwood described in his comment(s), and I'm also unsure as to whether it's a nice enough addition to be merged here (ie: does it fit style-wise, as it would seem to introduce (quite some) asymmetry, given only rcl_remap_t would get this special function).

Refactoring the current implementation to work with intermediary representations of arguments and then adding CRUD functionality for those data structs would be nice(r), but would also be much more work than I need right now.

Would something like this be acceptable?