Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Publish parameter events #226

Merged
merged 10 commits into from
Aug 24, 2018
34 changes: 31 additions & 3 deletions rclpy/rclpy/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

import weakref

from rcl_interfaces.msg import SetParametersResult
from rcl_interfaces.msg import ParameterEvent, SetParametersResult
from rclpy.callback_groups import MutuallyExclusiveCallbackGroup
from rclpy.client import Client
from rclpy.clock import ROSClock
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

from rclpy.service import Service
from rclpy.subscription import Subscription
from rclpy.time_source import TimeSource
Expand Down Expand Up @@ -100,6 +101,9 @@ def __init__(

self.__executor_weakref = None

self._parameter_event_publisher = self.create_publisher(
ParameterEvent, 'parameter_events', qos_profile=qos_profile_parameter_events)

if start_parameter_services:
self._parameter_service = ParameterService(self)

Expand Down Expand Up @@ -166,8 +170,28 @@ def set_parameters_atomically(self, parameter_list):
result = SetParametersResult(successful=True)

if result.successful:
parameter_event = ParameterEvent()
for param in parameter_list:
self._parameters[param.name] = param
if Parameter.Type.NOT_SET == param.type_:
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.to_rcl_interface_parameter())
# Delete any unset parameters regardless of their previous value.
# We don't currently store NOT_SET parameters so this is an extra precaution.
if param.name in self._parameters:
del self._parameters[param.name]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
if Parameter.Type.NOT_SET == self.get_parameter(param.name).type_:
# Parameter is new. (Parameter had no value and new value is set)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

parameter_event.new_parameters.append(param.to_rcl_interface_parameter())
else:
# Parameter changed. (Parameter had a value and new value is set)
parameter_event.changed_parameters.append(
param.to_rcl_interface_parameter())
self._parameters[param.name] = param
self._parameter_event_publisher.publish(parameter_event)

return result

def _validate_topic_or_service_name(self, topic_or_service_name, *, is_service=False):
Expand Down Expand Up @@ -339,6 +363,10 @@ def destroy_node(self):
if self.handle is None:
return ret

# Drop extra reference to parameter event publisher.
# It will be destroyed with other publishers below.
self._parameter_event_publisher = None

while self.publishers:
pub = self.publishers.pop()
_rclpy.rclpy_destroy_node_entity(pub.publisher_handle, self.handle)
Expand Down
4 changes: 4 additions & 0 deletions rclpy/rclpy/parameter.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

from enum import Enum

from rcl_interfaces.msg import Parameter as RCLParameter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

from rcl_interfaces.msg import ParameterDescriptor, ParameterType, ParameterValue

PARAMETER_SEPARATOR_STRING = '.'
Expand Down Expand Up @@ -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 to_rcl_interface_parameter(self):
return RCLParameter(name=self.name, value=self.get_parameter_value())
2 changes: 1 addition & 1 deletion rclpy/rclpy/parameter_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,6 @@ def _set_parameters_callback(self, request, response):
return response

def _set_parameters_atomically_callback(self, request, response):
response.results = self._node.set_parameters_atomically([
response.result = self._node.set_parameters_atomically([
Parameter.from_rcl_interface_parameter(p) for p in request.parameters])
return response
2 changes: 0 additions & 2 deletions rclpy/rclpy/qos.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,6 @@ class QoSDurabilityPolicy(IntEnum):
'qos_profile_sensor_data')
qos_profile_services_default = _rclpy.rclpy_get_rmw_qos_profile(
'qos_profile_services_default')
# NOTE(mikaelarguedas) the following are defined but not used because
# parameters are not implemented in Python yet
qos_profile_parameters = _rclpy.rclpy_get_rmw_qos_profile(
'qos_profile_parameters')
qos_profile_parameter_events = _rclpy.rclpy_get_rmw_qos_profile(
Expand Down
6 changes: 3 additions & 3 deletions rclpy/test/test_destruction.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,10 @@ def func_destroy_entities():
timer # noqa
assert 1 == len(node.timers)
pub1 = node.create_publisher(Primitives, 'pub1_topic')
assert 1 == len(node.publishers)
assert 2 == len(node.publishers)
pub2 = node.create_publisher(Primitives, 'pub2_topic')
pub2 # noqa
assert 2 == len(node.publishers)
assert 3 == len(node.publishers)
sub1 = node.create_subscription(
Primitives, 'sub1_topic', lambda msg: print('Received %r' % msg))
assert 1 == len(node.subscriptions)
Expand All @@ -120,7 +120,7 @@ def func_destroy_entities():
assert 2 == len(node.subscriptions)

assert node.destroy_publisher(pub1) is True
assert 1 == len(node.publishers)
assert 2 == len(node.publishers)

assert node.destroy_subscription(sub1) is True
assert 1 == len(node.subscriptions)
Expand Down