-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Added support for MPPI Controller to adjust wz_std parameter based on linear speed #5294
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
base: main
Are you sure you want to change the base?
Added support for MPPI Controller to adjust wz_std parameter based on linear speed #5294
Conversation
… linear speed Signed-off-by: V. Oguz TOKMAK <5289529+vahapt@users.noreply.github.com>
… linear speed Signed-off-by: Vahap Oguz TOKMAK <5289529+vahapt@users.noreply.github.com>
… linear speed Signed-off-by: Vahap Oguz TOKMAK <5289529+vahapt@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
… linear speed Added test cases Signed-off-by: Vahap Oguz TOKMAK <5289529+vahapt@users.noreply.github.com>
@@ -331,7 +331,7 @@ TEST(CriticTests, PathAngleCritic) | |||
costmap_ros->on_configure(lstate); | |||
|
|||
models::State state; | |||
state.reset(1000, 30); | |||
state.reset(1000, 30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll cause linting issues
nav2_mppi_controller/include/nav2_mppi_controller/models/constraints.hpp
Outdated
Show resolved
Hide resolved
nav2_mppi_controller/include/nav2_mppi_controller/models/constraints.hpp
Outdated
Show resolved
Hide resolved
nav2_mppi_controller/include/nav2_mppi_controller/models/constraints.hpp
Outdated
Show resolved
Hide resolved
… linear speed - Refactored the code to simplify flow - Merged to latest `main` branch Signed-off-by: Vahap Oguz TOKMAK <5289529+vahapt@users.noreply.github.com>
… linear speed - Code cleanup & bugfix Signed-off-by: Vahap Oguz TOKMAK <5289529+vahapt@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't resolve the issue that the noise generator's wz sampling doesn't actually change when regenerate_noises = false. We need to regenerate the wz samples on getNextNoises
when regenerate_noises = false regardless to reflect on the newest values.
This seems to have undone some points of previous discussion and create other gaps. I've identified a few bugs / logical gaps that need to be addressed.
Overall, I think this is what we need to do:
- Revert all unecessary changes. We shouldn't be messing with the thread, state variables, constructors, etc.
- Add the adaptive wz to the
SamplingStd
class along with validate, compute and reset (to reset back to original value). These are members of the wz std sampling data and advanced optimizer settings, not the noise generator utility.
From there, the data and its operating functions will all be grouped together and we just need to figure out where to call them.
I'd argue it should be in the optimizer:
- The wz adaptive should be reset and validated on Optimizer::Reset (validate can be called within reset)
- The wz adaptive should be computed and validated on generateNoisedTrajectories (validate can be called within the compute function implementation)
- The new wz adaptive value can be given to generateNextNoises as an argument
- If the current wz is different and regenerate_noises = false, then store the new value and recompute wz. If the current wz is different and regenerate_noises = true, then store and call the thread to make the full update. If current wz is the same as the stored wz, do nothing more than it does now.
I'd then argue that perhaps moving wz_std_adaptive
from SamplingStd
to AdvancedConstraints
makes sense to bin it altogether. And that way we pass in the AdvancedConstraints
to the generateNextNoises
which could later contain other Vx/Vy noises to regenerate based off of, from speed changes as well in the future. We just need to make sure we keep SamplingStd.wz
and AdvancedConstraints.wz_std_adaptive
aligned in all the right places.
The other way I could see this going is that we do it in the noise generator. As an implementation detail of generateNextNoises we update and validate the adaptive wz using the methods in the AdvancedConstraints
.
If the current wz is different and regenerate_noises = false, then store the new value and recompute wz. If the current wz is different and regenerate_noises = true, then store and call the thread to make the full update. If current wz is the same as the stored wz, do nothing more than it does now.
In the noise generator reset, we call the adaptive wz reset method back to the base settings value.
The downside of this is that then the current state of the adaptive wz is managed by a utility rather than the optimizer, which is a little odd to have to query to get the state for the updateControlSequence
method. Especially considering that the AdvancedConstraints is a member of the OptimizerSettings
so these are optimizer-specific states
nav2_mppi_controller/include/nav2_mppi_controller/tools/noise_generator.hpp
Show resolved
Hide resolved
nav2_mppi_controller/include/nav2_mppi_controller/tools/noise_generator.hpp
Show resolved
Hide resolved
* @brief Internal variable that holds wz_std after decay is applied. | ||
* If decay is disabled, SamplingStd.wz == wz_std_adaptive | ||
*/ | ||
float wz_std_adaptive; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not need to be stored here. We talked about this being in the std model alongside the validate / update / reset functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still strongly disagree. It's not a setting. It's a part of current state just like vx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But its not part of the noise generator state, its part of the optimizer state
Comments noted, I'll minimize changes and leave potential improvements to later updates.
So, we'll convert adaptive_wz back to shared_pointer and move functions to SamplingStd? Note: My work schedule is a bit hectic within this month, please bear with me. |
I don't think we need to change it to a shared pointer, we can just pass in the updated / validated adaptive Stds for the noise generator computation from the main Optimizer Something we could do also/instead is change the settings itself to being a shared pointer. We pass it into a few places and it is not intended that these aren't updated in their respective places, I don't think. |
If we don't keep a pointer we'll have 2 copies of settings hence 2 copies of adaptive stds. Reset is called only once per waypoint but we update adaptive std on every tick. Updated version of adaptive std values are both accessed inside noise generator and optimizer. I think we can make settings_ a unique pointer owned by the Optimizer. Noise generator's thread can create a copy of required variables at initialization phase to avoid concurrent modifications. |
I think that's one of the key points I want to change about the current implementation. I would like the validate and update to happen in the Optimizer so that the flow of validate/update/reset are clear as day and only used in a single object so that it can be more easily understood the control flow. Also we may update these values over time based on other criteria (std vx, vy based on the velocity as well, perhaps) so my thinking of the separation of concerns in this package wouldn't put this in the noise generator. Its goal is just to generate noises, not compute the values to use for noises. That seems to me more an We could do something like the |
Hi, thanks for this PR. Another point that may cause oscillations on heavy robots is the "latency" of the mechanical control. We work to integrate this in the motion_model to take it into account. |
Let me follow up with you about that over email @BriceRenaudeau ! |
Basic Info
Description of contribution in a few bullet points
Added feature enables dynamic modification of wz_std (angular deviation) based on linear velocity of the robot.
When a robot with high inertia (e.g. 500kg) is moving fast and if wz_std is above 0.3, oscillation behavior can be observed.
Lowering wz_std stabilizes the robot but then the maneuvers take more time.
Dynamically reducing wz_std as vx, vy increase (speed of the robot) solves both problems.
Suggested values to start with: wz_std = 0.3, wz_std_decay_to = 0.05, wz_std_decay_strength = 5.0
The following is used as the decay function
f(x) = (wz_std - wz_std_decay_to) * e^(-wz_std_decay_strength * v_linear) + wz_std_decay_to
Description of documentation updates required from your changes
2 new parameters are added to nav2_mppi_controller, README.md updated accordingly
advanced.wz_std_decay_strength
advanced.wz_std_decay_to
Description of how this change was tested
On both physical and simulated robots, changes deployed and parameters changed through live parameter update.
Robot behavior change and oscillation effects observed and confirmed.
It should be noted that tests are performed with a differential drive robot setup, thus holonomic behavior or potential anomalies with ackermann control are not tested yet.
Unit tests created to test new computeAdaptiveStds function
Future work that may be required in bullet points
For Maintainers:
backport-*
.