Get parameter map (#575) (crystal-backport) by clalancette · Pull Request #619 · ros2/rclcpp

clalancette · 2019-01-17T16:18:03Z

This is the backport of 99dd031 onto the crystal branch, since I'd like to get it into the 2nd crystal sync ros2/ros2#647 . There are no differences between this change and the one that was merged onto master, and this change only adds an API, so there is no API break.

Add in the ability to get parameters in a map.

Any parameters that have a "." in them will be considered to
be part of a "map" (though they can also be get and set
individually). This PR adds two new template specializations
to the public node API so that it can take a map, and store
the list of values (so setting the parameter with a name of
"foo" and a key of "x" will end up with a parameter of "foo.x").
It also adds an API to get all of the keys corresponding to
a prefix, and returing that as a map (so a get of "foo" will
get all parameters that begin with "foo."). Note that all
parameters within the map must have the same type, otherwise
an rclcpp::ParameterTypeException will be thrown.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Fix style problems pointed out by uncrustify/cpplint.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Move tests for set_parameter_if_not_set/get_parameter map to rclcpp.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Rename get_parameter -> get_parameters.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Add in documentation from review.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

* Add in the ability to get parameters in a map. Any parameters that have a "." in them will be considered to be part of a "map" (though they can also be get and set individually). This PR adds two new template specializations to the public node API so that it can take a map, and store the list of values (so setting the parameter with a name of "foo" and a key of "x" will end up with a parameter of "foo.x"). It also adds an API to get all of the keys corresponding to a prefix, and returing that as a map (so a get of "foo" will get all parameters that begin with "foo."). Note that all parameters within the map must have the same type, otherwise an rclcpp::ParameterTypeException will be thrown. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Fix style problems pointed out by uncrustify/cpplint. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Move tests for set_parameter_if_not_set/get_parameter map to rclcpp. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Rename get_parameter -> get_parameters. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in documentation from review. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

dirk-thomas · 2019-01-18T18:53:37Z

This is the backport of 99dd031 onto the crystal branch, since I'd like to get it into the 2nd crystal sync

Is there a specific use case / problem why this additional API is needed in Crystal?

clalancette · 2019-01-18T19:09:18Z

Is there a specific use case / problem why this additional API is needed in Crystal?

Yes, it's so that we can release ros2/teleop_twist_joy#8 into Crystal (once it has been approved/merged).

sloretz · 2019-01-24T00:36:51Z

rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp

    rclcpp::Parameter & parameter) const;

+  RCLCPP_PUBLIC
+  virtual


I think adding a virtual method breaks ABI

Great catch. You are completely right; this does break ABI.

I've been trying to find a workaround here (essentially by bubbling the API up through the layers), but I haven't found anything that works yet. Any thoughts on ways to maybe workaround this (just for the Crystal branch)?

Never mind, I found another way. Update to this PR coming.

sloretz · 2019-01-24T00:37:14Z

rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp

+   * \return false otherwise.
+   */
+  RCLCPP_PUBLIC
+  virtual


Same comment about adding a virtual method

This way, we don't break ABI in crystal. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

dirk-thomas · 2019-01-24T19:56:27Z

Since this patch has changed from a backport to a modified patch with different logic can you please describe how the backported change is different than what is available on master and if you are planning to update the code on master to match this patch or if it should stay as it is.

clalancette · 2019-01-25T21:18:42Z

Since this patch has changed from a backport to a modified patch with different logic can you please describe how the backported change is different than what is available on master and if you are planning to update the code on master to match this patch or if it should stay as it is.

Right. For whatever reason when I did the original patch, I totally missed that list_parameters existed, which more-or-less provides the same functionality as get_parameters_by_prefix. The main benefit to using get_parameters_by_prefix is that it is atomic; it takes the mutex, iterates over all of the keys with the prefix, gets their values, and stuffs them in the map to be returned. When using list_parameters as in this patch, the mutex is taken to get the list of parameters with the prefix, but then it is dropped. We then have to call get_parameter in a loop to get the value for each parameter. This leads to a possible TOCTTOU bug, where we could fetch the list of parameters, one is deleted, and then we try to get_parameter on the now-deleted parameter.

My view on it is that we should keep get_parameters_by_prefix for master since it is atomic (and slightly faster), but we can live with the (probably rare) race in crystal to make sure we don't break ABI. Thoughts?

dirk-thomas · 2019-01-25T21:42:11Z

I am not convinced if it is worth introducing a new API into Crystal in a patch release which will not be present in master / future distros.

clalancette · 2019-01-25T21:50:44Z

I am not convinced if it is worth introducing a new API into Crystal in a patch release which will not be present in master / future distros.

But that's not what would happen here.

On master, #575 added the following new APIs:

Node class:
- set_parameters_if_not_set
- get_parameters
NodeParameters class:
- get_parameters_by_prefix

