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.

Sign up for GitHub

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

Update demo_nodes_cpp to use new rclcpp_components CMake API. #392

Draft
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Draft

Update demo_nodes_cpp to use new rclcpp_components CMake API. #392

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
188 changes: 143 additions & 45 deletions demo_nodes_cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,51 +77,149 @@ add_dependencies(services_library)
add_dependencies(parameters_library)
add_dependencies(topics_library)

rclcpp_components_register_node(timers_library
PLUGIN "demo_nodes_cpp::OneOffTimerNode"
EXECUTABLE one_off_timer)
rclcpp_components_register_node(timers_library
PLUGIN "demo_nodes_cpp::ReuseTimerNode"
EXECUTABLE reuse_timer)

rclcpp_components_register_node(services_library
PLUGIN "demo_nodes_cpp::ServerNode"
EXECUTABLE add_two_ints_server)
rclcpp_components_register_node(services_library
PLUGIN "demo_nodes_cpp::ClientNode"
EXECUTABLE add_two_ints_client_async)

rclcpp_components_register_node(parameters_library
PLUGIN "demo_nodes_cpp::ListParameters"
EXECUTABLE list_parameters)
rclcpp_components_register_node(parameters_library
PLUGIN "demo_nodes_cpp::ParameterBlackboard"
EXECUTABLE parameter_blackboard)
rclcpp_components_register_node(parameters_library
PLUGIN "demo_nodes_cpp::SetAndGetParameters"
EXECUTABLE set_and_get_parameters)
rclcpp_components_register_node(parameters_library
PLUGIN "demo_nodes_cpp::ParameterEventsAsyncNode"
EXECUTABLE parameter_events_async)
rclcpp_components_register_node(parameters_library
PLUGIN "demo_nodes_cpp::EvenParameterNode"
EXECUTABLE even_parameters_node)

rclcpp_components_register_node(topics_library
PLUGIN "demo_nodes_cpp::Talker"
EXECUTABLE talker)
rclcpp_components_register_node(topics_library
PLUGIN "demo_nodes_cpp::SerializedMessageTalker"
EXECUTABLE talker_serialized_message)
rclcpp_components_register_node(topics_library
PLUGIN "demo_nodes_cpp::Listener"
EXECUTABLE listener)
rclcpp_components_register_node(topics_library
PLUGIN "demo_nodes_cpp::SerializedMessageListener"
EXECUTABLE listener_serialized_message)
rclcpp_components_register_node(topics_library
PLUGIN "demo_nodes_cpp::ListenerBestEffort"
EXECUTABLE listener_best_effort)
rclcpp_components_register_nodes(
PLUGINS
"demo_nodes_cpp::ReuseTimerNode"
"demo_nodes_cpp::OneOffTimerNode"
FROM timers_library
)

