feat: allow aliases to be passed to ca transport#385
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an ChangesEPICS CA Aliases Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #385 +/- ##
==========================================
+ Coverage 91.20% 91.22% +0.02%
==========================================
Files 72 72
Lines 2875 2882 +7
==========================================
+ Hits 2622 2629 +7
Misses 253 253 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/fastcs/transports/epics/ca/ioc.py (1)
138-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
AttrRWignores explicit readback alias keys.Line 138 only looks up the write PV key, then reuses it for both records. A mapping like
PREFIX:MyPv_RBV -> ...is never read forAttrRW, so that config is silently ignored.Suggested fix (preserve fallback behavior, allow explicit RBV aliases)
- alias = aliases.get(f"{pv_prefix}:{pv_name}", None) + write_alias = aliases.get(f"{pv_prefix}:{pv_name}") + read_alias = aliases.get(f"{pv_prefix}:{pv_name}_RBV", write_alias) match attribute: case AttrRW(): @@ _create_and_link_read_pv( - pv_prefix, f"{pv_name}_RBV", attr_name, alias, attribute + pv_prefix, f"{pv_name}_RBV", attr_name, read_alias, attribute ) _create_and_link_write_pv( - pv_prefix, pv_name, attr_name, alias, attribute + pv_prefix, pv_name, attr_name, write_alias, attribute ) case AttrR(): _create_and_link_read_pv( pv_prefix, pv_name, attr_name, - alias, + aliases.get(f"{pv_prefix}:{pv_name}"), attribute, ) case AttrW(): _create_and_link_write_pv( - pv_prefix, pv_name, attr_name, alias, attribute + pv_prefix, pv_name, attr_name, aliases.get(f"{pv_prefix}:{pv_name}"), attribute ) @@ - if alias: - suffix = "_RBV" if pv_name.endswith("_RBV") else "" - record.add_alias(f"{alias}{suffix}") + if alias: + alias_name = ( + f"{alias}_RBV" if pv_name.endswith("_RBV") and not alias.endswith("_RBV") else alias + ) + record.add_alias(alias_name)Also applies to: 187-190
🤖 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/ca/ioc.py` around lines 138 - 154, The AttrRW branch currently only looks up the alias for the write PV and reuses it for the readback, so explicit RBV alias keys (e.g. f"{pv_prefix}:{pv_name}_RBV") are ignored; change the code to separately resolve an RBV alias with fallback to the write alias (e.g. rbv_alias = aliases.get(f"{pv_prefix}:{pv_name}_RBV", alias)) and pass rbv_alias to _create_and_link_read_pv while passing alias to _create_and_link_write_pv; apply the same pattern to the other AttrRW handling block referenced (around lines 187-190) so explicit RBV alias keys are honored but the previous fallback behavior remains.tests/transports/epics/ca/test_softioc.py (1)
628-632:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix stale expected call signature in waveform test (currently breaks CI).
_create_and_link_read_pvnow receives(pv_prefix, pv_name, attr_name, alias, attribute), but this assertion still expects the old 4-arg call.Proposed test fix
create_mock.assert_called_once_with( - DEVICE, "Waveform1d", "waveform_1d", api.attributes["waveform_1d"] + DEVICE, "Waveform1d", "waveform_1d", None, api.attributes["waveform_1d"] )🤖 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/ca/test_softioc.py` around lines 628 - 632, The test's expected call signature is stale: _create_and_link_read_pv now takes (pv_prefix, pv_name, attr_name, alias, attribute), but the assertion in tests/transports/epics/ca/test_softioc.py still expects 4 args; update the create_mock.assert_called_once_with invocation used after EpicsCAIOC([api], {}) to include the missing alias argument between the attr_name ("waveform_1d") and the attribute (api.attributes["waveform_1d"]) — use the same alias value the code passes (e.g., the alias string or None) so the assertion matches _create_and_link_read_pv's new signature.
🧹 Nitpick comments (1)
src/fastcs/transports/epics/options.py (1)
36-40: ⚡ Quick winUpdate
EpicsCAOptionsdocstring; it’s now outdated.Line 38 says this options class is currently empty, but Line 44 adds
aliases. This can mislead users reading config docs.Proposed docstring update
class EpicsCAOptions: """Channel-Access-specific options. - Currently empty: present so ``epicsca:`` survives in fastcs.yaml as the - transport discriminator key. Reserved for future CA-specific knobs. + Channel-Access-specific options configured under ``epicsca:`` in fastcs.yaml. + ``aliases`` maps generated PV names to alias base names. """🤖 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/options.py` around lines 36 - 40, Update the EpicsCAOptions class docstring to reflect that the class is not empty: mention the added "aliases" attribute and briefly describe its purpose (e.g., mapping alternate transport names to this transport) and any intended usage in fastcs.yaml; edit the docstring in the EpicsCAOptions definition so it no longer claims the class is currently empty and instead documents the aliases field and its role for the epicsca transport discriminator.
🤖 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.
Outside diff comments:
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 138-154: The AttrRW branch currently only looks up the alias for
the write PV and reuses it for the readback, so explicit RBV alias keys (e.g.
f"{pv_prefix}:{pv_name}_RBV") are ignored; change the code to separately resolve
an RBV alias with fallback to the write alias (e.g. rbv_alias =
aliases.get(f"{pv_prefix}:{pv_name}_RBV", alias)) and pass rbv_alias to
_create_and_link_read_pv while passing alias to _create_and_link_write_pv; apply
the same pattern to the other AttrRW handling block referenced (around lines
187-190) so explicit RBV alias keys are honored but the previous fallback
behavior remains.
In `@tests/transports/epics/ca/test_softioc.py`:
- Around line 628-632: The test's expected call signature is stale:
_create_and_link_read_pv now takes (pv_prefix, pv_name, attr_name, alias,
attribute), but the assertion in tests/transports/epics/ca/test_softioc.py still
expects 4 args; update the create_mock.assert_called_once_with invocation used
after EpicsCAIOC([api], {}) to include the missing alias argument between the
attr_name ("waveform_1d") and the attribute (api.attributes["waveform_1d"]) —
use the same alias value the code passes (e.g., the alias string or None) so the
assertion matches _create_and_link_read_pv's new signature.
---
Nitpick comments:
In `@src/fastcs/transports/epics/options.py`:
- Around line 36-40: Update the EpicsCAOptions class docstring to reflect that
the class is not empty: mention the added "aliases" attribute and briefly
describe its purpose (e.g., mapping alternate transport names to this transport)
and any intended usage in fastcs.yaml; edit the docstring in the EpicsCAOptions
definition so it no longer claims the class is currently empty and instead
documents the aliases field and its role for the epicsca transport
discriminator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2da62478-6221-49d5-8c10-f756644d25ab
📒 Files selected for processing (6)
src/fastcs/demo/schema.jsonsrc/fastcs/transports/epics/ca/ioc.pysrc/fastcs/transports/epics/ca/transport.pysrc/fastcs/transports/epics/options.pytests/data/schema.jsontests/transports/epics/ca/test_softioc.py
coretl
left a comment
coretl
left a comment
There was a problem hiding this comment.
Implementation looks fine.
Are there any system tests for CA? If so, then it should be added there too.
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 `@tests/transports/epics/ca/test_softioc_system.py`:
- Around line 48-56: The test incorrectly expects the AliasB_RBV readback to
follow writes to the setpoint PV; update the assertion logic in this test (using
ctxt.get and pv_prefix symbols) to verify alias equivalence only — i.e., assert
AliasB and the canonical PV (:B) match each other and separately assert
AliasB_RBV equals the canonical RBV value as provided by the IOC, instead of
expecting AliasB_RBV to become 10 after ctxt.put(f"{pv_prefix}:B", 10); locate
the assertions that reference f"{pv_prefix}:B", f"{pv_prefix}:AliasB" and
f"{pv_prefix}:AliasB_RBV" and change them to compare aliases to the canonical PV
rather than asserting RBV propagation.
🪄 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: 291df94c-4ca1-4430-895c-0ed54b09c8c4
📒 Files selected for processing (3)
tests/example_softioc.pytests/transports/epics/ca/test_softioc.pytests/transports/epics/ca/test_softioc_system.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/transports/epics/ca/test_softioc.py
2 participants
Closes #384
This PR allows passing aliases to the
catransport. An Example would be:Summary by CodeRabbit
New Features
Tests