Launchpad has imported 18 comments from the remote bug at https://bugs.freedesktop.org/show_bug.cgi?id=61692.
If you reply to an imported comment from within Launchpad, your comment will be sent to the remote bug automatically. Read more about Launchpad's inter-bugtracker facilities at https://help.launchpad.net/InterBugTracking. ------------------------------------------------------------------------ On 2013-03-02T15:48:38+00:00 Arseniy wrote: Created attachment 75783 Patch (against latest git version) with driver implementation So far no one said that my driver sucks, so maybe let's try to merge it. I tried to create a proper patch, please have a look at it. I'll be happy to make any necessary changes to the patch and/or my code. Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/25 ------------------------------------------------------------------------ On 2013-03-19T13:29:00+00:00 anarsoul wrote: Comment on attachment 75783 Patch (against latest git version) with driver implementation Review of attachment 75783: ----------------------------------------------------------------- ::: 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? Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/28 ------------------------------------------------------------------------ On 2013-03-23T11:32:39+00:00 Arseniy wrote: > 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. Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/29 ------------------------------------------------------------------------ On 2013-03-26T12:05:25+00:00 anarsoul wrote: (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 #[email protected] IRC channel and I'll try to help you Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/30 ------------------------------------------------------------------------ On 2013-04-01T18:35:16+00:00 Arseniy wrote: Marco Hoffmeier reports that 138a:0018 also works with this driver: https://github.com/ars3niy/fprint_vfs5011/pull/1 Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/31 ------------------------------------------------------------------------ On 2013-04-07T18:48:57+00:00 Arseniy wrote: Created attachment 77561 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. Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/32 ------------------------------------------------------------------------ On 2013-04-07T18:49:55+00:00 Arseniy wrote: Created attachment 77562 Make all USB transfer asynchronous Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/33 ------------------------------------------------------------------------ On 2013-04-24T15:19:28+00:00 Bugzilla-x wrote: Vasily, can you review it? Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/35 ------------------------------------------------------------------------ On 2013-04-24T15:40:01+00:00 anarsoul wrote: Comment on attachment 77561 Patch with driver implementation (suits both 0.5.0 and latest git snapshot) Review of attachment 77561: ----------------------------------------------------------------- ::: 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) Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/36 ------------------------------------------------------------------------ On 2013-04-24T15:43:08+00:00 anarsoul wrote: Arseniy, could you catch me in IRC, so we can discuss all driver details and avoid another submit/review round? Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/37 ------------------------------------------------------------------------ On 2013-04-24T17:43:21+00:00 Arseniy wrote: > 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). Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/38 ------------------------------------------------------------------------ On 2013-04-24T18:31:41+00:00 anarsoul wrote: (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 Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/39 ------------------------------------------------------------------------ On 2013-04-24T19:21:35+00:00 Arseniy wrote: > 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. Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/40 ------------------------------------------------------------------------ On 2013-04-24T19:39:24+00:00 anarsoul wrote: (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 Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/41 ------------------------------------------------------------------------ On 2013-04-25T19:21:06+00:00 Arseniy wrote: > 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). Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/42 ------------------------------------------------------------------------ On 2013-04-25T19:35:05+00:00 anarsoul wrote: (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 :) Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/43 ------------------------------------------------------------------------ On 2013-04-25T20:00:37+00:00 Bugzilla-x wrote: (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. Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/44 ------------------------------------------------------------------------ On 2013-04-30T20:03:37+00:00 Arseniy wrote: > 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? Reply at: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/790183/comments/45 ** Changed in: libfprint Status: Unknown => Confirmed ** Changed in: libfprint Importance: Unknown => Wishlist -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/790183 Title: [138a:0011] Fingerprint reader Validity Sensors not recognized To manage notifications about this bug go to: https://bugs.launchpad.net/libfprint/+bug/790183/+subscriptions -- ubuntu-bugs mailing list [email protected] https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