rclcpp_components_add_executable(one_off_timer
Copy link
Member

Choose a reason for hiding this comment

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

This is somewhat subjective (I'd like to know what others think about it), but I would avoid _add_executable as it doesn't really behave like cmake's add_executable. Instead something like rclcpp_components_create_node_executable(...) makes a bit more sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, add_executable takes sources while rclcpp_components_add_executable would take components in already built libraries. That's assuming no form of install takes place, which I think it shouldn't. But it may still not be the best naming. I just mimic'd CMake as I had no better ideas.

Copy link
Member

Choose a reason for hiding this comment

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

It also creates a new target out of a source file the user doesn't have control over... There's already magic there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that doesn't have to be the case :)

I don't want to make it overly complicated, with user predefined templates and stuff. I just saw the opportunity to confine boilerplate code to a single place while still allowing the user to tinker with the target.

PLUGINS "demo_nodes_cpp::ReuseTimerNode"
Copy link
Member

Choose a reason for hiding this comment

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

COMPONENTS or NODE_COMPONENTS instead? Plugins is just so generic, since they're very specific kinds of plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I fully agree. Actually, we use terms like composable nodes, components, plugins and node plugins interchangeably in many places and that's nuts. Components sounds reasonable to me to standardize to.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with standardizing on component

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yeah, I dunno, components is just such a generic term. I'd prefer COMPOSABLE_NODES or otherwise NODE_COMPONENTS, but I if you guys really feel like COMPONENTS is right, then I'd defer to that.

Copy link
Member

Choose a reason for hiding this comment

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

Either way I think it would be best for us to get our terminology straight "composable node" versus "node component" and use it everywhere. We could also use "component", but again I think that has too much overlap potential with existing computer science tools/ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Composable node is also fine, node components sound like there are other kinds of components (will there ever be?)

Copy link
Member

Choose a reason for hiding this comment

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

node components sound like there are other kinds of components (will there ever be?)

No idea.

I like "composable node" because it can be more naturally extended in English, e.g. "composable lifecycle node" or "composable light-weight node" or whatever. I guess you can do the same with "node component", but it feels more natural to me the other way at least.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for standardizing the term(s). The way I've been thinking about it is that there are composable nodes and one way we can compose them by making components. Who knows, maybe in the future there will exist other mechanisms other than (node) components for composing nodes.

My two cents.

FROM timers_library
)

rclcpp_components_add_executable(reuse_timer
PLUGINS "demo_nodes_cpp::ReuseTimerNode"
FROM timers_library
)

install(TARGETS
one_off_timer reuse_timer
DESTINATION lib/${PROJECT_NAME}
)
Copy link
Member

Choose a reason for hiding this comment

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

See, I think that this is equally modular and more concise:

rclcpp_components_register_node(timers_library
  PLUGIN "demo_nodes_cpp::OneOffTimerNode"
  EXECUTABLE one_off_timer)
rclcpp_components_register_node(timers_library
  PLUGIN "demo_nodes_cpp::ReuseTimerNode"
  EXECUTABLE reuse_timer)

If you don't want to have an executable generated, then simply leave off the EXECUTABLE option. If you want an executable, but then want to install it somewhere else, or something, then you could leave off the executable option, call rclcpp_components_create_node_executable(reuse_timer NODE_PLUGINS "demo_nodes_cpp::ReuseTimerNode" FROM timers_library NO_INSTALL), then you could install it where ever you want.

Copy link
Member

Choose a reason for hiding this comment

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

The name could be better in this case, maybe rclcpp_components_make_node() or rclcpp_components_create_node(). To convey something like, "give me that library with your node in it, and we'll do everything else to make it a 'node' as you expect". Where "as you expect" doesn't just include being discoverable with the component command line tools, but also that it has an executable and can be run with ros2 run, etc...

Copy link
Contributor Author

@hidmic hidmic Sep 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think that this is equally modular and more concise:

And that's completely fair, for sure. I just want to point out that:

  1. It obscures an install step, implicitly relying on an explicit install step for the library it depends on. One may argue registering a component does the same thing, but failure modes are different for a missing entry in ament index and a missing library upon execution.
  2. It performs two seemingly unrelated tasks: it registers a component to be discoverable and it creates a standalone executable for that component. One does not need the other.
  3. It bounds us to one component per executable.

The name could be better in this case, maybe rclcpp_components_make_node() or rclcpp_components_create_node()

That's implying there's only one component per executable. Also, it somewhat conflates executable and node concepts.

I tried to picture how it'd be like to actually use this. Every CMakeLists.txt from now on will likely add_library with components, ament_target_dependencies for it and install it somewhere. There, what gets done with that library may vary quite a bit.

I can imagine people exposing those components, and there's rclcpp_components_register_nodes or rclcpp_components_register_components for that. I can also imagine people not doing so. For instance, one may want to keep tightly coupled components "private" and only provide an executable composing them. Components are the new way of writing ROS2 code, whether it makes sense to dynamically compose them or not. There's rclcpp_components_add_executable for that, that purposefully does not register anything.

I can imagine people using their components as standalone executables in the early stages of a project. Specially those that come from a ROS1 setting. I can then picture that same people realizing there's room for system optimization by grouping nodes together. If going full dynamic is an option, then use launch, but if not or if it doesn't make sense to go that way rclcpp_components_target_compose allows to bundle multiple components in a single executable at build time. I can imagine people building an entire ROS ecosystem in a single process, heavily leveraging intra-process communication.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense for it to be rclcpp_components_register_component rather than register_node, for clarity.

Copy link
Member

Choose a reason for hiding this comment

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

That's implying there's only one component per executable. Also, it somewhat conflates executable and node concepts.

Yeah, but that's the like 99% most common use case in my opinion. I'm not saying we shouldn't build this macro out of more macros that give you more flexibility, but I do think the single component with a single executable for it is the most common case.

I actually don't see a need for multi-component executables, since the users can write their own main to do this, and they can compose the equivalent at runtime with launch or command line tools.

I think it's fine if we have that feature, but it's by no means as important as the single component executable, which I think really every component ought to have, just to preserve the idea that a "composable node" is something that I can either load into a container or run on it's own (where that means it has a corresponding executable).

Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense for it to be rclcpp_components_register_component rather than register_node, for clarity.

What's the difference in a "component" and a "node"? For me "component" is too generic, it should be "composable_node" or just "node".

Copy link
Contributor Author

@hidmic hidmic Sep 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I do think the single component with a single executable for it is the most common case.

It is. But I do have a bias towards a unified, general API. We can for sure provide a simple API on top... if we can come up with a meaningful name :o

I actually don't see a need for multi-component executables, since the users can write their own main to do this

True, but it's going to be boilerplate most of the time -- unless one gets very specific about NodeOptions.

Copy link
Member

Choose a reason for hiding this comment

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

Well, not necessarily, they can have any number of additional arguments to their constructor, for example, maybe you have a scenario where you want two nodes to have access to the same instance of a library (maybe one that reads data from a camera or something. You couldn't specify that with dynamic composition, but you could if you were manually composing. Also you might want to get fancy with how many and what kinds of executors you want to use.

Copy link
Member

Choose a reason for hiding this comment

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

I actually don't see a need for multi-component executables, since the users can write their own main to do this

True, but it's going to be boilerplate most of the time -- unless one gets very specific about NodeOptions.

I think this type of "boilerplate" won't show up too much if we expect this to be a rare use-case.

Seems like we all agree that it's good to have tools in place to support this feature, but we should have the simpler, recommended, API built on top. Now we just need to determine the color of the shed.


rclcpp_components_register_nodes(
PLUGINS
"demo_nodes_cpp::ServerNode"
"demo_nodes_cpp::ClientNode"
FROM services_library
)
rclcpp_components_add_executable(add_two_ints_server
PLUGINS "demo_nodes_cpp::ServerNode"
FROM services_library
)
rclcpp_components_add_executable(add_two_ints_client_async
PLUGINS "demo_nodes_cpp::ClientNode"
FROM services_library
)

install(TARGETS
add_two_ints_server add_two_ints_client_async
DESTINATION lib/${PROJECT_NAME}
)

rclcpp_components_register_nodes(
PLUGINS
"demo_nodes_cpp::ListParameters"
"demo_nodes_cpp::ParameterBlackboard"
"demo_nodes_cpp::SetAndGetParameters"
"demo_nodes_cpp::ParameterEventsAsyncNode"
"demo_nodes_cpp::EvenParameterNode"
FROM parameters_library
)

rclcpp_components_add_executable(list_parameters
PLUGINS "demo_nodes_cpp::ListParameters"
FROM parameters_library
)

rclcpp_components_add_executable(parameter_blackboard
PLUGINS "demo_nodes_cpp::ParameterBlackboard"
FROM parameters_library
)

# rclcpp_components_add_executable(compound_server
# PLUGINS
# "demos_nodes_cpp::ServerNode"
# FROM services_library
# )
#
# rclcpp_components_target_compose(compound_server
Copy link
Member

Choose a reason for hiding this comment

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

How does this work? Does it add to compound_server?

Copy link
Contributor Author

@hidmic hidmic Sep 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Does it add to compound_server?

Correct. There's a small gap to bridge if we go from

rclcpp_components_add_executable(my_executable
   COMPONENT "some_ns::Talker"
   FROM my_library 
)

to

rclcpp_components_add_executable(my_executable
   COMPONENTS 
     "some_ns::Talker"
     "some_ns::Listener"
   FROM my_library 
)

It gets somewhat larger if we want to enable adding components from multiple libraries. That's
rclcpp_components_target_compose() purpose.
Naming suggestions are much appreciated.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we have the ability to support the second case, correct? The current setup only allows for the 1-to-1 mapping of component to executable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current setup only allows for the 1-to-1 mapping of component to executable?

Correct, but jumping from one component in one library to N components in M libraries is straightforward. Not something that we have to address now though, but we can gear the API in that direction and add missing bits later on.

# PLUGINS
# "demos_nodes_cpp::ParameterBlackboard"
# FROM parameters_library
# )

rclcpp_components_add_executable(set_and_get_parameters
PLUGINS "demo_nodes_cpp::SetAndGetParameters"
FROM parameters_library
)

rclcpp_components_add_executable(parameter_events_async
PLUGINS "demo_nodes_cpp::ParameterEventsAsyncNode"
FROM parameters_library
)

rclcpp_components_add_executable(even_parameters_node
PLUGINS "demo_nodes_cpp::EvenParameterNode"
FROM parameters_library
)

install(TARGETS
list_parameters parameter_blackboard set_and_get_parameters
parameter_events_async even_parameters_node
DESTINATION lib/${PROJECT_NAME}
)

rclcpp_components_register_nodes(
PLUGINS
"demo_nodes_cpp::Talker"
"demo_nodes_cpp::SerializedMessageTalker"
"demo_nodes_cpp::Listener"
"demo_nodes_cpp::SerializedMessageListener"
"demo_nodes_cpp::ListenerBestEffort"
FROM topics_library
)

rclcpp_components_add_executable(talker
PLUGINS "demo_nodes_cpp::Talker"
FROM topics_library
)

# rclcpp_components_add_executable(talker_and_listener
# PLUGINS
# "demos_nodes_cpp::Talker"
# "demos_nodes_cpp::Listener"
# FROM topics_library
# )

rclcpp_components_add_executable(talker_serialized_message
PLUGINS "demo_nodes_cpp::SerializedMessageTalker"
FROM topics_library
)

rclcpp_components_add_executable(listener
PLUGINS "demo_nodes_cpp::Listener"
FROM topics_library
)

rclcpp_components_add_executable(listener_serialized_message
PLUGINS "demo_nodes_cpp::SerializedMessageListener"
FROM topics_library
)

rclcpp_components_add_executable(listener_best_effort
PLUGINS "demo_nodes_cpp::ListenerBestEffort"
FROM topics_library
)

install(TARGETS
talker talker_serialized_message
listener listener_serialized_message
listener_best_effort
DESTINATION lib/${PROJECT_NAME}
)

install(TARGETS
timers_library
Expand Down