RE: asynPortDriver callbacks to I/O Intr, how to propagate an error?

[Mark Rivers <rivers@cars.uchicago.edu>]

> One of the bullet-points for ASYN is that it's actually rather loosely coupled to EPICS.  
> It seems that proposal 2, especially if the new fields are enums, makes the coupling much tighter.
> My vote is for proposal 1 -- it maintains the separation but still provides some flexibility in setting the record status.

I agree. 

I've added a new function, pasynEpicsUtils->asynStatusToEpicsAlarm() which converts the asynStatus return values to EPICS STAT and SEVR values using this mapping:

  asynSuccess = epicsAlarmNone
  asynTimeout = epicsAlarmTimeout
  asynOverflow = epicsAlarmHWLimit
  asynError = epicsAlarmRead or epicsAlarmWrite
  asynDisconnected = epicsAlarmComm
  asynDisabled = epicsAlarmDisable

That function is in asyn/devEpics, which is where the EPICS dependent code in asyn belongs.

All standard device support has been changed to use this function to set the record alarm status.  This applies both the records that have SCAN=I/O Intr and those that do not.  

The new test application, asyn/testErrors has been changed to allow setting the asynStatus values that all functions and callbacks return via an mbbo record.  All records and all interfaces supported by the standard asyn device support now have the correct alarm status and severity in these tests.

It is easy to support more alarm STAT values in the future by extending the error codes in the asynStatus enum.

Again, everything has been committed to SVN for those who can test it:

https://svn.aps.anl.gov/epics/asyn/trunk/

Thanks,
Mark



-----Original Message-----
From: Eric Norum [mailto:eric@norum.ca] 
Sent: Wednesday, May 02, 2012 8:52 AM
To: Mark Rivers
Cc: tech-talk@aps.anl.gov Talk
Subject: Re: asynPortDriver callbacks to I/O Intr, how to propagate an error?

One of the bullet-points for ASYN is that it's actually rather loosely coupled to EPICS.  It seems that proposal 2, especially if the new fields are enums, makes the coupling much tighter.
My vote is for proposal 1 -- it maintains the separation but still provides some flexibility in setting the record status.

On May 2, 2012, at 5:51 AM, Mark Rivers wrote:

> The idea of drivers being able to provide more specific values for STAT and SEVR is attractive, but I'm not sure the mechanism you've proposed is the best way to do it.
> 
> - It introduces a large difference in the manner in which I/O Intr scanned records and non I/O Intr scanned records set STAT and SEVR.  Non I/O Intr scanned records set the alarm state based on the status return from the interface methods, typically pInterface->write() and pInterface->read().  All existing drivers return one of the asynStatus enum values, and device support always sets STAT=READ_ALARM or WRITE_ALARM, and SEVR=INVALID_ALARM.  For those functions we cannot pack STAT and SEVR into the status return, because it is not backwards compatible.  This would mean that device support would handle alarms very differently depending on how the record is scanned.
> 
> - The auxStatus field is defined to be an asynStatus, which is an enum defined as follows:
> typedef enum {
>    asynSuccess,asynTimeout,asynOverflow,asynError,asynDisconnected,asynDisabled
> }asynStatus;
> 
> Setting auxStatus to something other than these enums (i.e. packing STAT and SEVR in it) is not clean programming.
> 
> I can think of two solutions:
> 
> Solution 1) Just use the existing asynStatus enums to set the alarm condition.  Allowed values of alarmCondition from alarm.h are:
> typedef enum {
>    epicsAlarmNone = NO_ALARM,
>    epicsAlarmRead,
>    epicsAlarmWrite,
>    epicsAlarmHiHi,
>    epicsAlarmHigh,
>    epicsAlarmLoLo,
>    epicsAlarmLow,
>    epicsAlarmState,
>    epicsAlarmCos,
>    epicsAlarmComm,
>    epicsAlarmTimeout,
>    epicsAlarmHwLimit,
>    epicsAlarmCalc,
>    epicsAlarmScan,
>    epicsAlarmLink,
>    epicsAlarmSoft,
>    epicsAlarmBadSub,
>    epicsAlarmUDF,
>    epicsAlarmDisable,
>    epicsAlarmSimm,
>    epicsAlarmReadAccess,
>    epicsAlarmWriteAccess,
>    ALARM_NSTATUS
> } epicsAlarmCondition;
> 
> These could map to the asynStatus enum values as follows:
>   asynSuccess = epicsAlarmNone
>   asynTimeout = epicsAlarmTimeout
>   asynOverflow = epicsAlarmHiHi or epicsAlaramHWLimit ??
>   asynError = epicsAlarmRead or epicsAlarmWrite (this is the current behavior for all values except asynSuccess)
>   asynDisconnected = epicsAlarmComm
>   asynDisabled = epicsAlarmDisable
> 
> This mechanism is simple and would provide additional alarm information even for existing drivers based on the status that they return in the interface methods.  It restricts the type of alarms that drivers can set to a subset of the possible values, but is this a significant problem?  Most of the other values are unlikely to arise in drivers.  I think we would need to continue to set SEVR=INVALID_ALARM for all of these conditions.
> 
> 
> Solution 2) Add 2 new fields to the pasynUser structure:
> 
> epicsAlarmSeverity alarmSeverity;
> epicsAlarmCondition alarmCondition;
> 
> Drivers can optionally set these values in the pasynUser passed by the interface methods and in the pasynUser passed to callbacks.  Device support would handle this as follows:
> 
> - If the status return for interface method, or the pasynUser->auxStatus for callbacks is not asynSuccess then
>  - If pasynUser->alarmSeverity is non-zero use pasynUser->alarmSeverity else use INVALID_ALARM
>  - If pasynUser->alarmCondition is non-zero use pasynUser->alarmCondition else use WRITE_ALARM or READ_ALARM
> 
> This allows more flexibility in how drivers set alarms, at the expense of more work in the driver.
> 
> Comments?
> 
> Mark

-- 
Eric Norum
eric@norum.ca