fix(ota_demo): green up the plugin gtest gate and the demo narrative - #72
Merged
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.
bburda
force-pushed
the
fix/ota-demo-ci-domain-gate-and-narrative
branch
from
August 13, 2026 16:17
af1029a to
13dda69
Compare
mfaferek93
reviewed
Aug 13, 2026
…en the stall match obstacle_min_range is measured at the laser, which sits ~0.268 m ahead of base_footprint, so 0.35 is ~0.62 m ahead of centre against robot_radius 0.45 - about 0.17 m in which a real obstacle is outside the footprint and still unseen by global planning. The comment read as if the number were a distance from the robot. Say which frame it is in, name the band, and say that the local costmap covers it. The stall match gains NoValidControl, which carries DWB's own "Could not find a legal trajectory". It is the controller saying it cannot move just as much as the other two, and it reaches ERROR when failure_tolerance is zero; above zero the controller retries and logs it at WARN, so today it widens the assertion against a config change rather than against the run we expect.
action_status_bridge fixes a fault's source on the first status message it sees for an action, and the action status topic is transient-local, so a bridge that subscribes while a goal is already in flight gets a latched sample at once - before the graph has resolved the publisher's node name. The source then stays /navigate_to_pose instead of /bt_navigator and the fault never reaches the bt-navigator entity, which is where this test looks for it. Two CI runs show exactly that source on the reported fault. demo.launch.py starts the bridges on a 15 s timer while send-goal.sh retries until nav2 accepts, so on a fast boot the goal wins that race. Hold the goal until the bridge says it is watching the action, so its first status message arrives seconds into its life rather than milliseconds.
mfaferek93
approved these changes
Aug 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.