Make Component Manager public by Karsten1987 · Pull Request #1065 · ros2/rclcpp

Karsten1987 · 2020-04-14T19:53:50Z

The first commit d81d494 basically just turns the component manager into public API.

The second commit 156ff16 is enhancing the CMake macros for registering nodes with a new option RESOURCE_INDEX to specify under which index the file should be written.

With respect to the upcoming work in ros-control (ros-controls/roadmap#7) that enables to not duplicate the CMake macros to adopt to ros-controls, but simply call the existing rclcpp_components macros with a proper resource index, e.g.:

rclcpp_components_register_node(timers_library
  PLUGIN "demo_nodes_cpp::OneOffTimerNode"
  EXECUTABLE one_off_timer
  RESOURCE_INDEX "some_proper_index")

The controller manager can then look into this exact index to only fetch controllers and no other components.

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

Karsten1987

As I follow up, I would like to modify the ros2component API similarly to take an optional resource index parameter. That way, a ros-control cli could reuse the same API for things like (component types).

Karsten1987 · 2020-04-15T01:46:14Z

rclcpp_components/cmake/rclcpp_components_register_node.cmake

+    message(STATUS "Setting component resource index to non-default value ${resource_index}")
+  endif()
+
  set(component ${ARGS_PLUGIN})


I didn't want to pollute the changes any further, but I think the two args (PLUGIN and EXECUTABLE) could be checked for empty strings.

Feel free to add it, or we can address it in a follow-up Issue/PR.

Karsten1987 · 2020-04-15T01:46:46Z

rclcpp_components/cmake/rclcpp_components_package_hook.cmake

+  #message(WARNING "registered index: ${resource_index}")
+  #message(WARNING "content for index: ${_RCLCPP_COMPONENTS_${resource_index}__NODES}")


I'll remove these lines before merging. I let them in for now in case the interested reviewers wants to debug ;-)

Reminder to remove before merging.

kfabian · 2020-04-15T12:41:25Z

rclcpp_components/include/rclcpp_components/component_manager.hpp

  ComponentManager(
-    std::weak_ptr<rclcpp::executor::Executor> executor);
+    std::weak_ptr<rclcpp::executor::Executor> executor,
+    std::string node_name = "ComponentManager");


Would it not make sense to have also a constructor that forwards the namespace and node options arguments?

I agree that would probably make sense in this case to make it general purpose.

mjcarroll · 2020-04-15T15:03:40Z

It won't let me comment since it's not a changed line, but does it make sense for the component_manager to remain STATIC here: https://github.com/ros2/rclcpp/pull/1065/files#diff-a31ef69821be6f5f4a19e0bf26f36fa1R24

In the case that it is changed to SHARED, I believe that you would also have to add the visibility macros.

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

Karsten1987 · 2020-04-15T16:27:03Z

First round of CI (build up to demo_nodes_cpp, testing rclcpp_components & demo_nodes_cpp):

Linux
Linux-aarch64
macOS
Windows

rclcpp_components/include/rclcpp_components/component_manager.hpp

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

mjcarroll

LGTM pending removing commented lines.

mjcarroll · 2020-04-15T18:14:07Z

rclcpp_components/cmake/rclcpp_components_register_node.cmake

+    message(STATUS "Setting component resource index to non-default value ${resource_index}")
+  endif()
+
  set(component ${ARGS_PLUGIN})


Feel free to add it, or we can address it in a follow-up Issue/PR.

mjcarroll · 2020-04-15T18:14:20Z

rclcpp_components/cmake/rclcpp_components_package_hook.cmake

+  #message(WARNING "registered index: ${resource_index}")
+  #message(WARNING "content for index: ${_RCLCPP_COMPONENTS_${resource_index}__NODES}")


Reminder to remove before merging.

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

Karsten1987 · 2020-04-15T18:26:50Z

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

Karsten1987 · 2020-04-15T19:19:32Z

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

Karsten1987 · 2020-04-15T21:42:59Z

CI rebuilding up to tf2_ros as it's using the second macro which hasn't been CI'ed before:

Linux
Linux-aarch64
macOS
Windows

mjcarroll

Looks good. This assumes that the list of targets is always the last set of arguments. It could be switched to a multiValueArg at some point in the future, but I agree with not breaking the CMake "api" right now.

nuclearsandwich · 2020-04-16T20:22:16Z

I've traced four new failed tests in last night's builds to this PR. ros2/build_farmer#272

Karsten1987 added 2 commits April 14, 2020 12:52

make functions public & virtual

d81d494

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

flexible resource index for cmake macros

156ff16

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

Karsten1987 marked this pull request as ready for review April 15, 2020 01:36

Karsten1987 requested a review from mjcarroll April 15, 2020 01:36

Karsten1987 commented Apr 15, 2020

View reviewed changes

kfabian reviewed Apr 15, 2020

View reviewed changes

review comments

101fe68

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

mjcarroll reviewed Apr 15, 2020

View reviewed changes

rclcpp_components/include/rclcpp_components/component_manager.hpp Show resolved Hide resolved

Karsten1987 added 2 commits April 15, 2020 10:58

remove superfluous include

799566c

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

remove wrong dllexort

a8832bd

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

mjcarroll approved these changes Apr 15, 2020

View reviewed changes

Karsten1987 added 2 commits April 15, 2020 11:22

check for empty plugin & executable args

2a2a1c1

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

remove commented lines

a02cfff

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

fix typo

1bdb888

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

relax macro constraints

7cd20c9

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

mjcarroll approved these changes Apr 16, 2020

View reviewed changes

Karsten1987 merged commit 50d500e into master Apr 16, 2020

delete-merged-branch bot deleted the public_component_manager branch April 16, 2020 02:08

		#message(WARNING "registered index: ${resource_index}")
		#message(WARNING "content for index: ${_RCLCPP_COMPONENTS_${resource_index}__NODES}")

Conversation

Karsten1987 commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Karsten1987 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjcarroll commented Apr 15, 2020

Uh oh!

Karsten1987 commented Apr 15, 2020

Uh oh!

Uh oh!

mjcarroll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karsten1987 commented Apr 15, 2020

Uh oh!

Karsten1987 commented Apr 15, 2020

Uh oh!

Karsten1987 commented Apr 15, 2020

Uh oh!

mjcarroll left a comment

Choose a reason for hiding this comment

Uh oh!

nuclearsandwich commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Karsten1987 commented Apr 14, 2020 •

edited

Loading