[ahmedsleem109]

What this is

Both halves of #668, which are the same complaint about the same message: a
DiagnosticStatus carries more than the bridge was willing to use.

The numbers behind the fault (#668 part 1)

A sensor fusion node publishes rejected_fixes=37, nis=0.03. The bridge read those
key-values only to pick a fault code and dropped the rest, so the fault record said a node
complained but not what it saw.

A FAILED report now carries them. ReportFault.srv gains
diagnostic_msgs/KeyValue[] evidence - the type the bridge already receives, and
ros2_medkit_msgs already depends on diagnostic_msgs, so this costs no new dependency and
no conversion. The fault manager keeps them in the fault's freeze frame under x-reported
and GET /api/v1/apps/{app}/faults/{code} serves them:

$ curl -s .../faults/FUSION_DIVERGED | jq '.environment_data.snapshots[0].data | fromjson'
{"x-reported": {"rejected_fixes": "37", "nis": "0.03"}}

A field rather than a separate attach call. The issue left this open. The evidence is
part of the event: a second call can fail on its own and leave a fault with no numbers, or
numbers with no fault. It also keeps the door open to putting evidence on near-miss entries
later without another API change - not done here, and out of scope.

Four decisions inside that are worth looking at:

• Written on every FAILED report, not only at confirmation. A code that keeps approaching
  confirmation without reaching it is exactly the case the near-miss series exists for, and
  evidence written only at confirmation would miss it.
• x-reported, not a flat merge. The frame's other keys are topic names, always fully
  qualified and so always starting with /. A reserved key that cannot collide is what lets
  a reader tell a value the reporter asserted from one the fault manager sampled.
• The capture path had to change. snapshot_capture.cpp rebuilds the frame from the
  topics it sampled and writes it whole, so evidence reported before a confirm was silently
  erased by it. Which of the two writes lands last is a matter of when the fault confirmed,
  not of intent, so the capture path now carries the key across. There is a test for exactly
  this ordering.
• The read path had to change. The frame was served only when no snapshots remained (or
  retention was on), because a frame of sampled topic values duplicates the snapshots beside
  it. Evidence is not in them at all, so a frame carrying evidence is now always served.

Bounded per fault code: 32 entries, 512 characters per value. Over-long values are dropped
whole rather than truncated - half a number read back later is worse than a logged absence -
and drops are reported with a throttled warning. A key already stored can still be updated,
so a steady reporter at the bound can refresh its own numbers instead of freezing at whatever
the first report said.

STALE is a decision, not a fact (#668 part 2)

map_to_severity sent STALE to CRITICAL unconditionally, and CRITICAL bypasses debounce
(fault_storage.cpp:148), so a GPS that goes STALE in every tunnel confirmed a CRITICAL
fault on its first sample - the debounce settings an operator chose never got to run.

Two parameters:

stale_severity: "WARN"                      # default CRITICAL - unconfigured behaviour is unchanged
"stale_severity_overrides.gps": "WARN"      # name or prefix, longest match wins
"stale_severity_overrides.safety_chain": "CRITICAL"

Only STALE is configurable. WARN and ERROR are facts about the status; STALE is the only
level a node reaches by design, and it is the only one that needed an operator's judgement.

Prefix matching because diagnostic names are conventionally <component>: <check>, so a
component prefix covers every check it publishes without listing them. Longest match wins,
the same rule the fault manager's entity thresholds already use - one matching rule for an
operator to learn, not two.

A typo is ignored, not defaulted. An unparseable override is reported and skipped,
leaving stale_severity in force. Applying CRITICAL to a misspelling would hand back exactly
the immediate-confirm behaviour the operator was configuring their way out of, and it would
look like the configuration had been accepted.

One pre-existing doc bug fixed

docs/config/diagnostic-bridge.rst documented STALE as SEVERITY_ERROR (2). The bridge has
always sent CRITICAL. The table now says what the code does, with a warning explaining why
the difference matters.

Tests

• test_reported_evidence.cpp - 14 cases: the merge, provenance separation, later reports
  updating keys, both bounds and their exact boundary, an unreadable stored frame,
  "everything dropped writes no key", the capture-path preservation in both directions, and
  what the read path gates on.
• test_diagnostic_bridge.cpp - 10 new cases: the default, the global parameter, override
  beating default, prefix and longest-prefix matching, a non-CRITICAL default combined with a
  CRITICAL override, both unparseable paths, case-insensitivity, and the parser itself.
• test_reported_evidence.test.py - the node end to end: the issue's own example read back
  through GetFault, evidence stored while the fault is still PREFAILED, a later report
  updating, a report without evidence not erasing it, and a code that never carried any
  still getting no frame at all.
• REQ_INTEROP_109 and REQ_INTEROP_110 added, both traced by @verifies.

Existing bridge tests were updated for the signature change: map_to_severity is a method
now that STALE resolves through configuration.

Verified locally

ROS 2 Jazzy / Ubuntu 24.04. colcon test over ros2_medkit_diagnostic_bridge,
ros2_medkit_fault_manager and ros2_medkit_fault_reporter: 54 tests, 0 failures,
every linter included. Whole workspace built (--packages-skip ros2_medkit_opcua) because
ros2_medkit_msgs changed.

Numbering starts at REQ_INTEROP_109 to leave 107/108 to #690, which is open against the
same tree.

Closes #668

[bburda]

Thanks for this one too :) The description explains every decision, which made the review much easier, and the STALE severity code looks good to me.

There are many comments, so a short guide. The UTF-8 crash and the missing PostgreSQL catch can take the fault manager down, so I would start with those two. The race with the capture thread, the moving timestamp and the duplicated topic values all go away if the evidence moves to its own table. I am happy to discuss that design with you before you start on it.

A few things outside this diff also need an update:

• src/ros2_medkit_fault_manager/README.md (freeze-frame section and the retain_on_clear paragraph) and the capture() comment in snapshot_capture.hpp still say that a frame exists only with a capture config and is served once the snapshots are gone.
• docs/api/rest.rst, Snapshot Types, describes freeze_frame as data captured at confirmation and does not mention x-reported.
• src/ros2_medkit_diagnostic_bridge/design/index.rst still shows map_to_severity(level), the fixed STALE -> CRITICAL mapping and the "STALE as CRITICAL" section.
• src/ros2_medkit_fault_reporter/README.md and its design doc do not list the report() overload with evidence.
• src/ros2_medkit_msgs/README.md has no evidence row for ReportFault, and no note that reporters and the fault manager now have to be updated together.

Also, src/ros2_medkit_diagnostic_bridge/test/test_integration.test.py runs the real bridge with the real fault manager, but it still only checks STALE -> CRITICAL. Could you add a STALE status with an override there, against confirmation_threshold: -3, and an ERROR status whose values you read back with GetFault? A check through the gateway REST fault detail would also catch the broken docs example.

[ahmedsleem109]

Thanks for the detailed review, and for reproducing the UTF-8 crash.

I rebased on main and pushed 237d92a with the fixes that don't depend on the storage design:

• the UTF-8 crash
• the missing PostgreSQL catches
• the rename to x-medkit-reported and the key length limit
• the doc and comment updates, including the files outside the diff you listed
• removing REQ_INTEROP_109
• a new bridge launch test, test_stale_and_evidence.test.py

The new launch test runs the real bridge with the real fault manager at confirmation_threshold: -3, in its own file so the old tests keep their timing. It checks:

• STALE with an override is first PREFAILED at WARN, then confirms
• STALE without an override is CRITICAL and confirms at once
• ERROR values come back from GetFault
• a value that is not valid UTF-8

I left out the gateway REST check for now, because the REST shape changes with the new table. I'll add it with that change.

For the evidence table I'd like to agree on the design with you first. My proposal is in the thread on fault_manager_node.cpp (the race comment). The related comments will be fixed together once we agree.

[ahmedsleem109]

Thanks for the answers :) Pushed 1545c13, which moves the evidence to its own table as agreed and fixes the remaining comments. Each thread has the details.

I also added the gateway REST check I left out last time: ros2_medkit_integration_tests/test/features/test_reported_evidence_rest.test.py reports a fault with evidence and reads it back from GET /apps/{app}/faults/{code}, in the shape the docs show. The docs example now uses .data["x-medkit"].reported.

One small new parameter: snapshots.max_evidence_occurrences (default 10) bounds the kept occurrences when retain_on_clear is on.

I ran the fault_manager unit and launch tests, the diagnostic_bridge launch tests, the new gateway test and the linters on Jazzy, and the PostgreSQL storage tests against a real PostgreSQL server.

[ahmedsleem109]

@bburda ready for another look when you have time :)

[ahmedsleem109]

Rebased on main now that #690 is merged, and pushed 757cb53. The conflicts were only where both PRs added code next to each other (fault_manager_node.cpp and faults.rst), so nothing changed in this PR's logic. The commits are the same as before plus one small docs fix: two wrapped lines in src/ros2_medkit_diagnostic_bridge/design/index.rst broke the docs build.

Ran the fault_manager, diagnostic_bridge and PostgreSQL tests, the linters and the docs build again on the rebased branch.