feat: allow aliases to be passed to ca transport#385
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:
📝 WalkthroughWalkthroughAdds an ChangesEPICS CA Aliases Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.25% +0.05%
==========================================
Files 72 72
Lines 2875 2892 +17
==========================================
+ Hits 2622 2639 +17
Misses 253 253 ☔ View full report in Codecov by Harness. 🚀 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
gilesknap
left a comment
gilesknap
left a comment
There was a problem hiding this comment.
Looks good. One minor comment inline, plus this is for CA only. Won't PVA want it too?
I think @coretl and I agreed that, for now at least, CA is the only transport that required aliasing, as we're doing this for backwards compatibility with GDA. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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 (2)
src/fastcs/transports/epics/options.py (1)
36-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the docstring to reflect the new
aliasesfield.The docstring still states "Currently empty" but the class now contains the
aliasesfield.📝 Proposed docstring update
`@dataclass` 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. + + Attributes: + aliases: Mapping of canonical PV names to alias names for Channel Access records. + """🤖 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, The docstring for the class at lines 36-40 in src/fastcs/transports/epics/options.py incorrectly states "Currently empty" when the class now contains an `aliases` field. Update the docstring to remove the "Currently empty" statement and instead document the purpose and usage of the `aliases` field that is now present in the class.src/fastcs/transports/epics/ca/ioc.py (1)
140-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate alias names against EPICS name length constraints.
Alias names retrieved from the configuration are not validated against
EPICS_MAX_NAME_LENGTH. If an alias exceeds 60 characters, the underlyingrecord.add_alias()call may fail at runtime.🛡️ Proposed validation
alias = aliases.get(f"{pv_prefix}:{pv_name}", None) + if alias is not None and len(alias) > EPICS_MAX_NAME_LENGTH: + logger.warning( + f"Alias '{alias}' for {pv_prefix}:{pv_name} exceeds" + f" {EPICS_MAX_NAME_LENGTH} characters and will be ignored" + ) + alias = None match attribute: case AttrRW(): if full_pv_name_length > (EPICS_MAX_NAME_LENGTH - 4):Apply similar validation at line 249 for command PV aliases.
🤖 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 140 - 153, Alias names retrieved from the configuration via aliases.get() are not validated against EPICS_MAX_NAME_LENGTH before being used. Add validation checks for both the alias variable and alias_rbv variable to ensure they do not exceed EPICS_MAX_NAME_LENGTH, similar to the validation already performed for full_pv_name_length. If either alias exceeds the limit, log a warning and disable the attribute before the alias is passed to record.add_alias(), preventing runtime failures.
🧹 Nitpick comments (2)
tests/transports/epics/ca/test_softioc.py (1)
70-114: ⚖️ Poor tradeoffConsider adding negative test cases for alias validation.
The new tests verify the happy path where
record.add_aliasis called with a valid alias. Consider adding tests that verify behavior when:
- An alias exceeds
EPICS_MAX_NAME_LENGTH- An alias conflicts with an existing PV name
These tests would help ensure the validation suggested in
ioc.pyworks correctly.🤖 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 70 - 114, Add negative test cases to verify alias validation behavior for the three PV creation functions (_create_and_link_write_pv, _create_and_link_read_pv, and _create_and_link_command_pv). For each function, add tests that verify the behavior when an alias exceeds EPICS_MAX_NAME_LENGTH and when an alias conflicts with an existing PV name, ensuring that appropriate validation errors or exceptions are raised in these edge cases.tests/transports/epics/ca/test_softioc_system.py (1)
48-51: ⚡ Quick winConsider testing bidirectional alias access and the
_RBValias.The test verifies that reading from
AliasBreturns the same value as the canonical PV after writing toB. Consider also:
- Writing to
AliasBand reading fromBto confirm bidirectional access- Verifying the
AliasB_RBValias points toB_RBV(the readback configured in example_softioc.py)🧪 Suggested additional assertions
# Assert alias. Aliases do not show up in PVI structure assert ctxt.get(f"{pv_prefix}:B") == ctxt.get(f"{pv_prefix}:AliasB") == 0 ctxt.put(f"{pv_prefix}:B", 10) assert ctxt.get(f"{pv_prefix}:AliasB") == 10 + # Test bidirectional access + ctxt.put(f"{pv_prefix}:AliasB", 20) + assert ctxt.get(f"{pv_prefix}:B") == 20 + # Verify RBV alias points to readback + assert ( + ctxt.get(f"{pv_prefix}:B_RBV") + == ctxt.get(f"{pv_prefix}:AliasB_RBV") + )🤖 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_system.py` around lines 48 - 51, The test currently only verifies reading from AliasB after writing to the canonical PV B, but it does not test bidirectional alias behavior or readback aliases. After the existing assertions, add two additional test scenarios: first, write a value to the AliasB alias and then read from the canonical B PV to confirm the write is reflected bidirectionally; second, add assertions to verify that the AliasB_RBV readback alias correctly points to B_RBV by getting and potentially setting values through this readback alias path, ensuring complete alias coverage including both the main alias and its associated readback alias counterpart.
🤖 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 151-153: The automatic readback PV alias lookup using RBV_SUFFIX
in the aliases.get() call at the specified location lacks documentation. Add a
code comment above or near the aliases.get() call that explains the implicit
behavior: that for read-write attributes (AttrRW), the code automatically
appends RBV_SUFFIX to construct the readback PV alias key, requiring users to
configure both the base setpoint alias and the RBV-suffixed readback alias when
setting up read-write attributes. Alternatively or additionally, update the
EpicsCAOptions.aliases docstring to document this automatic suffix aliasing
behavior for read-write attributes.
---
Outside diff comments:
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 140-153: Alias names retrieved from the configuration via
aliases.get() are not validated against EPICS_MAX_NAME_LENGTH before being used.
Add validation checks for both the alias variable and alias_rbv variable to
ensure they do not exceed EPICS_MAX_NAME_LENGTH, similar to the validation
already performed for full_pv_name_length. If either alias exceeds the limit,
log a warning and disable the attribute before the alias is passed to
record.add_alias(), preventing runtime failures.
In `@src/fastcs/transports/epics/options.py`:
- Around line 36-40: The docstring for the class at lines 36-40 in
src/fastcs/transports/epics/options.py incorrectly states "Currently empty" when
the class now contains an `aliases` field. Update the docstring to remove the
"Currently empty" statement and instead document the purpose and usage of the
`aliases` field that is now present in the class.
---
Nitpick comments:
In `@tests/transports/epics/ca/test_softioc_system.py`:
- Around line 48-51: The test currently only verifies reading from AliasB after
writing to the canonical PV B, but it does not test bidirectional alias behavior
or readback aliases. After the existing assertions, add two additional test
scenarios: first, write a value to the AliasB alias and then read from the
canonical B PV to confirm the write is reflected bidirectionally; second, add
assertions to verify that the AliasB_RBV readback alias correctly points to
B_RBV by getting and potentially setting values through this readback alias
path, ensuring complete alias coverage including both the main alias and its
associated readback alias counterpart.
In `@tests/transports/epics/ca/test_softioc.py`:
- Around line 70-114: Add negative test cases to verify alias validation
behavior for the three PV creation functions (_create_and_link_write_pv,
_create_and_link_read_pv, and _create_and_link_command_pv). For each function,
add tests that verify the behavior when an alias exceeds EPICS_MAX_NAME_LENGTH
and when an alias conflicts with an existing PV name, ensuring that appropriate
validation errors or exceptions are raised in these edge cases.
🪄 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: 97f8dd9a-8636-4c34-8834-b6a29022caf0
📒 Files selected for processing (8)
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/example_softioc.pytests/transports/epics/ca/test_softioc.pytests/transports/epics/ca/test_softioc_system.py
|
@shihab-dls I feel that the _RBV is still possibly confusing:
I can't remember - if the RW attribute readback name is not a simple _RBV suffix, does that mean the attributes need to be declared as separate R and W? If not then 2 would break. Either way their are some assumptions here that don't immediately jump out. Maybe we don't care? |
It is indeed the case that, if you need an AttrRW with different PV names for the setpoint and readback, then you need to make a separate AttrW and AttrR; making an AttrRW currently implies that you intend to have a setpoint and an There is a case where we want to alias: where our aliased PVs need to be Different for AttrRWs. This is likely, as legacy IOCs can have odd PV naming. Because of this, this comment of yours is very valid, and we shouldn't handle making the alias EDIT: Either way, I should document this somewhere in the fastcs docs. I'll add that to the new issue I'll create about AttrRWs, just so it doesn't block this PR. |
gilesknap
left a comment
gilesknap
left a comment
There was a problem hiding this comment.
That seems fine to me. Approved.
3 participants
Closes #384
This PR allows passing aliases to the
catransport. An Example would be:Summary by CodeRabbit
aliasestransport option, registering alternate PV names for attributes, command PVs, and readback (*_RBV) variants._RBV), and command PVs, plus updated IOC initialization and expectations.aliasesfield.