Skip to content

Use declare_or_get_param API#5336

Merged
SteveMacenski merged 3 commits intoros-navigation:mainfrom
elsayedelsheikh:new_declare_or_get_param_api
Jul 23, 2025
Merged

Use declare_or_get_param API#5336
SteveMacenski merged 3 commits intoros-navigation:mainfrom
elsayedelsheikh:new_declare_or_get_param_api

Conversation

@elsayedelsheikh
Copy link
Contributor

@elsayedelsheikh elsayedelsheikh commented Jul 7, 2025 •
edited
Loading

Basic Info

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

Description of contribution in a few bullet points

Related to #5299
Use declare_or_get_param API instead of declare_parameter - get_parameter

Target packages:

  • nav2_amcl

Description of documentation updates required from your changes

Nothing

Description of how this change was tested

  • Unit tests

Future work that may be required in bullet points

Nothing

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-*.

@elsayedelsheikh
Copy link
Contributor Author

elsayedelsheikh commented Jul 7, 2025

@SteveMacenski
if this is looking good, LMK to continue 👍

@elsayedelsheikh elsayedelsheikh force-pushed the new_declare_or_get_param_api branch from 76f108a to 25c8a6b Compare July 9, 2025 06:30
@elsayedelsheikh
Copy link
Contributor Author

elsayedelsheikh commented Jul 11, 2025

If you have time, Can you give me a green flag to continue working on this PR if it's up to what you're expecting or/ if there are any things to consider? This should be straight forward right?! :) @SteveMacenski

SteveMacenski
SteveMacenski reviewed Jul 14, 2025
View reviewed 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.

Yes! This looks like a good direction to me.

@SteveMacenski
Copy link
Member

SteveMacenski commented Jul 14, 2025

I think this makes sense to break up into a few pull requests. Maybe 1-2 packages per PR so that these are small and bite sized for review. 1 massive PR is going to be near impossible to review. If you start a checklist in the ticket, when you open a PR for a package, you can check that off and link the PR so we can keep track of it.

I think we should just have a "final check" box for both of us going over the code semi-manually and checking for any stragglers or other similar get/declare parameter workflows we can use to increase code quality! :-)

@elsayedelsheikh elsayedelsheikh mentioned this pull request Jul 20, 2025
Closed
@elsayedelsheikh elsayedelsheikh force-pushed the new_declare_or_get_param_api branch from 25c8a6b to 553acd1 Compare July 20, 2025 00:33
@elsayedelsheikh elsayedelsheikh marked this pull request as ready for review July 20, 2025 01:24
@elsayedelsheikh elsayedelsheikh changed the title Update declares and use declare_or_get_param API Use declare_or_get_param API Jul 20, 2025
@SteveMacenski
Copy link
Member

SteveMacenski commented Jul 22, 2025

@elsayedelsheikh some hundreds of tests are failing from this change

SteveMacenski
SteveMacenski reviewed Jul 22, 2025
View reviewed changes
@elsayedelsheikh
initial thoughs
cca5bb0 
Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>
@elsayedelsheikh
Remove line
3064777 
Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>
@elsayedelsheikh
Feedback
e5b1b2b 
Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>
@elsayedelsheikh elsayedelsheikh force-pushed the new_declare_or_get_param_api branch from 2c756b7 to e5b1b2b Compare July 22, 2025 18:04
@elsayedelsheikh
Copy link
Contributor Author

elsayedelsheikh commented Jul 22, 2025

@SteveMacenski Ready to merge! 😀

SteveMacenski
SteveMacenski approved these changes Jul 23, 2025
View reviewed 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.

@elsayedelsheikh did you do this with AI? If so, please check closely that these default values are all the same as before. If you did manually, just do a once over. This is alot to check and error prone on my side

@elsayedelsheikh
Copy link
Contributor Author

elsayedelsheikh commented Jul 23, 2025

@elsayedelsheikh did you do this with AI? If so, please check closely that these default values are all the same as before. If you did manually, just do a once over. This is alot to check and error prone on my side

It was manual. I checked again and the values are the same 💯 We are good here!

@SteveMacenski SteveMacenski merged commit 335d415 into ros-navigation:main Jul 23, 2025
14 of 15 checks passed
SakshayMahna pushed a commit to SakshayMahna/navigation2 that referenced this pull request Aug 8, 2025
@elsayedelsheikh @SakshayMahna
Use declare_or_get_param API (ros-navigation#5336)
ab57bf4 
* initial thoughs

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

* Remove line

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

* Feedback

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

---------

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveMacenski SteveMacenski SteveMacenski approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@elsayedelsheikh @SteveMacenski