navigation2
navigation2 copied to clipboard
Add DynamicParameterPatterns to pose_pogress_checker plugin
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 how this change was tested
No additional test added.
Future work that may be required in bullet points
Apply dynamic parameter patters to the rest of nav2_controller.
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'm not sure how you want to structure this for the plugins of nav2_controller. Before applying changes to the rest of the nav2_controller plugins some feedback would be great (naming, namespaces, general structure).
For trivial nodes / objects like this, I think it would be fine to define their parameter handler in the scope of its header file (or even inside of the class itself to be a specialization for the class). I don't think we can do that for the main planner/controller/smoother/etc that have alot of parameters and validations going on - plus some of the execution of the servers are in different threads as the dynamic parameter callbacks so we need mutex locking. But, for small things like this it seems overkill.
@SteveMacenski thank you very much. I have made the changes. If it fits now, I would apply them to the rest of the nav2_controller plugins in this PR.
Codecov Report
Attention: Patch coverage is 88.88889% with 5 lines in your changes missing coverage. Please review.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| nav2_controller/plugins/pose_progress_checker.cpp | 88.63% | 5 Missing :warning: |
| Files with missing lines | Coverage Δ | |
|---|---|---|
| .../nav2_controller/plugins/pose_progress_checker.hpp | 100.00% <100.00%> (ø) |
|
| nav2_controller/plugins/pose_progress_checker.cpp | 91.37% <88.63%> (-5.29%) |
:arrow_down: |
... and 8 files with indirect coverage changes
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
Let me know when you want me to take a look again, you can request a new review in the github UX (or just say so in a comment)
* Maybe better to return a shared pointer to the parameters rather than a raw pointer?
Changed that, a raw pointer for this is also present in some other controllers/plugins.
* There are going to be situations where dynamic parameters are on another executor from the main task server thread so we need to lock the changes / access. * Maybe atomic for the Parameters object would be sensible in that case, have a lock for accessing values on the value object, or do similar to what we do now and lock the dynamic callbacks and the action server callbacks (param_handler->getLock()) to generally lock up the resources during the entire scope of execution to not keep locking/unlocking
I have too look into this in more detail. After a first glimps i don't get the implementation of the mutex locks that are present in the other param handlers.
@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).
@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).
This pull request is in conflict. Could you fix it @Nils-ChristianIseke?