Add hook to generate node options in ComponentManager #1702
Conversation
Signed-off-by: Rebecca Butler <[email protected]>
Signed-off-by: Rebecca Butler <[email protected]>
rebecca-butler
force-pushed
the
domain_bridge_container
branch
from
73d0cb5 to
a37b38b
Compare
Signed-off-by: Rebecca Butler <[email protected]>
|
I don't see anything wrong in letting users configure the domain ID for individual components; but I'd like someone else to take a look, since I'm not sure if there could be any unintended consequences. The alternative is to only allow this in the domain bridge specific container. |
|
I'm not sure it makes sense, because the components are nodes, but the domain id (AFAIK) can only be changed on the context level, as that is where it maps to a participant. @ivanpauno might have more precise input. |
Signed-off-by: Rebecca Butler <[email protected]>
Signed-off-by: Rebecca Butler <[email protected]>
rebecca-butler
force-pushed
the
domain_bridge_container
branch
from
a80fa14 to
f28a4eb
Compare
|
During our offline discussion, it was decided that the functionality for setting the domain ID should not live in I was going to add setters/getters to the existing component manager so the derived class could have its own implementation of |
rclcpp_components/include/rclcpp_components/component_manager.hpp
Outdated
Show resolved
Hide resolved
rclcpp_components/include/rclcpp_components/component_manager.hpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Rebecca Butler <[email protected]>
rebecca-butler
changed the title
|
| */ | ||
| RCLCPP_COMPONENTS_PUBLIC | ||
| virtual rclcpp::NodeOptions | ||
| CreateNodeOptions(const std::shared_ptr<LoadNode::Request> request); |
There was a problem hiding this comment.
I missed this when skimming before, but can we please change this to be create_node_options? We don't use CamelCase for methods in core ROS 2 packages like this one.
There was a problem hiding this comment.
Well, I just saw that more of these methods (protected ones only) are CamelCase... so I guess this can be left as-is for now... we really need to change them though.
There was a problem hiding this comment.
I've opened #1716 to address these style issues.
| @@ -131,6 +131,15 @@ class ComponentManager : public rclcpp::Node | |||
| create_component_factory(const ComponentResource & resource); | |||
|
|
|||
| protected: | |||
| /// Create node options for loaded component | |||
| /* | |||
There was a problem hiding this comment.
Also, this should be /**, but this whole file is wrong...
|
Can this be backported to Foxy without breaking ABI? I suspect not, but correct me if I'm wrong. |
|
@aprotyas I think if we drop the |
This PR adds a new container that creates an instance of the domain bridge on startup. The user must pass in a YAML config file specifying the domain IDs and topics to be bridged.
This PR also adds an extra argument to specify domain IDs when loading in components.
Fixes ros2/domain_bridge#31