Re: COMEDI wrapper (was: Re: Data acquisition in Octave)

[Olaf Till]

On Fri, Nov 21, 2008 at 01:03:50PM -0500, John W. Eaton wrote:
> On 21-Nov-2008, Olaf Till wrote:
> 
> | On Thu, Nov 20, 2008 at 12:04:49PM -0500, John W. Eaton wrote:
> | > I have the beginnings of a package here:
> | > 
> | >   ftp://velveeta.che.wisc.edu/pub/octave-comedi/comedi-0.1.tar.gz
> | > 
> | Some first remarks:
> | 
> | The crash:
> | 
> | octave-3.0.2:1> dev = comedi_open ("/dev/comedi0")
> | dev = <comedi_t object>
> | octave-3.0.2:2> comedi_close (dev)
> | ans = 0
> | octave-3.0.2:3> comedi_close (dev)
> | panic: Segmentation fault -- stopping myself...
> | attempting to save variables to `octave-core'...
> | error: octave_base_value::save_binary(): wrong type argument `comedi_t'
> | save to `octave-core' complete
> | Segmentation fault
> | address@hidden:~/devel/src/octave-comedi-0.1/src$ 
> | 
> | can probably be cured by something like this patch:
> | 
> | --- orig-comedi_open.cc     2008-11-20 17:16:50.000000000 +0100
> | +++ comedi_open.cc  2008-11-21 10:45:39.000000000 +0100
> | @@ -163,6 +163,8 @@
> |  
> |    bool print_as_scalar (void) const { return true; }
> |  
> | +  void comedi_t_set_null (void) {dev = NULL;}
> | +
> |  private:
> |  
> |    comedi_t *dev;
> | @@ -222,6 +224,25 @@
> |    return retval;
> |  }
> |  
> | +static void
> | +octave_comedi_t_set_null (octave_value& arg)
> | +{
> | +
> | +  try
> | +    {
> | +      octave_base_value& rep = (octave_base_value&) arg.get_rep ();
> 
> You are casting away const with this C-style cast, so that may be
> asking for trouble.
> 
> I think the real problem is that octave_value is not really the right
> kind of object for wrapping a pointer like this.  One way around this
> problem is to do something like what we currently use for files.
> Instead of returning an object that wraps a C FILE pointer, we return
> an index that can be used to look up the FILE pointer as needed.  So
> maybe I should do that instead.  Properly implemnting an octave_value
> type that can do the right thing here would probably be a lot more
> work...
> 
Sounds reasonable. The obvious index to return for comedi_t should be
its contained file-number, for those who prefer the raw read and
write.

> | comedilib allocates comedi_range's for fields within  comedi_t, frees
> | the ranges in comedi_close and luckily sets the pointers to NULL after
> | freeing, so it's probably not problematic to wrap comedi_range into an
> | octave_value as you did.
> | 
> | However, one usually allocates some more with or for comedilib. I
> | don't know if wrapping those into octave_value is feasible, since I do
> | not know the Octave internals good enough:
> 
> OK, I think we may have the same kinds of problems with this as with
> the comedi_t type.  The real trouble is that Octave just doesn't have
> a pointer (or reference) type, so wrapping bare pointers the way I did
> is probably not the right thing to do.  The reference counting in the
> octave_value class is probably not going to do the right thing for us.
> 
One additional thought for that (but probably you already see it the
same way): the user should not need to call something like a "delete"
function for each returned index to free the associated internal
resources, but this freeing should be performed automatically if
Octaves comedi_close is called for the respective device.

> | - comedi_polynomial_t
> | 
> | for comedi_to/from_physical (comedi_to/from_phys is deprecated);
> | allocated by user, filled in by comedilib, one potentially for each
> | range in each channel in each device;
> 
> In the version of comedi I have, these are not marked as deprecated,
> and the new functions that use the polynomial type are in the section
> of the comedilib.h header file marked with
> 
>   #ifndef _COMEDILIB_STRICT_ABI
>   /*
>      The following prototypes are _NOT_ part of the Comedilib ABI, and
>      may change in future versions without regard to source or binary
>      compatibility.  In practice, this is a holding place for the next
>      library ABI version change.
>    */
> 
> I intentionally avoided functions declared in this section of the
> header because they could change in future versions.  I'm not sure how
> likely that is, but I wanted to try to get something working first.
> 
I was wrong saying "deprecated", but the manual says these functions
are meant to take the place of comedi_to/from_phys. The new functions
can deal with hardware- and software-calibrated devices, the old
functions only with hardware-calibrated ones (the widely --- and by me
--- used National Instruments m-series devices use only software
calibration). Even if the new interface should change, the problem
with many polynomials (or other relations) will remain: one
potentially for each device/subdevice/channel/range --- if an internal
database for _these_ things is made, it needs something like a
4-dimensional index (to avoid the possibility of allocating one
specific combination many times). Instead of storing this index or a
handle to it in a user variable, one could make an exception here and
store the polynomials directly in a user variable (not as pointer, so
there is probably no problem).

Olaf