Make the elio role more flexible - automatically get num terminals, num channels per terminal, and allow for customizing specific record fields#249
Conversation
…um channels per terminal, and allow for customizing specific record fields
Jakub Wlodek (jwlodek)
requested review from
Copilot,
Dyon Buitenkamp (dbuitenka) and
hxu-bnl
There was a problem hiding this comment.
Pull request overview
This PR updates the elio device role configuration model to make EK9000 terminal setup more data-driven and to support channel-specific macro overrides when loading per-terminal records.
Changes:
- Replace the old
NUM_TERMSenvironment variable with deriving terminal count from theterminalslist. - Restructure
terminalsentries from positional lists to structured mappings (POS,MODEL, optionalCHAN_SPECIFIC_CONFIG). - Add optional channel-specific macro injection when loading templates, and update the role example accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| roles/device_roles/elio/templates/base.cmd.j2 | Derives terminal count and adds optional per-channel template loading with channel-specific macros. |
| roles/device_roles/elio/schema.yml | Updates schema to require structured terminals entries and adds optional CHAN_SPECIFIC_CONFIG. |
| roles/device_roles/elio/example.yml | Updates example config format and demonstrates channel-specific configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Configure the low level communication port | ||
| ek9000Configure("${EK9K}", "${HOST}", 502, ${NUM_TERMS}) | ||
| ek9000Configure("${EK9K}", "${HOST}", 502, {{ ioc.terminals | length }}) |
There was a problem hiding this comment.
Dyon Buitenkamp (@dbuitenka) I am not sure if copilot is right to be concerned here. Would there ever be a case that the POS fields don't start at 1 and are contiguous from there?
There was a problem hiding this comment.
Jakub Wlodek (@jwlodek) The POS field should always start at 1 and be contiguous. There should never be a need for anything else.
There was a problem hiding this comment.
In that case, could we perhaps remove it and just use the index in the terminals list as the position?
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| {% for key, value in chan_config.items() %} | ||
| {% set _ = chan_macros.append(key ~ "=" ~ value) %} | ||
| {% endfor %} | ||
| dbLoadTemplate("{{ deploy_ioc_template_root_path }}/db/$(MODEL).template","DEVICE=$(EK9K), Sys=$(SYS), Dev=$(Dev), POS=$(POS), CHAN={{ chan }}{% if chan_macros %}, {{ chan_macros | join(', ') }}{% endif %}") |
There was a problem hiding this comment.
I believe this should be dbLoadRecords instead of dbLoadTemplate as it is expecting a substitution file otherwise.
Dyon Buitenkamp (dbuitenka)
left a comment
Dyon Buitenkamp (dbuitenka)
left a comment
There was a problem hiding this comment.
The testing of this code looks to be successful now.
|
hxu-bnl do you mind reviewing before I merge this? |
Allows for specifying custom fields for specific channel's records.
Note: I slightly changed the schema, so any already deployed configurations in ioc_host_vars will need to be updated.