-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Unclamp noise velocity. #5266
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
Unclamp noise velocity. #5266
Conversation
@chanhhoang99, your PR has failed to build. Please check CI outputs and resolve issues. |
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.
Please remove the commented out lines, I don't like keeping dead code inline :-)
Please also rebase on main so CI should turn over - there were some message filter API changes that are fixed in main now that should make this compile (minus any potential issues in this PR)
Also fix the linting error:
+++ nav2_mppi_controller/include/nav2_mppi_controller/motion_models.hpp.uncrustify
@@ -96,2 +96,2 @@
- .cwiseMax(lower_bound_vx)
- .cwiseMin(upper_bound_vx);
+ .cwiseMax(lower_bound_vx)
+ .cwiseMin(upper_bound_vx);
@@ -103,2 +103,2 @@
- .cwiseMax(state.wz.col(i - 1) - max_delta_wz)
- .cwiseMin(state.wz.col(i - 1) + max_delta_wz);
+ .cwiseMax(state.wz.col(i - 1) - max_delta_wz)
+ .cwiseMin(state.wz.col(i - 1) + max_delta_wz);
@@ -117,2 +117,2 @@
- .cwiseMax(lower_bound_vy)
- .cwiseMin(upper_bound_vy);
+ .cwiseMax(lower_bound_vy)
+ .cwiseMin(upper_bound_vy);
See my comment in huynhduc9905#6 (comment) |
87c7f34
to
781b790
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Your compiler warnings are odd - I just opened a PR that should address it #5277. I'll merge once CI passes in the morning and then you should rebase on it to get the Build Against Released Distributions" jobs to pass |
Merged - please rebase! I’m not sure what’s going on with mypy linting - feel free to ignore that for the scope of this PR if we get everything else passing. |
a038a8f
to
c74adaa
Compare
@chanhhoang99 please sign off with DCO :-) That's the only thing blocking for merge other than asking others to validate since this is a major change (even though its like 3 changes 😆 ) |
Signed-off-by: Chanh Hoang <[email protected]>
Signed-off-by: Chanh Hoang <[email protected]>
Signed-off-by: Chanh Hoang <[email protected]>
Signed-off-by: Chanh Hoang <[email protected]>
c74adaa
to
c783bec
Compare
Just waiting on testing BTW! |
I'm having a hard time finding beta testers with similar situations that aren't too underwater to test this. I'm going to go ahead and merge this after some preliminary testing done be 2-3 people and didn't notice an issue. I'll just keep a look out on the issue tracker if there are any regressions reported. Thanks for this contribution @chanhhoang99 ! |
@chanhhoang99 should we keep chatting about the lag compensation piece? |
@SteveMacenski , I have not yet make progress on that. I will create another issue when there is improvement. |
Great thanks for the update! Let me know if I can be of help! I really appreciate you digging into MPPI, not many folks have given me critical feedback on the internal functioning to date (only that they like the outputs) |
* unclamp state.cvx Signed-off-by: Chanh Hoang <[email protected]> * remove uncommented code Signed-off-by: Chanh Hoang <[email protected]> * fix linting Signed-off-by: Chanh Hoang <[email protected]> * fix wrong linting Signed-off-by: Chanh Hoang <[email protected]> --------- Signed-off-by: Chanh Hoang <[email protected]>
* unclamp state.cvx Signed-off-by: Chanh Hoang <[email protected]> * remove uncommented code Signed-off-by: Chanh Hoang <[email protected]> * fix linting Signed-off-by: Chanh Hoang <[email protected]> * fix wrong linting Signed-off-by: Chanh Hoang <[email protected]> --------- Signed-off-by: Chanh Hoang <[email protected]>
For me these changes are breaking; the acceleration limits are no longer respected: before this PR ![]() after this PR ![]() I'm not too familiar with this part to debug, can you guys see an issue with it? I created #5464 to track |
This reverts commit fb25b2f.
Basic Info
Description of contribution in a few bullet points
Verify acceleration constrain applied to noise velocity sample.
Description of documentation updates required from your changes
Description of how this change was tested
Tested with robot base which has acceleration clamp in driver controller and verify if robot MPPI controller can predict that acceleration clamp or not.(Tested with low acceleration e.g az = 0.1 rad/s^2, ax max = 0.5 m/s^2, ax min = -0.5m/s^2)
Future work that may be required in bullet points
For Maintainers: