Skip to content

Fix warning dynamic_reconfigure#479

Merged
bmagyar merged 1 commit intoros-controls:melodic-develfrom
rbonghi:fix-warning_dynamic_reconfigure
Apr 16, 2020
Merged

Fix warning dynamic_reconfigure#479
bmagyar merged 1 commit intoros-controls:melodic-develfrom
rbonghi:fix-warning_dynamic_reconfigure

Conversation

@rbonghi
Copy link
Copy Markdown
Contributor

@rbonghi rbonghi commented Apr 6, 2020

Hi all,

I was working on my package when I approach this warning on my screen
image

[ WARN] [1586198292.260228386]: updateConfig() called on a dynamic_reconfigure::Server that provides its own mutex. This can lead to deadlocks if updateConfig() is called during an update. Providing a mutex to the constructor is highly recommended in this case. Please forward this message to the node author.

This pull-request fix the initialisation dynamic_reconfigure and now this warning is not show anymore.

PS: This pull-request should fix this answer https://answers.ros.org/question/310267/updateconfig-called-on-a-dynamic_reconfigureserver-that-provides-its-own-mutex/

Comment thread diff_drive_controller/include/diff_drive_controller/diff_drive_controller.h Outdated
Comment thread diff_drive_controller/src/diff_drive_controller.cpp Outdated
Comment thread diff_drive_controller/src/diff_drive_controller.cpp Outdated
Comment thread diff_drive_controller/src/diff_drive_controller.cpp Outdated
@rbonghi
Copy link
Copy Markdown
Contributor Author

rbonghi commented Apr 7, 2020

I updated the mutex mDynServerMutex to dyn_reconf_server_mutex_ following @matthew-reynolds .

I removed the line

-    dyn_reconf_server_->getConfigDefault(config);

Should be fine now.

Comment thread diff_drive_controller/src/diff_drive_controller.cpp
@matthew-reynolds
Copy link
Copy Markdown
Member

matthew-reynolds commented Apr 7, 2020

Not sure what's up with Travis... I re-ran the build for the last commit (merge 5c3ff61 into melodic-devel) but it didn't update the status on GitHub, and now newest commit isn't being built either. @bmagyar thoughts?

Similar issue here: travis-ci/travis-ci#7643, but since the GitHub-Travis integration is still working properly on other PRs in this repo (#466), I don't think that's the issue.

Copy link
Copy Markdown
Member

@matthew-reynolds matthew-reynolds left a comment

Choose a reason for hiding this comment

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

Looks good to me, just need to switch the boost::shared_ptr back to std::shared_ptr and a couple style nitpicks.

Comment thread diff_drive_controller/include/diff_drive_controller/diff_drive_controller.h Outdated
Comment thread diff_drive_controller/src/diff_drive_controller.cpp Outdated
@rbonghi
Copy link
Copy Markdown
Contributor Author

rbonghi commented Apr 10, 2020

Hi @matthew-reynolds ,

I followed your hints and I updated all changes. In the first time #479 (comment) I thought was the same to use a boost and I didn't change it.

On travis is all green https://travis-ci.org/github/ros-controls/ros_controllers/builds/673500215 I do not why is in pending here.

Copy link
Copy Markdown
Member

@matthew-reynolds matthew-reynolds left a comment

Choose a reason for hiding this comment

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

Yeah, Travis seems to be having some difficulty in the last few days.

Thanks for iterating, looks good to me.

@bmagyar bmagyar force-pushed the fix-warning_dynamic_reconfigure branch from 5cdbb34 to 352aeba Compare April 16, 2020 09:11
@bmagyar bmagyar removed the request for review from mathias-luedtke April 16, 2020 09:12
@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented Apr 16, 2020

I've squashed & rebased on the latest melodic-devel, going to merge when the CI is happy

@bmagyar bmagyar merged commit 0cd5d7a into ros-controls:melodic-devel Apr 16, 2020
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