Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Archiver info tag #223

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

cdn-lnk
Copy link
Contributor

@cdn-lnk cdn-lnk commented Aug 27, 2024

Hi @kivel and @anderssandstrom,

The guys here at ESS recently released a new feature that allow us to automatically register PVs for archival using info tags.

This PR adds support to said info tags. This means all ecmc IOCs at ESS can (can, not will, see below) be archived by default.

Initially I am only adding this to the motor record PV.
In the future, depending on success and/or necessity, we may want to expand this to other PVs.

The behavior is disabled by default. It will only work if NAMING == ESSnaming and the integrator specify the archiver cluster via the ECMC_ARCHIVER variable (currently only 3 options are available tn, nin and lab).

I believe I wrote the code in such way that should not affect other facilities and is easily expandable if necessary.
But please let me know your thoughts.


PS1: please do not merge before @mac-kan agrees with the changes.

PS2: I took the opportunity to fix the formatting of the motor record .db files.

PS3: I think I found a bug (?): The way it stands right now ECMC_AXISFIELDINIT has no effect.
https://github.com/paulscherrerinstitute/ecmccfg/blob/master/motion/ecmc_axis_mr.cmd#L21-L22

PS4: I set up a virtual axis that has been constantly moving... It amounts to approx. 1.5 GB per year of data usage with .RBV being the main culprit.

Carlos Doro Neto added 2 commits August 27, 2024 17:03
Use 4 space for indentation
Align values for 3 and 4 letter fields
{ on same line as record
Space after commas
Remove trailing spaces
@cdn-lnk
Copy link
Contributor Author

cdn-lnk commented Aug 27, 2024

@mac-kan, do you think we should include a macro for the archiver policy?

Copy link
Collaborator

@kivel kivel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at the comment of the first suggestion.
In my mind, there is no downside of having the info fields by default for all.
Happy to discuss the matter, though.

db/core/ecmcMotorRecord.template Outdated Show resolved Hide resolved
motion/ecmc_axis_mr.cmd Outdated Show resolved Hide resolved
motion/ecmc_axis_mr.cmd Outdated Show resolved Hide resolved
motion/ecmc_axis_mr.cmd Outdated Show resolved Hide resolved
@kivel
Copy link
Collaborator

kivel commented Aug 27, 2024

@mac-kan, do you think we should include a macro for the archiver policy?

I'd say it's mandatory to have it as a MACRO, otherwise you need to tinker with the template all the time, which is not really in the spirit of a module.

@mac-kan
Copy link

mac-kan commented Aug 28, 2024

@cdn-lnk @kivel
Looks alright with me, especially the clean up ;)
Regarding the info tags: If a facility does not use the info-tags to load the archiver, will a default tag result in a lot of error messages in the IOC?

@kivel
Copy link
Collaborator

kivel commented Aug 28, 2024

Regarding the info tags: If a facility does not use the info-tags to load the archiver, will a default tag result in a lot of error messages in the IOC?

I'm not aware that info fields create error. To my best knowledge, they are metadata that's consumed by a service that's actively looking for them. You'll get errors if the service can't find them, though.

Carlos Doro Neto added 2 commits September 2, 2024 19:12
- Fix minor bugs (missing quotes, unnecessary commands, etc)
- Create ECMC_ARCHIVER_POLICY macro
- Create configureArchiver.cmd
It is not used
@cdn-lnk cdn-lnk force-pushed the archiver-info-tag branch from e0f51cb to ee3ed2c Compare October 9, 2024 12:17
@cdn-lnk
Copy link
Contributor Author

cdn-lnk commented Oct 9, 2024

Hi @kivel ,
I solved the conflict introduced by Anders commit.
Could you please re-review?

@cdn-lnk cdn-lnk requested a review from kivel October 9, 2024 15:00
Copy link
Collaborator

@kivel kivel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@cdn-lnk cdn-lnk requested a review from anderslindho October 15, 2024 09:42
Copy link

@anderslindho anderslindho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

field(OOPT, "Transition To Non-zero")
field(OUT, "$(PREFIX)$(MOTOR_NAME)-MR-SyncTrgSeq PP")
record(calcout, "$(PREFIX)$(MOTOR_NAME)-MR-SyncTrgH2L") {
#field(PINI, "1")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're fixing the formatting here you could consider removing this (same in the following record def) - your call 🔧

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants