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

[ROS2] reconfigurable transport scoped parameters for theora #146

Merged

Conversation

bmegli
Copy link
Contributor

@bmegli bmegli commented Apr 28, 2023

#140 fix for theora_image_transport (publisher and subscriber)
#108 like (runtime reconfigurable)

Implementation is #143 like + minor deviation for publisher.

The original logic behind dynamic reconfigure callbacks was kept.

publisher notes

The publisher is more complex here

  • in general we reload config on publish
  • but some code paths (conditional compilation)
    • would result in resetting theora context on every config reload
      • so we flag to reload config only on parameter event change
        • and reload it also once on init to mimic ROS1 dynamic reconfigure setup

This is different from all other transport scoped changes so far where we could make transport reconfigurable without depending on parameter event monitoring (in the spirit of #108).

The difference is only in setting/resetting/checking flag but event handling code exists only for the purpose of deprecated parameters in other transports.

code repetition

There is some code repetition between:

  • compressed_image_transport
  • compressed_depth_image_transport
  • theora_image_transport

This should be temporary until deprecated parameters are removed.

The way to proceed was sketched in:

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

Apart from the suggestions left, things look great!
Good to see the parameters added back in and not left as a TODO.

- example parameter paths (comments) are now theora specific

Requested in ros-perception#146
@bmegli
Copy link
Contributor Author

bmegli commented Apr 30, 2023

Should be fixed now.

The only deviation was in subscriber which has parameter post_processing_level, the comments where based on it.

@ijnek ijnek merged commit 29e29e3 into ros-perception:rolling May 2, 2023
1 check passed
@ijnek
Copy link
Member

ijnek commented May 2, 2023

@Mergifyio backport iron

@mergify
Copy link

mergify bot commented May 2, 2023

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 2, 2023
- example parameter paths (comments) are now theora specific

Requested in #146

(cherry picked from commit ecf8f7e)

# Conflicts:
#	theora_image_transport/src/theora_publisher.cpp
#	theora_image_transport/src/theora_subscriber.cpp
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.

None yet

2 participants