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

Enable setting alarm status of Out records #157

Merged
merged 5 commits into from
Apr 9, 2024

Conversation

AlexanderWells-diamond
Copy link
Collaborator

Adds the set_alarm method to ProcessDeviceSupportOut, along with extra keyword arguments to set. As these are keyword args with defaults, there should be no compatibility problems.

Tests added to check calling set_alarm() before or after IocInit() causes the severity and status attributes to be set as expected.

Closes #53.

Copy link

github-actions bot commented Apr 8, 2024

Unit Test Results

     12 files  ±  0       12 suites  ±0   22m 13s ⏱️ + 1m 14s
   284 tests +  5     268 ✔️ +  5    16 💤 ±0  0 ±0 
3 408 runs  +60  2 924 ✔️ +52  484 💤 +8  0 ±0 

Results for commit 4388c69. ± Comparison against base commit 7babcde.

♻️ This comment has been updated with latest results.

This ensures we only set INVALID_ALARM+UDF_ALARM when there is no value,
but otherwise both the severity and the status are set to NO_ALARM
Previously we did this as we wanted to spot the case where no value
was provided, either during record creation or by a set() call at some
later time, so that we could mark the record as being in an invalid
state.

Now that we store the severity and status alongside the value, we don't
need to handle this special case.
@AlexanderWells-diamond
Copy link
Collaborator Author

I've done a few more tweaks, and changed the default Status value to NO_ALARM, which is technically a breaking change we may need to highlight (although maybe not as a non-zero STAT is fairly meaningless with a SEVR of 0)

I have found one problem: Trying to set STAT and SEVR using the EPICS attribute names during record declaration doesn't work i.e.:

ai = builder.aIn('AI', SEVR="MINOR", STAT="HIHI")
ao = builder.aOut('AO', SEVR="MINOR", STAT="HIHI")

The records created here will not respect these SEVR and STAT values:

$ caget -a AI
MY-DEVICE-PREFIX:AI            2024-04-08 14:04:54.829984 0  
$ caget -a MY-DEVICE-PREFIX:AO
MY-DEVICE-PREFIX:AO            2024-04-08 14:04:54.328884 0 UDF INVALID

I believe this has never worked; they are passed to the epicsdbbuilder (as proven by giving invalid values producing an error message), but for reasons unknown at this time they are gone after record initialization.

Is it worth trying to support these keywords properly?

@Araneidae
Copy link
Collaborator

I suspect that SEVR and STAT aren't supposed to be set directly, but am not sure.

Copy link
Collaborator

@Araneidae Araneidae left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me

softioc/device.py Outdated Show resolved Hide resolved
Comment on lines 268 to 295
if self._value is None:
if self._value[0] is None:
# Before startup complete if no value set return default value
value = self._default_value()
else:
value = self._value
value = self._value[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can simplify this, _value[0] is now never None, can now just be

return self._epics_to_value(self._value[0])

Comment on lines 226 to 231
# Ignore memoized value, retrieve it from the VAL field directly later
_, severity, alarm = self._value

self.process_severity(record, severity, alarm)

value = self._read_value(record)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put reading self._value and self._read_value(record) on adjacent lines, then it's much clearer that they're working together and you might not need the comment!

@AlexanderWells-diamond AlexanderWells-diamond merged commit 4388c69 into master Apr 9, 2024
30 checks passed
@AlexanderWells-diamond AlexanderWells-diamond deleted the enable_out_record_alarms branch April 9, 2024 08:41
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.

Be able to force Out records into an error state
2 participants