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

Add acc limit consideration to avoid overshooting in RotationShimController #4864

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

RBT22
Copy link
Contributor

@RBT22 RBT22 commented Jan 17, 2025


Basic Info

Info Please fill out this column
Ticket(s) this addresses NA
Primary OS tested on Ubuntu
Robotic platform tested on own robot hardware
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

Currently we don't do the deceleration as a function of its target orientation with RotationShimController. This PR implements this behavior using the max_angular_accel param.

Description of documentation updates required from your changes

The parameter description has to be updated.


Future work that may be required in bullet points

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@RBT22
Copy link
Contributor Author

RBT22 commented Jan 17, 2025

@SteveMacenski Let me know if you would introduce a separate deceleration limit parameter or if a min_angular_speed param is needed here.

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...m_controller/src/nav2_rotation_shim_controller.cpp 66.66% 1 Missing ⚠️
Files with missing lines Coverage Δ
...m_controller/src/nav2_rotation_shim_controller.cpp 94.73% <66.66%> (-0.51%) ⬇️

... and 2 files with indirect coverage changes

@RBT22 RBT22 force-pushed the rolling/rotation-shim-acc branch from 3e9b083 to 7b2d2b7 Compare January 22, 2025 09:59
@RBT22 RBT22 force-pushed the rolling/rotation-shim-acc branch from 7b2d2b7 to 7c1986a Compare January 22, 2025 10:07
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

i'm curious - did you test this with a small value for angular_disengage_threshold?

If this worked as expected, I would expect the behavior to be a:

  • Rotate accelerating to full rotational speed
  • Rotating decelerating to the angle of the path, once in deceleration area
  • Stop relatively close to the angle, with a small disengagement threshold (20 deg or less?)
  • Then controller starts moving

If so, that would make a really nice behavior - very precise, very 'robotic'. A gif to put in the migration guide for this would be splendid to show it at work and the value of it. My hope was that this would be used to create very dynamic hand offs with the controllers, but I think experience has told me now from feedback that this isn't what people want (and most of the controllers don't take kindly to being started in a high-acceleration-turn when starting path tracking)

Let me know if you would introduce a separate deceleration limit parameter or if a min_angular_speed param is needed here.

Nah, I think this can be symmetric. If someone has a problem with that, they can have a follow up PR. A bunch of controllers for rotating to heading do this, so this is consistent behavior

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.

2 participants