roscpp: Adding std::vector and std::map rosparam sets/gets #279
Conversation
|
Fix for #263 |
|
I have needed this in the past. |
|
@wjwwood Where should unit tests for this crap go? |
|
There is an external package, https://github.com/ros/ros_comm/tree/groovy-devel/test/test_roscpp |
Ah cool. I'll add some unit tests there, and then ping back. |
|
Ok. Unit tests are in place (and passing): https://github.com/ros/ros_comm/pull/279/files#diff-4 Currently, it will automatically convert between float/double/int/bool but not string, but that's easy to change if we want. Repro: unset CMAKE_PREFIX_PATH
source /opt/ros/hydro/setup.sh
mkdir -p ws/src
git clone git@github.com:jhu-lcsr-forks/ros_comm.git -b feature-roscpp-composite-rosparam ws/src/ros_comm
cd ws
catkin_make
catkin_make tests
catkin_make run_tests_test_roscpp_rostest_test_launch_params.xml |
|
Nice! Thanks for slogging through the unit tests. Is there a reason to define a function for every single vector and map? A single function template would be much shorter. And acceptable types could be limited with boost, though other types wouldn't compile anyway, so I wouldn't bother. Is it to hide the implementation in the cpp file? Is that worth doing? Is there a reason to mess with type safety? Int to float is pretty obvious ([0, .1] should not fail), but float to int? If users mean an int, that's what they should enter! I think, since YAML is already parsed to numbers, and because C++ is strongly typed, we should do strict type safety. |
A globally-exposed function template would be much shorter to write, but the end cost would then be put onto the user and the user's compiler. When templates are meant to take arbitrary or a broad spectrum of types, then they need to be exposed to the user, but when they're only used to simplify implementation, there's no real benefit to exposing them, it just increases the time it would take to compile every binary which includes "ros/ros.h". Also, adding all the wrapper functions was the easiest part of the patch. Additionally, by not exposing the templated code in what should arguably be one of the simplest parts of ROS, then you make the error messages much clearer when someone tries to pull the wrong type off of the parameter server.
Well, float to int, or double to int, or int to bool are all reasonable casts, they just come with the cost of loss of precision. Even in C++, this doesn't even generate a warning when you compile it, let alone an error. I think in this case we should be as consistent as we can be with C++ standards. In this case, if it's possible to |
|
Ah, I see. " In this case, if it's possible to static_cast() the parameter type to the output type, we should let the user do so." |
|
I thought of another, slight enhancement: It seems to me like filling an xmlRpcValue with a std::vector/std::map should have its own function in that class. But I'm not sure what the best way to call that templated code would be. Can constructors be templated? Member functions? Perhaps name it after an appropriate stl function? Or would it have to be a non-member function? Of the form template<class T> void createFromVector(const std::vector<T>& vec, xmlRpcValue& val);or even an operator template<class T> xmlRpcValue& operator<<(xmlRpcValue& val, const std::vector<T>& vec); |
|
@b-adkins Maybe more advanced APIs can be discussed later on after we've gotten someone to review / merge this simple one. Anyone? |
|
@dirk-thomas just got back from Germany, maybe he can review it. |
|
I just took a first look and the patch looks very good. I will add some comments but they should be all be pretty minor. It would be great if you could address them (even if some are doc and code style only) before the merge - probably squash your commits afterwards. |
| * \return true if the parameter value was retrieved, false otherwise | ||
| * \throws InvalidNameException If the parameter key begins with a tilde, or is an otherwise invalid graph resource name | ||
| */ | ||
| bool getParamCached(const std::string& key, float& d) const; |
There was a problem hiding this comment.
Can you change the parameter name to f and also update the doc string which currently refers to the parameter as i?
|
@dirk-thomas, all requested fixes made in latest commits. |
|
The Can you please also squash the commits at the end - that keeps the log more compact. |
roscpp: adding unit tests for vector/map rosparam roscpp: vector/dict rosparam fixing commas roscpp: rosparam left-handed amps roscpp: rosparam making map/vec sets references roscpp: rosparam const references in sets roscpp: rosparam more left-handed amps roscpp: adding consts to rosparam std::map setters
|
@dirk-thomas The const for |
|
Great, thank you very much for implementing the feature and taking the time to iterate (on my pedantic comments 😄 ). |
…rosparam roscpp: Adding std::vector and std::map rosparam sets/gets
|
#302 might be a regression of this pull request. |
Yeah I think this could happen if someone adds |
|
These utility functions are a huge win. Thanks! Any reason we shouldn't include std::list as well? |
rqt_bag: Fix topic type retrieval for multiple bag files
…te-rosparam roscpp: Adding std::vector and std::map rosparam sets/gets
This patch adds the ability to call
ros::param::get()and ros::param::set() withstd::vector<T>andstd::map<std::string,T>where T is one of (std::string,double,float,int,bool).Also some trivial doc fixes.