fault_manager: debounce a fault code by the code, not only by who reported it - #690
Open
ahmedsleem109 wants to merge 1 commit into
Open
ahmedsleem109 wants to merge 1 commit into
ahmedsleem109 wants to merge 1 commit into
Conversation
…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
ahmedsleem109
force-pushed
the
feat/per-fault-code-debounce-thresholds
branch
from
September 15, 2026 22:41
b48a005 to
8fe77fa
Compare
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.
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 inhandle_report_faultis gone.The layer (#275)
fault_thresholds.config_filetakes a map of fault code to the same three fields anentity override carries:
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 onlyconfirmation_thresholdstill 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
MOTORmust not captureMOTOR_OVERHEATthe way/sensorscaptures/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 anentity 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:
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_thresholdbut not thehealing 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 paperedover.
Comparison ignores
auto_confirm_after_sec, which is global-only and therefore identicalfor every source by construction.
One file rename
entity_threshold_resolver.{hpp,cpp}becomethreshold_resolver.{hpp,cpp}. Both resolverslive there now and share the YAML field parsing and the sign correction (a
confirmation_thresholdwritten positive, ahealing_thresholdwritten negative), so thetwo loaders cannot drift apart.
EntityThresholdResolveritself 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 bothlayers 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_107andREQ_INTEROP_108added todocs/requirements/specs/faults.rst,both traced by
@verifiestags throughgenerate_verification.py.Verified locally
ROS 2 Jazzy / Ubuntu 24.04,
colcon testonros2_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 dependentpackage.
Closes #275
Closes #276