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 dynamic parameters #532

Merged
merged 18 commits into from Jan 29, 2019

Conversation

Projects
None yet
3 participants
@matt-attack
Copy link
Contributor

commented Dec 10, 2018

This adds a class that handles dynamic configuration of parameters in a much cleaner way than with standard dynamic_reconfigure.

It also allows for setting the list of configurable parameters are runtime rather than compile time.

@matt-attack

This comment has been minimized.

Copy link
Owner

commented on c34629f Aug 4, 2017

Implements swri-robotics#471

@matt-attack

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

@pjreed looks like we had another test failure. Looking at the test I'm not really sure what caused it as there doesn't appear to be a way for that publisher to be closed.

@pjreed

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

How does this work? Could you add some documentation for it?

Seems like the test failure must be timing-based. Re-running it eventually makes it pass. Sometime I'll investigate and see if I can figure out what's causing it...

@matt-attack

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

Yeah, can do.

It implements the same interface as the dynamic_reconfigure system, so it works with all the same tools, but sets up the configuration at run time in the code instead of at build time in the cfg files.

Right now, it handles changes by directly changing variables in your class, so you need to make sure they persist and to lock the mutex if you don't want them changing out from under you. It's a much simpler interface, but you need to be careful. Adding a callback for a change instead would also be easy to do for cases where that would be useful.

pjreed and others added some commits Dec 18, 2018

Matthew Bries
@matt-attack

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

@pjreed added a description if you want to take a look. I changed the swri_transform_util dynamic publisher to use this as an example if you would like to see how it works.

Matthew Bries added some commits Jan 3, 2019

@matt-attack

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

@pjreed Does this look good to merge?

@pjreed

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

I think it'd also be useful to be able to register a callback in order to be notified whenever any values are reconfigured. I can think of two scenarios where I'd want that:

  1. If there's some expensive operation that you only want to perform when a value actually changes
  2. If you want to cache the reconfigurable values in non-volatile variables so that you can access them repeatedly without needing to lock a mutex
Show resolved Hide resolved swri_roscpp/include/swri_roscpp/dynamic_parameters.h
Show resolved Hide resolved swri_roscpp/include/swri_roscpp/dynamic_parameters.h Outdated
Show resolved Hide resolved swri_roscpp/include/swri_roscpp/dynamic_parameters.h Outdated
Show resolved Hide resolved swri_roscpp/readme.md Outdated
Show resolved Hide resolved swri_transform_util/src/nodelets/dynamic_publisher.cpp Outdated
Show resolved Hide resolved swri_roscpp/readme.md Outdated

Matthew Bries added some commits Jan 8, 2019

Matthew Bries
Matthew Bries
Matthew Bries
Matthew Bries
@matt-attack

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

If you think it is okay, I might leave adding the callbacks for another issue. Otherwise should be ready for review

@pjreed

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Seems like the build is failing:

Errors     << swri_visualization_util:cmake /root/catkin_ws/logs/swri_visualization_util/build.cmake.000.log
CMake Error at /opt/ros/indigo/share/catkin/cmake/catkin_package.cmake:298 (message):
  catkin_package() include dir 'include' does not exist relative to
  '/root/catkin_ws/src/marti_common/swri_visualization_util'
Call Stack (most recent call first):
  /opt/ros/indigo/share/catkin/cmake/catkin_package.cmake:100 (_catkin_package)
  CMakeLists.txt:29 (catkin_package)
@matt-attack

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Hmm. I seem to have accidentally merged in the wrong master causing these issues

Matthew Bries
Revert "Merge branch 'master' of https://github.com/matt-attack/marti…
…_common into add-dynamic-parameters"

This reverts commit 2013a15, reversing
changes made to 6f7b8ed.
@matt-attack

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Should be fixed now

@pjreed pjreed requested a review from daniel-dsouza Jan 28, 2019

@daniel-dsouza
Copy link

left a comment

rather than asking the user to manually lock/unlock the mutex, could we use std::lock_guard to handle this automatically?

Matthew Bries
@matt-attack

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

@daniel-dsouza added a function to let the user do that

@daniel-dsouza

This comment has been minimized.

Copy link

commented Jan 29, 2019

looks good to me

@daniel-dsouza

This comment has been minimized.

Copy link

commented Jan 29, 2019

@pjreed have you resolved your review?

@pjreed

pjreed approved these changes Jan 29, 2019

@daniel-dsouza daniel-dsouza merged commit 6201ad7 into swri-robotics:master Jan 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.