refactor init to allow options to be passed and to not be global

[wjwwood]

tl;dr I would like reviews of the proposed changes to this API while I finish refactoring things up and down the stack to use this new API. So it's ready for review, but not for CI nor merge.

---

This pull request is motivated by new rmw implementations that require additional configuration during initialization.

This pull request makes two major changes:

• it allows for options to be passed to rmw_init() and then be accessed via a returned rmw_init_context_t instance, and
• the rmw_init_context_t allows rmw_init() to be non-global

This also:

• actually adds rmw_shutdown() (previously referenced in documentation but never implemented)
• changes the rmw_create_node() and rmw_create_guard_condition() functions to take the rmw_init_context_t as an argument, so they do not need to access global variables

I explicitly avoided refactoring rmw_create_node() to return a ret value rather than the pointer (so you can get more discrete error reasons) so I can do that when refactoring these interfaces to take allocators. I added a TODO in the meantime.

[Karsten1987]

This generally looks good to me (afaict). I like the idea of not having rmw_init() as global.

Just a few things which came to my mind here:

• if context or init_options are implementation dependent, I believe it makes sense to add the rmw identifier to it.
• if the idea is to access the init options from the context object, what's the difference between the two data types? Is context a superset of init_options then?
• I wonder whether it makes sense to have these data types as generic key-value pairs which might be written to disk (a.k.a rosbags) to have a consistent environment when replaying data. But that's just an idea as I don't really know what kind of options you're thinking about here. You could also come around that later by having maybe a serialization function for them.

[wjwwood]

Thanks for the feedback.

• if context or init_options are implementation dependent, I believe it makes sense to add the rmw identifier to it.

That's a good idea. I'll do that.

• if the idea is to access the init options from the context object, what's the difference between the two data types? Is context a superset of init_options then?

There is overlap between the two types, but neither is necessarily a superset of the other. I could choose to include the entire options structure in the context just for potential use in the future, but that's not necessary.

For example, imagine an option like "do_not_register_signal_handlers", which is useful during init, but not necessarily afterwards, so it might not make it into the context by itself. But an option like instance_id is potentially needed to do initialization and should end up in the context for later use, so it has to be in the options, and so it exists in both.

• I wonder whether it makes sense to have these data types as generic key-value pairs which might be written to disk (a.k.a rosbags) to have a consistent environment when replaying data. But that's just an idea as I don't really know what kind of options you're thinking about here. You could also come around that later by having maybe a serialization function for them.

I don't think that's in scope here. Also these settings will probably not influence things like node names, topic names, etc., and so I'm not sure how important they are to playback.

I think the key-value pairs might work for the options, but would be very efficient for accessing elements of the context.

On the other hand, I don't think the key-value pair makes it possible to serialize them somehow where it was impossible before. So I think I'd pass on this for now.

[dirk-thomas]

👍 to rmw_init_context_t enabling to encapsulate state through the lifetime of an rmw between init and shutdown. For the layers above rmw it is a blackbox for passing state between rmw function calls. I would suggest to drop the init part from the name since it seems to provide context beyond just the initialization.

Regarding the rmw_init_options_t I am not sure I understand the purpose fully. Since the actual options in the strict are rmw implementation specific (do I understand that correctly?) how can the caller make use of any of this if using the generic rmw interface without locking itself to a specific implementation?

[wjwwood]

I would suggest to drop the init part from the name since it seems to provide context beyond just the initialization.

Ok. My thinking was that there was one context per init, that's why I left init in the name. I thought of it like the init handle is analogous to the node handle or something like that. But I don't feel strongly about it, I'll drop the init.

how can the caller make use of any of this if using the generic rmw interface without locking itself to a specific implementation?

Well they cannot, but the idea for now was that an rmw specific function could be used to "mutate" the implementation specific init options before passing them into the init. Doing so will tie you to a specific implementation.

What this does enable, however, is to have code in the implementation of rmw_init() that can do different things based on the options without relying on global state.

In the future, we might come up with a way to declare which ROS primitives you're going to create at the start (storing them in the init options) and then pass that to init, but not right now. This was talked about here:

#126

[dirk-thomas]

Well they cannot, but the idea for now was that an rmw specific function could be used to "mutate" the implementation specific init options before passing them into the init. Doing so will tie you to a specific implementation.

Thanks for clarifying. Sounds good to me. I just wanted to check I understood it correctly. Maybe adding a note in the docblock would be good - just to avoid users being unsure how to set options if they are not aware that it only works for rmw specific implementation?

[jacobperron]

LGTM

[jacobperron]

Consider adding another type: RMW_RET_OPTIONS_ALREADY_INIT

[gonzodepedro]

LGTM

[wjwwood]

CI:

• Linux
• Linux (OpenSplice)
• Linux-aarch64
• macOS
• Windows

[wjwwood]

The two warnings on macOS are unrelated to my changes, so I'm going to merge! 🎉

[dirk-thomas]

The two warnings on macOS are unrelated to my changes

The two new warning are caused by changes in ros2/rcl#336. Also in the latest packaging job: https://ci.ros2.org/job/ci_packaging_osx/59/warnings11Result/ The patch introduced new member to existing structs hence the initialization of these structs needs to be updated to initialize one more member.

[wjwwood]

Ok, you're right, I assumed that the statement was unaffected by me because in C {0} is supposed to initialize the whole struct. I thought it must have been a change to how the code was being compiled as part of other changes. Also I remember a pr from @sloretz about removing clang warnings which I associated with this.

I'll fix it asap.

[wjwwood]

See ros2/rcl#345, interestingly I don't see this warning on my macbook...