fault_manager: debounce a fault code by the code, not only by who reported it by ahmedsleem109 · Pull Request #690 · selfpatch/ros2_medkit

ahmedsleem109 · 2026-09-15T20:02:23Z

What this is

A debounce counter belongs to the fault code. A per-entity override is picked by the
reporting source. So when two entities report one code they share one counter under two
policies, and whichever report happens to arrive decides the transition - the other
entity's policy is bypassed, silently. That is #275, and the bypass being invisible is
#276. They are one mechanism seen from two sides, so they are fixed together here.

The TODO(#276) that sat on the resolve site in handle_report_fault is gone.

The layer (#275)

fault_thresholds.config_file takes a map of fault code to the same three fields an
entity override carries:

# fault_thresholds.yaml
MOTOR_OVERHEAT:
  confirmation_threshold: -5    # five events, whoever reports them
  healing_threshold: 10

LIDAR_FAIL:
  confirmation_threshold: -1
  healing_threshold: 1

Three layers, each applied on top of the last - global, then the entity override matching
source_id, then the fault code's own - so the priority the issue asks for,
fault_code > source_id > global, holds field by field: a code that pins only
confirmation_threshold still takes its healing fields from whichever entity matched.

Matching on the code is exact. A fault code is an identifier, not a path, so an entry for
MOTOR must not capture MOTOR_OVERHEAT the way /sensors captures /sensors/lidar;
there is a test pinning that.

A code that is not listed resolves exactly as it did before. The layer is opt-in and
changes nothing on its own - the existing per-entity suite passes untouched.

A separate file rather than a section in the entity file. The entity file's top level
is a map of entity prefixes, so a fault_thresholds: key in it would be ambiguous with an
entity literally named that, and would silently change meaning for anyone who already has
one. The issue allowed either; this way nothing existing can be reinterpreted.

The warning (#276)

Where no override settles a code, the node says so rather than let the bypass stay
invisible:

[WARN] Fault code 'OVERHEAT' is debounced two ways: '/powertrain/motor/left' resolves
confirmation=-5 healing_enabled=false healing=10, '/sensors/lidar/front' resolves
confirmation=-1 healing_enabled=true healing=1. The debounce counter belongs to the fault
code, so whichever source reports decides the transition and the other policy is bypassed.
Give the code an entry in fault_thresholds.config_file to settle it for every source.

It names both sources, both resolved policies and the remedy, because a warning that only
said "conflict" would leave the operator to reconstruct which two configs met.

Once per fault code, not throttled: the node keeps the first policy resolved for a
code and the source that produced it, and warns the first time a different one arrives. A
warning per report is a warning per fault event on a busy robot; a throttle would repeat
forever for a configuration that is not going to change until someone restarts the node.
The witness is one small entry per fault code seen, the same cardinality as the fault store
itself.

It is computed from the resolved policies, not from the presence of an entry, which
matters for a partial override: a code that pins confirmation_threshold but not the
healing fields is only half settled, and the warning still fires on what still differs.
That was not a design I anticipated - the integration test caught it, and it is now a case
of its own (test_04_a_half_pinned_code_is_still_reported) rather than something papered
over.

Comparison ignores auto_confirm_after_sec, which is global-only and therefore identical
for every source by construction.

One file rename

entity_threshold_resolver.{hpp,cpp} become threshold_resolver.{hpp,cpp}. Both resolvers
live there now and share the YAML field parsing and the sign correction (a
confirmation_threshold written positive, a healing_threshold written negative), so the
two loaders cannot drift apart. EntityThresholdResolver itself is unchanged - same class,
same behaviour, same tests.

Tests

test/test_fault_code_thresholds.cpp - 21 cases: resolution, exact-not-prefix matching,
the three-layer merge, debounce_policy_equal, YAML loading (valid, missing, malformed,
non-map root, non-map entry, sign correction), and the storage-level proof that lidar no
longer confirms the motor's code early.
test/integration/test_fault_code_thresholds_integration.test.py - the node with both
layers configured: a code pinned over its entity, one code debouncing alike from two
entities, an unlisted code keeping entity behaviour, the half-pinned case, the warning
firing exactly once, and - after shutdown, when the whole output is in hand - no warning
for a settled code or for any code only one source reports. That last absence is the
regression guard: a witness compared against the global config instead of against the
first report would warn on every ordinary single-reporter robot.
REQ_INTEROP_107 and REQ_INTEROP_108 added to docs/requirements/specs/faults.rst,
both traced by @verifies tags through generate_verification.py.

Verified locally

ROS 2 Jazzy / Ubuntu 24.04, colcon test on ros2_medkit_fault_manager: 32/32 pass,
linters included (clang_format, flake8, pep257, copyright, cppcheck, lint_cmake, xmllint).
clang-tidy run on both changed sources - no finding in the new code. Whole workspace built
(--packages-skip ros2_medkit_opcua) to confirm the header rename breaks no dependent
package.

Closes #275
Closes #276

bburda

Thanks for contribution :)

bburda · 2026-10-08T11:06:28Z

+      continue;
+    }
+
+    auto fields = parse_override_fields(item.second, kEntityLogName, entry.prefix);


Could you wrap this loop in a try/catch for YAML::Exception that returns an empty vector, as the header says? load_override_map only wraps LoadFile, so a bad value now throws out of the node constructor. On this branch an entity file with confirmation_threshold: three ends in terminate called after throwing an instance of 'YAML::TypedBadConversion<int>' at startup. On the base commit the same file logs bad conversion and the node starts. The fault-code loop below has the same problem. Please also add a case with a bad value next to MalformedYamlReturnsEmpty.

Fixed in a9cb43b. Both loops are now inside a try/catch for YAML::Exception. A bad value logs an error and the file is ignored, like before. Added a BadValueReturnsEmpty test for both loaders.

bburda · 2026-10-08T11:06:28Z


+  // Load per-fault_code threshold overrides (optional). Applied after the entity layer, so a
+  // code listed here debounces the same whichever source reports it.
+  auto fault_thresholds_file = declare_parameter<std::string>("fault_thresholds.config_file", "");


The startup pass at line 273 (reclassify_healed_as_cleared) checks only the global healing_enabled and runs before this file is loaded, so every restart undoes a per-code healing_enabled: true. I ran it with SQLite, global healing off and X.HEAL: {confirmation_threshold: -1, healing_enabled: true, healing_threshold: 1}: the fault reaches HEALED, and after a restart it is CLEARED, with a startup_reclassify entry in the audit log. Could you load this file before that pass and make the pass use each code's own healing_enabled? The PostgreSQL backend on main has the same pass. A launch test with SQLite and a restart would cover it.

Fixed in a9cb43b. The startup pass now runs after both override files are loaded, and it checks each code's own healing_enabled. This is done in the memory, SQLite and PostgreSQL backends.

One side effect: if global healing is on and a code sets healing_enabled: false, that code's HEALED faults are now cleared at startup too. Let me know if you'd prefer to keep the pass only for global healing off.

Tests: a storage test in the SQLite and PostgreSQL suites, and a new launch test, test_fault_code_healing_restart.test.py, that starts the node twice on the same SQLite file. The launch test fails with the old behavior.

bburda · 2026-10-08T11:06:28Z

+  // Resolve the debounce config: global, then the entity override matching
+  // source_id, then the fault code's own override.
+  auto resolved_config = resolve_config(request->source_id, request->fault_code);
+  warn_on_conflicting_debounce_policy(request->fault_code, request->source_id, resolved_config);


This adds a witness before the store has seen the report. The store ignores a PASSED for a code with no fault (fault_storage.cpp:133), but the map still gets an entry, so the comment on debounce_policy_witness_ ("bounded by the same thing the fault store is") is not true. diagnostic_bridge sends PASSED for every OK diagnostic, which adds one entry per diagnostic name. Could you call this after report_fault_event, and only when fault_after exists?

Moved in a9cb43b. It now runs after report_fault_event, and only when fault_after exists. Also updated the comment on debounce_policy_witness_.

bburda · 2026-10-08T11:06:28Z

+  FaultCodeThresholdResolver() = default;
+
+  /// Construct with a list of fault-code overrides. A code repeated in the list
+  /// keeps its first entry; YAML loading cannot produce one, a caller can.


yaml-cpp accepts duplicate keys, so YAML loading can produce one. With DUP.CODE listed twice (-2, then -7), the loader logs Loaded 2 fault-code threshold entries, the node logs Loaded 1, and -2 is used with no message. Could you log a warning that names the code when load_from_yaml sees it a second time, and update this comment?

Done in a9cb43b. load_from_yaml now warns with the code name and skips the second entry, so the loader and the node show the same count. Updated the comment and added a test.

bburda · 2026-10-08T11:06:28Z

+              "Fault code '%s' is debounced two ways: '%s' resolves confirmation=%d healing_enabled=%s healing=%d, "
+              "'%s' resolves confirmation=%d healing_enabled=%s healing=%d. The debounce counter belongs to the fault "
+              "code, so whichever source reports decides the transition and the other policy is bypassed. Give the "
+              "code an entry in fault_thresholds.config_file to settle it for every source.",


For a code that already has a partial entry, like SHARED.PARTIAL in your test, this asks the operator to add an entry they already have. Could you name the three fields to pin, for example "Pin confirmation_threshold, healing_enabled and healing_threshold for this code in fault_thresholds.config_file"? Please copy the final text into the sample in docs/config/fault-manager.rst (line 314) too. It ends with different words now.

Changed in a9cb43b to "Pin confirmation_threshold, healing_enabled and healing_threshold for this code in fault_thresholds.config_file." The docs sample uses the same text now.

bburda · 2026-10-08T11:06:28Z

+.. code-block:: text
+
+   motor  (confirmation_threshold=-5) reports OVERHEAT -> counter=-1, PREFAILED
+   lidar  (confirmation_threshold=-1) reports OVERHEAT -> counter=-2, CONFIRMED


clamp_debounce_counter keeps the counter inside the band of the reporting policy, so with lidar's -1 the second report gives counter=-1. Could you change the example?

Fixed in a9cb43b, it now says counter=-1.

bburda · 2026-10-08T11:06:28Z

+        self._report('CONFLICTING.CODE', LIDAR)
+        self._report('CONFLICTING.CODE', MOTOR)
+        text = _output_text(proc_output, fault_manager_node)
+        self.assertEqual(text.count("Fault code 'CONFLICTING.CODE'"), 1)


This count can pass when the node warns twice, because proc_output may not have read the last warning yet. I removed it->second.warned || from the guard: the node warned twice in all 5 runs, and this test passed in 3 of them. Could you move the count into TestShutdown, where the whole output is available, and check there that the message names both sources and both policies?

Moved to TestShutdown in a9cb43b. It also checks that the message names both sources and both policies.

bburda · 2026-10-08T11:06:28Z

+   has no override, the resolved per-entity or global configuration shall apply unchanged.
+
+.. req:: Conflicting Debounce Policies Are Reported
+   :id: REQ_INTEROP_108


Could you drop REQ_INTEROP_108 and its @verifies tags? This list tracks SOVD interoperability, and 108 describes a ROS log line. The tags also do not reach the tests that check the warning: generate_verification.py only matches def test_x(self):, so the tags on test_04, test_05 and test_an_overridden_code_never_warns are dropped. 108 is then linked only to unit tests that never call warn_on_conflicting_debounce_policy.

Removed REQ_INTEROP_108 and its @verifies tags in a9cb43b.

bburda · 2026-10-08T11:06:29Z

+  /// Warn once per fault code when two sources debounce it under different
+  /// policies. The counter belongs to the fault code and the entity override to
+  /// the source, so the report that arrives decides the transition and the other
+  /// source's policy is silently bypassed (issue #276).


Could you remove the issue numbers from code comments? We keep them in commit messages and the PR text. Same in test_fault_code_thresholds.cpp (lines 199 and 222) and the integration test (lines 23-24 and 176).

Removed in a9cb43b, in all the places you listed.

bburda · 2026-10-08T11:06:29Z

-is therefore not by itself the distance to confirmation for the fault as a whole.
+is therefore not by itself the distance to confirmation for the fault as a whole. Giving the code
+an entry in `fault_thresholds.config_file` makes the two agree again: a fault-code override
+resolves the same for every source, so the recorded threshold is the fault's own. The node warns


This is true only when the entry sets confirmation_threshold. An entry with only healing fields keeps the recorded threshold per source. Could you write "an entry that sets confirmation_threshold" here?

Changed in a9cb43b to "an entry that sets confirmation_threshold".

…ho reported it A debounce counter belongs to the fault code, while a per-entity override is picked by the reporting source. Two entities reporting one code therefore share one counter under two policies, and whichever report arrives decides the transition - the other entity's policy is bypassed without a trace. Add the layer that settles it. `fault_thresholds.config_file` takes a map of fault code to the same three fields an entity override carries, matched exactly on the code, and applied on top of whatever the entity layer produced: fault code > source_id > global, field by field. A code that is not listed resolves exactly as before, so the layer is opt-in and changes nothing on its own. Where no override settles a code, say so rather than let the bypass stay invisible. The node keeps the first policy resolved for each code and warns once per code when a later source resolves a different one, naming both sources, both policies and the remedy. Once per code, because a warning per report is a warning per fault event on a busy robot. An override that names only some fields settles only those, and the warning still fires on what still differs - it is computed from the resolved policies, not from the presence of an entry. entity_threshold_resolver.{hpp,cpp} become threshold_resolver.{hpp,cpp}: both resolvers now live there, sharing the YAML field parsing and the sign correction, so a change to one cannot drift from the other. Closes selfpatch#275 Closes selfpatch#276

- A bad value in a threshold YAML file is logged and the file is ignored, instead of throwing out of the node constructor. - Warn when a fault code is listed twice in the fault thresholds file. The first entry is used. - The startup pass that turns HEALED into CLEARED now runs after the override files are loaded and checks each code's own healing_enabled, so a restart no longer clears a code that still heals. Done for the memory, SQLite and PostgreSQL backends. - Record a fault code's debounce policy only after the store has the fault, so PASSED reports for unknown codes do not grow the map. - The conflict warning names the three fields to pin. - Check the "warned once" count after shutdown, when all output is read. - Drop REQ_INTEROP_108 and its @verifies tags, and remove issue numbers from code comments. - Fix the docs example (the counter is clamped to -1) and the README wording about which entries settle the recorded threshold.

ahmedsleem109 · 2026-10-09T21:21:00Z

Thanks for the review :)

I rebased on main and pushed a9cb43b, which fixes all ten comments. The main changes:

a bad YAML value no longer crashes the node at startup
the startup HEALED to CLEARED pass now checks each code's own healing_enabled, with a restart test
the debounce policy is only recorded once the fault is stored

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

ahmedsleem109 mentioned this pull request Sep 15, 2026

diagnostic_bridge: keep the numbers behind a fault, and stop confirming every planned STALE #691

Open

ahmedsleem109 force-pushed the feat/per-fault-code-debounce-thresholds branch from b48a005 to 8fe77fa Compare September 15, 2026 22:41

bburda self-requested a review October 6, 2026 11:59

bburda reviewed Oct 8, 2026

View reviewed changes

ahmedsleem109 added 2 commits October 8, 2026 20:51

ahmedsleem109 force-pushed the feat/per-fault-code-debounce-thresholds branch from 8fe77fa to a9cb43b Compare October 9, 2026 21:20

Conversation

ahmedsleem109 commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this is

The layer (#275)

The warning (#276)

One file rename

Tests

Verified locally

Uh oh!

bburda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmedsleem109 commented Oct 9, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ahmedsleem109 commented Sep 15, 2026 •

edited

Loading