Add 'executor_type' parameter to ComponentManager and deprecate redundant component containers#3055
Add 'executor_type' parameter to ComponentManager and deprecate redundant component containers#3055armaho wants to merge 2 commits intoros2:rollingfrom
Conversation
…dant component containers Signed-off-by: Arman Hosseini <armanhosseini878787@gmail.com>
|
@armaho thanks for creating PR, is this good for review? |
|
@fujitatomoya yes |
| /** | ||
| * Initializes the component manager. It creates the services: load node, unload node | ||
| * and list nodes. Initialize executor after construction using set_executor. | ||
| * | ||
| * \param executor the executor which will spin the node. | ||
| * \param node_name the name of the node that the data originates from. | ||
| * \param node_options additional options to control creation of the node. | ||
| */ | ||
| RCLCPP_COMPONENTS_PUBLIC | ||
| ComponentManager( | ||
| std::string node_name = "ComponentManager", | ||
| const rclcpp::NodeOptions & node_options = rclcpp::NodeOptions()); |
There was a problem hiding this comment.
can you tell me why this new ctor is required to add?
| std::weak_ptr<rclcpp::Executor> executor = | ||
| std::weak_ptr<rclcpp::executors::MultiThreadedExecutor>(), | ||
| std::weak_ptr<rclcpp::Executor> executor, | ||
| std::string node_name = "ComponentManager", | ||
| const rclcpp::NodeOptions & node_options = rclcpp::NodeOptions() | ||
| .start_parameter_services(false) | ||
| .start_parameter_event_publisher(false)); | ||
| const rclcpp::NodeOptions & node_options = rclcpp::NodeOptions()); |
There was a problem hiding this comment.
this breaks the API/ABI, we should deprecate the ctor 1st before breaking changes. see https://docs.ros.org/en/rolling/The-ROS2-Project/Contributing/Developer-Guide.html#deprecation-strategy
There was a problem hiding this comment.
tbh, I did not understand the default value for the executor parameter. Isn't this default value destructed right after the constructor is done? and there's no way for user to actually get this executor to call spin on it. so the user either have to use:
exec = some type of executor
ComponentManager(exec)
which is covered with this constructor, or do something like:
cm = ComponentManager()
cm.set_executor(exec)
which is covered with the other constructor I added. Am I right?
|
|
||
| RCLCPP_WARN(node->get_logger(), | ||
| "component_container_event is depracated and will be removed " | ||
| "in the next version. use 'ros2 run rclcpp_components " | ||
| "component_container --ros-args -p executor_type:=EventsExecutor'."); | ||
|
|
There was a problem hiding this comment.
can we instead use [[deprecated("message")]] attribute?
There was a problem hiding this comment.
I just tried that, it showed nothing when I used ros2 run to run the component container. I will add the deprecated attribute but I think it's good to also keep the warning.
| rcl_interfaces::msg::ParameterDescriptor exec_type_desc{}; | ||
| exec_type_desc.description = "Type of executor used for ComponentManager"; | ||
| exec_type_desc.type = rclcpp::PARAMETER_STRING; | ||
| // The value is changed only within set_executor. It cannot be changed externally. | ||
| exec_type_desc.read_only = true; | ||
| this->declare_parameter("executor_type", "No Executor", exec_type_desc); |
There was a problem hiding this comment.
why we need to do this including set_executor change? once we create the node (ComponentManager), we are able to get the parameters via command line interfaces? that was the original plan? and then, create the corresponding executor and bind it to the node (ComponentManager)?
There was a problem hiding this comment.
I'm sorry I don't quite understand this. Are you asking about the comment I added here, or the changes I made to set_executor?
| RCLCPP_PUBLIC | ||
| std::string | ||
| get_class_name() const override; | ||
|
|
There was a problem hiding this comment.
i really do not understand why this needs to be added? even if needed, can we use typeid instead?
There was a problem hiding this comment.
i really do not understand why this needs to be added?
Any executor can be used as the underlying executor for ComponentContainer. So even if a user-defined executor is used for ComponentContainer, the value of executor_type should be updated to reflect that.
even if needed, can we use typeid instead?
tbh, I didn't like the get_class_name() approach either. But I was afraid that the value of typeid(exec).name() would be mangled. I wasn't sure what to do :)
2 participants
Description
This pull request is related to issue #2930. The proposed design consists of two parts:
component_container_eventsandcomponent_container_mtand instead configuring the type ofComponentManagerwith a parameter (executor_type).ComponentManagerIsolatedandcomponent_container_isolatedand moving the functionality (with the requested feature) toComponentManageritself.Since these are two separate tasks, I think it's appropriate to open a pull request for each one. This pull request implements the first part. Let me know if I should implement both of them in a single pull request.
Is this user-facing behavior change?
Users can keep using
component_containerlike before, but instead of usingcomponent_container_mtandcomponent_container_eventsthey should usecomponent_containersupplied with the appropriate value forexecutor_typeparameter.Did you use Generative AI?
No.
Additional Information
Nothing.