-
Notifications
You must be signed in to change notification settings - Fork 9
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
PV Comparison in NALMS #61
base: master
Are you sure you want to change the base?
Conversation
--- a/Spreadsheet/KFE/TMO-alarms.csv
|
Spreadsheet/KFE/TMO-alarms.csv
Outdated
@@ -90,6 +91,7 @@ | |||
,,ST1K4:TEST:MMS:STATE:ERR_RBV,ST1K4 error state,,,, | |||
,,ST1K4:TEST:MMS:STATE:GET_RBV,ST1K4 state,,,PPS:NEH1:1:ST3K4INSUM!=0, | |||
,,ST1K4:TEST:MMS:STATE:ST3K4_AUTO_RBV,ST1K4 follow ST3K4 status,,,, | |||
,,CMP:ST1K4:TEST:MMS:STATE:GET_RBV,ST1K4 and ST3K4 state,,,,,PPS:NEH1:1:ST3K4INSUM!=0&ST1K4:TEST:MMS:STATE:GET_RBV=2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
,,CMP:ST1K4:TEST:MMS:STATE:GET_RBV,ST1K4 and ST3K4 state,,,,,PPS:NEH1:1:ST3K4INSUM!=0&ST1K4:TEST:MMS:STATE:GET_RBV=2 | |
,,CMP:ST1K4:TEST:MMS:STATE:GET_RBV,ST1K4 and ST3K4 state,,,,PPS:NEH1:1:ST3K4INSUM!=0&ST1K4:TEST:MMS:STATE:GET_RBV=2, | |
I think you've misplaced the expression here, we should have a trailing comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I messed up some stuff when I was resolving the merge conflict. I think I've fixed it in my latest commits
An interesting take on the problem. While I'm in favor of keeping all of the nalms config in one place, this does toe the line as something that might be outside of the scope of Interestingly the example expression you provided in this PR is not a comparison between two PVs, but rather two independent expressions that you want to both be true? (or at least that's how it reads to me) On that note, it's interesting to consider the order of operations for these comparison operators. Python evaluates logical (and/or/not) operators after relational (>, <, ==) operators, but does your IOC? Have you built one of these IOCS yet? |
def get_field(match): | ||
nonlocal index | ||
index = index + 1 | ||
return ascii_uppercase[index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also just be a generator of ascii_uppercase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I initially tried, but re.sub wouldn't accept a generator only a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking something like this:
In [8]: gen_upper = (letter for letter in ascii_uppercase)
In [9]: gen_upper
Out[9]: <generator object <genexpr> at 0x7f63b21d6c80>
In [10]: def get_field(match):
...: return next(gen_upper)
...:
In [11]: get_field('s')
Out[11]: 'A'
In [12]: get_field('s')
Out[12]: 'B'
In [13]: get_field('s')
Out[13]: 'C'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or even making that function a lambda
@@ -41,6 +44,8 @@ def csvtoxml(infile, outfile, cname): | |||
if 'Guidance' in row and row['Guidance']: | |||
guidance = ET.SubElement(pv, 'guidance') | |||
guidance.text = row['Guidance'] | |||
if 'Expression' in row and row['Expression']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So are we adding an additional column here? I don't see this column in the CSV you modified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to header now,
#Indent,Branch,PV,Description,Latch,Delay,Filter,Guidance,Expression,LOLO,LLSV,LOW,LSV,HIGH,HSV,HIHI,HHSV |
--- a/Spreadsheet/KFE/TMO-alarms.csv
|
I originally wanted to make this part of the repo to piggyback off of the existing scripts that parse the csvs and because I thought it'd be nice if the ioc building process was done by a workflow so the "comparison" pvs nalms looks for are made at around the same time that the iocs that host them are built, but I would be fine splitting this off.
Calling it a comparison is not a great name. I just used it because it was the name of the ticket.
I don't think the order that python would evaluate operators matters here, since Python is only extracting PV strings out of the expression field and replacing them with alphabetical variable names that a calc record would expect. epics does have operator precedence which I assume matches how c does it. https://github.com/slac-epics/epics-base/blob/308e234c41f59b9103b6d309f3256262a3175da2/modules/libcom/src/calc/postfix.c#L133. Parentheses have the highest priority so you can also right an expression and evaluates in the expected order, and I think it makes sense that you would write this expression the same way you would the calc field of a calc record (which is what this is used for). I have built an ioc for this. The parent is /cds/group/pcds/epics-dev/kaushikm/ioc/common/nalms, the ioc is ioc-nalms-TMO-alarms in the tst iocmanager with the one alarm that I added in this pr. |
Now that you've added the columns to the csv, I see more clearly what you're trying to accomplish here. I think I'm leaning even more toward the camp of having these IOCs be defined by config files separate from the nalms config. There is already confusion as to where the alarm thresholds come from, and I don't want to suggest that the user can set them through the NALMS csv. I do think that this can reasonably live in the Does this properly handle the comparison between two PVs? That was brought up in the ticket and was what I understood to be the missing feature here. |
This pr will allow users to add pv comparison expressions to nalms configuration files which will automatically be parsed and used to generate ioc configuration files, which alongside a new nalms ioc (analogous to ads-ioc) will host calc records to perform those calculations and provide pvs with user specified alarm states based on the calculated value that nalms can use to create alarms. The motivation comes from https://jira.slac.stanford.edu/browse/ECS-6818.
I have written some of the non boilerplate-part of the nalms ioc
comparison.db
st.cmd
The spreadsheets, with the use of nine new columns allows users to specify
Ideally, whenever pcds-nalms is pushed to these iocs cfgs would not only be generated but also automatically deployed just like the phoebus alarm servers are. Alternatively, the cfgs could be used by users to make deployment as simple as possible. I think there is a lot of flexibility in how to do this, so I am open to suggestions/questions.