Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for two new EPICS IOC device roles: Wiener Crate power supply monitoring via SNMP and Harvard Apparatus syringe pumps (pump33dds and phdultra models). The implementation includes configuration files, database definitions, protocol files, and deployment automation.
Key changes:
- New
wienercratedevice role with SNMP-based monitoring for power supply crates - New
harvarddevice role supporting two pump models with StreamDevice protocols - Module configuration for SNMP-NSCL dependency
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| roles/install_module/vars/snmp_nscl_3cd2804.yml | Defines SNMP-NSCL module configuration and dependencies |
| roles/device_roles/wienercrate/* | Complete wienercrate device role with templates, schema, tasks, and database files |
| roles/device_roles/harvard/* | Complete harvard device role with protocol files, databases, and templates for two pump models |
| roles/deploy_ioc/vars/wienercrate.yml | IOC deployment variables for wienercrate role |
| roles/deploy_ioc/vars/harvard.yml | IOC deployment variables for harvard role |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| field(DOL1, "1") | ||
| field(DOL2, "1") | ||
| field(LNK1, "$(P)$(DEV)AXIS-A:UPDATE.PROC") | ||
| field(LNK1, "$(P)$(DEV)AXIS-B:UPDATE.PROC") |
There was a problem hiding this comment.
Duplicate field name 'LNK1' on lines 73-74. The second link should use 'LNK2' instead of 'LNK1', otherwise the first link will be overwritten.
| field(LNK1, "$(P)$(DEV)AXIS-B:UPDATE.PROC") | |
| field(LNK2, "$(P)$(DEV)AXIS-B:UPDATE.PROC") |
| field(PREC, "0") | ||
| field(DRVH, "100") | ||
| field(DRVL, "0") | ||
| field(SDIS, "$(P)$(R)DISABLE") |
There was a problem hiding this comment.
Inconsistent macro usage. Lines 9 and 18 use '$(P)$(R)DISABLE' but the record prefix is '$(P){$(DEV)-Ax:$(AX)}', suggesting it should be '$(P){$(DEV)}DISABLE' to match the pattern used in other files.
| } | ||
|
|
||
| record(mbbi, "$(P){$(DEV)-Ax:$(AX)}$(CMD):EGU") { | ||
| #field(DESC, "Ax $(AX) $(CMD) EGU") |
There was a problem hiding this comment.
Commented out field definition on line 23. If the DESC field is intentionally omitted, remove the commented line. Otherwise, uncomment it to provide proper description.
| #field(DESC, "Ax $(AX) $(CMD) EGU") | |
| field(DESC, "Ax $(AX) $(CMD) EGU") |
| } | ||
|
|
||
| record(mbbi, "$(P){$(DEV)-Ax:$(AX)}$(CMD):RBV:EGU") { | ||
| #field(DESC, "Ax $(AX) $(CMD) readback EGU") |
There was a problem hiding this comment.
Commented out field definition on line 36. If the DESC field is intentionally omitted, remove the commented line. Otherwise, uncomment it to provide proper description.
| #field(DESC, "Ax $(AX) $(CMD) readback EGU") | |
| field(DESC, "Ax $(AX) $(CMD) readback EGU") |
|
|
||
|
|
||
| record(mbbi, "$(P){$(DEV)-Ax:$(AX)}$(CMD):RBV:EGU") { | ||
| #field(DESC, "Pump Status") |
There was a problem hiding this comment.
Commented out DESC field on line 25 has misleading description 'Pump Status' for an EGU (engineering units) record. Either uncomment with a correct description or remove the comment.
| #field(DESC, "Pump Status") |
Garrett Bischof (gwbischof)
force-pushed
the
harvard
branch
from
04418ce to
dfa44cf
Compare
|
Needs some work to refactor the substitution files so it can be much simpler. |
Migrate Harvard role from ioc-deploy-roles to nsls2.ioc_deploy collection format.