[ROS2] parameter namespace not scoped with transport used #140

bmegli · 2023-03-30T08:30:13Z

Short Example

Current behavior (ROS2 foxy and humble)

ros2 param list
/camera/camera:
  .color.image_raw.format
  .color.image_raw.jpeg_quality
  .color.image_raw.png_level

Instead of

# this is artificial, what I would expect
ros2 param list
/camera/camera:
  .color.image_raw.compressed.format
  .color.image_raw.compressed.jpeg_quality
  .color.image_raw.compressed.png_level

Rationale

ROS1 documentation

image_transport 4.1.2 parameters section

Conventionally, they take the following form:
Reconfigurable parameters

<base topic>/<transport name>/<parameter name> (type, default: ?)

Transport-specific publisher parameter.

/camera/image/compressed/jpeg_quality (int, default: 80)

An example transport-specific parameter which sets the JPEG quality for "compressed" transport for image topic /camera/image.

Potential clash of parameter names between transport plugins

Currently if there are two transports, say:

compressed
my_transport

And both use say format parameter

There is a clash between transports on parameter namespace.

ros2 param list
/camera/camera:
  .color.image_raw.format # would be used by both plugins

Question

Is there a reason this was not taken into account when migrating from ROS1 to ROS2?

The text was updated successfully, but these errors were encountered:

bmegli · 2023-03-30T08:37:09Z

The fix is relatively easy

getTransportName() used as part of parameter path during parameter declaration

The question is:

if current behavior is design decision for some reason
or inconsistency with documentation is by mistake
and if we can live with the consequences in the long-term
- clashing parameters between plugins causing side-effects

bmegli · 2023-03-30T08:55:48Z

@lukaszmitka, I am asking for your opinion

as author of Declare parameters in topic namespace #58
- that fixed [ROS 2] multiple publishers redeclare the same parameters #57 clash between multiple publishers
and author of Add parameter declarations #52

@mjcarroll, I am asking for your opinion as

author of original migration to ROS2 in Update compressed_image_transport to ros2 #26

@ijnek, I am asking for your opinion as official maintainer and active contributor

lukaszmitka · 2023-03-30T13:05:09Z

I had no intention to declare parameters against documentation. Most probably a mistake.

mjcarroll · 2023-03-31T01:49:23Z

Thanks for doing the leg-work on this. While it has been a little bit, I would agree that this is likely a mistake.

I am in favor of moving to the fully-qualified parameter names. I think we should come up with a way of deprecating the current behavior (perhaps preserving it while emitting warnings that those parameters are ambiguous and will be removed in the next iteration) to make sure that we don't unexpectedly break users relying on this implementation, though.

ijnek · 2023-03-31T01:52:41Z

Thanks for bringing this up! I would be happy to review a PR for this change. I was just about to comment on the deprecation - we should print warnings with useful migration instructions when trying to:

set a deprecated parameter
get a deprecated parameter

In regards to getting the deprecated parameter, I don't think there is a way to notify the node that is trying to access the deprecated parameter, so printing a warning in image_transport_plugins would be the best we can do.

- <image>.<transport>.<param> as future replacement of <image>.<param> - e.g. `image.compressed.format` instead of `image.format` - support both ways for now - emit deprecation warnings on setting through non-transport scoped parameter Similar changes will be needed for: - other transports - subscribers Related to ros-perception#140

bmegli · 2023-04-04T08:43:43Z

[...] I think we should come up with a way of deprecating the current behavior (perhaps preserving it while emitting warnings that those parameters are ambiguous and will be removed in the next iteration) to make sure that we don't unexpectedly break users relying on this implementation, though.

[...] I was just about to comment on the deprecation - we should print warnings with useful migration instructions when trying to:
* set a deprecated parameter

* get a deprecated parameter
In regards to getting the deprecated parameter, I don't think there is a way to notify the node that is trying to access the deprecated parameter, so printing a warning in image_transport_plugins would be the best we can do.

I believe all of above apart from notifications on getting a deprecated parameter may be implemented reasonably with existing ROS2 API.

Pull request in #142, at the beginning only for CompressedPublisher.
I may add rest within this pull if we agree on direction.

As for notification on getting a deprecated parameter - I can't find any reasonable way to do it. Maybe somebody else has idea.

ROS2 /parameter_events works on new, changed, or deleted params (not reads).

ijnek · 2023-04-09T16:34:19Z

Thanks for putting the PR together! I'm in the process of reviewing it and seeing if we can somehow notify the prameter deprecation when calling get_parameter. I assumed this was possible, but I may have been wrong.

I think this is a rather general question, so I've posted it here: https://robotics.stackexchange.com/questions/24634/deprecating-a-ros-2-parameter-name @mjcarroll Do you have ideas on this?

ijnek · 2023-04-14T02:06:27Z

Got a response on that question from Tully. To summarize, we should:

notify when setParameter is called on the deprecated parameter
not notify when getParameter is called on the deprecated parameter (there is no way to)
we should provide good documentation in the CHANGELOG

so I think #142 satisfies 1 and 2, when making the next release, I would ensure 3 happens.

bmegli · 2023-04-16T12:29:01Z

@ijnek, thank you for clarifying that

Also from response

As well the callers can remap relatively quickly when they discover the mismatch

This might be alternative way to change behavior and support old behavior at the same time.

But in my quick tests parameter remapping doesn't seem to work in ROS2 at all (foxy, humble, I haven't tried rolling)

The CLI interface (ros_2 run) would not even accept . in the remapping path but simple parameters remapping (without .) also doesn't seem to work.

Separate issue, not related to image_transport_plugins repository.

As a side note - parameter remapping works in ROS1

- <image>.<transport>.<param> as future replacement of <image>.<param> - e.g. `image.compressed.format` instead of `image.format` - support both ways for now - emit deprecation warnings on setting through non-transport scoped parameter - synchronize deprecated changes to new ones - this is necessary for dynamic reconfigure Similar changes will be needed for: - other transports - subscribers Related to ros-perception#140

ijnek · 2023-04-18T01:58:42Z

I might be wrong about this, but I believe ros2 doesn't have parameter remapping since there is no need for it without the global parameter server that existed in ROS 1 - parameters are specific to each node.

ijnek · 2023-04-18T03:58:45Z

Resolved by #143, and due to be released in ros/rosdistro#36894

- runtime reconfiguration - <image>.<transport>.<param> as future replacement of <image>.<param> - e.g. `image.compressed.mode` instead of `image.mode` - support both ways for now - emit deprecation warnings on setting through non-transport scoped parameter - synchronize deprecated changes to new ones - this is necessary for dynamic reconfigure Similar changes will be needed for: - other transports Related to ros-perception#140

bmegli · 2023-04-20T08:42:35Z

@ijnek - this should stay open until

compressed_depth_image_transport is also reworked
theora_image_transport is also reworked

Also, this should touch both Publisher in Subscriber

For compressed_image_transport

publisher fix was merged in add transport scoped parameters for CompressedPublisher #143
- and next steps were described in this pull
subscriber fix is awaiting in [rolling] reconfigurable transport scoped parameters for CompressedSubscriber #144

I have no way to reopen this issue myself

ijnek · 2023-04-20T22:33:45Z

You're right, I've reopened this ticket. I'll try and review the subscriber fix as soon as possible.

ijnek · 2023-04-26T23:24:06Z

@bmegli Thanks for your work so far on this. We really appreciate it.

Since this is one of the ROS packages in ROS Iron Desktop as defined in REP 2001, I'd want these set of changes propogated across the remaining transport_plugins before the Iron Beta release which is on May 1st. @bmegli would you be able to prepare the changes for the other plugins too? If not, these changes may have to wait until the next distro.

Personally, I want to see these great changes in Iron :)

- runtime reconfiguration - <image>.<transport>.<param> as future replacement of <image>.<param> - e.g. `image.compressedDepth.png_level` instead of `image.png_level` - support both ways for now - emit deprecation warnings on setting through non-transport scoped parameter - synchronize deprecated changes to new ones - this is necessary for dynamic reconfigure - add ROS1 like ranges for parameters (min/max) - some cleanup - remove unnecessary includes, using statements, etc. Default png_level was left the same as for compressed_image_transport - this is OpenCV default (3) - the deprecated paramterer name clashes here with compressed_image_transport - I don't really want to consider if there is a race between plugin loading for default value - side notes: - in ROS2 default was 9 - 9 is not usable for real-time processing on most machines - in ROS1 default is now 1 - 3 is typically usable for real-time processing - this should probably be made even with ROS1 after removing deprecated parameters Related to ros-perception#140 Builds up on ros-perception#110

