search

ros-perception / image_transport_plugins

A set of plugins for publishing and subscribing to sensor_msgs/Image topics in representations other than raw pixel data.
BSD 3-Clause "New" or "Revised" License
55 stars 120 forks source link

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

Closed bmegli closed 1 year ago

bmegli commented 1 year ago

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:

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?

bmegli commented 1 year ago

The fix is relatively easy

The question is:

bmegli commented 1 year ago

@lukaszmitka, I am asking for your opinion

@mjcarroll, I am asking for your opinion as

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

lukaszmitka commented 1 year ago

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

mjcarroll commented 1 year ago

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 commented 1 year ago

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:

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.

bmegli commented 1 year ago

[...] 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 commented 1 year ago

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 commented 1 year ago

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

  1. notify when setParameter is called on the deprecated parameter
  2. not notify when getParameter is called on the deprecated parameter (there is no way to)
  3. 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 commented 1 year ago

@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

ijnek commented 1 year ago

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 commented 1 year ago

Resolved by #143, and due to be released in https://github.com/ros/rosdistro/pull/36894

bmegli commented 1 year ago

@ijnek - this should stay open until

Also, this should touch both Publisher in Subscriber

For compressed_image_transport

I have no way to reopen this issue myself

ijnek commented 1 year ago

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

ijnek commented 1 year ago

@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 :)

bmegli commented 1 year ago

@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 commented 1 year ago

In the long term image_transport specific:

Should probably land in:

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.

bmegli commented 1 year ago

theora_image_transport fixes are in #146

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

In the future

ijnek commented 1 year ago

In the long term image_transport specific:

  • declare_parameter
  • set_parameter
  • get_parameter

Should probably land in:

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.

ijnek commented 1 year ago

@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 commented 1 year ago

Releases were made in: