fault_manager: document which debounce lever fits which reporter, and test it - #643
Draft
bburda wants to merge 1 commit into
Draft
fault_manager: document which debounce lever fits which reporter, and test it#643bburda wants to merge 1 commit into
bburda wants to merge 1 commit into
Conversation
… test it The debounce counter only moves when an event arrives, so confirmation_threshold and healing_threshold only work for a reporter that keeps sending FAILED while a condition is still there. The docs recommended confirmation_threshold: -3 with healing_threshold: 3 to everyone. For a reporter that sends one FAILED per raise and one clear per de-assert the second event never comes. The fault stops at PREFAILED and never confirms, and ListFaults with an empty status filter returns CONFIRMED only, so nobody sees it. Healing breaks the same way: it needs healing_threshold minus confirmation_threshold consecutive PASSED events and only one is sent, so a confirmed fault stays CONFIRMED until someone calls ~/clear_fault. auto_confirm_after_sec is the lever for that kind of reporter and already works. It holds the first FAILED in PREFAILED and confirms it only if it is still there when the window closes, so a condition that recovers never reaches an operator. The docs listed the parameter but never said what it is for. Add an integration test driving the real node over the real services with the event counts such a reporter sends: one FAILED per raise, one PASSED per clear. It also pins the case where a condition clears inside the window, which is what stops a config from passing by only delaying a false alarm. No production code change.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR clarifies fault-manager debounce/healing parameter guidance for edge-triggered (one-event-per-transition) reporters and adds an integration test that exercises the auto_confirm_after_sec + healing_threshold: 0 path end-to-end (SQLite backend), without changing production code.
Changes:
- Document when count-based thresholds work vs when time-based auto-confirm is required (README + Sphinx docs).
- Add a launch-based integration test that drives report/list services with edge-triggered event patterns.
- Register the new integration test in
ros2_medkit_fault_manager’s CMake.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ros2_medkit_fault_manager/test/integration/test_debounce_and_healing.test.py | New launch integration test covering edge-triggered debounce + timer confirm + healing behavior. |
| src/ros2_medkit_fault_manager/README.md | Adds guidance on selecting debounce/healing levers based on reporter behavior. |
| src/ros2_medkit_fault_manager/CMakeLists.txt | Registers the new launch test with an appropriate timeout. |
| docs/config/fault-manager.rst | Adds an “important” note explaining edge-triggered reporter implications and recommended settings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+246
to
+249
| def test_exit_code(self, proc_info, fault_manager_node): | ||
| launch_testing.asserts.assertExitCodes( | ||
| proc_info, allowable_exit_codes=[0, -2, -15], process=fault_manager_node | ||
| ) |
bburda
marked this pull request as draft
August 27, 2026 15:39
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.
Pull Request
Summary
The debounce docs told everyone to use
confirmation_threshold: -3withhealing_threshold: 3.That works only for a reporter that keeps sending FAILED while a condition is still there. For a
reporter that sends one FAILED per raise and one clear per de-assert, the second event never
arrives, so the fault stays PREFAILED and never confirms. The default
ListFaultsfilter returnsCONFIRMED only, so such a fault is never seen at all. Healing breaks the same way in the other
direction: it needs
healing_threshold - confirmation_thresholdconsecutive PASSED events and onlyone is ever sent, so a confirmed fault stays CONFIRMED until someone calls
~/clear_fault.auto_confirm_after_secis the right lever for that kind of reporter and it already works, butnothing said so. This PR documents which lever fits which reporter, and adds the integration test
that was missing on that path.
No production code changed. All four parameters already behave correctly and both storage backends
agree on the arithmetic.
Issue
Type
Testing
New integration test
test_debounce_and_healing.test.pydrives the real node over the realservices with SQLite storage, and sends the event counts a one-event-per-transition reporter really
sends: one FAILED per raise, one PASSED per clear. It pins five things:
auto_confirm_after_secwith nobody actingThe fourth case is the one that keeps the setting honest. Without it a config could pass by only
delaying a false alarm instead of filtering it.
The test was written first and run against the parameter defaults, where 4 of the 5 cases fail:
Case 3 passes on the defaults, which is intended. It is the guard that stops anyone from "fixing"
the other four by making faults invisible.
With the settings under test it is 5/5 green.
To run it:
colcon test --packages-select ros2_medkit_fault_manager \ --ctest-args -R test_debounce_and_healing --parallel-workers 1Checklist