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

Generate standalone nodelet executable with oneline cmake macro #47

Open
wants to merge 3 commits into
base: indigo-devel
Choose a base branch
from

Conversation

wkentaro
Copy link
Contributor

@wkentaro wkentaro commented Sep 29, 2016

Example:

# CMakeLists.txt
project(image_proc)
find_package(catkin REQUIRED COMPONENTS nodelet)
nodelet_add_executable(rectify image_proc/rectify)
install(TARGETS ${PROJECT_NAME}_nodelet_exe_targets ...)

# Command line
$ rosrun nodelet manager standalone image_proc/rectify  # Before
$ rosrun image_proc rectify  # After

@wkentaro wkentaro force-pushed the nodelet-add-executable branch 4 times, most recently from 2751d88 to cbb7568 Compare September 29, 2016 10:37
@tfoote
Copy link
Member

tfoote commented Sep 29, 2016

This seems like a lot of extra infrastructure and and complexity to support something that could be done with a one line script that gets installed.

For example if you create an executable script rectify like this:

#!/bin/bash

rosrun nodelet manager standalone image_proc/rectify

It has the same effect.

Looking at the code this duplicates the code here: https://github.com/ros/nodelet_core/blob/indigo-devel/nodelet/src/nodelet.cpp#L295-L310 also is already showing divergence in not checking the return code.

In general I think that this will create more confusion as it's creating another use type of hybrid object that's between a nodelet and a node that will only be implemented in some nodelets so it won't be consistent between packages. And the script requires even less cmake lines than the above since it only needs an install rule.

@wkentaro
Copy link
Contributor Author

wkentaro commented Sep 29, 2016

It seems fine.

To pass command line arguments for remapping or setting rosparam, below is collect?

#!/bin/bash

rosrun nodelet nodelet standalone image_proc/rectify $@

@tfoote
Copy link
Member

tfoote commented Sep 29, 2016

Yes, it's recommended to use quotes too: "$@"

https://www.gnu.org/software/bash/manual/bash.html#Special-Parameters

@wkentaro
Copy link
Contributor Author

wkentaro commented Sep 29, 2016

Then, what about adding a macro to generate that bash script?
That enables user remove the many node scripts that only contain rosrun nodelet nodelet standalone ....

@wkentaro wkentaro changed the title Generate single nodelet executable with oneline cmake macro Generate standalone nodelet executable with oneline cmake macro Sep 29, 2016
@wkentaro
Copy link
Contributor Author

Any idea?

@tfoote
Copy link
Member

tfoote commented Oct 13, 2016

I'm not sure that I believe that this is beneficial in the long run. It's adding a lot of complexity for relatively small benefit.

The benefits are:

  • That you could save a few characters on the command line or in the launch file.

rosrun nodelet nodelet standalone image_proc/rectify vs
rosrun image_proc rectify_nodelet_standalone is a savings of 8 characters if you give the bash script a descriptive name that's easy to understand.

  • It is easier to discover the executable via tab completion.

The downsides are:

  • It hides from the user that what they're running is a nodelet and that it's running in standalone mode.
  • It adds more cmake code to maintain going forward.
  • It requres another line of code in everyone's CMakeLists.txt that will need to be documented and maintained going forward.
  • It adds more files to any installation.
  • There are now 2 ways to launch nodelets in standalone mode that needs to be documented and used consistently throughout tuturials etc.

For usage on the command line I think the benefit of discoverability could be added by adding tab completion for the current standalone nodelet instead of creating a new redirection entity that adds complexity but does not provide any additional functionality.

@wkentaro
Copy link
Contributor Author

I think what you're saying is reasonable, but adding some completions to rosrun seems difficult (I posted to ros.answers but no answer):
http://answers.ros.org/question/217105/completion-for-rosrun-with-zsh/

Making nodelet executable as a global solves this problem, do you have idea?

@tfoote
Copy link
Member

tfoote commented Oct 20, 2016

I don't think there's a standard way to do it. The optimal would be that rosrun could generically detect tab completion for the target executables and invoke the appropriate completion rules for any ros node if that node has defined completion. I expect that's possible but I am not familiar with tab completion so cannot make suggestions how to implement it.

@peci1
Copy link

peci1 commented Sep 4, 2024

I've implemented a CMake helper macro cras_node_from_nodelet which we successfully use in many places. It's really just one line of code.

https://github.com/ctu-vras/ros-utils/tree/master/cras_cpp_common#list-of-provided-cmake-helpers

Install ros-noetic-cras-cpp-common, then:

add_library(cras_count_messages_nodelet src/count_messages.cpp)
add_dependencies(cras_count_messages_nodelet ${${PROJECT_NAME}_EXPORTED_TARGETS} ${catkin_EXPORTED_TARGETS})
target_link_libraries(cras_count_messages_nodelet ${catkin_LIBRARIES})
# This is the added line
cras_node_from_nodelet(cras_count_messages_nodelet cras::CountMessagesNodelet OUTPUT_NAME count_messages)

It only requires two arguments - the CMake target generating the nodelet, and the fully qualified C++ class name of the nodelet's class. Specifically, it does not require the nodelet to be split into header/implementation files (but if your nodelet is, you can use the header, too, with a slightly different notation).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants