Skip to content

Commit dac0b72

Browse files
committed
docs(opcua,#386): document HEALED as internal-only + ShelvingState precision (bburda review)
Three documentation gaps surfaced by bburda's review on PR #387: (1) **HEALED design-doc fiction.** ``alarm_state_machine.hpp`` advertised ``SovdAlarmStatus::Healed`` as a first-class lifecycle state and the design table listed it as rule 5a outcome, but ``ReportHealed`` is a no-op in ``OpcuaPlugin::on_event_alarm`` so the state never surfaces in ``/faults`` (status stays ``CONFIRMED`` until ``Cleared`` fires). Header docstring on ``SovdAlarmStatus`` and ``AlarmAction`` now spells out which values are externally visible vs internal-only, and the design doc has a new ``.. note::`` block explaining why ``EVENT_PASSED`` on every latch would defeat Part 9's mandatory ack/confirm contract via fault_manager's debounce engine. Future ``STATUS_LATCHED`` extension on ``ros2_medkit_msgs/msg/Fault`` is called out as the path to make this visible externally; until then, a known UX gap is documented. (2) **ShelvingState rule 3 precision.** Doc said ``ShelvingState != Unshelved`` -> Suppressed. Code (poller :382-389) only treats the specific NodeIds ``i=2930`` (TimedShelved) and ``i=2932`` (OneShotShelved) as suppressing - a null/unset/unknown ``Id`` is interpreted as Unshelved (the deliberate code path with a "some servers leave the optional field uninitialized" comment). Doc table now lists the explicit NodeId set + the null/unset behaviour. (3) **Plugin docs not in published aggregator.** The 162 lines of ``design/index.rst`` and the CHANGELOG entry added in this PR did not render on https://selfpatch.github.io/ros2_medkit/ because: - ``docs/changelog.rst`` aggregator did not ``.. include::`` the opcua-plugin CHANGELOG. - ``docs/design/index.rst`` toctree was missing ``ros2_medkit_opcua/index``. Fix: - Append the opcua CHANGELOG include to ``docs/changelog.rst`` (matches the precedent of every other in-tree package). - Add ``docs/design/ros2_medkit_opcua/index.rst`` as a thin stub that ``.. include::``-s the in-package design doc (cleaner than the existing duplicated-file pattern under ``docs/design/ros2_medkit_graph_provider/index.rst`` - duplicated files drift; an include directive cannot). - Add ``ros2_medkit_opcua/index`` to the design toctree (alphabetically between ``ros2_medkit_msgs`` and ``ros2_medkit_param_beacon``). No code change; the state machine still has 4 internal lifecycle states and the bridge still keeps HEALED entries at ``status=CONFIRMED``. Local verify: 27/27 test_alarm_state_machine.
1 parent 8b3ee9d commit dac0b72

5 files changed

Lines changed: 61 additions & 9 deletions

File tree

docs/changelog.rst

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,5 @@ This page aggregates changelogs from all ros2_medkit packages.
3131
.. include:: ../src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/CHANGELOG.rst
3232

3333
.. include:: ../src/ros2_medkit_plugins/ros2_medkit_graph_provider/CHANGELOG.rst
34+
35+
.. include:: ../src/ros2_medkit_plugins/ros2_medkit_opcua/CHANGELOG.rst

docs/design/index.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ This section contains design documentation for the ros2_medkit project packages.
1616
ros2_medkit_integration_tests/index
1717
ros2_medkit_linux_introspection/index
1818
ros2_medkit_msgs/index
19+
ros2_medkit_opcua/index
1920
ros2_medkit_param_beacon/index
2021
ros2_medkit_serialization/index
2122
ros2_medkit_sovd_service_interface/index
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
.. Aggregator stub - actual content lives in
2+
.. ``src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst`` and is
3+
.. pulled in here so the published docs match what package maintainers
4+
.. edit alongside the code (bburda review on PR #387).
5+
6+
.. include:: ../../../src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst

src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -236,12 +236,17 @@ Decision order, first match wins:
236236
+-----+--------------------------------------+---------------------------------------+
237237
| 2 | ``EnabledState == false`` | clear if was active, else no-op |
238238
+-----+--------------------------------------+---------------------------------------+
239-
| 3 | ``ShelvingState != Unshelved`` | clear if was active, else no-op |
239+
| 3 | ``ShelvingState/CurrentState/Id`` | clear if was active, else no-op. |
240+
| | in {``TimedShelved`` (i=2930), | A null/unset/unknown ``Id`` is |
241+
| | ``OneShotShelved`` (i=2932)} | treated as ``Unshelved`` (some |
242+
| | | servers leave the optional field |
243+
| | | uninitialized). |
240244
+-----+--------------------------------------+---------------------------------------+
241245
| 4 | ``ActiveState == true`` | ``CONFIRMED`` (idempotent) |
242246
+-----+--------------------------------------+---------------------------------------+
243-
| 5a | ``ActiveState == false`` and | ``HEALED`` (latched, awaiting ack) |
244-
| | not (``Acked`` and ``Confirmed``) | |
247+
| 5a | ``ActiveState == false`` and | internal ``HEALED`` state; ``/faults``|
248+
| | not (``Acked`` and ``Confirmed``) | still shows ``CONFIRMED`` (see note |
249+
| | | below) |
245250
+-----+--------------------------------------+---------------------------------------+
246251
| 5b | ``ActiveState == false``, | ``CLEARED`` |
247252
| | ``Acked == true``, | |
@@ -254,6 +259,26 @@ lifecycle is driven entirely by Active / Acked / Confirmed. The SOVD
254259
``PREFAILED`` state has no native equivalent and is reserved for the
255260
threshold-polling pre-trigger path.
256261

262+
.. note::
263+
264+
**HEALED is internal-only.** Rule 5a transitions the bridge's internal
265+
``SovdAlarmStatus`` to ``Healed`` and emits the ``ReportHealed`` action,
266+
but ``OpcuaPlugin::on_event_alarm`` deliberately treats that action as a
267+
no-op. The reason: ``ros2_medkit_msgs/srv/ReportFault`` has only
268+
``EVENT_FAILED`` / ``EVENT_PASSED`` verbs, and routing ``EVENT_PASSED``
269+
on every latch would feed the fault_manager debounce engine - which has
270+
its own statistical ``HEALED`` semantics (``healing_threshold`` cycles
271+
of PASSED events) and may auto-clear the fault, defeating Part 9's
272+
mandatory ack/confirm contract. Until ``ros2_medkit_msgs/msg/Fault``
273+
gains a ``STATUS_LATCHED`` (or equivalent) value, ``/faults`` keeps
274+
``status=CONFIRMED`` for a latched alarm; the next ``Cleared`` (rule 5b)
275+
removes the entry.
276+
277+
This is a known UX gap: an operator looking at ``/faults`` cannot
278+
distinguish "alarm physically active" from "alarm physically cleared,
279+
awaiting confirm". The ack/confirm SOVD operations work end-to-end; the
280+
visibility limitation is tracked as a follow-up.
281+
257282
Severity mapping
258283
----------------
259284

src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/alarm_state_machine.hpp

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,23 @@
1919

2020
namespace ros2_medkit_gateway {
2121

22-
/// SOVD fault statuses surfaced by the OPC-UA AlarmCondition bridge.
22+
/// Internal lifecycle states the AlarmCondition bridge tracks per condition.
23+
///
24+
/// **Public-facing vs internal**: not every value here surfaces in the SOVD
25+
/// ``/faults`` payload (bburda review on PR #387):
26+
/// - ``Suppressed``: condition exists but is administratively masked
27+
/// (``EnabledState=false`` or shelved); no entry in ``/faults``.
28+
/// - ``Confirmed``: visible as ``status=CONFIRMED`` in ``/faults``.
29+
/// - ``Cleared``: removed from ``/faults`` via ``clear_fault``.
30+
/// - ``Healed``: **internal-only**. Means "ActiveState=false but operator
31+
/// workflow incomplete (ack and/or confirm pending)". This is NOT exposed
32+
/// as a separate ``/faults`` status - the bridge keeps the entry at
33+
/// ``CONFIRMED`` until ``Cleared`` fires (see ``ReportHealed`` below).
34+
/// ``ros2_medkit_msgs/srv/ReportFault`` has no HEALED verb today; routing
35+
/// ``EVENT_PASSED`` here would let ``fault_manager``'s debounce engine
36+
/// auto-clear the fault, defeating Part 9's mandatory ack/confirm
37+
/// contract. A future ``STATUS_LATCHED`` extension to the message would
38+
/// make this state externally visible; until then it stays internal.
2339
///
2440
/// PREFAILED is reserved for the threshold-polling pre-trigger path and is
2541
/// never produced by ``AlarmStateMachine::compute`` from native event input -
@@ -28,11 +44,13 @@ enum class SovdAlarmStatus { Suppressed, Confirmed, Healed, Cleared };
2844

2945
/// Action the poller should take after running the state machine on one event.
3046
///
31-
/// ``ReportConfirmed`` / ``ReportHealed`` correspond to ``send_report_fault``
32-
/// (FAILED event_type for Confirmed; PASSED-but-still-tracked for Healed).
33-
/// ``ClearFault`` issues ``clear_fault`` against fault_manager. ``NoOp`` means
34-
/// the event was redundant (same as last_known_status) and we suppress
35-
/// downstream noise.
47+
/// ``ReportConfirmed`` triggers ``send_report_fault`` (FAILED event_type).
48+
/// ``ClearFault`` issues ``clear_fault`` against fault_manager.
49+
/// ``ReportHealed`` is currently a no-op in ``OpcuaPlugin::on_event_alarm`` -
50+
/// see ``SovdAlarmStatus::Healed`` above for the rationale and the future
51+
/// ``STATUS_LATCHED`` extension that would make this surface externally.
52+
/// ``NoOp`` means the event was redundant (same as last_known_status) and we
53+
/// suppress downstream noise.
3654
enum class AlarmAction { NoOp, ReportConfirmed, ReportHealed, ClearFault };
3755

3856
/// Inputs from a single AlarmConditionType event payload.

0 commit comments

Comments
 (0)