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

Bringing back dynamic parameters: Amcl #1378

Merged
merged 1 commit into from
Dec 4, 2019

Conversation

bpwilcox
Copy link

This is primarily a direct port of the callback (though refactored) for the ROS1 navigation reconfiugreCB.

Info Please fill out this column
Ticket(s) this addresses #956
Primary OS tested on Ubuntu
Robotic platform tested on tb3 waffle simulation

@SteveMacenski
Copy link
Member

SteveMacenski commented Nov 21, 2019

This also has the costmap stuff in it too.

Is there a way to update the parameters without resetting the filter? That makes it challenging to effectively tune.

@bpwilcox
Copy link
Author

bpwilcox commented Nov 21, 2019

This also has the costmap stuff in it too.

Yes, sorry, I had intended to clean up these PRs, but hadn't. Maybe I can separate the PR adding the ParameterEventsSubscriber class to nav2_utils and then have these PRs for amcl and costmap be independent.

Is there a way to update the parameters without resetting the filter? That makes it challenging to effectively tune.

Well, firstly, this behavior matches the ROS1 behavior. I do think, however, we could check for if certain parameters have been changed that are relevant to resetting the filter, though.

@SteveMacenski
Copy link
Member

Well I think the Costmap one can go in soon so I wouldn't go too far out of your way.

Maybe this is why I hated dynamic reconfigure... OK. I have no issue here then once the costmap stuff is backed out

setRobotFootprint(new_footprint);
}
}
map_update_thread_ = new std::thread(std::bind(

Choose a reason for hiding this comment

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

As we discussed in our meeting, we shouldn't recreate the thread on every parameter update. The parameter changes should only impact those resources that actually depend on their values. Should be more granular in the responses to parameter changes.

Copy link
Author

Choose a reason for hiding this comment

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

Will address in another PR

@bpwilcox bpwilcox force-pushed the amcl_parameter_callbacks branch from 6f34c06 to 5131fad Compare December 4, 2019 18:36
@codecov
Copy link

codecov bot commented Dec 4, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@7ddaaf4). Click here to learn what that means.
The diff coverage is 10.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1378   +/-   ##
=========================================
  Coverage          ?   25.27%           
=========================================
  Files             ?      235           
  Lines             ?    11504           
  Branches          ?     3714           
=========================================
  Hits              ?     2908           
  Misses            ?     6618           
  Partials          ?     1978
Flag Coverage Δ
#project 25.27% <10.42%> (?)
Impacted Files Coverage Δ
nav2_costmap_2d/include/nav2_costmap_2d/layer.hpp 54.54% <ø> (ø)
...map_2d/include/nav2_costmap_2d/inflation_layer.hpp 82.75% <ø> (ø)
...tmap_2d/include/nav2_costmap_2d/costmap_2d_ros.hpp 0% <ø> (ø)
...costmap_2d/include/nav2_costmap_2d/voxel_layer.hpp 0% <ø> (ø)
...tmap_2d/include/nav2_costmap_2d/obstacle_layer.hpp 100% <ø> (ø)
nav2_costmap_2d/plugins/voxel_layer.cpp 0.37% <0%> (ø)
nav2_costmap_2d/src/costmap_2d_ros.cpp 0% <0%> (ø)
nav2_costmap_2d/plugins/static_layer.cpp 31.21% <10%> (ø)
nav2_costmap_2d/src/layer.cpp 84% <100%> (ø)
nav2_util/src/parameter_events_subscriber.cpp 11.11% <11.11%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ddaaf4...5131fad. Read the comment docs.

@bpwilcox bpwilcox merged commit 35aa859 into ros-navigation:master Dec 4, 2019
@crdelsey crdelsey mentioned this pull request Dec 5, 2019
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