Publish parameter events #226

nuclearsandwich · 2018-08-17T00:40:57Z

This is a recreation of a change that was accidentally authored in a temporary container 😭.

Adds a parameter events publisher to rclpy nodes and publishes events when parameters are successfully set atomically.

I did some basic testing of the earlier version of this using ros2 topic echo and comparing the results with rclcpp nodes but I need to do a bit more before I feel good putting this in review.

Connects to #202

Adds a parameter event publisher to rclpy nodes.

Because every node has a parameter events publisher bump the number of expected publishers in a couple of cases.

tfoote

LGTM, maybe clear the TODO though

tfoote · 2018-08-17T20:41:24Z

rclpy/rclpy/node.py

@@ -339,6 +358,11 @@ def destroy_node(self):
        if self.handle is None:
            return ret

+        # TODO: is this needed/desired?


Explicit cleanup can't hurt.

In tfoote's words explicit cleanup can't hurt.

nuclearsandwich · 2018-08-17T22:23:56Z

For reviewers: I tested this pull request by echoing the parameter events topic and running this script https://gist.github.com/nuclearsandwich/944595aad28cb1323c909be45f4d4259 while either the python or cpp talker node was running and comparing the results.

nuclearsandwich · 2018-08-17T23:40:05Z

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas

Code looks good. I dropped a couple questions below about naming and corner cases

mikaelarguedas · 2018-08-17T16:11:01Z

rclpy/rclpy/parameter.py

@@ -132,3 +133,6 @@ def get_parameter_value(self):
        elif Parameter.Type.STRING_ARRAY == self.type_:
            parameter_value.string_array_value = self.value
        return parameter_value
+
+    def get_rcl_parameter(self):


Does rcl parameter means "a parameter message"?
As we don't have the notion of "rcl parameter" but have a structure "rcl_params_t" maybe we should rename this to get_parameter_msg() to avoid confusion.
Or maybe something more symmetric to "from_rcl_interface_parameter" like "to_rcl_interface_parameter" maybe?

yeah the rcl prefix was to distinguish it from the current Parameter class. I'm happy to change it if another method name will be clearer.

I think to_rcl_interface_parameter to match the from_rcl_interface_parameter method from the same class would be clearer.

mikaelarguedas · 2018-08-17T22:51:41Z

rclpy/rclpy/node.py

@@ -28,7 +28,8 @@
 from rclpy.parameter import Parameter
 from rclpy.parameter_service import ParameterService
 from rclpy.publisher import Publisher
-from rclpy.qos import qos_profile_default, qos_profile_services_default
+from rclpy.qos import qos_profile_default, qos_profile_parameter_events
+from rclpy.qos import qos_profile_services_default


please remove this comment that is now obsolete:

rclpy/rclpy/rclpy/qos.py

Lines 177 to 178 in 5b74d24

# NOTE(mikaelarguedas) the following are defined but not used because

# parameters are not implemented in Python yet

mikaelarguedas · 2018-08-17T23:35:48Z

rclpy/rclpy/node.py

+                        del self._parameters[param.name]
+                else:
+                    if Parameter.Type.NOT_SET == self.get_parameter(param.name).type_:
+                        #  Parameter is new. (Parameter had no value and new value is set)


(Parameter had no value and new value is set)

