Open gavanderhoorn opened 1 year ago
Friendly ping.
Would a PR which adds the described access/functionality be considered for inclusion into rcl
?
@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
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.
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.
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.
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?
Feature request
Feature description
tl;dr: provide API to CRUD
rcl_remap_t
types and access/populatenode_options_t::arguments::remap_rules
without having to go throughrcl_parse_arguments(..)
(with a 'fake'argv
).longer:
The current implementation of
node_options_t
and related functions completely hide all CRUD operations onrcl_remap_t
types. Additionally,node_options_t::rcl_arguments_t
contains a list ofrcl_remap_t
instances which is also inaccessible (due to the use ofrcl_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 populatednode_options_t
which they can then pass to functions likerclc_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 ofComponentManager::on_load_node(..)
callsComponentManager::create_node_options(..)
which essentially constructs a fauxargv
-- based on the incoming service request fields -- which it then passes to rclcpp::NodeOptions(..) which basically usesrcl_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 whatComponentManager::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 anode_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 behindrcl_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 ofnode_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
).