Skip to content

refactor(dc_measurements): deepen the Mission adapter lifecycle and pending-record queue - #418

Merged
Minipada merged 3 commits into
jazzyfrom
refactor/mission-measurement-shared-modules
Aug 21, 2026
Merged

refactor(dc_measurements): deepen the Mission adapter lifecycle and pending-record queue#418
Minipada merged 3 commits into
jazzyfrom
refactor/mission-measurement-shared-modules

Conversation

@Minipada

@Minipada Minipada commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Summary

Architecture-review-driven refactor of dc_measurements' Mission Measurement adapter family (nav2 NavigateToPose/NavigateThroughPoses/FollowWaypoints, Open-RMF) plus Fault/Ros2ControlStatus — the hottest area of the repo by recent commit volume. All three review candidates are implemented.

1. Mission adapter lifecycle boilerplate

The four Mission adapters converge on one ADR-0010 Record shape but each duplicated the outcome vocabulary, UUID formatting, JSON record building, and bounded-id bookkeeping. Extracted into small, single-purpose modules under dc_measurements/include/dc_measurements/:

  • mission_outcome.hpp — one shared MissionOutcome enum + missionOutcomeName() + MissionStartFact, replacing four duplicated copies.
  • mission_registry.hppPrunedMissionMap<Active>, the bounded id → state bookkeeping (oldest-finished-first eviction, sequence counter) MissionNav2ThroughPosesCore and MissionOpenRmfCore had each reimplemented identically.
  • mission_record_json.hpp — shared missionStartJson()/missionEndJsonBase() builders, replacing MissionNav2/MissionNav2FollowWaypoints' static methods and the inline JSON blocks in the other two adapters.
  • mission_uuid.hpp — the UUID formatter duplicated between MissionNav2 and MissionNav2FollowWaypoints. MissionNav2ThroughPoses' distinct (non-dashed) mission_id format is kept as a separate function rather than unified, since collapsing the two would silently change an already-shipped adapter's mission_id format.

Bug fix riding along: MissionNav2ThroughPoses' status subscription was on plain QoS(10) while its two siblings use reliable().transient_local() for late-joiner correctness. It now matches — this drift was direct evidence the duplication was letting real bugs through.

Deliberately not done: merging MissionNav2Tracker/MissionFollowWaypointsTracker's state-machine internals. Once the above is factored out, what differs between them is each endMission()'s extra parameter (recoveries vs missed_waypoints) — a small enough remainder that a shared base would add indirection for little gain.

2. Pending-record queue

pending_record_queue.hpp — the capped deque + drop-oldest + throttled-warning pattern duplicated across all four Mission adapters plus Fault and Ros2ControlStatus (six call sites).

3. Shared Mission Record JSON Schema

The four mission_*.json schemas duplicated ~90% of their property definitions — the schema-level mirror of the C++ duplication above. mission_base.json now holds the shared property definitions (event, mission_id, sequence, outcome, reason, error_code, duration_sec); each adapter schema allOf/$refs it and adds only its own mission_type/extra-property/required/conditional rules.

