Re: NI PCI-6221 encoder (Was: JR3 force sensor driver)

On Tuesday 26 June 2007 07:00, Anders Blomdell wrote:
> Anders Blomdell wrote:

> > I have looked a little more on the comedi code. The functionality
> > seems to be lacking (or I'm totally blind). Which of the following
> > implementations is preferred:
> >
> > 1. Adding INSN_CONFIG_SET_ABZ_SRC, which accepts two arguments:
> >      index [0=A, 1=B, 2=Z]
> >      source [1-10=PFI0-PFI9, 21-26=PFI10-PFI15, 31=DISABLED, ...]
> >
> > 2. Adding INSN_CONFIG_SET_A_SRC, INSN_CONFIG_SET_B_SRC,
> > INSN_CONFIG_SET_Z_SRC, which accepts 1 argument:
> >     source [1-10=PFI0-PFI9, 21-26=PFI10-PFI15, 31=DISABLED, ...]
> >
> > 3. Modify INSN_CONFIG_SET_GATE_SRC to accept
> >      index [2=A, 3=B, 4=Z]
> >      source [1-10=PFI0-PFI9, 21-26=PFI10-PFI15, 31=DISABLED, ...]
> >
> > 4. None of the above, please fill in a rough sketch on dotted line
> > below:
> >
> >       ...........................................
>
> Selected #3 above, patches to comedi & comedilib are included. Have not
> got Z signal to work, but at least A/B works (will get back if I find
> what is missing)...

Nice work, thanks for getting the quadrature counting working and providing 
a useful demo program.  I don't use quadrature counting, and it might have 
been forever before I got around to testing it myself.

I'd prefer you add a different insn_config id, since the a/b/z inputs 
aren't really gates.  I'd go with a generic configure source, like 
INSN_CONFIG_SET_SOURCE that is otherwise similar to 
INSN_CONFIG_SET_GATE_SRC, that can be used as a catch-all for configuring 
sources for things other than gates (or clocks).  Also, you should define 
a new enum + inline functions for the possible a/b/z sources, even if they 
are mostly the same as the gate sources, especially since other than the 
PFI pins they are unknown (right?).

Some other suggestions: 

Your demo should use the same command line options as the other demos, as 
far as possible, and not conflict with them (the -a option).  Probably the 
easiest way to handle the generic options would be to split 
a "parse_option" function you could use out of the inside of the while 
loop in "parse_options" in demo/common.c.  

It would be nice if your demo explicitly set the index phase bits to 
something, just so it's clear to the reader that its something they might 
need to think about.

-- 
Frank