This is not a request for changes just a remark:
This is more: "Parameter existed with value not set or didn't exist on the node" (as the API doesn't allow us to distinguish ATM)
Pointing it out as it may be useful to distinguish these cases in some contexts.

For this behavior I mirrored the rclcpp API which drops deleted parameters from the internal data structure on deletion as well. When getting parameters via ROS service NOT_SET is the type assigned to parameters with no current value, whether they have ever been assigned a value or not.

While it might be useful to treat an explicitly NOT_SET parameter different from a parameter absent from internal storage it isn't a state that is representable via the current interfaces and I don't think we should make it part of the local API without amending the interface definitions to convey both different potential meanings of NOT_SET.

I guess it's just unclear to me how setting "NOT_SET" is handled in both languages.
As you say setting a parameter to "NOT_SET" and value "None" has no effect on the node (no storage allocated and not added to the list of parameters) I think it's fine to merge as is (same below).

We can revisit when we go further on the "parameter declaration" and "read-only" parameters where the distinction between "a parameter that exist but doesnt have a value yet" and a "parameter that doesnt exist", will be needed.

Setting NOT_SET in rclcpp will erase the parameter from internal storage if it was previously allocated: https://github.com/ros2/rclcpp/blob/25a9b4e339867bacb8ba5b92b955420cec86596a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp#L216-L221

then I guess we should do the same here for consistency? aka unconditionally delete it if it exists regardless of its type/value, but add it to the deleted_parameters list only if it had a value

mikaelarguedas · 2018-08-17T23:40:49Z

rclpy/rclpy/node.py

+                    if Parameter.Type.NOT_SET != self.get_parameter(param.name).type_:
+                        # Parameter deleted. (Parameter had value and new value is not set)
+                        parameter_event.deleted_parameters.append(param.get_rcl_parameter())
+                        del self._parameters[param.name]


What happens in the else case?

(I didnt try it but by reading the code I wonder)

As a user does that mean that if I:

run list parameters: see parameter foo

send a request to delete foo and receive success

I run list_parameters again:

if the parameter was of any type but "NOT_SET": it has been deleted and is not part of the list

if the parameter was of type "NOT_SET": it will still be in the list

to actually delete it I should:

set a value

request a deletion

Is that correct?

That's correct but the final subcase "if the parameter was of type "NOT_SET": it will still be in the list" is somewhat vacuous as in order for a NOT_SET parameter to get in the list it would need to be added directly to the parameters map circumventing our API.

What does vacuous mean?
Do you mean that scenario should not happen and should be handled as such (warning, error, something else than just ignoring it?) ?

What does vacuous mean?

If they use the node parameters API, there won't be a Parameter of type NOT_SET stored and thus it wouldn't be enumerated in a list_parameters service call.

Do you mean that scenario should not happen and should be handled as such (warning, error, something else than just ignoring it?) ?

The scenario shouldn't happen but I also don't know that it needs to be explicitly warned or errored any more than any other behavior that would arise from manual manipulation of Node class internals. If you're concerned about seeing NOT_SET parameters in the list_parameters results I think an explicit check in that service handler would be the way to achieve it.

All that said, if there's an IllegalStateException or similar in python that we want to raise here I'm not opposed.

wjwwood

lgtm

wjwwood · 2018-08-23T22:16:55Z

rclpy/rclpy/parameter.py

@@ -14,6 +14,7 @@

 from enum import Enum

+from rcl_interfaces.msg import Parameter as RCLParameter


This is a strange rename to me, at first glance, since it's clearly a message a not a type defined within rcl. I see that it comes from rcl_interfaces which make some sense, but it was surprising to me. ParameterMsg is a clearer name to me, since it will always be a message but my not always be in the rcl interfaces or have anything to do with rcl in the future.

This is a pure nitpick, feel free to ignore it.

👍 I meant to scope that with the comment with about renaming the method and apparently didn't post it. I'm also in favor of a name expressing the fact that this is a message and not a structure coming from rcl.

Following that logic, the method converting to and from parameter message could also be named to refer to messages rather than rcl_interfaces

This is a strange rename to me, at first glance

I agree with both of you given the explanation and have pushed 6ddfbba to use ParameterMsg and to/from_parameter_msg. As Mikael suspected the rcl prefix was being used because it was an rcl_interfaces type but using just rcl adds confusing with rcl types. ParameterMsg is apt and available so it's what we'll use.

Changes the name assigned to the imported type (which cannot be simply `Parameter` since it will conflict with rcply.parameter.Parameter) so it makes more sense to the team. Per their feedback the name of RCLParameter would imply a type supplied by rcl rather than one from rcl_interfaces.

mikaelarguedas

👍,
thanks for the multiple iterations on this!

nuclearsandwich added 2 commits August 16, 2018 17:36

Publish parameter events.

0182452

Adds a parameter event publisher to rclpy nodes.

Increase base number of publishers for testing.

ec65473

Because every node has a parameter events publisher bump the number of expected publishers in a couple of cases.

nuclearsandwich added the in progress Actively being worked on (Kanban column) label Aug 17, 2018

nuclearsandwich self-assigned this Aug 17, 2018

nuclearsandwich requested a review from mikaelarguedas August 17, 2018 00:40

dirk-thomas mentioned this pull request Aug 17, 2018

Support for parameters #202

Closed

6 tasks

tfoote approved these changes Aug 17, 2018

View reviewed changes

nuclearsandwich added 4 commits August 17, 2018 15:17

Fix typo.

d451bd6

Compare parameter type not the parameter itself.

2cdfd65

Fix indentation.

6bb5a65

Drop TODO.

8a06a59

In tfoote's words explicit cleanup can't hurt.

nuclearsandwich added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 17, 2018

mikaelarguedas reviewed Aug 17, 2018

View reviewed changes

nuclearsandwich added 3 commits August 21, 2018 09:15

Remove comment now that parameter services are implemented.

ab91681

Rename method for better clarity / symmetry.

3152df1

Delete NOT_SET parameters if present regardless of prior type.

edd9c36

wjwwood approved these changes Aug 23, 2018

View reviewed changes

mikaelarguedas approved these changes Aug 23, 2018

View reviewed changes

nuclearsandwich merged commit 20fe752 into master Aug 24, 2018

nuclearsandwich deleted the parameter-events branch August 24, 2018 17:52

nuclearsandwich removed the in review Waiting for review (Kanban column) label Aug 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Publish parameter events #226

Publish parameter events #226

nuclearsandwich commented Aug 17, 2018

tfoote left a comment

tfoote Aug 17, 2018

nuclearsandwich commented Aug 17, 2018

nuclearsandwich commented Aug 17, 2018

mikaelarguedas left a comment

mikaelarguedas Aug 17, 2018

nuclearsandwich Aug 20, 2018

mikaelarguedas Aug 21, 2018

mikaelarguedas Aug 17, 2018

mikaelarguedas Aug 17, 2018

nuclearsandwich Aug 20, 2018

mikaelarguedas Aug 21, 2018

nuclearsandwich Aug 21, 2018

mikaelarguedas Aug 21, 2018

mikaelarguedas Aug 17, 2018

nuclearsandwich Aug 20, 2018

mikaelarguedas Aug 20, 2018

nuclearsandwich Aug 20, 2018

wjwwood left a comment

wjwwood Aug 23, 2018

mikaelarguedas Aug 23, 2018

nuclearsandwich Aug 23, 2018

mikaelarguedas left a comment

	# NOTE(mikaelarguedas) the following are defined but not used because
	# parameters are not implemented in Python yet

		@@ -14,6 +14,7 @@

		from enum import Enum

		from rcl_interfaces.msg import Parameter as RCLParameter

Publish parameter events #226

Publish parameter events #226

Conversation

nuclearsandwich commented Aug 17, 2018

tfoote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuclearsandwich commented Aug 17, 2018

nuclearsandwich commented Aug 17, 2018

mikaelarguedas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelarguedas left a comment

Choose a reason for hiding this comment