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

Support loading lists of booleans from parameter server #6

Open
wants to merge 2 commits into
base: melodic-devel
Choose a base branch
from

Conversation

gavanderhoorn
Copy link
Contributor

Something I needed and might be of use to others.

This PR also changes getDebugArrayString(..) to a templated function, so as to avoid having to create versions for all the different types of std::vector<..> that NodeHandle::getParam(..) supports.

@gavanderhoorn
Copy link
Contributor Author

NodeHandle::getParam(..) supports a lot of overloads, many more than rosparam_shortcuts exposes through get(..).

Would it make sense to use templating for get(..) as well to be able to pass that through to NodeHandle::getParam(..) or would that not be of interest?

@gavanderhoorn
Copy link
Contributor Author

Oh and target branch is melodic-devel, but I've only tested this on Kinetic. Seeing as kinetic-devel == melodic-devel, I figured you'd want to target PRs against the newest branch.

Avoids having to create multiple variants for vectors with different value_types.
Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

@gavanderhoorn thanks for creating this PR! Would you mind adding the bool list param to the example implementation and README as well?

NodeHandle::getParam(..) supports a lot of overloads, many more than rosparam_shortcuts exposes through get(..).

Would it make sense to use templating for get(..) as well to be able to pass that through to NodeHandle::getParam(..) or would that not be of interest?

I think it definitely makes sense to use template functions where possible. It should be just a matter of fixing the debug strings for the different value types which could probably be done using template functions as well, just like your added getDebugArrayString() template.

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

Great improvement. I like Henning's point about the README

@bhomberg
Copy link

bhomberg commented Jul 11, 2019

@gavanderhoorn, when I switched to using the master branch of MoveIt (from melodic-devel), using the add_vector_of_bool branch broke compiling moveit for me with this error (see below). I fixed it by moving the template specialization to the .cpp file instead of the .h file (PR to our fork here) -- let me know if you'd like me to make a PR to your branch here as well.

CMakeFiles/jog_arm_server.dir/src/jog_arm/jog_ros_interface.cpp.o: In function std::__cxx11::basic_string<char, std::char_traits, std::allocator > rosparam_shortcuts::getDebugArrayString<std::__cxx11::basic_string<char, std::char_traits, std::allocator > >(std::vector<std::__cxx11::basic_string<char, std::char_traits, std::allocator >, std::allocator<std::__cxx11::basic_string<char, std::char_traits, std::allocator > > >)':
jog_ros_interface.cpp:(.text+0x2d0): multiple definition of std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > rosparam_shortcuts::getDebugArrayString<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >)' CMakeFiles/jog_arm_server.dir/src/jog_arm/jog_arm_server.cpp.o:jog_arm_server.cpp:(.text+0x0): first defined here collect2: error: ld returned 1 exit status moveit/moveit_experimental/jog_arm/CMakeFiles/jog_arm_server.dir/build.make:342: recipe for target '/home/bhomberg/ws/iron-ox/devel/lib/moveit_experimental/jog_arm_server' failed make[2]: *** [/home/bhomberg/ws/iron-ox/devel/lib/moveit_experimental/jog_arm_server] Error 1 CMakeFiles/Makefile2:56880: recipe for target 'moveit/moveit_experimental/jog_arm/CMakeFiles/jog_arm_server.dir/all' failed make[1]: *** [moveit/moveit_experimental/jog_arm/CMakeFiles/jog_arm_server.dir/all] Error 2 Makefile:140: recipe for target 'all' failed

@gavanderhoorn
Copy link
Contributor Author

Seems I forgot to add inline on all templated methods.

@davetcoleman
Copy link
Member

I'd love to merge this in, if we can update the README and address the inline issue

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.

4 participants