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 reconfigurable parameters to disparity node #490

Merged
merged 2 commits into from
Jan 8, 2020

Conversation

jacobperron
Copy link
Contributor

@jacobperron jacobperron commented Jan 7, 2020

Follow-up to #486

The parameters are declared with descriptors containing constraints. A callback is used to update the matcher when a parameter is changed.

Also removed the old dynamic reconfigure config file.

The parameter names and defaults are the same as ROS 1, with the following exceptions:

  • fullDP was renamed to full_dp
  • disp12MaxDiff was renamed to disp12_max_diff

both renamed for consistency with other parameter names.

@jacobperron
Copy link
Contributor Author

jacobperron commented Jan 7, 2020

This change uncovered a bug. See #491 for a fix (it should resolve the CI failure).

@SteveMacenski
Copy link
Member

SteveMacenski commented Jan 7, 2020

I can definitely live with this, but another option would be to have lambdas for the registered callbacks when you setup the on change parameter stuff. That would let you register unique lambdas for each to effect that parameter's client rather than the long if else else else statement checking on what the parameter is.

If you prefer this way, I don't have an issue with it but I wanted to check that you intentionally chose this way for some reason you prefer. I generally avoid lambdas, but that particular use-case is compelling.

@jacobperron
Copy link
Contributor Author

jacobperron commented Jan 7, 2020

@SteveMacenski I'm not sure I follow what you're suggesting. Any callback we register with set_on_parameters_set_callback or add_on_set_parameters_callback is passed a vector of parameters that have changed. We could maintain a map of parameter names to lambdas that would remove the need for the if-else block, but I'm not sure that it would be more readable or save lines of code.

The parameters are declared with descriptors containing constraints. A callback is used to update the matcher when a parameter is changed.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Contributor Author

Rebased on ros2 now that #491 is merged.

@jacobperron
Copy link
Contributor Author

Also, I opted to not use add_on_set_parameters_callback since it is not available in Dashing.

@SteveMacenski
Copy link
Member

SteveMacenski commented Jan 8, 2020

Ok that’s fine, just wanted to highlight the option

Copy link
Collaborator

@JWhitleyWork JWhitleyWork left a comment

Choose a reason for hiding this comment

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

LGTM

@JWhitleyWork JWhitleyWork merged commit fcaf02b into ros2 Jan 8, 2020
@JWhitleyWork JWhitleyWork deleted the jacob/reconfig_stereo_params branch January 8, 2020 02:58
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.

3 participants