Bringing back dynamic parameters: Costmap2D #1370
Conversation
add lambda callbacks directly
Codecov Report
@@ Coverage Diff @@
## master #1370 +/- ##
===========================================
- Coverage 40.73% 24.71% -16.02%
===========================================
Files 233 235 +2
Lines 11185 11477 +292
Branches 4822 3679 -1143
===========================================
- Hits 4556 2837 -1719
- Misses 3452 6709 +3257
+ Partials 3177 1931 -1246
Continue to review full report at Codecov.
|
|
@SteveMacenski from what I can tell on my machine, rqt_reconfigure is having trouble loading up all the parameters from all the nodes, gets stuck and never comes up for me to try changing parameters with the full stack. |
|
Can you try with just 1 node running? |
|
@SteveMacenski I was once able to get it to work on a test node (non-navstack related), but it has been very buggy, many freezes or crashes since. Have you played with the plugin yourself recently? |
|
No, reconfigure is something I use once when setting up a node and then usually never again |
|
I'm not blocking over that, but it would be nice to know that this works via that as a general rule |
|
Seemingly the reconfigure tool is not very stable currently, so perhaps we can come back to this issue at some point later, |
|
From a discussion today, there was the concern of whether dynamic parameters would/should be updated in a real production system. If by default as in ROS1, dynamic parameters are always allowed, it would be up to the user to know not to change them for their use case. An additional measure would be to set parameters as read-only in the their descriptor field, which at the moment is only specified in code, but with my PRs upstream to parse descriptors in yaml, would be configurable at launch. If we want to allow the parameters to be set on the node, but simply not to take effect when in the active state of the lifecycle node, we could either define a mode in which parameters callbacks are allowed (i.e in active or inactive state), or a parameter flag which would turn off dynamic callbacks entirely, still allowing the node to be re-configured via the lifecycle reset ( I believe we arrived at thinking we should allow the flag to skip all parameter callbacks if the user doesn't desire. Also, moving more code from the event callback to be smaller parameter updates and only doing extra work when related to the parameter change. I also realized that the callback handles should be cleared when the node is cleaned up, so I will add that. |
|
I don’t think you need to go crazy to stop people from doing things like this. People that value the lifecycle elements are going to understand this break some of the benefits. People dont really use dyn reconfigure in production (using to change things in usual modes) or when they do I think we all understand its not good practice and is a bit hacky. I think making the capability, thats analog to dynamic reconfigure, is fine. I wouldn’t overthink it. |
| setRobotFootprint(new_footprint); | ||
| } | ||
| } | ||
| map_update_thread_ = new std::thread(std::bind( |
There was a problem hiding this comment.
Responses to parameter changes should be more granular. That is, only data structures/resources that depend on the parameter changes should be re-created and/or re-initialized.
There was a problem hiding this comment.
I agree, but I think we can make that a separate PR.
This PR brings the
ParameterEventsSubscriberclass into thenav2_utilsand re-enables run-time configuration of parameters. As a first re-integration step, this PR adds parity with ROS1 dynamic parameters for thecostmap2dpackage.Basic Info
Description of contribution in a few bullet points
ParameterEventsSusbcriberparameter event callbacksFuture work that may be required in bullet points
nav2_amclandnav2_dwb_controllernav2_utils