Split ParameterVariant into Parameter and ParameterValue by sloretz · Pull Request #481 · ros2/rclcpp

sloretz · 2018-05-29T22:19:33Z

In progress while CI runs

This PR looks large, but most of the changed lines in this PR are due to renaming ParameterVariant to Parameter. I can split it into smaller PRs if need be.

Changes

Renamed ParameterVariant to Parameter
Removed rclcpp::parameter namespace (as was done for other namespaces in Remove e.g. node:: namespaces and namespace escalation #416)
Moved functions related to getting a parameter value into new ParameterValue class.
Raise ParameterTypeException instead of std::runtime_error when there is a type mismatch
Rename Parameter::get_parameter_value() to Parameter::get_value_message()
Added const to Parameter::to_parameter()

Test expects ParameterTypeException

sloretz · 2018-05-29T22:22:43Z

CI (6c441ca)

Linux
Linux-aarch64
macOS
- Flaky test? projectroot.test_requester_replier__Primitives__rclpy__rmw_fastrtps_cpp
Windows
- New CI with e182a0b
  - New CI with 895bd4c
    - New CI with f002629

sloretz · 2018-05-30T15:29:53Z

CI (f002629)

Linux
Linux-aarch64
macOS
Windows

dhood · 2018-05-30T17:18:16Z

If there was context around this change on another PR, could you link to it please?

sloretz · 2018-05-30T17:37:25Z

@dhood context in #477. Reason behind splitting ParameterVariant is to add

NodeParameters::create_parameter(std::string name, ParameterType type, ParameterValue default_value = ParameterValue())

There are other options with some drawbacks

create_parameter(std::string name, ParameterType type, ParameterVariant default_value = ParameterVariant()).
- Need to check consistency between name in ParameterVariant and name.
create_parameter(ParameterVariant param, ParameterType type)
- Currently no ParameterVariant constructor allowing param to both have a name and be initially unset.
create_parameter(std::string name, ParameterType type, rcl_interfaces::msg::ParameterValue default_value)
- Loses convenience of using constructors on ParameterVariant from c++ types

Admittedly these drawbacks are all pretty minor. Subjectively splitting the classes feels nicer to me because it matches rcl_interfaces::msg::Parameter and rcl_interfaces::msg::ParameterValue, as well as ParameterValue matching rcl_variant_t added in ros2/rcl#235

sloretz · 2018-05-31T21:21:03Z

rclcpp/include/rclcpp/parameter_value.hpp

+};
+
+/// Indicate the parameter type does not match the expected type.
+class ParameterTypeException : public std::exception


exception type should be moved to in rclcpp/exceptions.hpp

I support this. Though I've gone back and forth on putting the exceptions all in one place versus in files where they might be used. You still need something like rclcpp/exceptions.hpp for exceptions which are used all over the place.

After discussing it offline a289f63 makes ParameterTypeException inherit from std::runtime_error but continue to live in this file so it's constructor can continue to use ParameterType.

wjwwood

lgtm, other than some style comments. No blockers.

wjwwood · 2018-05-31T22:39:48Z

rclcpp/include/rclcpp/parameter.hpp

-  typename std::enable_if<std::is_convertible<type, std::string>::value, const std::string &>::type
+  /// Get value of parameter using rclcpp::ParameterType as template argument.
+  template<ParameterType ParamT>
+  decltype(ParameterValue().get<ParamT>())


This is a bit clunky, is it possible to use auto for the return type? or to improve it with something in the ParameterValue class? So that you could do something like ParameterValue::type<ParamT>::type?

Would just decltype(ParameterValue::get<ParamT>) work?

Or decltype(auto) (http://en.cppreference.com/w/cpp/language/auto)?

I would try one of the alternatives, but I don't have these branches setup locally.

The rules for auto don't seem to allow for only some of the functions return references. decltype(auto) works! f8646d0

wjwwood · 2018-05-31T22:44:30Z

rclcpp/include/rclcpp/parameter_value.hpp

+public:
+  /// Construct an instance.
+  /// \param[in] expected the expected parameter type.
+  /// \param[in] actual the actual parameter type.


nitpick: the rest of the code uses:

/// Brief on one line. /** * absolutely anything else... */ code...

You might want to use this style in the future for consistency.

Fixed in bea1335

wjwwood · 2018-05-31T22:48:00Z

rclcpp/include/rclcpp/parameter_value.hpp

+};
+
+/// Indicate the parameter type does not match the expected type.
+class ParameterTypeException : public std::exception


I support this. Though I've gone back and forth on putting the exceptions all in one place versus in files where they might be used. You still need something like rclcpp/exceptions.hpp for exceptions which are used all over the place.

dhood

I didn't get to finish a thorough review but these are some comments from what I got to

dhood · 2018-05-30T17:28:21Z

rclcpp/include/rclcpp/rclcpp.hpp

 *   - rclcpp::Node::list_parameters()
 *   - rclcpp::Node::register_param_change_callback()
