Parameters support ranges #72

wjwwood · 2019-04-19T02:15:50Z

Based on the recommendation from @dirk-thomas here: #71 (comment)

So that we can iterate on it more effectively. See also the discussion in that issue.

This is currently targeted at the read_only_parameters branch because that one adds the read_only constraint already, but I will re-target the pull request to master once that one is merged.

Connects to #71

wjwwood · 2019-04-19T02:20:57Z

Proposed a change to add description strings here: #73

rcl_interfaces/msg/IntegerRange.msg

rcl_interfaces/msg/ParameterDescriptor.msg

rcl_interfaces/msg/FloatingPointRange.msg

rcl_interfaces/msg/IntegerRange.msg

rcl_interfaces/msg/ParameterDescriptor.msg

tfoote · 2019-04-19T23:27:25Z

In #71 we talked about potentially supporting minimum and maximum constraints separately which I don't see supported here. @jacobperron suggested using arrays to make one end optional. Or we could encode them as separate constraints.

The other thing that we aren't supporting here is open intervals. I'm not sure that we need to but wanted to make sure it was a conscious decision.

wjwwood · 2019-04-20T00:14:51Z

In #71 we talked about potentially supporting minimum and maximum constraints separately which I don't see supported here.

And over there I suggested you make a pr to this pr to demonstrate what you mean. I didn't do it because I don't understand how it would work or what its purpose is.

The other thing that we aren't supporting here is open intervals. I'm not sure that we need to but wanted to make sure it was a conscious decision.

What is an "open interval"?

jubeira

I've left a couple of comments / questions; just to add one more opinion in general terms I'm aligned with this draft 👍

In #71 we talked about potentially supporting minimum and maximum constraints separately which I don't see supported here.

Isn't this what @wjwwood is saying in the last paragraph here when talking about numeric constraints? If that's the case, it could be added as another constraint in the ParameterDescriptor.

What is an "open interval"?

