fix(ota_demo): green up the plugin gtest gate and the demo narrative - #72
fix(ota_demo): green up the plugin gtest gate and the demo narrative#72bburda wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses two CI failures in the OTA demo by (1) explicitly declaring the OTA update plugin gtest suite as ROS/DDS-free under the new ros2_medkit per-package DDS domain gating, and (2) making the Nav2 “broken_lidar” demo failure deterministic by making the global costmap map-only so the controller (not the planner) is the expected point of failure.
Changes:
- Updated
ota_update_plugintest registration to declare it needs no DDS domain viamedkit_test_needs_no_domain(). - Removed the global costmap obstacle layer so global planning relies on the static map + inflation, leaving lidar-reactive behavior to the local costmap.
- Added explanatory documentation in the Nav2 params to clarify why the configuration is map-only and how it stabilizes the demo narrative.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| demos/ota_nav2_sensor_fix/ros2_packages/ota_nav2_sensor_fix_demo/config/nav2_params.yaml | Removes the global obstacle layer and documents the rationale to make the demo failure mode consistent. |
| demos/ota_nav2_sensor_fix/ota_update_plugin/CMakeLists.txt | Declares the gtest target as not requiring a DDS domain to satisfy the ros2_medkit test gate. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The suite drives the catalog client, the operation dispatcher and the plugin against doubles and never creates a ROS entity, so it takes no DDS domain. Say so at the registration site, which is what the per-package domain allocation gate reads. Guarded on the target, because ament_add_gtest registers nothing when the executable was not created, and setting a property on a test that does not exist is a configure error rather than the skip ament intended.
…roller is what fails The phantom sector the regressed lidar overlays is fixed to the robot and reports a constant 0.22 m, so the global costmap painted it around the robot's own position and navfn gave up before the controller ran out of trajectories. Which of the two failed first depended on the geometry at onset, and the log bridge watches controller_server, not planner_server, so the supporting LOG_ fault on controller-server appeared or did not. The global costmap now ignores scan returns below 0.35 m. The phantom never reaches the planner, real obstacles still do - which matters, because the committed map is 62% unknown and eleven warehouse models stand outside it, so global planning cannot fall back on the static map alone. Nav2 fails where the README says it does: the controller cannot make progress, logs it, and navigate_to_pose aborts.
…not any error The check was "controller-server has at least one fault". The log bridge promotes every controller_server ERROR above its severity floor, so a TF error or a lifecycle error passed it just as well, and nothing in the suite could tell "the controller stalled on the phantom" from "the controller logged something". Match the fault message against the two errors the controller raises when it cannot move: the progress checker and the controller patience. A goal that aborts anywhere else in Nav2 now fails this assertion instead of sliding through on an unrelated error.
af1029a to
13dda69
Compare
| # phantom_range_m and below the distance at which real obstacles | ||
| # matter to a global plan; anything that close is the local | ||
| # costmap's problem, and it reads the same scan with no floor. | ||
| obstacle_min_range: 0.35 |
There was a problem hiding this comment.
Minor, and only about the comment: obstacle_min_range is a range measured at the sensor, but the comment reads as if 0.35 were a distance from the robot ("below the distance at which real obstacles matter to a global plan").
The laser sits ~0.268 m ahead of base_footprint (broken_lidar_node.cpp:47-48), so this floor blinds the global costmap out to ~0.62 m from robot centre in the forward direction, against robot_radius: 0.45 on the same costmap (:233). That leaves a ~0.17 m band where a real obstacle is outside the footprint and invisible to global planning.
Not arguing with the value - global plans are routing, the local costmap has no floor, and it catches that band. Just that one clause saying the number is a sensor range would save the next person doing the arithmetic I just did to check it.
Same place, worth knowing: phantom_range_m is never set anywhere, only the C++ default at broken_lidar_node.cpp:53. So the invariant is a default in one package against a literal in another, with the smoke test as the only backstop. Naming the parameter in the comment already helps; raising it past 0.35 in a launch override would silently restore the original flake.
| # If the global costmap ever starts marking the phantom again, planner_server | ||
| # aborts the goal first, controller_server logs neither of these, and this is | ||
| # the assertion that goes red. | ||
| CONTROLLER_STALL_MSG="Failed to make progress|Controller patience exceeded" |
There was a problem hiding this comment.
Minor: this might be a touch too tight in the other direction.
controller_server logs six other exception types at ERROR (checked upstream controller_server.cpp, jazzy): InvalidController :635, ControllerTFError :641, NoValidControl :651, InvalidPath :661, ControllerTimedOut :665, generic ControllerException :669. Excluding TF and lifecycle errors is exactly the point and is right - but NoValidControl is also the controller saying it cannot move, and it would not match.
With failure_tolerance: 0.3 the expected terminal really is PatienceExceeded, so the risk is low and this is reasoned rather than observed. Adding No valid control to the alternation hardens it without giving back any of the discrimination the change exists for.
For the record the two you picked check out: throw nav2_core::FailedToMakeProgress("Failed to make progress") at :580 caught and RCLCPP_ERROR-logged at :647, throw nav2_core::PatienceExceeded("Controller patience exceeded") at :623 logged at :657. Both reach the bridge at ERROR.
Description
CI on main fails in two OTA demo jobs. This fixes both.
plugin-tests fails on every run. ros2_medkit now arms a per-package DDS domain gate on
find_package(ros2_medkit_cmake), andtest_ota_update_pluginis registered with a plainament_add_gtest:The suite uses doubles for the catalog client, the process runner and the plugin, and never creates a ROS entity, so it needs no domain. It now says so with
medkit_test_needs_no_domain(), guarded on the target becauseament_add_gtestregisters nothing when the executable was not created.ota-demo-narrative fails about one run in three, always on the same assertion: no supporting
LOG_*fault onapps/controller-server. The reason is which nav2 node fails first. The phantom sector thatbroken_lidaroverlays is fixed to the robot and reports a constant 0.22 m, so the global costmap painted it around the robot's own position. On a bad run navfn gave up before the controller ran out of trajectories:controller_serverlogged nothing at all in that run, and the log bridge only watchescontroller_server, so the supporting fault never appeared.The global costmap now ignores scan returns below 0.35 m, which is above the phantom range and below the distance at which an obstacle matters to a global plan. The phantom no longer reaches the planner and real obstacles still do. Dropping the obstacle layer instead would have been wrong:
maps/warehouse.pgmis 62% unknown and eleven models fromwarehouse.sdfstand in cells the map does not record, so global planning cannot fall back on the static map alone. The local costmap reads the same scan with no floor, so the controller still stalls.The third commit closes a gap in the test itself. The assertion was "controller-server has at least one fault", which any
controller_serverERROR satisfies, so it could not tell a phantom stall from an unrelated error. It now matches the message against the two errors the controller raises when it cannot move.Related Issue
None.
Checklist
Verification
Plugin gtest, built the way CI does (
docker build --target ota-plugin-test) against ros2_medkit main:Demo narrative, four full cycles (compose down, compose up, smoke), all
21 passed, 0 failed. Container logs from every run show the controller as the node that fails and no planner failure:The new assertion was checked against inputs that must fail it, not only against a passing run:
Controller patience exceededInvalid path, Path is empty.smoke_test_ota.shwas not re-run locally. It drives the/updatesAPI and the process swap, never navigation, and it is green on every CI run including the ones where the narrative failed.