Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cmake infrastructure for creating components #784

Merged
merged 25 commits into from
Jul 24, 2019
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions rclcpp_components/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ ament_target_dependencies(component_container
"rclcpp"
)

install(FILES
src/node_main.cpp.in
DESTINATION ${PROJECT_BINARY_DIR}
skucheria marked this conversation as resolved.
Show resolved Hide resolved
skucheria marked this conversation as resolved.
Show resolved Hide resolved
)

add_executable(
component_container_mt
src/component_container_mt.cpp
Expand Down
69 changes: 69 additions & 0 deletions rclcpp_components/cmake/rclcpp_components_register_node.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# Copyright 2019 Open Source Robotics Foundation, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# usage: rclcpp_components_register_node(
# <target> PLUGIN <component> EXECUTABLE <node>)
skucheria marked this conversation as resolved.
Show resolved Hide resolved
#
# Register an rclcpp component with the ament
# resource index and create an executable.
#
# :param target: the shared library target
# :type target: string
# :param ARGS: the plugin name and node name for executable
# :type ARGS: list of strings
# :param component: the component plugin name
# :type component: string
# :param node: the node's executable name
# :type node: string
#
macro(rclcpp_components_register_node target)
cmake_parse_arguments(
ARGS
""
"PLUGIN;EXECUTABLE"
""
${ARGN})
skucheria marked this conversation as resolved.
Show resolved Hide resolved
set(component ${ARGS_PLUGIN})
set(node ${ARGS_EXECUTABLE})
_rclcpp_components_register_package_hook()
if(WIN32)
set(_path "bin")
else()
set(_path "lib")
endif()
dirk-thomas marked this conversation as resolved.
Show resolved Hide resolved
set(_RCLCPP_COMPONENTS__NODES
"${_RCLCPP_COMPONENTS__NODES}${component};${_path}/$<TARGET_FILE_NAME:${target}>\n")
set(CLASS_NAME ${component})
set(build_dir ${PROJECT_BINARY_DIR}/../rclcpp_components)
skucheria marked this conversation as resolved.
Show resolved Hide resolved
configure_file(${build_dir}/node_main.cpp.in
${build_dir}/node_main_${node}.cpp @ONLY)
add_executable(${node} ${build_dir}/node_main_${node}.cpp)
set(lib ${target})
# Needed so symbols aren't dropped if not usesd
skucheria marked this conversation as resolved.
Show resolved Hide resolved
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(lib
"-Wl,--no-as-needed"
${target}
"-Wl,--as-needed")
endif()
target_link_libraries(${node}
${lib})
skucheria marked this conversation as resolved.
Show resolved Hide resolved
ament_target_dependencies(${node}
"rclcpp"
"class_loader"
"rclcpp_components")
install(TARGETS
${node}
DESTINATION lib/${PROJECT_NAME})
endmacro()
2 changes: 1 addition & 1 deletion rclcpp_components/rclcpp_components-extras.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@ macro(_rclcpp_components_register_package_hook)
endmacro()

include("${rclcpp_components_DIR}/rclcpp_components_register_nodes.cmake")

include("${rclcpp_components_DIR}/rclcpp_components_register_node.cmake")
70 changes: 70 additions & 0 deletions rclcpp_components/src/node_main.cpp.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright 2016 Open Source Robotics Foundation, Inc.
skucheria marked this conversation as resolved.
Show resolved Hide resolved
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include <memory>
#include <string>
#include <vector>

#include "class_loader/class_loader.hpp"
#include "rclcpp/rclcpp.hpp"
#include "rclcpp_components/node_factory.hpp"
#include "rclcpp_components/node_factory_template.hpp"

#define NODE_MAIN_LOGGER_NAME "node_main"

int main(int argc, char * argv[])
{
// Force flush of the stdout buffer.
setvbuf(stdout, NULL, _IONBF, BUFSIZ);
skucheria marked this conversation as resolved.
Show resolved Hide resolved

auto args = rclcpp::init_and_remove_ros_arguments(argc, argv);
rclcpp::Logger logger = rclcpp::get_logger(NODE_MAIN_LOGGER_NAME);
rclcpp::executors::SingleThreadedExecutor exec;
rclcpp::NodeOptions options;
options.arguments(args);
Copy link
Contributor

@hidmic hidmic Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skucheria @jacobperron why do we remove ROS arguments and pass non-ROS arguments to NodeOptions? I don't follow. Just for context, I'm having issues with argument validation as whatever is fed here is decidedly not a ROS argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation was some of our demos take non-ROS arguments via the command-line. When we went turn them into components, it wasn't obvious how to pass along these non-ROS arguments to the code.
What's the recommended way to get non-ROS arguments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, image_tools parses options by passing the nodes arguments to this function:

https://github.com/ros2/demos/blob/97651912ebad439a6bc62c34671cad6a5bfee785/image_tools/src/options.cpp#L46

IMO, the demos should be refactored to make use of ROS parameters instead of custom CLI arguments (but that's beside the point).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we had a discussion about this on #816 and ros2/rclpy#405 (comment). As a result, all NodeOptions::arguments() are implicitly ROS arguments. To achieve what you want here, you'd have to do -- non-ros-arg0 ... non-ros-argN, which is inconvenient but we were operating under the assumption that passing non-ROS arguments to a ROS node was a rare use case.

It looks like it's not @wjwwood @ivanpauno. Unless either of you strongly disagrees, I'll force --ros-args to be given explicitly in both rclpy and rclcpp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to explicit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll force --ros-args to be given explicitly in both rclpy and rclcpp.

I think that's reasonable. I had forgotten this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it then possible for --ros-args to clash with non-ros args? E.g. what if my node expects a CLI argument named -p?

Does NodeOptions.arguments() potentially return something like

"--ros-args -p foo:=bar -- -p my_non_ros_arg" ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, --ros-args was prepended except if it was already part of the arguments.
@hidmic sent a bunch of PRs for changing that, so --ros-args won't be prepended more.
NodeOptions.arguments({"-p"}) will set a user specific argument.
NodeOptions.arguments({"--ros-args", "-p"}) will set a ros specific argument.

IDK if that's exactly what you asked or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobperron even with the changes I'm making, that could be the case, yes. But here you're removing ROS arguments and thus --ros-args ... -- sets will be removed. That also means that no --ros-args will reach your node as local arguments (they may still make it as global arguments) with this implementation, but it doesn't look like it cares about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation.

std::vector<class_loader::ClassLoader *> loaders;
skucheria marked this conversation as resolved.
Show resolved Hide resolved
std::vector<rclcpp_components::NodeInstanceWrapper> node_wrappers;
std::vector<std::string> libraries = {
// all classes from libraries linked by the linker (rather then dlopen)
// are registered under the library_path ""
"",
};
std::string className = "rclcpp_components::NodeFactoryTemplate<@CLASS_NAME@>";
for (const auto & library : libraries) {
RCLCPP_DEBUG(logger, "Loading library %s", library.c_str());
auto loader = new class_loader::ClassLoader(library);
auto classes = loader->getAvailableClasses<rclcpp_components::NodeFactory>();
for (auto clazz : classes) {
std::string name = clazz.c_str();
if (!(name.compare(className))) {
RCLCPP_DEBUG(logger, "Instantiate class %s", clazz.c_str());
auto node_factory = loader->createInstance<rclcpp_components::NodeFactory>(clazz);
auto wrapper = node_factory->create_node_instance(options);
auto node = wrapper.get_node_base_interface();
node_wrappers.push_back(wrapper);
exec.add_node(node);
}
}
loaders.push_back(loader);
}

skucheria marked this conversation as resolved.
Show resolved Hide resolved
exec.spin();

for (auto wrapper : node_wrappers) {
exec.remove_node(wrapper.get_node_base_interface());
}
node_wrappers.clear();
rclcpp::shutdown();
return 0;
}