Raise Exceptions on Hardware I/O to Set Record Alarms#389
Open
shihab-dls wants to merge 16 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAttribute write failures now propagate, and EPICS CA/PVA write and command handlers report failures through operation errors and major alarm states. Typed PVA alarm models and datatype-aware posting are added, with tests covering the updated behavior. ChangesEPICS error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)EPICS CA failure flowsequenceDiagram
participant Client
participant EPICS_CA
participant AttrW
participant EPICS_Record
Client->>EPICS_CA: write PV or command PV
EPICS_CA->>AttrW: put attribute or run method
AttrW-->>EPICS_CA: exception
EPICS_CA->>EPICS_Record: set current value and MAJOR_ALARM
EPICS PVA failure flowsequenceDiagram
participant Client
participant WritePvHandler
participant AttrW
participant PVA
Client->>WritePvHandler: put value
WritePvHandler->>AttrW: await put
AttrW-->>WritePvHandler: exception
WritePvHandler->>PVA: post major alarm
WritePvHandler-->>Client: op.done(error)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
shihab-dls
force-pushed
the
investigate_error_prop
branch
from
July 15, 2026 14:06
25e95c9 to
b0466d2
Compare
shihab-dls
changed the title
shihab-dls
force-pushed
the
investigate_error_prop
branch
from
July 15, 2026 14:08
b0466d2 to
b2b5e9b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
==========================================
+ Coverage 91.25% 91.41% +0.16%
==========================================
Files 72 72
Lines 2892 2935 +43
==========================================
+ Hits 2639 2683 +44
+ Misses 253 252 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
shihab-dls
force-pushed
the
investigate_error_prop
branch
from
July 15, 2026 15:51
e2fb84c to
48791cc
Compare
shihab-dls
marked this pull request as ready for review
July 15, 2026 16:43
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/transports/epics/pva/test_p4p.py (1)
215-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using an
async deftest function.Since other tests in this file are async (e.g.,
test_command_method), you can define this test asasync defto avoid manually interacting with the event loop. This is more idiomatic and prevents potentialRuntimeErrorissues withget_event_loop()in Python 3.12+.♻️ Proposed refactor to make the test natively async
-def test_alarms_set_if_put_fails(caplog): +async def test_alarms_set_if_put_fails(caplog): `@dataclass` class SimpleAttributeIORef(AttributeIORef): pass class SimpleAttributeIO(AttributeIO[DType_T, SimpleAttributeIORef]): async def send(self, attr: AttrW[DType_T, SimpleAttributeIORef], value): raise ValueError("Failed") class SomeController(Controller): a = AttrW(Int(), io_ref=SimpleAttributeIORef()) controller = SomeController(ios=[SimpleAttributeIO()]) pv_prefix = str(uuid4()) fastcs = make_fastcs(pv_prefix, controller) async def put_pvs(): await asyncio.sleep(0.1) ctxt = Context("pva") await ctxt.put(f"{pv_prefix}:A", 1) - serve = asyncio.ensure_future(fastcs.serve(interactive=False)) - try: - asyncio.get_event_loop().run_until_complete( - asyncio.wait_for(asyncio.gather(serve, put_pvs()), timeout=1) - ) - except RuntimeError as exc: - assert str(exc) == "Exception raised during put operation: ValueError('Failed')" - serve.cancel() + serve = asyncio.create_task(fastcs.serve(interactive=False)) + try: + await asyncio.wait_for(asyncio.gather(serve, put_pvs()), timeout=1) + except RuntimeError as exc: + assert str(exc) == "Exception raised during put operation: ValueError('Failed')" + finally: + serve.cancel()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/transports/epics/pva/test_p4p.py` around lines 215 - 245, Convert test_alarms_set_if_put_fails to an async test and await the concurrent serve and put_pvs tasks directly, following the pattern used by test_command_method. Remove the manual get_event_loop().run_until_complete and wait_for usage while preserving the existing timeout, exception assertion, and serve cancellation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyproject.toml`:
- Around line 37-40: Update the epicsca dependency list in pyproject.toml to
replace the softioc Git master reference with a released softioc version
specifier, preserving the existing pvi dependency and ensuring installs use the
stable published package.
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 231-239: Update both write PV and command PV callbacks in
src/fastcs/transports/epics/ca/ioc.py at lines 231-239 and 291-299: bind caught
exceptions explicitly, log each with the existing logger and relevant PV context
before setting the MAJOR_ALARM record state, and avoid blind exception catches.
---
Nitpick comments:
In `@tests/transports/epics/pva/test_p4p.py`:
- Around line 215-245: Convert test_alarms_set_if_put_fails to an async test and
await the concurrent serve and put_pvs tasks directly, following the pattern
used by test_command_method. Remove the manual
get_event_loop().run_until_complete and wait_for usage while preserving the
existing timeout, exception assertion, and serve cancellation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3502c74e-caf1-455e-af90-0199f1affa9b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.tomlsrc/fastcs/attributes/attr_w.pysrc/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/pva/_pv_handlers.pytests/example_softioc.pytests/test_attributes.pytests/transports/epics/ca/test_softioc_system.pytests/transports/epics/pva/test_p4p.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fastcs/transports/epics/pva/_pv_handlers.py (1)
51-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not clear alarms before the hardware put succeeds.
The new default
p4p_alarm_states()is posted beforeawait self._attr_w.put(cast_value), so an existing major alarm is temporarily cleared while I/O is still pending. A stalled or cancelled operation can leave the PV falsely healthy; post the value/reset alarm only in the success path, or preserve the current alarm until the operation completes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fastcs/transports/epics/pva/_pv_handlers.py` around lines 51 - 64, Move the normal value/alarm post in the handler around the put operation so it occurs only after await self._attr_w.put(cast_value) succeeds. Preserve the existing alarm state while the put is pending or fails, and keep the exception path’s error alarm posting and op.done(error=...) behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 234-235: Update the write PV on_update callback at
src/fastcs/transports/epics/ca/ioc.py lines 234-235 to bind the caught exception
and log its details before setting the MAJOR alarm. Apply the same explicit
exception binding and logging in the command PV wrapped_method callback at lines
290-291, preserving the existing alarm behavior.
---
Outside diff comments:
In `@src/fastcs/transports/epics/pva/_pv_handlers.py`:
- Around line 51-64: Move the normal value/alarm post in the handler around the
put operation so it occurs only after await self._attr_w.put(cast_value)
succeeds. Preserve the existing alarm state while the put is pending or fails,
and keep the exception path’s error alarm posting and op.done(error=...)
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f2f9465-f2a4-4206-a3bb-21b55a1f826e
📒 Files selected for processing (4)
src/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/pva/_pv_handlers.pysrc/fastcs/transports/epics/pva/pvi.pysrc/fastcs/transports/epics/pva/types.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fastcs/transports/epics/pva/_pv_handlers.py`:
- Around line 192-199: Update _set_alarm to avoid mutating alarm_states["alarm"]
in place by copying the nested alarm dictionary. Only remove "status" from that
copy for Table or Waveform datatypes, then call pv.post with one positional,
properly nested dictionary containing the value and alarm structure; preserve
the existing alarm_states structure for other datatypes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e97e013-1f54-4c51-b55d-88e09ab22a43
📒 Files selected for processing (1)
src/fastcs/transports/epics/pva/_pv_handlers.py
olliesilvester
left a comment
olliesilvester
left a comment
There was a problem hiding this comment.
Not finished the review just yet but some initial comments
olliesilvester
left a comment
olliesilvester
left a comment
There was a problem hiding this comment.
I don't have permission to actually approve anymore, but I would have
Left a bunch of comments but the fix works, thanks!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 351-363: The _run_and_set_alarm exception path must preserve
command failures instead of silently swallowing them. Bind the caught exception
and log it with the relevant record/PV context, while retaining the MAJOR_ALARM
update; either narrow the handler to the project-specific hardware-I/O exception
or explicitly justify the deliberate broad CA boundary.
In `@src/fastcs/transports/epics/pva/types.py`:
- Around line 138-151: The scalar record-payload callsites in
cast_to_p4p_value() must serialize the P4PAlarmState returned by
p4p_alarm_states() before merging it into record_fields. Update both numeric
alarm-check and untyped scalar payload paths to dump the model into the nested
{"alarm": {...}} dictionary expected by Value(...), while leaving other payload
handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 32fc1095-ca3d-46d1-b075-3a3b7013fcba
📒 Files selected for processing (4)
src/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/pva/_pv_handlers.pysrc/fastcs/transports/epics/pva/pvi.pysrc/fastcs/transports/epics/pva/types.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fastcs/transports/epics/pva/_pv_handlers.py (1)
60-65: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPublish the failure alarm before completing the operation.
op.done(error=...)currently returns the failed put response before_post_with_alarm_states()publishes the major-alarm state, so a client can immediately read/reset the PV with the reset alarm still visible. Post the failure state first, then complete the operation.Proposed fix
except Exception as e: error_msg = f"Exception raised during put operation: {e!r}" - op.done(error=error_msg) alarm_states = p4p_alarm_states(Severity.MAJOR, Status.RECORD, error_msg) # Raise alarm on failed put _post_with_alarm_states(pv, datatype, raw_value, alarm_states) + op.done(error=error_msg)[data_integrity_and_inspection]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fastcs/transports/epics/pva/_pv_handlers.py` around lines 60 - 65, Reorder the failed-put handling in the exception path so _post_with_alarm_states publishes the MAJOR alarm before op.done(error=error_msg) completes the operation. Keep the existing error message and alarm-state construction unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/transports/epics/pva/test_p4p.py`:
- Around line 233-234: Update the test around Context and the failed put to read
or monitor ${pv_prefix}:A after the operation, then assert the published alarm
severity, status, and message in addition to the existing exception assertion.
Use the test’s established alarm fields and expected MAJOR/error values.
- Around line 236-242: Update the test’s serve task cleanup so cancellation
always occurs in a finally block, including when the assertion or timeout fails.
After calling serve.cancel(), drive the task to completion by awaiting it and
handle the expected cancellation, ensuring the FastCS/PVA server task cannot
leak between tests.
- Around line 231-234: Update put_pvs to wait for a bounded PVA readiness signal
before creating the Context and issuing the put; replace the zero-duration sleep
with the existing server/transport readiness mechanism, or expose one if needed.
Ensure the wait completes only after P4P providers are initialized while
retaining a timeout so startup failures do not hang the test.
---
Outside diff comments:
In `@src/fastcs/transports/epics/pva/_pv_handlers.py`:
- Around line 60-65: Reorder the failed-put handling in the exception path so
_post_with_alarm_states publishes the MAJOR alarm before
op.done(error=error_msg) completes the operation. Keep the existing error
message and alarm-state construction unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0977cde3-5e67-4315-9785-8bff74e8affa
📒 Files selected for processing (5)
src/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/pva/_pv_handlers.pysrc/fastcs/transports/epics/pva/types.pytests/example_softioc.pytests/transports/epics/pva/test_p4p.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/example_softioc.py
- src/fastcs/transports/epics/ca/ioc.py
- src/fastcs/transports/epics/pva/types.py
shihab-dls
force-pushed
the
investigate_error_prop
branch
from
July 24, 2026 15:26
0fcb4b3 to
179a49d
Compare
Contributor
Author
|
@coretl Silvester doesn't have approval permission, so I'm requesting you as a second reviewer. |
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
2 participants
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.
Closes #303
This depends on a new release of
softioc.This PR sets and resets alarm states on failed and successful PV puts for CA and PVA.
This addresses the underlying bug in ophyd-async, where a failed command does not raise an error. However, only PVA allows these errors to immediately propagate up to the signal call, otherwise requiring a check on the PV status after the put on CA. Therefore, fastcs-eiger/odin must be converted to use PVA to close the ophyd-async bug.
Summary by CodeRabbit
Summary