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

add transport scoped parameters for CompressedPublisher #143

Merged

Conversation

bmegli
Copy link
Contributor

@bmegli bmegli commented Apr 17, 2023

Fix for #140

This is #142 re-targeted at rolling

Changes

Compared to #142

  • rolling supports runtime reconfiguration
  • rolling has nested attributes (tiff.xdpi)

runtime reconfiguration

rolling supports runtime reconfiguration (reads params at the beginning of publish)

This forced us to synchronize deprecated values to correct ones. Otherwise deprecated values would not be used.

I purposefully synchronize only in deprecated -> non-deprecated direction.

nested attributes

Handler emitting deprecation warnings now handles nested attributes (like tiff.xdpi)

warnings

Now not emitted if new value was already set like deprecated one.

This will not emit warnings if somebody is using default values (not setting anything)

structure

All constants (parameter definition)

  • were separated from logic
  • and exist in constant compile time defined array
  • parameters are declared automatically from this table

Next Steps

Again, this only targets compressed_publisher

If we agree on direction I can rework the rest along.

Refactoring

If generalizing for multiple transports:

  • constant parameter array would be thrown to new file like compressed_publisher_cfg.h
  • declareParameter may become free function shared by all transports
  • onParameterEvent may become free function shared by all transports

In particular header and implementation would be further cleared from (common) parameter related code

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

Thank you for your contribution of the refactoring and the great improvements!
I will make sure that these changes will be available in the upcoming iron release.

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