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

[foxy-devel] transport scoped parameters #142

Conversation

bmegli
Copy link
Contributor

@bmegli bmegli commented Apr 4, 2023

Aim

Reason

  • following ROS1 image_transport documentation in ROS2 way
  • avoiding clash between parameters from image_transport plugins

Details in #140

Scope

  • only CompressedPublisher

If agreed on implementation I may rework the rest within this pull.

Short Description

  • <image>.<transport>.<param> as future replacement of <image>.<param>
  • support both ways for now
  • emit deprecation warnings on setting through non-transport scoped parameter

Example

ros2 launch realsense2_camera rs_launch.py

# ...
realsense2_camera_node-1] [WARN] [1680595074.360039582] [CompressedPublisher]: parameter `.depth.image_rect_raw.format` is deprecated; use transport qualified name `.depth.image_rect_raw.compressed.format`
[realsense2_camera_node-1] [WARN] [1680595074.360153760] [CompressedPublisher]: parameter `.depth.image_rect_raw.png_level` is deprecated; use transport qualified name `.depth.image_rect_raw.compressed.png_level`
[realsense2_camera_node-1] [WARN] [1680595074.360203519] [CompressedPublisher]: parameter `.depth.image_rect_raw.jpeg_quality` is deprecated; use transport qualified name `.depth.image_rect_raw.compressed.jpeg_quality`
# ...
[realsense2_camera_node-1] [WARN] [1680595074.412855043] [CompressedPublisher]: parameter `.color.image_raw.format` is deprecated; use transport qualified name `.color.image_raw.compressed.format`
[realsense2_camera_node-1] [WARN] [1680595074.412951851] [CompressedPublisher]: parameter `.color.image_raw.png_level` is deprecated; use transport qualified name `.color.image_raw.compressed.png_level`
[realsense2_camera_node-1] [WARN] [1680595074.413044543] [CompressedPublisher]: parameter `.color.image_raw.jpeg_quality` is deprecated; use transport qualified name `.color.image_raw.compressed.jpeg_quality`
ros2 param list
/camera/camera:
  .color.image_raw.compressed.format
  .color.image_raw.compressed.jpeg_quality
  .color.image_raw.compressed.png_level
  .color.image_raw.format
  .color.image_raw.jpeg_quality
  .color.image_raw.png_level
  .depth.image_rect_raw.compressed.format
  .depth.image_rect_raw.compressed.jpeg_quality
  .depth.image_rect_raw.compressed.png_level
  .depth.image_rect_raw.format
  .depth.image_rect_raw.jpeg_quality
  .depth.image_rect_raw.png_level
# ...

Implementation

  • parameter event handler emitting deprecation warnings on setting non-scoped parameters
  • declare and sync value
    • transport scoped parameter
    • non-scoped parameter

- <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
Copy link
Contributor Author

bmegli commented Apr 4, 2023

Now, if we intended only to make parameters transport scoped the fix is trivial 3 liner.

The difficulty lies in having both types support, ensuring values are synced on init and emitting deprecation warnings.

@bmegli
Copy link
Contributor Author

bmegli commented Apr 4, 2023

There is alternative way to implement through add_on_set_parameters_callback but:

  • it will flood the user with deprecation warnings
  • as those events are generated multiple times with combinations of parameters

The ParameterEvent handler way may be also used to reimplement ROS1 dynamic reconfigure behavior in ROS2


Edit - stroked through, a much simpler way for runtime reconfiguration was implemented in rolling

@bmegli
Copy link
Contributor Author

bmegli commented Apr 4, 2023

And finally, I am not entirely happy with templated function to set parameters in a header.

I now see a way to avoid it with:

@ijnek
Copy link
Member

ijnek commented Apr 16, 2023

@bmegli I see this PR is targetting the foxy-devel branch. Could you retarget it to the rolling branch?

@bmegli
Copy link
Contributor Author

bmegli commented Apr 17, 2023

Re-targeted to rolling in #143

Closing this one

@bmegli bmegli closed this Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants