Conversation
Signed-off-by: Francisco Martín Rico <fmrico@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the navmap-localizer initialization issue, add missing test manifest dependencies, satisfy the EasyNavigation#120 prerequisite, and build/export the octomap filter.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prepares EasyNav planners, map managers, localizers, and controllers for lifecycle reconfiguration by making parameter initialization repeatable and adding regression coverage.
Changes:
- Guard parameter declarations and loading during repeated initialization.
- Add and enable reconfiguration tests.
- Update UKF parameter loading for repeated initialization.
Approval is blocked by unresolved initialization, dependency, prerequisite, and filter build-integration findings.
File summaries
| File | Reviewed change |
|---|---|
planners/easynav_simple_planner/tests/simple_planner_reconfigure_tests.cpp |
Adds a repeated-initialization test; requires the EasyNavigation#120 base-class change. |
planners/easynav_simple_planner/tests/CMakeLists.txt |
Builds the test; requires the rclcpp_lifecycle test manifest dependency. |
planners/easynav_simple_planner/src/easynav_simple_planner/SimplePlanner.cpp |
Guards parameter declarations. |
planners/easynav_simple_planner/CMakeLists.txt |
Enables tests. |
planners/easynav_navmap_planner/tests/navmap_planner_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
planners/easynav_navmap_planner/tests/CMakeLists.txt |
Builds the test; requires the rclcpp_lifecycle test manifest dependency. |
planners/easynav_navmap_planner/src/easynav_navmap_planner/AStarPlanner.cpp |
Guards parameter declarations. |
planners/easynav_navmap_planner/CMakeLists.txt |
Enables tests. |
planners/easynav_costmap_planner/tests/costmap_planner_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
planners/easynav_costmap_planner/tests/CMakeLists.txt |
Builds the test; requires the rclcpp_lifecycle test manifest dependency. |
planners/easynav_costmap_planner/src/easynav_costmap_planner/CostmapPlanner.cpp |
Guards parameter declarations. |
planners/easynav_costmap_planner/CMakeLists.txt |
Enables tests. |
maps_managers/easynav_simple_maps_manager/tests/simple_mapsmanager_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
maps_managers/easynav_simple_maps_manager/tests/CMakeLists.txt |
Registers the test. |
maps_managers/easynav_simple_maps_manager/src/easynav_simple_maps_manager/SimpleMapsManager.cpp |
Guards parameter declarations. |
maps_managers/easynav_routes_maps_manager/tests/routes_mapsmanager_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
maps_managers/easynav_routes_maps_manager/tests/CMakeLists.txt |
Registers the test. |
maps_managers/easynav_octomap_maps_manager/src/easynav_octomap_maps_manager/filters/InflationFilter.cpp |
Guards inflation parameters; the filter must be built and exported for runtime effect. |
maps_managers/easynav_navmap_maps_manager/tests/navmap_mapsmanager_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
maps_managers/easynav_navmap_maps_manager/tests/CMakeLists.txt |
Registers the test. |
maps_managers/easynav_navmap_maps_manager/src/easynav_navmap_maps_manager/NavMapMapsManager.cpp |
Guards manager and filter parameters. |
maps_managers/easynav_navmap_maps_manager/src/easynav_navmap_maps_manager/filters/InflationFilter.cpp |
Guards filter parameters. |
maps_managers/easynav_costmap_maps_manager/tests/costmap_mapsmanager_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
maps_managers/easynav_costmap_maps_manager/tests/CMakeLists.txt |
Registers the test. |
maps_managers/easynav_costmap_maps_manager/src/easynav_costmap_maps_manager/filters/InflationFilter.cpp |
Guards inflation parameters. |
maps_managers/easynav_costmap_maps_manager/src/easynav_costmap_maps_manager/CostmapMapsManager.cpp |
Guards manager and filter parameters. |
maps_managers/easynav_bonxai_maps_manager/tests/CMakeLists.txt |
Builds the test; requires the rclcpp_lifecycle test manifest dependency. |
maps_managers/easynav_bonxai_maps_manager/tests/bonxai_maps_manager_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
maps_managers/easynav_bonxai_maps_manager/src/easynav_bonxai_maps_manager/BonxaiMapsManager.cpp |
Guards parameter declarations. |
maps_managers/easynav_bonxai_maps_manager/CMakeLists.txt |
Enables tests. |
localizers/easynav_simple_localizer/tests/simple_localizer_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
localizers/easynav_simple_localizer/tests/CMakeLists.txt |
Registers the test. |
localizers/easynav_simple_localizer/src/easynav_simple_localizer/AMCLLocalizer.cpp |
Guards parameter declarations. |
localizers/easynav_navmap_localizer/tests/navmap_localizer_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
localizers/easynav_navmap_localizer/tests/CMakeLists.txt |
Registers the test. |
localizers/easynav_navmap_localizer/src/easynav_navmap_localizer/AMCLLocalizer.cpp |
Guards parameter declarations; independently initialized defaults are needed before distributions are constructed. |
localizers/easynav_gps_localizer/tests/gps_localizer_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
localizers/easynav_gps_localizer/tests/CMakeLists.txt |
Registers the test. |
localizers/easynav_gps_localizer/src/easynav_gps_localizer/GpsLocalizer.cpp |
Guards optional parameters. |
localizers/easynav_fusion_localizer/tests/fusion_localizer_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
localizers/easynav_fusion_localizer/tests/CMakeLists.txt |
Registers the test. |
localizers/easynav_fusion_localizer/src/easynav_fusion_localizer/ukf_wrapper.cpp |
Makes UKF parameter loading repeatable. |
localizers/easynav_fusion_localizer/src/easynav_fusion_localizer/FusionLocalizer.cpp |
Guards GPS parameters. |
localizers/easynav_costmap_localizer/tests/costmap_localizer_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
localizers/easynav_costmap_localizer/tests/CMakeLists.txt |
Registers the test. |
localizers/easynav_costmap_localizer/src/easynav_costmap_localizer/AMCLLocalizer.cpp |
Guards parameter declarations. |
controllers/easynav_vff_controller/tests/vff_controller_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
controllers/easynav_vff_controller/tests/CMakeLists.txt |
Builds the test; requires the ament_cmake_gtest test manifest dependency. |
controllers/easynav_vff_controller/src/easynav_vff_controller/VffController.cpp |
Guards parameter declarations. |
controllers/easynav_vff_controller/CMakeLists.txt |
Enables tests; requires the ament_cmake_gtest test manifest dependency. |
controllers/easynav_simple_controller/src/easynav_simple_controller/SimpleController.cpp |
Guards parameter declarations. |
controllers/easynav_serest_controller/tests/serest_controller_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
controllers/easynav_serest_controller/tests/CMakeLists.txt |
Builds the test. |
controllers/easynav_serest_controller/src/easynav_serest_controller/SerestController.cpp |
Guards parameter declarations. |
controllers/easynav_serest_controller/CMakeLists.txt |
Enables tests. |
controllers/easynav_regulated_pp_controller/tests/regulated_pp_controller_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
controllers/easynav_regulated_pp_controller/tests/CMakeLists.txt |
Builds the test. |
controllers/easynav_regulated_pp_controller/src/easynav_regulated_pp_controller/RegulatedPurePursuitController.cpp |
Makes parameter loading repeatable. |
controllers/easynav_mppi_controller/tests/mppi_controller_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
controllers/easynav_mppi_controller/tests/CMakeLists.txt |
Builds the test. |
controllers/easynav_mppi_controller/src/easynav_mppi_controller/MPPIController.cpp |
Guards parameter declarations. |
controllers/easynav_mppi_controller/CMakeLists.txt |
Enables tests. |
controllers/easynav_mpc_controller/tests/mpc_controller_reconfigure_tests.cpp |
Adds a repeated-initialization test. |
controllers/easynav_mpc_controller/tests/CMakeLists.txt |
Builds the test. |
controllers/easynav_mpc_controller/src/easynav_mpc_controller/MPCController.cpp |
Guards parameter declarations. |
controllers/easynav_mpc_controller/CMakeLists.txt |
Enables tests. |
Review details
Suppressed comments (4)
controllers/easynav_vff_controller/CMakeLists.txt:70
- The newly enabled test target requires ament_cmake_gtest, but controllers/easynav_vff_controller/package.xml does not declare it as a test_depend. rosdep may therefore omit the package and make this REQUIRED lookup fail in an isolated build; add the manifest dependency.
find_package(ament_cmake_gtest REQUIRED)
add_subdirectory(tests)
controllers/easynav_vff_controller/tests/CMakeLists.txt:1
- This newly enabled test target requires
ament_cmake_gtest, but this package'spackage.xmlhas no<test_depend>ament_cmake_gtest</test_depend>. In a clean package build the test dependency may not be installed, so add it to the manifest along with enabling this target.
find_package(ament_cmake_gtest REQUIRED)
maps_managers/easynav_octomap_maps_manager/src/easynav_octomap_maps_manager/filters/InflationFilter.cpp:193
- This filter is not compiled into
${PROJECT_NAME}:easynav_octomap_maps_manager/CMakeLists.txtcomments outsrc/easynav_octomap_maps_manager/filters/InflationFilter.cpp(and its plugin export). Consequently this lifecycle fix has no runtime effect; include the source/plugin in the build if this filter is intended to be supported.
planners/easynav_simple_planner/tests/simple_planner_reconfigure_tests.cpp:46 - This new test directly calls MethodBase::initialize() twice, but the currently referenced easynav_core implementation still declares
<plugin>.rt_freqand<plugin>.frequnconditionally before on_initialize(), so the second call throws before this plugin's new guards run. Please make the EasyNavigation#120 base-class change an explicit prerequisite/available dependency, or this regression test will fail in a normal build.
- Files reviewed: 66/66 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!node->has_parameter(plugin_name + ".num_particles")) { | ||
| node->declare_parameter<int>(plugin_name + ".num_particles", 100); | ||
| node->declare_parameter<double>(plugin_name + ".initial_pose.x", 0.0); | ||
| node->declare_parameter<double>(plugin_name + ".initial_pose.y", 0.0); | ||
| node->declare_parameter<double>(plugin_name + ".initial_pose.yaw", 0.0); |
| @@ -0,0 +1,11 @@ | |||
| find_package(ament_cmake_gtest REQUIRED) | |||
| find_package(rclcpp REQUIRED) | |||
| find_package(rclcpp_lifecycle REQUIRED) | |||
| @@ -0,0 +1,11 @@ | |||
| find_package(ament_cmake_gtest REQUIRED) | |||
| find_package(rclcpp REQUIRED) | |||
| find_package(rclcpp_lifecycle REQUIRED) | |||
| @@ -0,0 +1,11 @@ | |||
| find_package(ament_cmake_gtest REQUIRED) | |||
| find_package(rclcpp REQUIRED) | |||
| find_package(rclcpp_lifecycle REQUIRED) | |||
| @@ -0,0 +1,11 @@ | |||
| find_package(ament_cmake_gtest REQUIRED) | |||
| find_package(rclcpp REQUIRED) | |||
| find_package(rclcpp_lifecycle REQUIRED) | |||
Hi,
This PR ensures that al the plugins in EasyNav are ready to any LifeCycle transition.
Related to EasyNavigation/EasyNavigation#120
I hope you find it useful