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.

Sign up for GitHub

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

Jump to bottom

[ROS2] reconfigurable transport scoped parameters for CompressedDepthPublisher #145

Merged
merged 9 commits into from
May 7, 2023
Merged

[ROS2] reconfigurable transport scoped parameters for CompressedDepthPublisher #145

merged 9 commits into from
May 7, 2023

Conversation

bmegli
Copy link
Contributor

@bmegli bmegli commented Apr 27, 2023

#140 fix for CompressedDepthPublisher
#108 like (runtime reconfigurable)

Builds up on top of #110 so if merged that one should also be closed.

Implementation is #143 like.

png_level default

I left default png_level the same as for compressed_image_transport.

Both plugins use png_level and clash on deprecated parameter name as mentioned in #140
and we don't want to consider race between plugin loaders (e.g. which default value wins).

We can set it again to something different once deprecated parameters are removed.

code repetition

There is some code repetition between:

  • compressed_image_transport
  • compressed_depth_image_transport

This should be temporary until deprecated parameters are removed.

Marcel Zeilinger and others added 2 commits April 27, 2023 13:41
@bmegli
Declare depth compression params in a per-topic namespace
020be42
@bmegli
reconfigurable transport scoped parameters for CompressedDepthPublisher
dfdf48b 
- 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 bmegli mentioned this pull request Apr 27, 2023
Closed
ijnek
ijnek requested changes Apr 28, 2023
View reviewed changes
Copy link
Member

@ijnek ijnek left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, apart from minor suggestions, rest looks good.
Happy to merge once suggestions are addressed.

bmegli and others added 4 commits April 30, 2023 16:30
@bmegli
Copy link
Contributor Author

bmegli commented Apr 30, 2023 •
edited
Loading

There is one more thing that your review made me aware of.

As:

  • both compressed and compressedDepth declare <some_path>.<image>.png_level
    • clash on this parameter
  • and we synchronize back deprecated value to transport scoped

Setting deprecated png_level will be reflected both in values for compressed and compressedDepth

But maybe that is expected.

We will get 2x warnings from both compressed and compressedDepth that their values were set from deprecated attribute.

@ijnek
Copy link
Member

ijnek commented Apr 30, 2023 •
edited
Loading

There is one more thing that your review made me aware of.

As:

  • both compressed and compressedDepth declare <some_path>.<image>.png_level

    • clash on this parameter

  • and we synchronize back deprecated value to transport scoped

Setting deprecated png_level will be reflected both in values for compressed and compressedDepth

But maybe that is expected.

We will get 2x warnings from both compressed and compressedDepth that their values were set from deprecated attribute.

The "deprecated" code you have in this PR seems to reflect this current strange behavior correctly, and will allow those that depend on this behavior to gracefully take on the new API).

How about we be explicit about this png_level parameter, in both depth and compressedDepth. We could change:
parameter xxx is deprecated, use transport qualified name aaa.xxx)

to the following if (the name of the topic is "png_level"):
parameter xxx is deprecated and ambiguous, use transport qualified name, either compressed.xxx or compressedDetph.xxx.

In regards to getting 2x warnings, this is not a problem as there are actually two plugins complaining.

@bmegli
Copy link
Contributor Author

bmegli commented May 2, 2023

How about we be explicit about this png_level parameter, in both depth and compressedDepth. We could change: parameter xxx is deprecated, use transport qualified name aaa.xxx)

to the following if (the name of the topic is "png_level"): parameter xxx is deprecated and ambiguous, use transport qualified name, either compressed.xxx or compressedDetph.xxx.

In regards to getting 2x warnings, this is not a problem as there are actually two plugins complaining.

A reasonable compromise is

parameter xxx is deprecated and ambiguous, use transport qualified name <transport>.xxx

Then we will get from plugins

  • parameter png_level is deprecated and ambiguous, use transport qualified name compressed.png_level
  • parameter png_level is deprecated and ambiguous, use transport qualified name compessesDepth.png_level

Without hardcoding any transport names in warning messeges.

@bmegli
adjust deprecated warning with "ambiguous"
1e7bad4 
For:
- compressed_depth_image_transport
- compressed_image_transport
@bmegli
remove excess deprecated value reads for compressed transports
b604913 
For already declared attributes those reads do nothing
@ijnek
Copy link
Member

ijnek commented May 4, 2023

That's a good compromise. Just one final thing, and we're good to go I think

@ijnek ijnek merged commit 5881ce9 into ros-perception:rolling May 7, 2023
@ijnek
Copy link
Member

ijnek commented May 7, 2023

@Mergifyio backport iron

@mergify
Copy link

mergify bot commented May 7, 2023 •
edited
Loading

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 7, 2023
@bmegli @mergify
remove comment block that slipped inside code
87430b1 
Requested in #145

(cherry picked from commit 992cc64)
@mergify mergify bot mentioned this pull request May 7, 2023
Merged
ijnek added a commit that referenced this pull request May 7, 2023
@ijnek
Merge pull request #148 from ros-perception/mergify/bp/iron/pr-145
a930f96 
[ROS2] reconfigurable transport scoped parameters for CompressedDepthPublisher (backport #145)
@ijnek ijnek mentioned this pull request May 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ijnek ijnek ijnek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bmegli @ijnek