Skip to content

Conversation

@brammmieee
Copy link

Basic Info

Info Please fill out this column
Ticket(s) this addresses N/A - Bug fix for missing dynamic parameter
Primary OS tested on Ubuntu 24.04
Robotic platform tested on Gazebo simulation, custom AMR platform
Does this PR contain AI generated software? No
Was this PR description generated by AI software? No

Description of contribution in a few bullet points

  • Fixed missing approach_velocity_scaling_dist parameter in dynamicParametersCallback function
  • Parameter was previously declared and initialized but could not be updated at runtime
  • Now consistent with all other dynamic parameters in the Regulated Pure Pursuit Controller

Description of documentation updates required from your changes

  • No documentation updates required - this fixes existing documented functionality
  • The parameter is already documented, this just makes dynamic reconfiguration work as expected

Description of how this change was tested

  • Verified parameter can be changed via rqt_reconfigure during runtime
  • Confirmed parameter updates take effect immediately without node restart
  • Tested that velocity scaling behavior responds correctly to parameter changes
  • All other existing parameters continue to work as expected
  • Performed linting validation using colcon test

Future work that may be required in bullet points

  • None - this is a standalone bug fix that makes the parameter work as originally intended

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
  • Should this be backported to current distributions? If so, tag with backport-humble, backport-iron.

@mini-1235
Copy link
Collaborator

mini-1235 commented Nov 21, 2025

@brammmieee Please check the build error: https://app.circleci.com/pipelines/github/ros-navigation/navigation2/16812/workflows/d927e01c-8538-4765-8795-10813d7c8c85/jobs/49285

Also, please sign with DCO

@brammmieee brammmieee force-pushed the fix-approach-velocity-scaling-dist-dynamic-param branch from 8111a30 to d4b31d1 Compare November 24, 2025 10:24
The approach_velocity_scaling_dist parameter was declared and initialized
but missing from the dynamicParametersCallback function, preventing it
from being updated via dynamic reconfigure (e.g., rqt_reconfigure).

This commit adds the parameter to the callback handler, enabling runtime
reconfiguration like other controller parameters.

Signed-off-by: Bram Odrosslij <[email protected]>
@brammmieee brammmieee force-pushed the fix-approach-velocity-scaling-dist-dynamic-param branch from d4b31d1 to 1fbb8df Compare November 24, 2025 10:24
@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
..._pure_pursuit_controller/src/parameter_handler.cpp 0.00% 2 Missing ⚠️
Files with missing lines Coverage Δ
..._pure_pursuit_controller/src/parameter_handler.cpp 92.09% <0.00%> (-0.74%) ⬇️

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Please add to the tests so we exercise the code for coverage https://github.com/ros-navigation/navigation2/blob/main/nav2_regulated_pure_pursuit_controller/test/test_regulated_pp.cpp#L425-L521. Should be a 2 line change.

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.

3 participants