navigation2
navigation2 copied to clipboard
Nav2 toolkit
Basic Info
| Info | Please fill out this column |
|---|---|
| Ticket(s) this addresses | #5070 |
| Primary OS tested on | Ubuntu |
| Robotic platform tested on | Turtlebot gazebo simulation. |
| Does this PR contain AI generated software? | Somewhat |
Description of testing performed
Functional tests were conducted on turtlebot3 and are working as expected
Description of contribution in a few bullet points
- This Contribution adds a new package named
nav2_toolkitwhich has QOL features, which can be handy while working with Nav2 .
Description of documentation updates required from your changes
- Addition of 3 new parameterauto_start_saving, auto_restore_pose, save_interval_sec, so need to add that to nav2_params and documentation page
Description of how this change was tested
All the changes were tested multiple times in simulation over a course of 3 weeks.
Future work that may be required in bullet points
In total , this package will contain 3 tools i.e
- [x] pose_saver & pose_restore with added launch file in nav2_bringup.
- [ ] Relocation of BaseFootprintPublisher.
- [ ] A node which can pause and resume nav2 goals.
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
@pranavk-2003, 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).
@pranavk-2003 let me know when you want me to take a look again! I see you pushed some changes but a number of items are not yet addressed :smile:
@pranavk-2003 let me know when you want me to take a look again! I see you pushed some changes but a number of items are not yet addressed 😄
Its somewhat figured out,I'll let you know soon , got busy with university stuff ☹ .the few pushed stuff i still have to test.I am right now finding it difficult to get the nav2_package() macro right for nav2_toolkit.
Hey @SteveMacenski ,I think i am done with the code updates , you can now have a look again throughout.The doxygen docs is in progress. : ) Also the checks are failing , let me know what has to be done for that too.
@pranavk-2003 open the failing jobs and fix the issues they lay out :-) It looks like alot of static code quality issues in linting
Needs test coverage as well!
Hey okay, so what shall it be?test as in demos for using the tool?
@pranavk-2003 open the failing jobs and fix the issues they lay out :-) It looks like alot of static code quality issues in linting
Okay i understood how to solve those errors, i would solve these after all the code adjustments are done , as the code adjustments would introduce more of them
Hey okay, so what shall it be?test as in demos for using the tool?
You should create a unit test that runs the object and then publishes some data to the topic its listening to. Check after awhile that there's a file at the requested location and the data is correct.
Then run another tests with autostart on and test that it correctly reads from that file and publishes / calls the service to set the pose correctly (the test object should expose the service, so nothing else should be running).
Then another test should be with autostart / auto-store off and call the services to start / stop / reset and check that it does what it should do.
Finally, a series of unit tests that checks read_pose_from_file write_pose_to_file individual function's correctness individually
CI should then tell you the coverage metrics in the codecov job - we need it to be at least 90% to be merged
Also as a matter of practice, do not resolve the PR topic discussions. I'll hit "resolve" when I verify that it was completed as intended. It makes my life easier as a maintainer to know that I validated each of my previous comments in your updated fixes without having to go back through all of the collapsed tabs each time :-)
This pull request is in conflict. Could you fix it @pranavk-2003?
Resolved the conflicts, @SteveMacenski i'll let you know when i want you to check this PR again.
@pranavk-2003, 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).
Hey @SteveMacenski its been a while.I have added tests, adjusted the code according to your recommendations and enhancement suggestions.I have tried to solve and fix most of what we discussed last time.Please review the changes and let me know any modifications.Also dont worry about the checks as i will fix them after all the code changes are done.I have also relocated the BaseFootprintPublisher to nav2_toolkit.:)
@pranavk-2003, 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).
Sincere apologies @SteveMacenski for the delay between last and this update and this dual mention .I understand its alot of work to review this PR from the start again , but i'll make sure to wrapup this PR to the earliest from now without any delays.
OK! Let me know when you're ready. Please also heed the linters + DCO sign offs. It will make it much easier for me to review if the styling is consistent and I'm not distracted with pointing out pedantic errors. It also doesn't seem to build - which would be a minimum requirement for me to review.
Try rebasing / pulling in main
@pranavk-2003, 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).
@pranavk-2003, 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).
@pranavk-2003, 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).
Sorry for the email spams of builds, ill remove you as a maintainer for now so it would not spam you.
Hi @SteveMacenski i think i might have done something wrong while trying to sign off my old commits :( . What should i do? maybe make a new draft PR.Although i Fixed most of the Build check fails.i would guess the failing ones are because of DCO.Please guide me through this.Apologies for the extra steps — really appreciate your patience.
There are alot of merge conflicts. You could either resolve them or possibly try applying your work on an up-to-date branch and opening a new PR. I'm still seeing alot of build and linter errors if you check the failing red jobs above. They're mostly from linting that if you check on the linting jobs will tell you exactly how to fix them
This pull request is in conflict. Could you fix it @pranavk-2003?
@pranavk-2003, 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).