RE: ASYN - calling read after interrupt - fix :)

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

> I can't really think of a record that would mind missing data?

Here is a real-world example, and the one which actually first motivated me to use an epicsMessageQueue between the interrupt routine and the asyn driver callback thread.
 
Consider a digital I/O module that generates interrupts on each transition (0 to 1 or 1 to 0).  Sometimes 2  transitions come very quickly together (like a short pulse).  I want the EPICS record to see all transitions, I really don't want to miss any.  So I don't want to throw away data in the device support callback routine, otherwise I will see the 0 to 1 transition but miss the 1 to 0 transition.
 
Mark
 

________________________________

From: tech-talk-bounces@aps.anl.gov on behalf of Heinrich du Toit
Sent: Thu 10/11/2007 2:05 AM
To: TechTalk EPICS
Subject: RE: ASYN - calling read after interrupt - fix :)



I think that missing data is not really a problem with records and
thereforce device support for records.

If all data is required it is most probably not a record but probably
something like streamdevice or some custom record that can implement
something else in the interrupt callback to make sure all data is
processed.

So I would lean towards a solution that put priority on the newest data
out there.

I think the mutex needs to include to things:
firstly the gotvalue variable.
And secondly the actual data inside pPvt and pr.

This makes it possible to implement the following:
gotValue will go 0 at the same time process put new data into the
record. but will stay 1 until then.
And the interrupt callback will not update the data midprocess to
corrupt data.
The interrupt callback will know exactly if data has been used or not.
The interrupt callback can make sure that newest data will be processed.
And it will know if process has used data and if a new scanIORequest
should be called or not.

I can't really think of a record that would mind missing data?
The only thing I think of where this is important is for Octet with a
communications interface - in which case StreamDevice is most likely
used and not record device support.

-Heinrich