- *   - rclcpp::parameter::ParameterVariant
+ *   - rclcpp::Parameter


could you add parameter_value.hpp below too

Added in 18555dc

dhood · 2018-05-30T17:38:14Z

rclcpp/include/rclcpp/parameter.hpp

  RCLCPP_PUBLIC
  rcl_interfaces::msg::Parameter
-  to_parameter();
+  to_parameter() const;


IMO the name of this function ought to be renamed now. Parameter.to_parameter is confusing to me. Maybe to_rcl_parameter and from_rcl_parameter above?

I'm wondering if xxx_rcl_parameter isn't specific enough now that ros2/rcl#235 added rcl_params_t. What do you think of xxx_parameter_message()?

yeah, or xx_parameter_msg (might be more natural when writing a line like this) but I don't have a strong preference either way: whatever fits better with our existing naming scheme

to_parameter_msg() and from_parameter_msg() in
22d89b9

Also changed ParameterValue::get_message() to to_value_msg() to match.

to_value_msg()

sloretz · 2018-06-01T00:07:11Z

All comments addressed. Rerunning CI

Linux
- Unrelated Flake 8 failures
Linux-aarch64
- Unrelated Flake 8 failures
macOS
- Unrelated Flake 8 failures
Windows
- Unrelated Flake 8 failures

sloretz · 2018-06-01T17:31:08Z

CI looks good. @wjwwood Would you mind checking if the changes since your review look ok? https://github.com/ros2/rclcpp/pull/481/files/f0026290fe8cdddc8aa5e3966c13a139f401c51f..22d89b9ba05bbfb38b389fb120ee31e76034f65b

mikaelarguedas · 2018-06-04T07:42:57Z

@sloretz FYI: follow up PR at ros2/navigation#28

sloretz added 6 commits May 25, 2018 16:05

Moved ParameterType + started ParameterValue

9e9f37c

Delegate get_value() to ParameterValue

ee1230a

Test expects ParameterTypeException

Remove as_*() from ParameterValue

fa8f19e

Remove unused includes

6d51a66

get_parameter_value() -> get_value_message()

a79730e

rclcpp::parameter::ParameterVariant -> rclcpp::Parameter

6c441ca

sloretz added the in progress Actively being worked on (Kanban column) label May 29, 2018

sloretz self-assigned this May 29, 2018

This was referenced May 29, 2018

Split ParameterVariant ros2/system_tests#271

Merged

Split ParameterVariant ros2/demos#237

Merged

Add missing rclcpp::

e182a0b

sloretz mentioned this pull request May 29, 2018

[WIP] Initialize parameters #477

Closed

10 tasks

sloretz added 4 commits May 29, 2018 16:23

array_to_string exists on ParameterValue

798880b

Make to_parameter() const

69f3421

Remove RCLCPP_PUBLIC before definition

895bd4c

Add RCLCPP_PUBLIC to methods

f002629

sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 30, 2018

sloretz mentioned this pull request May 31, 2018

Convert rcl_params_t to ParameterMap #485

Merged

sloretz commented May 31, 2018

View reviewed changes

sloretz mentioned this pull request May 31, 2018

Pass initial parameter values to node constructor #486

Merged

sloretz added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 31, 2018

wjwwood approved these changes May 31, 2018

View reviewed changes

dhood reviewed May 31, 2018

View reviewed changes

ParameterTypeException inherit from runtime_error

a289f63

sloretz added 5 commits May 31, 2018 16:17

decltype(auto) return type

f8646d0

consistent doc block

bea1335

Minor comment fixes

374b81a

Add ParameterValue to main API comment

18555dc

to_parameter_msg() from_parameter_msg()

22d89b9

to_value_msg()

sloretz force-pushed the split_parametervariant branch from 4c071ae to 22d89b9 Compare June 1, 2018 00:02

sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 1, 2018

wjwwood approved these changes Jun 1, 2018

View reviewed changes

sloretz merged commit d298fa4 into master Jun 1, 2018

sloretz removed the in review Waiting for review (Kanban column) label Jun 1, 2018

sloretz deleted the split_parametervariant branch June 1, 2018 20:26

mikaelarguedas mentioned this pull request Jun 4, 2018

parameter::ParameterVariant -> Parameter ros2/navigation#28

Merged

sloretz mentioned this pull request Jul 5, 2018

Parameter doesn't allow setting of name or value #238

Open

Conversation

sloretz commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented May 30, 2018

Uh oh!

dhood commented May 30, 2018

Uh oh!

sloretz commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhood May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented Jun 1, 2018

Uh oh!

mikaelarguedas commented Jun 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sloretz commented May 29, 2018 •

edited

Loading

sloretz commented May 29, 2018 •

edited

Loading

sloretz May 31, 2018 •

edited

Loading

dhood May 31, 2018 •

edited

Loading

sloretz commented Jun 1, 2018 •

edited

Loading