bmegli · 2023-04-27T14:32:16Z

@ijnek, sure, we will try to squeeze in other plugins before end of the week

CompressedDepthPublisher pull in #145, subscriber doesn't use parameters so nothing to do.

The last one to do is theora

bmegli · 2023-04-27T14:32:32Z

In the long term image_transport specific:

declare_parameter
set_parameter
get_parameter

Should probably land in:

image_transport SubscriberePlugin
image_transport PublisherPlugin

The logic of parameter namespaces is complex and if every plugin does it on its own we are asking for trouble.

Plugins could just operate on parameter names without worrying about the scope where those live.

- runtime reconfiguration for publisher and subscriber - <image>.<transport>.<param> as future replacement of <image>.<param> - e.g. `image.theora.target_bitrate` instead of `image.target_bitrate` - support both ways for now - emit deprecation warnings on setting through non-transport scoped parameter - synchronize deprecated changes to new ones - this is necessary for dynamic reconfigure The publisher is a bit more complex here - in general we reload config on publish - but some code paths (conditional compilation) - would result in resetting context on every config reload - so we flag to reload config only parameter event change - and reload it once on init to mimic ROS1 dynamic reconfigure setup The subscriber is simple as existing code already had necessary checks. Related to ros-perception#140

- runtime reconfiguration for publisher and subscriber - <image>.<transport>.<param> as future replacement of <image>.<param> - e.g. `image.theora.target_bitrate` instead of `image.target_bitrate` - support both ways for now - emit deprecation warnings on setting through non-transport scoped parameter - synchronize deprecated changes to new ones - this is necessary for dynamic reconfigure The publisher is a bit more complex here - in general we reload config on publish - but some code paths (conditional compilation) - would result in resetting context on every config reload - so we flag to reload config only on parameter event change - and reload it once on init to mimic ROS1 dynamic reconfigure setup The subscriber is simple as it already had necessary checks. Related to ros-perception#140

bmegli · 2023-04-28T10:13:43Z

theora_image_transport fixes are in #146

If #145 and #146 are merged this issue can be closed.

In the future

code supporting deprecated parameters should be removed
parameter path handling should be moved to image_transport (post above)
plugins should only work on parameter names without worrying where they live in

ijnek · 2023-04-29T00:02:57Z

In the long term image_transport specific:

declare_parameter

set_parameter

get_parameter

Should probably land in:

image_transport SubscriberePlugin

image_transport PublisherPlugin

The logic of parameter namespaces is complex and if every plugin does it on its own we are asking for trouble.

Plugins could just operate on parameter names without worrying about the scope where those live.

Yes, I agree with your suggestion of having this in the image_transport library. I am not a maintainer of ros-perception/image_common, so you will have to open an issue there to make that happen.

- runtime reconfiguration for publisher and subscriber - <image>.<transport>.<param> as future replacement of <image>.<param> - e.g. `image.theora.target_bitrate` instead of `image.target_bitrate` - support both ways for now - emit deprecation warnings on setting through non-transport scoped parameter - synchronize deprecated changes to new ones - this is necessary for dynamic reconfigure The publisher is a bit more complex here - in general we reload config on publish - but some code paths (conditional compilation) - would result in resetting context on every config reload - so we flag to reload config only on parameter event change - and reload it once on init to mimic ROS1 dynamic reconfigure setup The subscriber is simple as it already had necessary checks. Related to #140 (cherry picked from commit b7d409f) # Conflicts: # theora_image_transport/src/theora_publisher.cpp # theora_image_transport/src/theora_subscriber.cpp

