Alarm handling with AMSG - #52
Conversation
… request. Accepted changes to dbrec.c made byhttps://github.com/epics-modules/pull/48
|
OK, done. I see test_db.py has continued conflicts that will need to be addressed. |
|
Thanks @pheest . I went ahead and merged the latest from master and resolved the conflicts. The main one being I renamed |
There does not appear to be a need for G._exec to be outside of the lock block.
| self.name = name | ||
| self.table, self.scan, self._value = table, scan, None | ||
| self.alarm, self.actions = 0, [] | ||
| self.stat = UDF_ALARM |
There was a problem hiding this comment.
I believe we should default this to COMM_ALARM? I understand the default of UDF_ALARM but I think this code already covers that case when the value hasn't been set at all:
if value is not None:
......
else:
# undefined value
rec.setSevr(INVALID_ALARM, UDF_ALARM)
So then the only time self.stat default value comes into play is when the user sets .alarm but does not set .stat. In that case defaulting to COMM_ALARM would maintain backwards compatibility with dbrec.c default and also seems better fit than UDF_ALARM? Otherwise you could have a MAJOR alarm with a default UDF_ALARM STAT field?
There was a problem hiding this comment.
Hi @tynanford, I believe we discussed this in #48 on July 22, and thought that we had agreement on the issue.
My intent was that the value of the value should default to the expected value after IOC start-up.
Which is UDF_ALARM if PINI is not set and NO_ALARM if it is.
My tests do explicitly check the value (without explicitly setting the stat field).
I'm aware that the setSevr function defaults to COMM_ALARM if the second parameter is not specified.
All cases in ptable.py do provide the first two parameters.
A change I could make, if you agree, is to default self.stat to None, and then set it explicitly in _ParamSupBase,init to the actual value read back from rec.STAT.
This could differ from the expected value if, for example the record is malformed or PINI processing causes an error .
I have tested this change locally.
There was a problem hiding this comment.
Hi @pheest yes sorry, after looking into this a bit I found that defaulting to UDF_ALARM means that a user who doesn't set .stat would then get a default of UDF for all alarms. Here is a test IOC to show what I mean: https://github.com/tynanford/alarmDemo
With the current master branch of pyDevSup:
$ camonitor PINI:YES PINI:NO
PINI:YES 2026-08-26 11:04:01.302082 0 COMM MAJOR
PINI:NO <undefined> 55 UDF NO_ALARM
PINI:YES 2026-08-26 11:04:06.302560 1
PINI:YES 2026-08-26 11:04:11.302999 2 COMM MAJOR
PINI:YES 2026-08-26 11:04:16.303536 3
PINI:YES 2026-08-26 11:04:21.303729 4 COMM MAJOR
With your changes now PINI:YES shows UDF alarm:
$ camonitor PINI:YES PINI:NO
PINI:YES 2026-08-26 11:05:04.059871 0 UDF MAJOR
PINI:NO <undefined> 55 UDF NO_ALARM
PINI:YES 2026-08-26 11:05:09.060527 1
PINI:YES 2026-08-26 11:05:14.061057 2 UDF MAJOR
PINI:YES 2026-08-26 11:05:19.061630 3
PINI:YES 2026-08-26 11:05:24.062074 4 UDF MAJOR
Also it seems like the UDF alarm handling already is covered by the if value is not None: line in ptable.py. Does this follow or am I thinking about this wrong?
There was a problem hiding this comment.
I'm not saying COMM_ALARM is the best default but it seems to fit better than UDF which should be for only before a record is initialized?
A change I could make, if you agree, is to default self.stat to None, and then set it explicitly in _ParamSupBase,init to the actual value read back from rec.STAT.
This sounds interesting.. then we don't decide in pyDevSup what the default is which seems good?
There was a problem hiding this comment.
field(INP, "@devsup.ptable alarm set pini_yes")
Is 'set' intentional here, for an input record?
There was a problem hiding this comment.
I believe I've arrived at a compromise that I think we can both agree with.
self.stat defaults to None.
If it is (still) None when the record is processed, then the local stat variable is set to COMM_ALARM and passed to the function. self.stat then remains None. It should much preferably be set by IOC applications that generate alarms.
What I didn't want to do was to was to default self.stat to a value that I saw as inappropriate.
There was a problem hiding this comment.
Nice yes I like this! One thing, should we execute any actions before assigning stat value? In case someone sets the alarm STAT in @onproc for instance?
diff --git a/devsupApp/src/devsup/ptable.py b/devsupApp/src/devsup/ptable.py
index 3c7665e..2e36574 100644
--- a/devsupApp/src/devsup/ptable.py
+++ b/devsupApp/src/devsup/ptable.py
@@ -273,11 +273,11 @@ class _ParamSupSet(_ParamSupGet):
with self.inst.table.lock:
oval, self.inst.value = self.inst.value, value
+ # Execute actions
+ self.inst._exec(oval)
stat = self.inst.stat
if stat is None:
stat = COMM_ALARM
- # Execute actions
- self.inst._exec(oval)
rec.setSevr(self.inst.alarm, stat, self.inst.amsg)
for G in self.inst._groups:
G._exec()2 participants
This PR incorporates and expands on #48
In dbrec.c, I have added a third parameter 'amsg' to the call, defaulting to None.
Use of the 'z' argument means that either a string or None is accepted.
If it is None, NULL is the resulting C value.
If EPICS_VERSION is < 7.0.6, the parameter is ignored.
In ptable.py, the stat field is initialised to UDF_ALARM.
This will be overridden if the PINI field is "YES".
This ensures the value of the field matches the actual output value after initial processing.
The alarm, stat and amsg values are applied to both input and output records. This requires the record be scanned in order to update the status.
(My project need is to alert the user when an incorrect value is set that does not meet complex validation rules.)
In test_db.py, I have added tests to verify correct operation of both input and output record alarms.