Make Component Manager public #1065

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

+    set(resource_index ${ARGS_RESOURCE_INDEX})
+    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

@@ -57,31 +62,34 @@ class ComponentManager : public rclcpp::Node
  using ComponentResource = std::pair<std::string, std::string>;

  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

+    set(resource_index ${ARGS_RESOURCE_INDEX})
+    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Component Manager public #1065

Make Component Manager public #1065

Karsten1987 commented Apr 14, 2020 •

edited

Loading

Karsten1987 left a comment

Karsten1987 Apr 15, 2020

mjcarroll Apr 15, 2020

Karsten1987 Apr 15, 2020

mjcarroll Apr 15, 2020

kfabian Apr 15, 2020

mjcarroll Apr 15, 2020

mjcarroll commented Apr 15, 2020

Karsten1987 commented Apr 15, 2020

mjcarroll left a comment

mjcarroll Apr 15, 2020

mjcarroll Apr 15, 2020

Karsten1987 commented Apr 15, 2020

Karsten1987 commented Apr 15, 2020

Karsten1987 commented Apr 15, 2020

mjcarroll left a comment

nuclearsandwich commented Apr 16, 2020

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

Make Component Manager public #1065

Make Component Manager public #1065

Conversation

Karsten1987 commented Apr 14, 2020 • edited Loading

Karsten1987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjcarroll commented Apr 15, 2020

Karsten1987 commented Apr 15, 2020

mjcarroll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karsten1987 commented Apr 15, 2020

Karsten1987 commented Apr 15, 2020

Karsten1987 commented Apr 15, 2020

mjcarroll left a comment

Choose a reason for hiding this comment

nuclearsandwich commented Apr 16, 2020

Karsten1987 commented Apr 14, 2020 •

edited

Loading