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

feat(controller-server): publish_zero_velocity parameter #4675

Merged

Conversation

reinzor
Copy link
Contributor

@reinzor reinzor commented Sep 13, 2024

For optionally publishing a zero velocity command reference on goal exit. Publishing a zero velocity is not desired when we are following consecutive path segments that end with a velocity.

#4674

For optionally publishing a zero velocity command reference on goal
exit. Publishing a zero velocity is not desired when we are following
consecutive path segments that end with a velocity.

Signed-off-by: Rein Appeldoorn <[email protected]>
@reinzor reinzor force-pushed the feat/optional-publish-zero-velocity branch from 9964673 to 9f611a6 Compare September 13, 2024 06:04
Copy link
Contributor

mergify bot commented Sep 13, 2024

This pull request is in conflict. Could you fix it @reinzor?

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nav2_controller/src/controller_server.cpp 87.50% 2 Missing ⚠️
Files with missing lines Coverage Δ
...ller/include/nav2_controller/controller_server.hpp 100.00% <ø> (ø)
nav2_controller/src/controller_server.cpp 84.49% <87.50%> (+0.20%) ⬆️

... and 4 files with indirect coverage changes

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.

Just the usual migration + configuration guide update to introduce the new parameter.

nav2_controller/src/controller_server.cpp Outdated Show resolved Hide resolved
@reinzor reinzor force-pushed the feat/optional-publish-zero-velocity branch from 476aa82 to 15698d4 Compare September 17, 2024 06:17
Signed-off-by: Rein Appeldoorn <[email protected]>
@reinzor reinzor force-pushed the feat/optional-publish-zero-velocity branch from 15698d4 to 6e5e0c3 Compare September 17, 2024 06:23
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.

LGTM. Add to the migration guide the change + new parameter and add to the controller server's configuration guide the new parameter for documentation for future lookers

Edit: Sorry that wasn't supposed to be an approval, but the remaining items are trivial

nav2_controller/src/controller_server.cpp Outdated Show resolved Hide resolved
nav2_controller/src/controller_server.cpp Show resolved Hide resolved
@SteveMacenski SteveMacenski linked an issue Sep 17, 2024 that may be closed by this pull request
Signed-off-by: Rein Appeldoorn <[email protected]>
@SteveMacenski SteveMacenski merged commit 5dd7ad1 into ros-navigation:main Sep 20, 2024
10 of 11 checks passed
@reinzor
Copy link
Contributor Author

reinzor commented Sep 23, 2024

@SteveMacenski can we backport this on Jazzy? I can alter the change so that we maintain ABI compatible.

@SteveMacenski
Copy link
Member

SteveMacenski commented Sep 23, 2024

@reinzor sure! An altered version would need to be done, but should be easy enough - basically just use the existing method and add in a comment with this PR number to mention this backport for why the naming doesn't 100% line up

reinzor added a commit to nobleo/navigation2 that referenced this pull request Sep 24, 2024
reinzor added a commit to nobleo/navigation2 that referenced this pull request Sep 24, 2024
SteveMacenski added a commit that referenced this pull request Sep 25, 2024
…port (#4687)

* feat(controller-server): `publish_zero_velocity` parameter (#4675)

Backport of #4675

Signed-off-by: Rein Appeldoorn <[email protected]>

* Update nav2_controller/src/controller_server.cpp

Co-authored-by: Steve Macenski <[email protected]>
Signed-off-by: Rein Appeldoorn <[email protected]>

---------

Signed-off-by: Rein Appeldoorn <[email protected]>
Signed-off-by: Rein Appeldoorn <[email protected]>
Co-authored-by: Steve Macenski <[email protected]>
@Timple
Copy link
Contributor

Timple commented Oct 22, 2024

This feature has been verified in production 🙂

Would a Jazzy bloom release be in order? As the last one was from August.

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.

Option to disable publishZeroVelocity in the controller server
4 participants