Skip to content

fix(ota_demo): green up the plugin gtest gate and the demo narrative #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bburda wants to merge 3 commits into
base: main
Choose a base branch
from
+45 −7
Open

fix(ota_demo): green up the plugin gtest gate and the demo narrative #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d16b63
build(ota_demo): declare the plugin gtest ROS-free for the domain gate
bburda Aug 13, 2026
c741a3c
fix(ota_demo): keep the phantom out of the global costmap so the cont…
bburda Aug 13, 2026
13dda69
test(ota_demo): assert the supporting fault is the controller stall, …
bburda Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions demos/ota_nav2_sensor_fix/ota_update_plugin/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,25 @@ install(DIRECTORY include/ DESTINATION include)

if(BUILD_TESTING)
find_package(ament_cmake_gtest REQUIRED)
include(ROS2MedkitTestDomain)
ament_add_gtest(test_ota_update_plugin
test/test_operation_dispatcher.cpp
test/test_catalog_client.cpp
test/test_plugin_smoke.cpp
)
target_link_libraries(test_ota_update_plugin ota_update_plugin_core)
target_include_directories(test_ota_update_plugin PRIVATE src)
# The suite creates no ROS entity: the catalog client, the operation
# dispatcher and the plugin are exercised against doubles, and the one test
# that uses the real ProcessRunner spawns a path that does not exist. Nothing
# here touches DDS, so the test needs no domain of its own.
#
# Guarded, because ament_add_gtest registers nothing when the executable was
# not created, and declaring a property on a test that does not exist is a
# hard configure error rather than the skip ament intended.
if(TARGET test_ota_update_plugin)
medkit_test_needs_no_domain(test_ota_update_plugin)
endif()
endif()

ament_package()
14 changes: 13 additions & 1 deletion demos/ota_nav2_sensor_fix/ros2_packages/ota_nav2_sensor_fix_demo/config/nav2_params.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,19 @@ global_costmap:
raytrace_max_range: 3.0
raytrace_min_range: 0.0
obstacle_max_range: 2.5
obstacle_min_range: 0.0
# Global planning ignores anything closer than this, and that decides
# WHICH Nav2 node fails when the lidar regresses. The phantom sector
# broken_lidar overlays is fixed to the robot and reports a constant
# range (phantom_range_m, 0.22 m at the laser), so without this floor
# the global costmap paints it around the robot's own position and
# navfn aborts with "failed to create plan" before the controller
# runs out of trajectories - and the log bridge watches
# controller_server, not planner_server, so the supporting fault
# would go missing. The floor must therefore stay above
# 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

@mfaferek93 mfaferek93 Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

static_layer:
plugin: "nav2_costmap_2d::StaticLayer"
map_subscribe_transient_local: True
Expand Down
26 changes: 20 additions & 6 deletions tests/smoke_test_demo_narrative.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
# scan_sensor_node is running broken_lidar_node; fixed_lidar_3_0_1 is
# NOT yet registered (boot catalog holds only the bad update).
# 2. send-goal.sh -> ACTION_NAVIGATE_TO_POSE_ABORTED reaches CONFIRMED on
# bt-navigator, and controller-server picks up a supporting LOG_* fault.
# bt-navigator, and controller-server picks up a supporting LOG_* fault
# whose message is the controller's own stall, not just any error.
# 3. Fault detail (bt-navigator) has environment_data.snapshots >= 1, and
# the rosbag bulk-data download returns a non-empty MCAP body.
# 4. publish-fix.sh -> fixed_lidar_3_0_1 appears in /updates (SOVD
Expand Down Expand Up @@ -68,6 +69,17 @@ CONTROLLER_ENTITY="apps/controller-server"
# controller-server's LOG_CONTROLLER_SERVER_* code is content-hashed (derived
# from the log message), so it is never matched by exact code - only by
# "does this entity have any fault at all" (see fault_present with code="").
#
# The message is matched instead, because "any fault on controller-server" is
# too weak for the one assertion that says WHICH Nav2 node failed. The log
# bridge promotes every controller_server ERROR at or above its severity floor,
# so a TF error or a lifecycle error would satisfy a bare count check. These two
# messages are the controller saying it cannot move: the progress checker
# (movement_time_allowance) and the controller patience (failure_tolerance).
# 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"

@mfaferek93 mfaferek93 Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# --- Helpers built on top of smoke_lib.sh's api_get/poll_until -------------

Expand Down Expand Up @@ -231,12 +243,14 @@ else
"fault never reached CONFIRMED within 60s - either nav2 didn't accept the goal or the action-status bridge is broken"
fi

echo " Waiting for a supporting LOG_* fault on ${CONTROLLER_ENTITY} (max 60s)..."
if poll_until "/${CONTROLLER_ENTITY}/faults" '.items | length > 0' 60; then
pass "supporting LOG_* fault present on ${CONTROLLER_ENTITY}"
echo " Waiting for a supporting LOG_* stall fault on ${CONTROLLER_ENTITY} (max 60s)..."
if poll_until "/${CONTROLLER_ENTITY}/faults" \
".items[] | select(.description | test(\"${CONTROLLER_STALL_MSG}\"))" \
60; then
pass "supporting LOG_* fault present on ${CONTROLLER_ENTITY} and reports the controller stall"
else
fail "supporting LOG_* fault present on ${CONTROLLER_ENTITY}" \
"no fault appeared within 60s - either nav2 didn't stall or the log bridge is broken"
fail "supporting LOG_* fault present on ${CONTROLLER_ENTITY} and reports the controller stall" \
"no fault matching '${CONTROLLER_STALL_MSG}' within 60s - either nav2 aborted somewhere other than the controller, or the log bridge is broken"
fi

# ---------------------------------------------------------------------
Expand Down
Loading