Dynamic param patterns

[Nils-ChristianIseke]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#4907
Primary OS tested on	Ubuntu
Robotic platform tested on	None
Does this PR contain AI generated software?	No

---

Description of contribution in a few bullet points

Description of documentation updates required from your changes

Description of how this change was tested

---

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

@SteveMacenski I hope I understood the ticket correctly. I will continue to work on the rest of the codebase if these changes are the expected ones :).

[codecov]

Codecov Report

Attention: Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
..._pure_pursuit_controller/src/parameter_handler.cpp	87.87%	4 Missing ⚠️

Files with missing lines	Coverage Δ
...ated_pure_pursuit_controller/parameter_handler.hpp	100.00% <ø> (ø)
..._pure_pursuit_controller/src/parameter_handler.cpp	93.13% <87.87%> (-1.24%)	⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[SteveMacenski]

Otherwise, looks good to me! It might be valuable as part of this work to also add in other parameter validations while we're making these changes and auditing dynamic parameters. For example:

• Are all parameters set as dynamic parameters? If not, should those missing ones be dynamic?
• Are there other range checks that should be made for parameters?

It might be worth adding to the ticket a checklist of all the nodes that need updating and we can have that discussion for each to make sure we fill any obvious gaps currently missing due to updates over time where parameters were mistakenly not added or some validation checks that might be good to have

[SteveMacenski]

Otherwise, this is a good template to use for other future ports!

[Nils-ChristianIseke]

@SteveMacenski i am not sure if i like the approach of 589474c. What is your opinion on it?

[mergify]

@Nils-ChristianIseke, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

@Nils-ChristianIseke, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[SteveMacenski]

@SteveMacenski i am not sure if i like the approach of 589474c. What is your opinion on it?

I'm in the same boat as you; I don't know that I like it either. The declares I like better than the gets, considering the param vectors are separated by types. I do like the simplicity of the below though:

      auto it = double_parameters.find(name);
      if (it != double_parameters.end()) {
        it->second(parameter.as_double());
      }

However, I think if we did something like this, I think we should have an internal struct within the param handler quick includes everything: the parameter name, its default, its value, the setter lambda, and update validation lambda. That way, we define that once and its used for declare/get/dynamic parameter update/dynamic parameter validation. The repeated vector lists I think make it less readable to a casual developer compared to what's already there with declare/get calls that are pretty standard across ROS 2. Something potentially nice about that is that we could register that map with the nav2_utils::LifecycleNode and completely abstract the dynamic parameter callbacks to the base class node behind the scenes.

FYI you might be interested in https://github.com/PickNikRobotics/generate_parameter_library. I'm not sure I love this method (what if we want more custom parts of our parameter handler, like wanting to mutex lock parameter access for multi-threaded situations where the dynamic parameter executor is different from the running executor) -- but maybe those are handled and/or you see value/inspiration here

[mergify]

@Nils-ChristianIseke, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

This pull request is in conflict. Could you fix it @Nils-ChristianIseke?

[SteveMacenski]

Anything further blocking? Also just waiting on CI to build

[Nils-ChristianIseke]

I'm not sure I'm happy with it yet (we discussed about abstracting some parts to get rid of boilerplate, however i can think about that during another PR).

You also mentioned that further validation should be done:

Otherwise, looks good to me! It might be valuable as part of this work to also add in other parameter validations while we're making these changes and auditing dynamic parameters. For example:

Should I take another closer look? So far I don't have enough context knowledge to know what range checks would be meaningful, for example.
But we can move on, and when I'm more comfortable with these changes, I can think about this stuff.

[SteveMacenski]

For this one, you can check if any parameter value is a double then it must be positive. All the floating point values in this particular algorithm must be >= 0.0. I think that's the only validation that can be done in this algorithm since the others are application dependent (i.e. lookaheads could be 10cm or 10m).

The only other validity check needed is the one that is already in place for the rotation / reverse :-)

[SteveMacenski]

I think we have the recipe down now! I think if you open the PR in this format, I can help on the parameter ranges and knock these out one at a time.

Next on your list is MPPI - lets skip that one and circle back since that parameter design is very different and disrupts our flow.

Pose Progress Checker is next up. Lets do all the nav2_controller progress checker / goal checker plugins next

Pose Progress Checker

• required_movement_angle should be > 0 and < 2PI

Simple Goal Checker:

• XY goal tolerance should be >0
• Yaw should be 0 <= tol <= 2PI

Simple progress checker

• required_movement_angle should be > 0 and < 2PI
• movement_time_allowance >= 0

Stopped goal checker

• rot_stopped_velocity >= 0
• trans_stopped_velocity >= 0

[Nianzu]

We just updated our nav2 after a while on the same version, and it looks like this PR is causing some issues for us. Is there some reason that a check for double's not being negative was added? We have had negative values in our param files for a long time, was there some change to how we are supposed to handle the yaml files that we missed?

[SteveMacenski]

@Nianzu what value? I thought we checked and made sure to call out any negatives, but if we missed one or more, please let us know

[Nianzu]

Sorry for the delay. I'm still trying to understand whats happening. It seems that this change is somehow applying outside of where it should. Our nav2_params.yaml file is affected, and we're seeing an error and then a crash anytime there is a negative value assigned to a double parameter. I'm wondering if the issue isn't in this PR, but in another later PR that causes this to apply to a wider scope than it should? Or is the parameter_handler.cpp here used elsewhere?

[Nianzu]

We checked out https://github.com/ros-navigation/navigation2/releases/tag/1.3.6 and that seemed to resolve the problem for us, but I haven't gone through the process of checking each release since to see if it was in fact a later PR that brought up this issue.

[Nianzu]

Maybe this is something I should open a separate ticket for?

[SteveMacenski]

To make sure I understand, you're saying a newer release fixed your issue? Then I think I know what's happening here and that was fully resolved - just use the newer release. I could dig through and find the specific patch for you, but we did find a couple of bugs with this initial PR that we resolved in the following weeks of its merge

[Nianzu]

Sorry for the lack of clarity, that might be the case, but so far I just tested the release right before this PR and that worked. Best I can tell, whatever is in the package manager right now is still causing this problem. I will try the latest release tag and report back whether we're still seeing the problem.