[Enhancement] Support optional alarm message in Record.setSevr()

[aqshafei]

Summary

add an optional parameter called message to Record.setSevr() while preserving all existing functionality

record.setSevr(MINOR_ALARM, HIGH_ALARM, message="Temperature T1 over 100 °C")

Existing calls without a message continue to behave as before.

Details

• Adds an optional message argument to Record.setSevr().
• Uses recGblSetSevrMsg() when EPICS Base provides alarm message support.
• Falls back to the existing recGblSetSevr() path on older EPICS Base versions.
• Treats message=None the same as omitting the message.
• Adds alarm message unit test coverage

[tynanford]

Hi @aqshafei , this looks good to me. The python tests for 3.15.9 are failing. I think we could skip test_set_severity_message for EPICS versions without HAS_ALARM_MESSAGE? Or just do a coarse check if _dbapi.EPICS_VERSION >= 7 to skip that test? Something like?

if _dbapi.EPICS_VERSION >= 7:                                                                                 
    self.assertEqual(rec.NAMSG, "Meaningful alarm message")

[pheest]

Hi @tynanford, @aqshafei,

This seems to be a duplication of work that is already included in my pull request 33 'win32_merge' (and should be separate from it).
I added the capability to set an alarm message using recGblSetSevrMsg when EPICS_BASE is >= 7.0.6.1 when the capability was added.
If a lesser version (e.g. 3.15.9) is being used, recGblSetSevr is used instead and the message is ignored (and the test for it is suppressed).

[tynanford]

Hi @pheest ,

Thanks for pointing that out. Your addition also has an update for ptable so it can set stat/amsg as well. I think that is worth adding to this PR: https://github.com/epics-modules/pyDevSup/pull/33/changes#diff-2c53d4e276a2b54b70ce9f2aaf0d4e7469847367228710d03146dc041da78111 . One thing on the default values in ptable.py. dbrec.c defaults to COMM_ALARM right now for STAT. I think we should keep that the same in ptable.py? And we should default amsg/message to None instead of "" so it defaults to NULL in the C code:

        self.stat = COMM_ALARM
        self.amsg = None

Also @aqshafei I am thinking we should use amsg instead of message to match the EPICS field name like Peter has. What do you both think?

[pheest]

The field is named 'amsg' in EPICS base.

• With the exception of the underlying recGblSetSevrMsg function declaration and definition (in recGbl.h and recGbl.c) which name it 'msg'.
  I take the view that field naming consistency is desirable, but I can't say I feel very strongly about this here.

I take note of your point about the defaults, but I would have to revisit to verify.
I seem to recall that passing Python 'None' to C code caused an interesting problem.

[aqshafei]

@tynanford I could change it to amsg to match the record field name NAMSGor NAMSG.
Alternatively, we could use msg, which matches the actual recGblSetSevrMsg parameter name. Since I am calling setSevr() method from Python rather than assigning to an object attributes.

@pheest the None value is not passed to the C code. When message is None recGblSetSevrMsg() is skipped and the normal recGblSetSevr() path is executed instead.

[pheest]

Hi @aqshafei,

It is OK to pass NULL as the msg parameter to the recGblSetSevrMsg function, it is just treated as being synonymous with the empty string.
I see that EPICS base implements recGblSetSevr in recGbl.c by calling recGblSetSevrMsg with the msg parameter passed as NULL.
So there is no need to call these 2 functions separately for this reason.

There is however a need to call the recGblSetSevr (rather than recGblSetSevrMsg) function if the base version is < 7.0.6 because the latter function was not implemented until that version. This is dependant on a compile-time constant.

[pheest]

I believe there are 3 use-cases for accessing the error number and string:

1. An error can be generated in the EPICS database.
   It would be feasible to read these out.
   This is conceptually valuable, but is not my use case and I haven't implemented it.
   COMM_ALARM is an appropriate default value for this case.

2. Python code can generate an error for an input variable.
   The error needs to be written out to the database.
   I implemented this, but haven't tested it because this also is not my use case.

3. Python code can generate an error for an output variable.
   In my case, the IOC has some quite complex inter-dependencies on settings parameter values.
   The user needs to be made aware that values set are invalid, and how to correct it.
   This is my use case and I have tested it extensively.

[zhangt58]

That's a good enhancement, please also update the documentaton about setSevr() with the new message parameter support, at PyMethodDef.

[pheest]

Hi @tynanford,

we should default amsg/message to None
I totally agree. I have made this change in my code.
The correction that I needed to make was to set the parse parameter to 'z' (string or None) rather than 's' (string only).
Thank you @aqshafei, you got this right and I did not.

the default values in ptable.py. dbrec.c defaults to COMM_ALARM right now for STAT

This is the default value of the function call if the parameter is not provided - which it was not, so the STAT field was always (and in my view inappropriately) set to COMM_ALARM.

I believe the value should default to the expected value of the field after IOC start-up.

• Which is UDF_ALARM if PINI is "NO" or NO_ALARM if PINI is "YES".

[tynanford]

I vote for amsg as the parameter name instead of message but it's not a deal breaker. @aqshafei and @zhangt58

@pheest thanks, how about you or I make a separate PR for your ptable amsg/stat enhancements and make that separate from this PR. I agree with you on the COMM_ALARM so maybe that can be changed as well in the new PR

[zhangt58]

I'd vote for amsg, too.

[pheest]

@tynanford

make a separate PR for your ptable amsg/stat enhancements and make that separate from this PR
Yes, will. I was rather hoping that my PR #45 "Long integers and strings" could first receive attention.

[aqshafei]

I have change the parameter message to amsg and updated the PyMethodDef documentation

[tynanford]

This looks good to me. There is overlap with #52 but I think we can merge this and then address the updates to ptable in that PR

[pheest]

I agree. This should work just fine.