- runtime reconfiguration - <image>.<transport>.<param> as future replacement of <image>.<param> - e.g. `image.compressedDepth.png_level` instead of `image.png_level` - support both ways for now - emit deprecation warnings on setting through non-transport scoped parameter - synchronize deprecated changes to new ones - this is necessary for dynamic reconfigure - add ROS1 like ranges for parameters (min/max) - some cleanup - remove unnecessary includes, using statements, etc. Default png_level was left the same as for compressed_image_transport - this is OpenCV default (3) - the deprecated paramterer name clashes here with compressed_image_transport - I don't really want to consider if there is a race between plugin loading for default value - side notes: - in ROS2 default was 9 - 9 is not usable for real-time processing on most machines - in ROS1 default is now 1 - 3 is typically usable for real-time processing - this should probably be made even with ROS1 after removing deprecated parameters Related to #140 Builds up on #110 (cherry picked from commit dfdf48b)

ijnek · 2023-05-07T14:17:54Z

@bmegli I think all API has been updated. I'm going to make a release into ros 2 iron (and rolling) with these changes tonight if you don't have addittional API/ABI changes.

ijnek · 2023-05-08T00:24:17Z

Releases were made in:

bmegli mentioned this issue Apr 4, 2023

[foxy-devel] transport scoped parameters #142

Closed

bmegli mentioned this issue Apr 17, 2023

add transport scoped parameters for CompressedPublisher #143

Merged

ijnek closed this as completed Apr 18, 2023

bmegli mentioned this issue Apr 18, 2023

[rolling] reconfigurable transport scoped parameters for CompressedSubscriber #144

Merged

ijnek reopened this Apr 20, 2023

bmegli mentioned this issue Apr 27, 2023

[ROS2] reconfigurable transport scoped parameters for CompressedDepthPublisher #145

Merged

bmegli mentioned this issue Apr 28, 2023

[ROS2] reconfigurable transport scoped parameters for theora #146

Merged

bmegli closed this as completed May 12, 2023

bmegli mentioned this issue Aug 24, 2023

How to change png_level parameter by yaml #151

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ROS2] parameter namespace not scoped with transport used #140

[ROS2] parameter namespace not scoped with transport used #140

bmegli commented Mar 30, 2023

bmegli commented Mar 30, 2023

bmegli commented Mar 30, 2023

lukaszmitka commented Mar 30, 2023

mjcarroll commented Mar 31, 2023

ijnek commented Mar 31, 2023

bmegli commented Apr 4, 2023

ijnek commented Apr 9, 2023

ijnek commented Apr 14, 2023

bmegli commented Apr 16, 2023

ijnek commented Apr 18, 2023 •

edited

Loading

ijnek commented Apr 18, 2023

bmegli commented Apr 20, 2023

ijnek commented Apr 20, 2023

ijnek commented Apr 26, 2023

bmegli commented Apr 27, 2023

bmegli commented Apr 27, 2023

bmegli commented Apr 28, 2023

ijnek commented Apr 29, 2023 •

edited

Loading

ijnek commented May 7, 2023 •

edited

Loading

ijnek commented May 8, 2023 •

edited

Loading

[ROS2] parameter namespace not scoped with transport used #140

[ROS2] parameter namespace not scoped with transport used #140

Comments

bmegli commented Mar 30, 2023

Short Example

Rationale

ROS1 documentation

Potential clash of parameter names between transport plugins

Question

bmegli commented Mar 30, 2023

bmegli commented Mar 30, 2023

lukaszmitka commented Mar 30, 2023

mjcarroll commented Mar 31, 2023

ijnek commented Mar 31, 2023

bmegli commented Apr 4, 2023

ijnek commented Apr 9, 2023

ijnek commented Apr 14, 2023

bmegli commented Apr 16, 2023

ijnek commented Apr 18, 2023 • edited Loading

ijnek commented Apr 18, 2023

bmegli commented Apr 20, 2023

ijnek commented Apr 20, 2023

ijnek commented Apr 26, 2023

bmegli commented Apr 27, 2023

bmegli commented Apr 27, 2023

bmegli commented Apr 28, 2023

ijnek commented Apr 29, 2023 • edited Loading

ijnek commented May 7, 2023 • edited Loading

ijnek commented May 8, 2023 • edited Loading

ijnek commented Apr 18, 2023 •

edited

Loading

ijnek commented Apr 29, 2023 •

edited

Loading

ijnek commented May 7, 2023 •

edited

Loading

ijnek commented May 8, 2023 •

edited

Loading