Skip to content

Support loading lists of booleans from parameter server#6

Open
gavanderhoorn wants to merge 2 commits intoPickNikRobotics:melodic-develfrom
gavanderhoorn:add_vector_of_bool
Open

Support loading lists of booleans from parameter server#6
gavanderhoorn wants to merge 2 commits intoPickNikRobotics:melodic-develfrom
gavanderhoorn:add_vector_of_bool

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
Contributor

@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