Bug 61692

Comment on attachment 75783 [details] [review]
Patch (against latest git version) with driver implementation

Review of attachment 75783 [details] [review]:
-----------------------------------------------------------------

::: libfprint/drivers/vfs5011.c
@@ +25,5 @@
> +}
> +
> +static void debugprint(const char *line)
> +{
> +#ifdef DEBUG

There's fp_dbg/fp_err/fp_whatever already. Please don't reinvent a wheel

@@ +67,5 @@
> +static int usb_send(libusb_device_handle *handle, unsigned char *data, unsigned size)
> +{
> +	int transferred = 0;
> +	int r = libusb_bulk_transfer(handle, VFS5011_OUT_ENDPOINT, data, size,
> +	                                    &transferred, VFS5011_DEFAULT_WAIT_TIMEOUT);

No synchronous transfers are allowed in driver

@@ +85,5 @@
> +static int usb_recv(libusb_device_handle *handle, unsigned endpoint, unsigned char *buf,
> +             unsigned max_bytes, int *transferred)
> +{
> +	int r = libusb_bulk_transfer(handle, endpoint, buf, max_bytes, transferred,
> +	                             VFS5011_DEFAULT_WAIT_TIMEOUT);

Same

@@ +404,5 @@
> +
> +// Rescale image to account for variable swiping speed
> +int vfs5011_rescale_image(unsigned char *image, int input_lines,
> +                          unsigned char *output, int max_output_lines)
> +{

Could you use existing scale functions?

@@ +435,5 @@
> +//  		if (! on_good_offsets && (i >= GOOD_OFFSETS_CRITERION)) {
> +//  			if (get_deviation_int(offsets + i - GOOD_OFFSETS_CRITERION, GOOD_OFFSETS_CRITERION) <
> +//  			    GOOD_OFFSETS_THRESHOLD)
> +//  				on_good_offsets = 1;
> +//  		}

Please drop commented out blocks of code

@@ +876,5 @@
> +struct fp_img_driver vfs5011_driver =
> +{
> +	.driver =
> +	{
> +		.id = 12,

Please don't use hardcoded ID anymore, define your own in driver_ids.h and use that one

@@ +886,5 @@
> +
> +	.flags = 0,
> +	.img_width = VFS5011_IMAGE_WIDTH,
> +	.img_height = -1,
> +	.bz3_threshold = 20,

Why threshold is so low?

> Why threshold is so low?

I wrote an e-mail to the mailing list explaining the situation in detail back in December: http://lists.freedesktop.org/archives/fprint/2012-December/000376.html

> No synchronous transfers are allowed in driver

The fact that you didn't tell this before is a bit surprising given that the code has been available for more than a month now.

Switching to asynchronous transfers is a major change that would require some effort. And let me put it frankly: your reply does not exactly motivate me to do that.

(In reply to comment #2)
> > Why threshold is so low?
> 
> I wrote an e-mail to the mailing list explaining the situation in detail
> back in December:
> http://lists.freedesktop.org/archives/fprint/2012-December/000376.html

OK

> > No synchronous transfers are allowed in driver
> 
> The fact that you didn't tell this before is a bit surprising given that the
> code has been available for more than a month now.

You didn't ask for assistance in driver development, and I didn't pay enough attention to maillist for last months. 
 
> Switching to asynchronous transfers is a major change that would require
> some effort. And let me put it frankly: your reply does not exactly motivate
> me to do that.

Well, I'll try to explain you why it's an requirement:
see libfprint/libfprint/poll.c quote:

 * libfprint does not create internal library threads and hence can only
 * execute when your application is calling a libfprint function. However,
 * libfprint often has work to be do, such as handling of completed USB
 * transfers, and processing of timeouts required in order for the library
 * to function. Therefore it is essential that your own application must
 * regularly "phone into" libfprint so that libfprint can handle any pending
 * events.
 *
 * The function you must call is fp_handle_events() or a variant of it. This
 * function will handle any pending events, and it is from this context that
 * all asynchronous event callbacks from the library will occur. You can view
 * this function as a kind of iteration function.

Usually application adds libfprint FDs to its poll/select call and calls fp_handle_events_timeout() on any event for those FDs. That basically means that libfprint runs in main loop, same loop in which UI runs. So it's not a good idea to use synchronous libusb calls, because they can take >1s to complete, so it can "freeze" UI for 1s.

Anyway, it's not complicated to convert your driver to asynchronous model. Join #fprint@irc.freenode.org IRC channel and I'll try to help you

Marco Hoffmeier reports that 138a:0018 also works with this driver: https://github.com/ars3niy/fprint_vfs5011/pull/1

Created attachment 77561 [details] [review]
Patch with driver implementation (suits both 0.5.0 and latest git snapshot)

I've updated the driver, partially switched to asynchronous transfers for device initialization together with some code cleanup.

There's still a short initialization sequence that has to be done before capturing, which is done synchronously.

If I understand correctly, an application using libfprint assumes that the device is ready for capturing the image right after fp_async_enroll/verify/identify_start returns.

So that if pre-capture initialization is done asynchronously, the user will be asked to deploy his finger right away, before the initalization is completed. And if there is a delay during initialization and the user is quick enough, hse will actually swipe the finger before the device is initialized.

I'm not sure that this is better than UI freeze, that's why I left synchronous transfers there. Some kind of intermediate "now you can swipe your finger" callback could seem a better solution, but that's a lot of work.

In any case, I'm also attaching a patch to make all initialization asynchronous.

Created attachment 77562 [details]
Make all USB transfer asynchronous

Vasily, can you review it?

Comment on attachment 77561 [details] [review]
Patch with driver implementation (suits both 0.5.0 and latest git snapshot)

Review of attachment 77561 [details] [review]:
-----------------------------------------------------------------

::: libfprint/drivers/vfs5011.c
@@ +1,1 @@
> +#include <stdio.h>

Please add a copyright header to vfs5011.c and vfs5011.h, you can borrow one from any driver

@@ +10,5 @@
> +#include "driver_ids.h"
> +
> +#include "vfs5011_proto.h"
> +
> +static char *get_debugfiles_path(void)

Please use fp_dbg macro, don't introduce driver-specific debug stuff.

@@ +147,5 @@
> +		fpi_ssm_mark_aborted(ssm, -EINVAL);
> +		goto out;
> +	}
> +	
> +	struct usb_action *action = &data->actions[ssm->cur_state];

Please don't mix declarations with code

@@ +189,5 @@
> +		fpi_ssm_mark_aborted(ssm, -EINVAL);
> +		return;
> +	}
> +	
> +	struct usb_action *action = &data->actions[ssm->cur_state];

Same as above

@@ +300,5 @@
> +}
> +
> +//====================== utils =======================
> +
> +#if VFS5011_LINE_SIZE > INT_MAX/(256*256)

It would be better to perform this test somewhere in configure.ac, I don't think that it's a good
idea to emit compilation error after configure script succeed

@@ +365,5 @@
> +
> +static void median_filter(int *data, int size, int filtersize)
> +{
> +	int i;
> +	int *result = (int *)malloc(size*sizeof(int));

Please use g_malloc from glib, or implement error handling, I prefer first way.

@@ +508,5 @@
> +};
> +
> +enum {
> +	DEV_OPEN_START,
> +// 	DEV_OPEN_INIT_COMPLETE,

No dead code please, even a single line

@@ +585,5 @@
> +	return 0;
> +}
> +
> +void save_pgm(const char *filename, unsigned char *data, int width, int height)
> +{

Remove all debug stuff that was used during development, I don't think we need it

@@ +984,5 @@
> +		return r;
> +	}
> +	
> +	struct fpi_ssm *ssm;
> +	ssm = fpi_ssm_new(dev->dev, open_loop, DEV_OPEN_NUM_STATES);

Don't perform device initialization in dev_open, right place to do init is dev_activate.
Please note that image capture loop should not finish until explicit dev_deactivate call.

Typical loop looks like this:
(1) device init -> (2) start finger detect -> (3) finger detected -> (4) image capture -> (5) send image to lib -> (6) jump to (2)

Arseniy, could you catch me in IRC, so we can discuss all driver details and avoid another submit/review round?

> Don't perform device initialization in dev_open, right place to do init is
> dev_activate.

Right now putting it in dev_open is the best choice. Doing it in dev_activate would cause a problem which I mentioned in comment #5 (any thought on this, by the way?) – only much worse since this initialization takes a second or two.

Also, AFAIR windows does this initialization immediately when I plug in the device into USB port. (Thin is embedded sensor in a laptop but one can still "plug it in" in VirtualBox).

(In reply to comment #10)
> > Don't perform device initialization in dev_open, right place to do init is
> > dev_activate.
> 
> Right now putting it in dev_open is the best choice. Doing it in
> dev_activate would cause a problem which I mentioned in comment #5 (any
> thought on this, by the way?) – only much worse since this initialization
> takes a second or two.

Could you implement some check too see if initialization was completed earlier? Maybe some register value changes after init, etc? Library doesn't expect any device init in dev_open, so it's not a good idea.

Anyway, I don't like workarounds, clean solution is always better. Maybe we need to add another state into fp_imgdev_state, something like IMGDEV_STATE_INITIALIZING, right before AWAIT_FINGER_ON, so libfprint user will be aware when it's necessary to show "scan your finger" message. 

> Also, AFAIR windows does this initialization immediately when I plug in the
> device into USB port. (Thin is embedded sensor in a laptop but one can still
> "plug it in" in VirtualBox).

That's OK, but unfortunately libfprint driver is "stateless", i.e. it can't preserve any data between fp scan sessions, so we can't mimic _that_ windows behavior

> Could you implement some check too see if initialization was completed
> earlier? Maybe some register value changes after init, etc? 

Unfortunately I don't know how to query registers on this device. During initialization I'm sending some blobs and occasionally read something back, but I have no idea what it all means :-/

But it does not harm to do this initialization many times. It would not harm to do it every time dev_activate is called – the hardware seems to be fine with that.

The only problem is how to handle the delay which happens during this initialization properly, so that it won't disappoint the user.

> Library doesn't expect any device init in dev_open, so it's not a good idea.

But is there some technical reason why it should not be there right now? I mean, can this (doing device initialization in dev_open) break things, cause incorrect behavior in some situations?

By the way, I also have an unrelated question. If an application opens the device and then enrolls/verifies/identifies fingers a few times, will dev_activate be called every time when the finger needs to be scanned, or just once?

Sorry, I cannot chat now, I don't have more time today.

(In reply to comment #12)
> > Could you implement some check too see if initialization was completed
> > earlier? Maybe some register value changes after init, etc? 
> 
> Unfortunately I don't know how to query registers on this device. During
> initialization I'm sending some blobs and occasionally read something back,
> but I have no idea what it all means :-/

Maybe it makes sense to reverse-engineer protocol a bit? I'll take a look at it probably this weekend

> But it does not harm to do this initialization many times. It would not harm
> to do it every time dev_activate is called – the hardware seems to be fine
> with that.
> 
> The only problem is how to handle the delay which happens during this
> initialization properly, so that it won't disappoint the user.
>
> 
> > Library doesn't expect any device init in dev_open, so it's not a good idea.
> 
> But is there some technical reason why it should not be there right now? I
> mean, can this (doing device initialization in dev_open) break things, cause
> incorrect behavior in some situations?

Yes, there's no synchronization points for dev_open, so you can't use async transfers in it, and synchronous transfers are not allowed in libfprint. 
 
> By the way, I also have an unrelated question. If an application opens the
> device and then enrolls/verifies/identifies fingers a few times, will
> dev_activate be called every time when the finger needs to be scanned, or
> just once?

Depends on libfprint internal usage. Current master calls dev_activate for each scan, but I've a pending commit that changes enroll routine to perform 5 scans, and it uses single dev_activate for 5 scans.
 
> Sorry, I cannot chat now, I don't have more time today.

OK

> there's no synchronization points for dev_open, so you can't use async
> transfers in it, and synchronous transfers are not allowed in libfprint.

But it works for me with async transfers in dev_open :-/ Both fprint_demo that uses asynchronous API and command-line examples using synchronous API work fine.

You gave a theoretical explanation why it's bad, but I'm not a computer programmer, I'm a physicist, and I don't understand this theory. Can you please also give a practical explanation, i.e. what exactly will break and in which situation?

As a side note, vfs301 driver, which just happened to be the one I used as an example, has synchronous transfers :p (and even separate debug system).

(In reply to comment #14)
> > there's no synchronization points for dev_open, so you can't use async
> > transfers in it, and synchronous transfers are not allowed in libfprint.
> 
> But it works for me with async transfers in dev_open :-/ Both fprint_demo
> that uses asynchronous API and command-line examples using synchronous API
> work fine.
> 
> You gave a theoretical explanation why it's bad, but I'm not a computer
> programmer, I'm a physicist, and I don't understand this theory. Can you
> please also give a practical explanation, i.e. what exactly will break and
> in which situation?

Just a simple race:

app calls dev_open -> init async transfer starts -> app calls dev_activate -> another async transfer starts -> it breaks initialization (e.g. response from init is interpreted as response from activate) -> driver failure

> As a side note, vfs301 driver, which just happened to be the one I used as
> an example, has synchronous transfers :p (and even separate debug system).

I didn't review vfs301 driver, so I'm not responsible for its inclusion :)

(In reply to comment #15)
> (In reply to comment #14)
<snip>
> > As a side note, vfs301 driver, which just happened to be the one I used as
> > an example, has synchronous transfers :p (and even separate debug system).
> 
> I didn't review vfs301 driver, so I'm not responsible for its inclusion :)

I'm probably the one who reviewed it. It worked (at least the original author claimed it did), and I'm really not familiar with the libfprint drivers, seeing as I mostly worked on fprintd and the integration with the rest of the user-space.

> app calls dev_open -> init async transfer starts -> app calls dev_activate
> -> another async transfer starts -> it breaks initialization

Hang on. Don't you need a pointer to fp_dev to activate a device? And you only get this pointer from fp_dev_open_cb callback, which callback is invoked after all async transfers in dev_open are completed. Am I missing something?

*poke*
Could you please speed up the review process?
There are many users suffered from this.
Thanks.

(In reply to comment #18)
> *poke*
> Could you please speed up the review process?
> There are many users suffered from this.
> Thanks.

It's already reviewed, and there were no new version to review.
I won't accept driver with synchronous transfers.

Could you advice us which driver is good for reference, so that Arseniy could follow that one to rewrite it.
Or if Arseniy is unavailable to do it now, I can give it a try.
Thanks.

(In reply to comment #20)
> Could you advice us which driver is good for reference, so that Arseniy
> could follow that one to rewrite it.

There's no need to rewrite my driver, just minor changes here and there.

> Or if Arseniy is unavailable to do it now, I can give it a try.

I'm generally available, but Vasily has been somewhat impolite to me. Never answered my question in comment #17, for example. That's why I'm not putting too much effort into speeding up the process, simply because it's not exactly fun.

There is, however, fully asynchronous version of the driver: https://bugs.freedesktop.org/attachment.cgi?id=77562.

(In reply to comment #21)
> (In reply to comment #20)
> > Could you advice us which driver is good for reference, so that Arseniy
> > could follow that one to rewrite it.
> 
> There's no need to rewrite my driver, just minor changes here and there.
> 
> > Or if Arseniy is unavailable to do it now, I can give it a try.
> 
> I'm generally available, but Vasily has been somewhat impolite to me.

I'm very sorry if I was impolite to you, it wasn't intended. I'm just a bit tired of arguing why drivers MUST be asynchronous. I'll write a libfprint driver guideline when I get some spare time

> Never
> answered my question in comment #17, for example.

Missed that one. Anyway:

> Hang on. Don't you need a pointer to fp_dev to activate a device? And you only > get this pointer from fp_dev_open_cb callback, which callback is invoked after > all async transfers in dev_open are completed. Am I missing something?

See img_dev_open(), it doesn't wait for any async xfers from dev_open(), only if you explicitly wait for async transfers to complete in dev_open()

> That's why I'm not putting
> too much effort into speeding up the process, simply because it's not
> exactly fun.

Heh

> There is, however, fully asynchronous version of the driver:
> https://bugs.freedesktop.org/attachment.cgi?id=77562.

OK, will take a look tonight. Could you squash 2 patches into single meanwhile? Thanks!

P.S. You still can catch me in IRC for faster communication...

Please allow me to do the boring job.
I refine the code by removing the tailing space, cutting off at 80 chars, moving the open brace, and removing the C99 // comments.
Hoping this will make the process more smoothly.

Created attachment 82310 [details] [review]
0001-lib-Add-VFS5011-driver.patch

Comment on attachment 82310 [details] [review]
0001-lib-Add-VFS5011-driver.patch

Review of attachment 82310 [details] [review]:
-----------------------------------------------------------------

::: libfprint/drivers/vfs5011.c
@@ +33,5 @@
> +	}
> +	fp_dbg(s);
> +#endif
> +}
> +

Please drop all debugging stuff from driver. Only debug logs via fp_dbg are allowed.

@@ +321,5 @@
> +static void median_filter(int *data, int size, int filtersize)
> +{
> +	int i;
> +	int *result = (int *)malloc(size*sizeof(int));
> +	int *sortbuf = (int *)malloc(filtersize*sizeof(int));

Please use g_malloc or g_malloc0 here.

@@ +329,5 @@
> +		if (i1 < 0)
> +			i1 = 0;
> +		if (i2 >= size)
> +			i2 = size-1;
> +		memmove(sortbuf, data+i1, (i2-i1+1)*sizeof(int));

g_memmove()

@@ +330,5 @@
> +			i1 = 0;
> +		if (i2 >= size)
> +			i2 = size-1;
> +		memmove(sortbuf, data+i1, (i2-i1+1)*sizeof(int));
> +		qsort(sortbuf, i2-i1+1, sizeof(int), cmpint);

I wonder if there's glib wrapper for that one...

@@ +364,5 @@
> +	};
> +	int i;
> +	float y = 0.0;
> +	int line_ind = 0;
> +	int *offsets = (int *)malloc(input_lines * sizeof(int));

g_malloc()

@@ +372,5 @@
> +
> +	if (debugpath != NULL) {
> +		sprintf(name, "%s/offsets%d.dat", debugpath, (int)time(NULL));
> +		debugfile = fopen(name, "wb");
> +	}

Drop debug stuff except logs

@@ +945,5 @@
> +			array_n_elements(vfs5011_initialization);
> +		data->init_sequence.actions = vfs5011_initialization;
> +		data->init_sequence.device = dev;
> +		data->init_sequence.receive_buf =
> +			malloc(VFS5011_RECEIVE_BUF_SIZE);

g_malloc()

@@ +989,5 @@
> +	int r = libusb_reset_device(dev->udev);
> +	if (r != 0) {
> +		fp_err("Failed to reset the device");
> +		return r;
> +	}

Is it necessary? Btw, doc says that handle may be invalid after reset, and you're reusing same handle later.

@@ +1000,5 @@
> +
> +	struct fpi_ssm *ssm;
> +	ssm = fpi_ssm_new(dev->dev, open_loop, DEV_OPEN_NUM_STATES);
> +	ssm->priv = dev;
> +	fpi_ssm_start(ssm, open_loop_complete);

And no one waits for this ssm (and async USB reqs) to complete here. It's a race condition I've described earlier. Please move device initialization into dev_activate().

@@ +1035,5 @@
> +}
> +
> +static void dev_deactivate(struct fp_img_dev *dev)
> +{
> +	fpi_imgdev_deactivate_complete(dev);

How do you ensure that _all_ transfers are completed now? fpi_imgdev_deactivate_complete should be called only after deactivation has been completed. For this driver "cancel" button in fprint_demo won't work.

Btw, I feel like not all issues from previous patch were fixed

(In reply to comment #25)
> @@ +330,5 @@
> > +			i1 = 0;
> > +		if (i2 >= size)
> > +			i2 = size-1;
> > +		memmove(sortbuf, data+i1, (i2-i1+1)*sizeof(int));
> > +		qsort(sortbuf, i2-i1+1, sizeof(int), cmpint);
> 
> I wonder if there's glib wrapper for that one...

https://developer.gnome.org/glib/2.34/glib-Miscellaneous-Utility-Functions.html#g-qsort-with-data

Might not be necessary to use the GLib wrapper.

Sorry for reply late, I was on vacation, I'll try to revise those part of code soon.

(In reply to comment #28)
> Sorry for reply late, I was on vacation, I'll try to revise those part of
> code soon.

No pb, I was busy as well :)

Created attachment 83699 [details] [review]
0001-lib-Add-VFS5011-driver.patch

Sorry to take so long to resend the patch.

@@ +1000,5 @@
>> +
>> +	struct fpi_ssm *ssm;
>> +	ssm = fpi_ssm_new(dev->dev, open_loop, DEV_OPEN_NUM_STATES);
>> +	ssm->priv = dev;
>> +	fpi_ssm_start(ssm, open_loop_complete);
>
>And no one waits for this ssm (and async USB reqs) to complete here. It's a race >condition I've described earlier. Please move device initialization into >dev_activate().
The code is required and I can imagine the race condition you mentioned, but the fingerprint device won't work if move the code to dev_activate() function.
So, I add a check in dev_activate() function to make sure open_loop_complete() have finished to continue.

And you had mentioned that do we need to do reset in dev_open()
r = libusb_reset_device(dev->udev);
        if (r != 0) {
                fp_err("Failed to reset the device");
                return r;
        }
I checked if I removed the code, the device doesn't work.

I'm learning libfprint by modifying the code, so please give me more explicit advise, so that I can know how to fix it.
Thanks.

(In reply to comment #30)
> Created attachment 83699 [details] [review] [review]
> 0001-lib-Add-VFS5011-driver.patch
> 
> Sorry to take so long to resend the patch.
> 
> @@ +1000,5 @@
> >> +
> >> +	struct fpi_ssm *ssm;
> >> +	ssm = fpi_ssm_new(dev->dev, open_loop, DEV_OPEN_NUM_STATES);
> >> +	ssm->priv = dev;
> >> +	fpi_ssm_start(ssm, open_loop_complete);
> >
> >And no one waits for this ssm (and async USB reqs) to complete here. It's a race >condition I've described earlier. Please move device initialization into >dev_activate().

> The code is required and I can imagine the race condition you mentioned, but
> the fingerprint device won't work if move the code to dev_activate()
> function.

Because you need to wait for completion of these transfers, but probably you don't.

> So, I add a check in dev_activate() function to make sure
> open_loop_complete() have finished to continue.

Please, no workarounds, let's fix an issue instead.

> And you had mentioned that do we need to do reset in dev_open()
> r = libusb_reset_device(dev->udev);
>         if (r != 0) {
>                 fp_err("Failed to reset the device");
>                 return r;
>         }
> I checked if I removed the code, the device doesn't work.

I guess it doesn't work on second time, because there's no proper deinit sequence and device is still in active state.

> I'm learning libfprint by modifying the code, so please give me more
> explicit advise, so that I can know how to fix it.
> Thanks.

Take a look at other drivers, almost all for AuthenTec devices are pretty simple. Try to catch me on IRC (#fprint@irc.freenode.net) if you have further questions.

Really looking forward to see this Patch included. Thanks for the hard work to all involved!

Sorry to take so long to re-submit the patch,
I'm not familiar with the lib, so takes some time to study it.

For your questions.
1. Because you need to wait for completion of these transfers, but probably you don't.
Actually, we don't have to wait.
When open the device, it won't continue to next state
if fpi_imgdev_open_complete() is not be called.
And we call it at open_loop_complete(), so that makes sure the transfers are all done. There is no race condition.

2. Please, no workarounds, let's fix an issue instead.
Okay, I removed them.
And this issue is not exist as explanation above.

3. I guess it doesn't work on second time, because there's no proper deinit sequence and device is still in active state.
I don't know the deinit commands.
Arseniy, please give me some advice if you know how to get the deinit commands.
BTW, since we don't have the deinit commands, so we call libusb_reset_device() every time when we open the device.
That will probably reset the device and then it can be init again.

Created attachment 88670 [details] [review]
0001-lib-Add-VFS5011-driver-v2.patch

(In reply to comment #33)
> Sorry to take so long to re-submit the patch,
> I'm not familiar with the lib, so takes some time to study it.
> 
> For your questions.
> 1. Because you need to wait for completion of these transfers, but probably
> you don't.
> Actually, we don't have to wait.
> When open the device, it won't continue to next state
> if fpi_imgdev_open_complete() is not be called.
> And we call it at open_loop_complete(), so that makes sure the transfers are
> all done. There is no race condition.

OK, you're right

> 2. Please, no workarounds, let's fix an issue instead.
> Okay, I removed them.
> And this issue is not exist as explanation above.

OK

> 3. I guess it doesn't work on second time, because there's no proper deinit
> sequence and device is still in active state.
> I don't know the deinit commands.
> Arseniy, please give me some advice if you know how to get the deinit
> commands.
> BTW, since we don't have the deinit commands, so we call
> libusb_reset_device() every time when we open the device.
> That will probably reset the device and then it can be init again.

Anyway, I'm OK for libusb_reset_device() here for now, but only if someone will continue to investigate how to deinit device properly.

Out of curiosity, is this device available as a standalone USB device?

I'll write "Validity Sensors" an email to see if they can provide us the commands.
And I don't know if there is a standalone USB drive with this chip. There is no information from their website.
http://www.validityinc.com/index.php

I don't get any reply from Validity Sensors for a whole week.
So, I think this is probably the best we can do for now.

(In reply to comment #37)
> I don't get any reply from Validity Sensors for a whole week.
> So, I think this is probably the best we can do for now.

Could you also estimate power consumption (via powertop) before first use of the fingerprint scanner (i.e. after fresh boot) and after first use? I'm a bit worrying that since scanner is not stopped properly it'll consume more power.

Created attachment 89064 [details]
powertop snapshot before and after running fprint_demo

I took 4 snapshots of powertop.
1. Before running fprint_demo
2. After running fprint_demo and do scan and verify fingerprint
3. Reboot my laptop and remove all USB devices and then do powertop
4. After running fprint_demo and do verify fingerprint
Looks good to me, no visible power loss.

Based on the windows driver readme here: http://download.lenovo.com/ibmdl/pub/pc/pccbbs/mobiles/lz4xr3a2.txt , I am inclined to beleive this patch will work for PIDs 0015 and 0017 too.

I should receive my T440p which has a 138a:0017 Validity Sensors fingerprint reader in a couple of weeks, I will confirm then.

Created attachment 91917 [details] [review]
Validity :0017 support

This patch is indeed working for :0017 Validity sensor, I've attached a small patch which enables its support.

(In reply to comment #41)
> Created attachment 91917 [details] [review] [review]
> Validity :0017 support
> 
> This patch is indeed working for :0017 Validity sensor, I've attached a
> small patch which enables its support.

Confirming that the patch for :0017 is working on my T440 with no issues so far.

What is chance to include this path in next release?

(In reply to comment #43)
> What is chance to include this path in next release?

It'll be included. I still need to finish several features and a single driver for a next release.

(In reply to comment #44)
> (In reply to comment #43)
> > What is chance to include this path in next release?
> 
> It'll be included. I still need to finish several features and a single
> driver for a next release.

Thank you for reply. :) You save my day.

*** Bug 74635 has been marked as a duplicate of this bug. ***

I use this driver and I have run into bug when sudo with pam_fprintd cannot be killed with ^C or anything besides SIGKILL when fingerprint is awaited. I can't check this for any driver besides this one, but I suspect that this is driver's issue.

Also, when I get "Swipe your finger again", the reader is shutted down. It doesn't react to my finger until "Verification timed out" is printed. After that, until fprintd dies (which stays running for some time), PAM module does not work.

Created attachment 98874 [details] [review]
Validity :0017 support (v2)

Removed unnecessary .rules file patching.

I can't use the driver with Ubuntu 14.04.

One of the 3 following things happens:
1. fprintd-enroll segfaults.
2. The message "Enroll result: enroll-completed" is shown, but there're no prints under ~/.fprint/prints.
3. I get the error message:
   Enroll result: enroll-unknown-error
   VerifyStop failed: Message did not receive a reply (timeout by message bus)

Whoopie,
The libfprint doesn't release new version yet.
If you would like to try it on Ubuntu 14.04, you can try the package I built.
http://people.ubuntu.com/~acelan/libfprint/

AceLan,

I patched libfprint git with the attached patch from this bugreport. Even with your compiled version, I have the same results as described above.

The syslog shows:

Oct  1 13:47:29 v131 kernel: [212177.755988] usb 2-1.3: reset full-speed USB device number 3 using ehci-pci
Oct  1 13:47:31 v131 kernel: [212179.490094] traps: fprintd[20034] trap int3 ip:7f3c9bb5dc13 sp:7fff06996f70 error:0

I tried the git version ( 35e356f62 ) and the 2 patchs on a RHEL 7 with a t440s, and it work fine. that work fine, no segfault on my side with fprintd-enroll and verify.

Ok, spoke too fast, it always say "no match"

So, testing with others people fingers, the problem is the speed of sliding the finger. So it work fine for me, provided I do slide more slowly.

Created attachment 110999 [details] [review]
0001-lib-Add-VFS5011-driver.patch

Please test if this version works for you.

With the lastest version, I get the error message:
   Enroll result: enroll-unknown-error
   VerifyStop failed: Message did not receive a reply (timeout by message bus)

The syslog shows:
Dec 18 21:07:52 v131 kernel: [26042.424432] usb 2-1.3: reset full-speed USB device number 3 using ehci-pci
Dec 18 21:07:54 v131 kernel: [26044.143346] traps: fprintd[20976] trap int3 ip:7f1ca1119c13 sp:7fffbcfcecd0 error:0

(In reply to Whoopie from comment #58)
> With the lastest version, I get the error message:
>    Enroll result: enroll-unknown-error
>    VerifyStop failed: Message did not receive a reply (timeout by message
> bus)
> 
> The syslog shows:
> Dec 18 21:07:52 v131 kernel: [26042.424432] usb 2-1.3: reset full-speed USB
> device number 3 using ehci-pci
> Dec 18 21:07:54 v131 kernel: [26044.143346] traps: fprintd[20976] trap int3
> ip:7f1ca1119c13 sp:7fffbcfcecd0 error:0

Well, I've only replaced free() calls with g_free() and time() with g_get_real_time(). AceLan, could you check what's wrong?

Hi, I'm on vacation until EOY.
I'll see what I can do when I'm available.

Got it working with the change mentioned in http://article.gmane.org/gmane.linux.fprint/2490 (removing the line 770
in file libfprint/drivers/vfs5011.c --"RECV(VFS5011_IN_ENDPOINT_CTRL2,
8)").

I can confirm this patch works fine applied on top of libfprint_0.5.1-1 from the Debian Archives. I just _debianized_ it for ease applying at https://gist.github.com/diega/2e1d2dbbbb7700480de0 (I though it was better to upload there instead of taint this bug report with another file)

(In reply to Diego López León from comment #62)
> I can confirm this patch works fine applied on top of libfprint_0.5.1-1 from
> the Debian Archives. I just _debianized_ it for ease applying at
> https://gist.github.com/diega/2e1d2dbbbb7700480de0 (I though it was better
> to upload there instead of taint this bug report with another file)

Diego, please test it against git master. There were some changes since 0.5.1, such as several scans during enrollment for all imaging scanners.

Vasily,
Thanks for your recommendation, I just tested it against current master and it seems to work fine.

I can even notice a better matching rate comparing with 5.1. I don't know if the differences between 5.1 and master justifies this behavior or it is just my impression, but it worth to mention.

Again, the patch for applying it on top of a debian build for master@35e356f is in https://gist.github.com/diega/3b8a8cc5f5588076fd39

(In reply to Diego López León from comment #64)
> Vasily,
> Thanks for your recommendation, I just tested it against current master and
> it seems to work fine.
> 
> I can even notice a better matching rate comparing with 5.1. I don't know if
> the differences between 5.1 and master justifies this behavior or it is just
> my impression, but it worth to mention.
> 
> Again, the patch for applying it on top of a debian build for master@35e356f
> is in https://gist.github.com/diega/3b8a8cc5f5588076fd39

Thanks for testing!

Vasily, let me know when you've merged/reviewed, and I'll do a release.
I'm a bit lost on the status :)

(In reply to Bastien Nocera from comment #66)
> Vasily, let me know when you've merged/reviewed, and I'll do a release.
> I'm a bit lost on the status :)

Sure.

Merged.

http://cgit.freedesktop.org/libfprint/libfprint/commit/?id=dc537ef2c9b1ee62e88db02560abcead4f9486f9