I think @tfoote is referring to constraints such as [1.0, 0.1, 5.0) i.e. excluding one of the bounds (please correct me if that's not the case). Considering the step is always discrete one way or another, this can be achieved by tweaking the bounds so as not to include the number in the border.
In other words, if I want a value to be between 1 and 5 with a step of 0.1, I can specify the constraint as [1.0, 0.1, 4.9] without needing any extra functionality.

rcl_interfaces/msg/FloatingPointRange.msg

gonzodepedro · 2019-04-23T15:08:42Z

I think that moving on with from/to is ok.
We can have a step >=0, 0 meaning no step.

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro · 2019-04-23T19:29:31Z

I created this PR #74 with changes based on the discussion on this PR.

* Added changes based on comments on PR #72 Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Minor changes based on comments Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Augmented comments on ParameterDescriptor constraints Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Changed comment on 'step' field of FloatingPointRange. One sentence per line on ParameterDescriptor Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Changed comments on IntegerRange and FloatingPointRange, to_value is valid even if it is not multiple of step Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Addressed multiple comments Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Improved documentation Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Oxford comma FTW! Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

* Added changes based on comments on PR #72 Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Minor changes based on comments Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Augmented comments on ParameterDescriptor constraints Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Changed comment on 'step' field of FloatingPointRange. One sentence per line on ParameterDescriptor Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Changed comments on IntegerRange and FloatingPointRange, to_value is valid even if it is not multiple of step Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Addressed multiple comments Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Improved documentation Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Oxford comma FTW! Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> Signed-off-by: William Woodall <william@osrfoundation.org>

Signed-off-by: William Woodall <william@osrfoundation.org>

* Added changes based on comments on PR #72 Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Minor changes based on comments Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Augmented comments on ParameterDescriptor constraints Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Changed comment on 'step' field of FloatingPointRange. One sentence per line on ParameterDescriptor Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Changed comments on IntegerRange and FloatingPointRange, to_value is valid even if it is not multiple of step Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Addressed multiple comments Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Improved documentation Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Oxford comma FTW! Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> Signed-off-by: William Woodall <william@osrfoundation.org>

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood · 2019-04-25T21:19:00Z

Ok, I targeted this at master and rebased to resolve some conflicts and took it out of draft mode.

I'll let others push this through final review and CI and stuff, if that's ok.

dirk-thomas · 2019-04-25T22:51:58Z

rcl_interfaces/msg/ParameterDescriptor.msg

+# with the available constraints, e.g. "only prime numbers".
+# By convention, this should only be used to clarify constraints which cannot
+# be completely expressed with the parameter constraints below.
+string additional_constraints


I would argue the text can already be part of the description and doesn't have to be separate. E.g. a UI could show the description in a tooltip. I find it rather intuitive if it needs to concatenate two descriptions to provide all information.

The idea is that is only about constraints, and should be short. Where as the description would be longer and have information like what units it is in or how it used in the node.

I feel that it has value as a separate field. Consider that you're just using float range, you may still put in the text description that the parameter should be between 0.0 and 1.0. I don't see this as any different, you might want to speak about some of the constraints in the description, but repeat them more specifically or concisely in this field.

Also, I imagine that the GUI may not show the description all the time, perhaps only on hover, whereas this string would be shown always, maybe just under things like sliders or combo boxes as a footnote.

It would be nice to hear what others think about this, I did ask for feedback on it originally (#73) and no one said anything.

I think there is a semantic difference between the describing a parameter and describing the constraints on a parameter. So having two fields makes sense to me.

On another note, I could see this field being abused (for better or worse) to actually do constraint checking in user-defined callbacks. For example, in the Navigation WG meeting today, there was a discussion about having a mechanism for constraining parameter updates in certain states of a lifecycle node. This string could be used to encode a valid state. I'm not sure if this is something we should be worried about though.

I suppose they could abuse it that way, but they could also just put that into the description :x

That's only helpful, however, if the GUI understands it. So if you have cooperation with the GUI and code, why not just propose a new constraint for this message or just publish another message with your custom constraints...

W.r.t. the constraints being based on the lifecycle state, we should make it so that the ParameterDescriptor can be updated (currently you have to undeclare and re-declare a parameter to do that), so that they can simply update the constraints when the node itself changes states.

ROS 1 has range limits as well as a description for dynamic parameters. That has worked well in the past without a second text field describing the constraint in text form. Without a strong reason I don't think we should add a second description just based on the thoughts about how it could useful.

First get a minimal version merged and implemented from APIs in C++ and Python, do constraint checking, and have an rqt plugin to interact with these parameters and then consider extending it if there is a strong need / use case to do so.

I think the idea of how it should be used is sufficiently concrete to include it now, so I think we should keep it.

To be clear here, I'm fine with removing it, if the consensus is to do that, but my personal opinion is that it should stay. Right now we've got a -1 from @dirk-thomas, +1 from me, and I'll call it a +0 from @jacobperron?

If anyone has a +/-1 opinion could comment here so we can have some more decisive conclusion would be nice.

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

gonzodepedro · 2019-04-26T16:06:35Z

I pushed a change that makes the double value in FloatingPointRange a float64 value, same type as used in ParameterValue.msg
I also added the new .msg files to the CMakeList

gonzodepedro · 2019-04-26T20:09:23Z

CI

Linux
Linux-aarch64
macOS
Windows

rcl_interfaces/CMakeLists.txt

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

jacobperron

LGTM! Would be good to get second approval though.

wjwwood

I'd like to see a resolution on the conversation about additional_constraints, but otherwise lgtm. (cannot approve my own pr)

wjwwood added the in progress Actively being worked on (Kanban column) label Apr 19, 2019

wjwwood mentioned this pull request Apr 19, 2019

add lower / upper boundary to parameter descriptor #71

Closed

wjwwood commented Apr 19, 2019

View reviewed changes

dirk-thomas reviewed Apr 19, 2019

View reviewed changes

jubeira reviewed Apr 22, 2019

View reviewed changes

rcl_interfaces/msg/FloatingPointRange.msg Outdated Show resolved Hide resolved

gonzodepedro pushed a commit to gonzodepedro/rcl_interfaces that referenced this pull request Apr 23, 2019

Added changes based on comments on PR ros2#72

b0892b0

gonzodepedro pushed a commit to gonzodepedro/rcl_interfaces that referenced this pull request Apr 23, 2019

Added changes based on comments on PR ros2#72

ce23afa

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

wjwwood force-pushed the parameters_support_ranges branch from e61fc50 to 0bdddd8 Compare April 25, 2019 21:09

wjwwood marked this pull request as ready for review April 25, 2019 21:11

wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 25, 2019

wjwwood changed the base branch from read_only_parameters to master April 25, 2019 21:13

wjwwood and others added 4 commits April 25, 2019 14:15

small cleanup commit

0fabc67

Signed-off-by: William Woodall <william@osrfoundation.org>

first proposal from Dirk

146de25

Signed-off-by: William Woodall <william@osrfoundation.org>

add description fields in the parameter descriptor (#73)

99a9f67

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood force-pushed the parameters_support_ranges branch from 0bdddd8 to ecb74a7 Compare April 25, 2019 21:15

remove draft comment

49107b8

Signed-off-by: William Woodall <william@osrfoundation.org>

dirk-thomas reviewed Apr 25, 2019

View reviewed changes

Gonzalo de Pedro added 2 commits April 26, 2019 12:45

Added new messages file to cmakelists

4b0f26d

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

Changed double to float64 to be consistent with parameter values

357106f

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

jacobperron reviewed Apr 26, 2019

View reviewed changes

rcl_interfaces/CMakeLists.txt Outdated Show resolved Hide resolved

Alphabetized msg file list on CMakeList

0265e44

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

jacobperron approved these changes Apr 26, 2019

View reviewed changes

wjwwood commented Apr 26, 2019

View reviewed changes

tfoote approved these changes Apr 27, 2019

View reviewed changes

wjwwood merged commit cd89d73 into master Apr 29, 2019

wjwwood deleted the parameters_support_ranges branch April 29, 2019 21:04

wjwwood removed the in review Waiting for review (Kanban column) label Apr 29, 2019

dirk-thomas mentioned this pull request May 16, 2019

ROS 2 Dashing Diademata ros2/ros2#607

Closed

20 tasks

jubeira mentioned this pull request May 17, 2019

Range support for parameters ros2/rclpy#350

Closed

ivanpauno mentioned this pull request May 21, 2019

Range support for parameters ros2/rclcpp#728

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameters support ranges #72

Parameters support ranges #72

wjwwood commented Apr 19, 2019

wjwwood commented Apr 19, 2019

tfoote commented Apr 19, 2019

wjwwood commented Apr 20, 2019

jubeira left a comment

gonzodepedro commented Apr 23, 2019

gonzodepedro commented Apr 23, 2019

wjwwood commented Apr 25, 2019

dirk-thomas Apr 25, 2019 •

edited

wjwwood Apr 25, 2019

jacobperron Apr 26, 2019

wjwwood Apr 26, 2019

dirk-thomas Apr 26, 2019

wjwwood Apr 26, 2019

wjwwood Apr 26, 2019

jacobperron Apr 26, 2019

gonzodepedro commented Apr 26, 2019

gonzodepedro commented Apr 26, 2019

jacobperron left a comment

wjwwood left a comment

Parameters support ranges #72

Parameters support ranges #72

Conversation

wjwwood commented Apr 19, 2019

wjwwood commented Apr 19, 2019

tfoote commented Apr 19, 2019

wjwwood commented Apr 20, 2019

jubeira left a comment

Choose a reason for hiding this comment

gonzodepedro commented Apr 23, 2019

gonzodepedro commented Apr 23, 2019

wjwwood commented Apr 25, 2019

dirk-thomas Apr 25, 2019 • edited

Choose a reason for hiding this comment

wjwwood Apr 25, 2019

Choose a reason for hiding this comment

jacobperron Apr 26, 2019

Choose a reason for hiding this comment

wjwwood Apr 26, 2019

Choose a reason for hiding this comment

dirk-thomas Apr 26, 2019

Choose a reason for hiding this comment

wjwwood Apr 26, 2019

Choose a reason for hiding this comment

wjwwood Apr 26, 2019

Choose a reason for hiding this comment

jacobperron Apr 26, 2019

Choose a reason for hiding this comment

gonzodepedro commented Apr 26, 2019

gonzodepedro commented Apr 26, 2019

jacobperron left a comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

dirk-thomas Apr 25, 2019 •

edited