On Wed, 2007-10-10 at 16:36 -0500, Mark Rivers wrote:
> Hi Heinrich,
>
> Thanks for finding this problem and suggesting one solution.
>
> A recap of the problem for those who have not followed it:  if 2 or more
> interrupts come in rapid sucession to asyn device support, it can result
> in the the device support routine (called from scanIoRequest record
> processing) calling the drivers read() routine when it should not do so.
>
> It seems like there are 2 possible solutions.
>
> Solution 1) (basically Heinrich's suggestion):
> Ignore callbacks that happen before the previous callback has finished
> executing processCommon in asyn device support.  The right way to do
> this would be to add a mutex to protect access to the gotValue and data
> fields.  This has the disadvantage of missing data when interrupts come
> close together, even if the system could handle the average interrupt
> rate.
>
> Solution 2).
> Rather than have the callback routine call scanIoRequest, have it
> directly call dbScanLock and pr->process.  This way the callback and
> record processing are done in the same thread, and there is no need for
> a mutex.  The consequences of this solution are:
>
> - There is no missed data, as long as the system can keep up with the
> average data rate.
>
> - The callback routine must not be called at interrupt level, because
> the record will not be processed in a separate callback thread.
> However, the asyn documentation is already explicit that the
> pasynManager->interruptStart routine cannot be called at interrupt
> level, so the callback routine cannot be running at interrupt level.
>
> - The asyn driver must have already created a thread that the callback
> will run in.  Under the new scheme this callback thread will not just
> copy the data to the device support private, set a flag and call
> scanIoRequest, but it will actually process the record.  It will also
> process any records that this record forward links to.  This could
> require more stack, and will take somewhat more time.
>
> - The driver must implement some data queuing mechanism if data is not
> to be lost.  For example, my ip330 ADC driver uses an epicsMessageQueue
> to send the ADC data from the interrupt routine to the driver thread
> that does callbacks to device support.  This queue must be large enough
> to buffer the data when interrupts come close together and the callback
> task is busy processing records.
>
> I am leaning towards solution 2), but I would like to hear comments from
> those who have an opinion.  Note that the solution must be applied to
> all of the asyn device support, not just the asynInt32Array device
> support Heinrich was working with.
>
> Thanks,
> Mark
>
>
> > -----Original Message-----
> > From: tech-talk-bounces@aps.anl.gov
> > [mailto:tech-talk-bounces@aps.anl.gov] On Behalf Of Heinrich du Toit
> > Sent: Wednesday, October 10, 2007 3:19 AM
> > To: TechTalk EPICS
> > Subject: ASYN - calling read after interrupt - fix :)
> >
> > Hi there
> >
> > A while ago I posted a question asking why asyn calls my read
> > and write
> > routines after interrupts. And it was said it shouldn't.
> >
> > Well I think I figured it out.
> >
> > The interrupt callback in asyn does pPvt->gotValue = 1.
> > This is then suppose to let the process part just put the value in and
> > not call read.
> >
> > Well the problem is this:
> > If you where to call interrupts very close to each other
> > (something not
> > always avoidable) then gotvalue = 1 and scanIoRequest is
> > called once for
> > each interrupt.
> > This seems to mean process is called once for each interrupt.
> >
> > Well the first process see gotvalue = 1 and skips the read part. This
> > then works correctly and sets gotvalue = 0.
> > Now the second and so on process execution finds gotvalue = 0 and then
> > calls the read routine.
> > This offcourse only happens if the second interruptCallback executes
> > before the queued process request can run.
> >
> > The fix is actually pretty simple it seems:
> >
> > in devAsynFloat64Array.c I changed the interruptCallbackInput to this:
> >
> >
> > static void interruptCallbackInput(void *drvPvt, asynUser *pasynUser,
> >                 epicsFloat64 *value, size_t len)
> > {
> >     devAsynWfPvt *pPvt = (devAsynWfPvt *)drvPvt;
> >     waveformRecord *pwf = (waveformRecord *)pPvt->pr;
> >     int i;
> >     epicsFloat64 *pfloat64 = (epicsFloat64 *)pwf->bptr;
> >
> >     asynPrintIO(pPvt->pasynUser, ASYN_TRACEIO_DEVICE,
> >         (char *)value, len*sizeof(epicsFloat64),
> >         "%s devAsynFloat64Array::interruptCallbackInput\n",
> >         pwf->name);
> >     if (len > pwf->nelm) len = pwf->nelm;
> >     for (i=0; i<len; i++) pfloat64[i] = value[i];
> >     pPvt->nord = len;
> >     if (pPvt->gotValue = 0){
> >       pPvt->gotValue = 1;
> >       scanIoRequest(pPvt->ioScanPvt);
> >     }
> > }
> >
> >
> > (See the last 4 lines)
> >
> >
> > I'm not sure if this is 100% bullet proof
> >
> > As technically the interrupt can run while gotValue is still = 1 but
> > pwf->nord = pPvt->nord is already executed in processCommon.
> >
> > so to make this 100% bullet proof a mutex is needed I believe.
> >
> > But I think doing this:
> >
> >
> >
> > static long processCommon(dbCommon *pr)
> > {
> >     devAsynWfPvt *pPvt = (devAsynWfPvt *)pr->dpvt;
> >     waveformRecord *pwf = (waveformRecord *)pr;
> >     int status;
> >
> >     if (!pPvt->gotValue && !pr->pact) {   /* This is an initial call
> > from record */
> >         if(pPvt->canBlock) pr->pact = 1;
> >         status = pasynManager->queueRequest(pPvt->pasynUser, 0, 0);
> >         if((status==asynSuccess) && pPvt->canBlock) return 0;
> >         if(pPvt->canBlock) pr->pact = 0;
> >         if (status != asynSuccess) {
> >             asynPrint(pPvt->pasynUser, ASYN_TRACE_ERROR,
> >                 "%s processCommon, error queuing request %s\n",
> >                  pr->name, pPvt->pasynUser->errorMessage);
> >             recGblSetSevr(pr, READ_ALARM, INVALID_ALARM);
> >         }
> >     }
> >     if (pPvt->gotValue){
> >       pPvt->gotValue = 0;
> >       pwf->nord = pPvt->nord;
> >     }
> >     pPvt->gotValue = 0;
> >     return 0;
> > }
> >
> > Might improve things a little.
> >
> >
> > I'm not sure if I got all my facts correct since I'm still
> > not 100% sure
> > how the threading thing works inside ASYN.
> >
> > But this seems atleast on my initial checks as if it stop the exstra
> > read commands that seem to happen between bursts of interrupts :)
> >
> > -Heinrich
> >
> >
> >
> >