Cross-file $ref needs a loader callback that nlohmann_json_schema_validator's default json_validator doesn't have (every existing schema in this codebase kept to same-file #/$defs/... refs, which is why this was never needed before — confirmed against the real library: without a loader, set_root_schema()/validate() throws "external schema reference ... needs loading, but no loader callback given"). Added makeSchemaFileLoader() to measurement.hpp, wired into both Measurement::validateSchema() overloads. Four test files each independently re-validate emitted Records via their own locally-constructed json_validator as a self-check; each needed the same loader wired in.

Verification

Public class names, method signatures, and Record JSON shapes are unchanged (aside from the QoS fix above and the schema restructuring, which is validation-semantics-preserving — see below).

This was built and tested for real against a container image with the actual ROS 2 Jazzy toolchain (not just reviewed by hand):

  • colcon build --packages-up-to dc_measurements: clean, zero errors, under -Wall -Wextra -Wpedantic -Werror -Wdeprecated.
  • colcon test --packages-select dc_measurements: 293 tests, 0 errors, 0 failures, 0 skipped.
  • The schema restructuring was additionally verified in isolation: 21 representative Records (valid and invalid) validated identically before/after the mission_base.json extraction, run directly against nlohmann_json_schema_validator.

Test plan

  • colcon build (dc_measurements) — verified in CI-equivalent container
  • colcon test — 293/293 passing, including all Mission/Fault/Ros2ControlStatus tests
  • prek run --all-files (clang-format / lint) — not run locally (no clang-format binary in this environment); please run in CI

🤖 Generated with Claude Code

https://claude.ai/code/session_01VoPZP5DPoJuuCWtctZAfBe

…ending-record queue

The four Mission Measurement adapters (nav2 NavigateToPose/NavigateThroughPoses/
FollowWaypoints, Open-RMF) converge on one ADR-0010 Record shape but each
duplicated the outcome vocabulary, UUID formatting, JSON record building, and
bounded id bookkeeping. Extract the genuinely shared pieces into small,
single-purpose modules and delegate to them:

- dc_measurements/mission_outcome.hpp: one shared MissionOutcome enum +
  missionOutcomeName() + MissionStartFact, replacing four duplicated copies.
- dc_measurements/mission_registry.hpp: PrunedMissionMap<Active>, the bounded
  id -> state bookkeeping (oldest-finished-first eviction, sequence counter)
  MissionNav2ThroughPosesCore and MissionOpenRmfCore had each reimplemented.
- dc_measurements/mission_record_json.hpp: shared missionStartJson()/
  missionEndJsonBase() builders, replacing MissionNav2/MissionNav2FollowWaypoints'
  static methods and the inline JSON blocks in the other two adapters.
- dc_measurements/mission_uuid.hpp: the UUID formatter duplicated between
  MissionNav2 and MissionNav2FollowWaypoints. MissionNav2ThroughPoses' distinct
  (non-dashed) mission_id format is kept as a separate function rather than
  unified -- collapsing the two would change an already-shipped adapter's
  mission_id format for anyone already querying on it.
- dc_measurements/pending_record_queue.hpp: the capped deque + drop-oldest +
  throttled-warning pattern duplicated across all four Mission adapters plus
  Fault and Ros2ControlStatus (six call sites total).

Also fixes a real correctness drift the duplication had let through:
MissionNav2ThroughPoses' status subscription was on plain QoS(10) while its two
siblings use reliable().transient_local() for late-joiner correctness; it now
matches.

Deliberately not merged: MissionNav2Tracker/MissionFollowWaypointsTracker's
state-machine internals -- once the above is factored out, what differs between
them is each endMission()'s extra parameter (recoveries vs missed_waypoints), a
small enough remainder that a shared base would add indirection for little gain.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VoPZP5DPoJuuCWtctZAfBe
Signed-off-by: David Bensoussan <d.bensoussan@proton.me>
@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.02564% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.60%. Comparing base (0ed5a64) to head (c148bc9).
⚠️ Report is 1 commits behind head on jazzy.

Files with missing lines Patch % Lines
...ments/include/dc_measurements/mission_registry.hpp 76.00% 6 Missing ⚠️
...asurements/include/dc_measurements/measurement.hpp 80.00% 3 Missing ⚠️
...s/include/dc_measurements/pending_record_queue.hpp 84.62% 2 Missing ⚠️
...ements/include/dc_measurements/mission_outcome.hpp 90.91% 1 Missing ⚠️
dc_measurements/plugins/measurements/fault.cpp 66.67% 1 Missing ⚠️
...ments/plugins/measurements/ros2_control_status.cpp 66.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            jazzy     #418      +/-   ##
==========================================
- Coverage   70.80%   70.60%   -0.19%     
==========================================
  Files         114      119       +5     
  Lines        7440     7363      -77     
==========================================
- Hits         5267     5198      -69     
+ Misses       2173     2165       -8     
Flag Coverage Δ
cpp-jazzy 70.60% <91.03%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Minipada and others added 2 commits August 21, 2026 04:41
… loader-backed $ref

The four Mission Measurement schemas (mission_nav2.json, mission_nav2_through_poses.json,
mission_nav2_follow_waypoints.json, mission_open_rmf.json) duplicated ~90% of their
property definitions -- the schema-level mirror of the C++ duplication the prior commit
addressed. Extract the shared property definitions (event, mission_id, sequence, outcome,
reason, error_code, duration_sec) into mission_base.json; each adapter schema now
allOf/$refs it and adds only its own mission_type/extra-property/required/conditional
rules on top. Verified against a real nlohmann_json_schema_validator that every
before/after pair of 21 representative Records (valid and invalid) validates identically.

Cross-file $ref needs a loader callback -- nlohmann_json_schema_validator's default
json_validator has none, and every existing schema in this codebase kept to same-file
"#/$defs/..." refs, which explains why this was never needed before. Without one,
set_root_schema()/validate() throws "external schema reference ... needs loading, but no
loader callback given" -- confirmed by hand against the real library before writing any
schema. Added makeSchemaFileLoader() to dc_measurements/measurement.hpp, resolving a $ref
against the directory the root schema was itself loaded from, and wired it into both
Measurement::validateSchema() overloads.

Four test files (test_measurement_mission_nav2{,_follow_waypoints,_through_poses}.cpp,
test_measurement_mission_open_rmf.cpp) each independently re-validate emitted Records
against a second, locally-constructed json_validator as a self-check; each needed the same
loader wired in, or that construction throws the same way production would have without
this commit's measurement.hpp fix.

Full colcon build + colcon test of dc_measurements (293 tests) verified green end to end
against a container image with the real ROS 2 Jazzy toolchain -- this and the prior commit
together are the first real compile/test run either has had.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VoPZP5DPoJuuCWtctZAfBe
Signed-off-by: David Bensoussan <d.bensoussan@proton.me>
CI's Format check flagged these files as unformatted.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VJktRnyZZSEDnp16Cr2crJ
Signed-off-by: David Bensoussan <d.bensoussan@proton.me>
@Minipada
Minipada merged commit 439b584 into jazzy Aug 21, 2026
10 checks passed
@github-actions
github-actions Bot deleted the refactor/mission-measurement-shared-modules branch August 21, 2026 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant