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

[rolling] reconfigurable transport scoped parameters for CompressedSubscriber #144

Conversation

bmegli
Copy link
Contributor

@bmegli bmegli commented Apr 18, 2023

CompressedSubscriber

#108 is simply reading mode before image decoding

Some CompressedPublisher cleanup to have common structure

In the screenshot:

  • camera driver (publisher)
  • rqt_image_view (subscriber)
    • in CLI deprecation warnings visible
  • rqt_reconfigure (parameters)
    • dynamically changing through deprecated and new parameter

image

Design Decisions

Constant parameter table thrown back to compressed_publisher.cpp and compressed_subscriber.cpp

  • having those in separate files would be overkill - e.g. subscriber has 1 parameter

I decided not to overthink this too much.

After deprecated parameters are removed in the future the only thing left will be constant table with definitions.

- runtime reconfiguration
- <image>.<transport>.<param> as future replacement of  <image>.<param>
  - e.g. `image.compressed.mode` instead of `image.mode`
- 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

Related to ros-perception#140
@ijnek
Copy link
Member

ijnek commented Apr 20, 2023

Considering the duplicated code between publisher and subscriber is going to be removed after deprecated parameter are removed in the future, these changes look good. Thanks!

@ijnek ijnek merged commit b8ac0b4 into ros-perception:rolling Apr 20, 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