In Crystal, this PR would add the following new APIs:

Node class:
- set_parameters_if_not_set
- get_parameters

So the APIs are compatible, its just that code that compiles against master would be able to call get_parameters slightly faster and with atomicity. Either way, code that calls set_parameters_if_not_set or get_parameters would work across Crystal and master.

dirk-thomas · 2019-01-25T22:12:41Z

So the APIs are compatible, its just that code that compiles against master would be able to call get_parameters slightly faster and with atomicity.

Do you mean get_parameters_by_prefix here?

If get_parameters_by_prefix is so much better should get_parameters be removed so that users don't accidentally call a function which isn't really safe to use?

clalancette · 2019-01-29T15:03:29Z

If get_parameters_by_prefix is so much better should get_parameters be removed so that users don't accidentally call a function which isn't really safe to use?

get_parameters_by_prefix is really an implementation detail of how to get parameters from the lower-level NodeParameters class. Users should really be interacting with the top-level Node APIs, which are get_parameters and set_parameters_if_not_set. On crystal, the Node API for get_parameters will be imlpemented by calling list_parameters, followed by a call to get_parameter for each of the returned values (this PR). This has the benefit of using existing APIs (preserving ABI), but has the downside that fetching the set of values with a prefix isn't atomic. On master, get_parameters is implemented by calling get_parameters_by_prefix, which has the downside of needing to change ABI, but has the benefit of fetching the set of values atomically.

In my mind, the question is really whether we want to add get_parameters_by_prefix to master at all. I think we do, since it provides atomicity of fetching the set of values, but I would definitely listen to arguments against that.

* Zero-initialize message_info prior to taking Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * Added test for timestamp presence Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * move message_info test to a new test file Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * Add conditional timestamp test to normal subscriber test Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * Remove dedicated timestamp test Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * Use rmw_time_point_value_t instead of rmw_time_t Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * Use rcutils for cross-platform compatibility. Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * Style fix. Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * More style fix. Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * test fastrtps_dynamic properly; also cmake linter error Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * Expect timestamps on cyclonedds as well Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com> * Distinguish received timestamp support. Signed-off-by: Luetkebohle Ingo (CR/AEX3) <ingo.luetkebohle@de.bosch.com>

…the first one. (ros2#619) * Fix --topics flag for ros2 bag play being ignored for all bags after the first one. Signed-off-by: Aleksandr Rozhdestvenskii <hexonxons@gmail.com>

clalancette added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jan 17, 2019

clalancette changed the title ~~Get parameter map (#575)~~ Get parameter map (#575) (crystal-backport) Jan 17, 2019

sloretz reviewed Jan 24, 2019

View reviewed changes

clalancette added 2 commits January 24, 2019 16:53

Use list_parameters instead of get_parameters_by_prefix.

cd4edf3

This way, we don't break ABI in crystal. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Fix style problems.

99dc044

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

dirk-thomas approved these changes Jan 29, 2019

View reviewed changes

clalancette merged commit cd6f195 into crystal Jan 29, 2019

clalancette deleted the crystal-backport-575 branch January 29, 2019 21:05

clalancette removed the in review Waiting for review (Kanban column) label Jan 29, 2019

nuclearsandwich mentioned this pull request Jan 29, 2019

Crystal Patch Release 2 ros2/ros2#647

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get parameter map (#575) (crystal-backport)#619

Get parameter map (#575) (crystal-backport)#619
clalancette merged 3 commits intocrystalfrom
crystal-backport-575

clalancette commented Jan 17, 2019

Uh oh!

dirk-thomas commented Jan 18, 2019

Uh oh!

clalancette commented Jan 18, 2019

Uh oh!

sloretz Jan 24, 2019

Uh oh!

clalancette Jan 24, 2019

Uh oh!

clalancette Jan 24, 2019

Uh oh!

sloretz Jan 24, 2019

Uh oh!

dirk-thomas commented Jan 24, 2019

Uh oh!

clalancette commented Jan 25, 2019

Uh oh!

dirk-thomas commented Jan 25, 2019

Uh oh!

clalancette commented Jan 25, 2019

Uh oh!

dirk-thomas commented Jan 25, 2019

Uh oh!

clalancette commented Jan 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

clalancette commented Jan 17, 2019

Uh oh!

dirk-thomas commented Jan 18, 2019

Uh oh!

clalancette commented Jan 18, 2019

Uh oh!

sloretz Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

clalancette Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

clalancette Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

sloretz Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Jan 24, 2019

Uh oh!

clalancette commented Jan 25, 2019

Uh oh!

dirk-thomas commented Jan 25, 2019

Uh oh!

clalancette commented Jan 25, 2019

Uh oh!

dirk-thomas commented Jan 25, 2019

Uh oh!

clalancette commented Jan 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants