Created attachment 58282 [details] es603 driver I haven't find driver for ES603 in libfprint, so I've decided to write one. It is quite alpha, but maybe it will be of use. I've tried it on Lenovo V560 notebook with fprint_demo and sample apps in libfprint and it works. Fingerprint device USB ID: 1c7a:0603 Main problems: - I don't have documentation, so some parts could be absolutely wrong - Resolution is very low (192px), so detection is not very accurate - Calibration part (on dev open) is done synchronously
How is this driver different from the one at http://code.google.com/p/etes603/ ?
Created attachment 70927 [details] [review] add support for Egistec ES603 device I am asking for review and for integration into trunk. Note that some functions are unused but could be used in the future. Thanks a lot.
(In reply to comment #2) > Created attachment 70927 [details] [review] [review] > add support for Egistec ES603 device > > I am asking for review and for integration into trunk. > Note that some functions are unused but could be used in the future. > Thanks a lot. Please use git format-patch to get proper git-formatted patch. See man git-format-patch and man git-am
Comment on attachment 70927 [details] [review] add support for Egistec ES603 device Review of attachment 70927 [details] [review]: ----------------------------------------------------------------- ::: libfprint/drivers/etes603.c @@ +231,5 @@ > +static uint8_t *process_frame(uint8_t *dst, uint8_t *src); > +static int process_frame_empty(uint8_t *f, size_t s, int mode); > +static int contact_detect(struct etes603_dev *dev); > + > +#ifdef DEBUG_TRANSFER Do you really need to keep this debug stuff in the driver? @@ +265,5 @@ > + > +/* > + * Transfer (in/out) egis command to the device using synchronous libusb. > + */ > +static int sync_transfer(libusb_device_handle *udev, unsigned char ep, Please, no synchronous transfers in drivers. @@ +286,5 @@ > + return actual_length; > +} > + > +/* The size of the message header is 5 plus 1 for the command. */ > +#define MSG_HDR_SIZE 6 It's good idea to keep all defines in the beginning of the file. Or move them into header. @@ +1186,5 @@ > + contact = -1; > + goto end; > + } > + > + if (gettimeofday(&endtime, NULL)) { It's not portable. Why do you need it here? @@ +1191,5 @@ > + fp_err("gettimeofday failed with %d", errno); > + contact = -2; > + goto end; > + } > + timeradd(&endtime, &curtime, &endtime); Use fpi_timeout @@ +1200,5 @@ > + goto end; > + } > + > + /* 5 ms between each detect */ > + usleep(CS_DETECT_DELAY * 1000); You really need to use fpi_ssm here. Imagine that it's UI thread, and you're doing while(1) {} loop with sleeps in _asynchronous_ calls. @@ +1365,5 @@ > + * Return the number of new lines in 'src'. > + */ > +static int process_find_dup(uint8_t *dst, uint8_t *src, uint8_t width, > + uint8_t height) > +{ Is it some frame assembling routine? We have one in aeslib already, can you use that one? If it does not fit - could it be adapted to fit here? If not - why? @@ +1434,5 @@ > +static uint8_t *process_frame(uint8_t *dst, uint8_t *src) > +{ > + int new_line; > + > + /* TODO sweep direction to determine... merging will be different. */ Already implemented in aeslib, please take a look. @@ +1560,5 @@ > + complete_deactivation(idev); > + goto err; > + } > + > + switch (pdata->state) { Please use fpi_ssm for state tracking. @@ +1779,5 @@ > + dev->braw_cur = dev->braw; > + memset(dev->braw, 0, (dev->braw_end - dev->braw)); > + /* Use default mode (FingerPrint, FP) or use environment defined mode */ > + dev->mode = 0; > + if ((mode = getenv("ETES603_MODE")) != NULL) { What is it? @@ +1877,5 @@ > + .scan_type = FP_SCAN_TYPE_SWIPE, > + }, > + .flags = 0, > + .img_height = -1, > + .img_width = -1, /* Once set it cannot be changed (except with -1) */ Why? Does your device returns images of variable width?
Btw, I'm under impression that your driver is not completely asynchronous.
(In reply to comment #3) > (In reply to comment #2) > > Created attachment 70927 [details] [review] [review] [review] > > add support for Egistec ES603 device > > > > I am asking for review and for integration into trunk. > > Note that some functions are unused but could be used in the future. > > Thanks a lot. > > Please use git format-patch to get proper git-formatted patch. See man > git-format-patch and man git-am The thing is that I don't want to import all commits from my branch since there is no added value to do that.
(In reply to comment #6) > (In reply to comment #3) > > (In reply to comment #2) > > > Created attachment 70927 [details] [review] [review] [review] [review] > > > add support for Egistec ES603 device > > > > > > I am asking for review and for integration into trunk. > > > Note that some functions are unused but could be used in the future. > > > Thanks a lot. > > > > Please use git format-patch to get proper git-formatted patch. See man > > git-format-patch and man git-am > > The thing is that I don't want to import all commits from my branch since > there is no added value to do that. Just create another branch and squash all your commits into one. See git rebase -i and git help rebase, or join #fprint channel at irc.freenode.org git formatted patch is necessary to preserve authorship.
Created attachment 71404 [details] [review] add support for Egistec ES603 device v2 I am asking for a new review round. In this new version: * Using asynchronous transfer (except for closing the device which is impossible AFAIK). * Using fpi_ssm. * Unused features are removed. Thanks.
Comment on attachment 71404 [details] [review] add support for Egistec ES603 device v2 Review of attachment 71404 [details] [review]: ----------------------------------------------------------------- Looks OK except two comments: ::: libfprint/drivers/etes603.c @@ +1438,5 @@ > + dev->is_active = TRUE; > + > + ssm = fpi_ssm_new(idev->dev, m_init_state, INIT_NUM_STATES); > + ssm->priv = idev; > + fpi_ssm_start(ssm, m_init_complete); Please move dev initialization into dev_activate(). I don't want to debug bugs caused by device init in dev_open() later. @@ +1450,5 @@ > +static void dev_close(struct fp_img_dev *idev) > +{ > + /* SSM with asynchronous transfer cannot be used here. > + * Deadlock in fpi_imgdev_close_complete() > + * libusb:do_close() usbi_mutex_lock(&ctx->open_devs_lock);*/ dev_close() is not expected to perform any USB transfers, do them in dev_deactivate().
Created attachment 71853 [details] [review] add support for Egistec ES603 device v3 It fixes the two last comments.
I have to fix a calibration issue. I will post a fixed version in the next days so please wait before to commit it to mainline.
Patrick, how is it going with fixing calibration issue?
Created attachment 73161 [details] [review] add support for Egistec ES603 device v4 Attached the patch with the calibration issue fixed.
Comment on attachment 73161 [details] [review] add support for Egistec ES603 device v4 Review of attachment 73161 [details] [review]: ----------------------------------------------------------------- Looks OK except minor comment: ::: libfprint/drivers/etes603.c @@ +342,5 @@ > + */ > +static void msg_set_regs(struct etes603_dev *dev, int n_args, ...) > +{ > + struct egis_msg *msg = dev->req; > + va_list ap; Isn't #include <stdarg.h> required to use vaargs stuff?
Created attachment 73382 [details] [review] add support for Egistec ES603 device v5 Good catch! Thanks.
Patrick, I've merged your patch with few fixes into my libfprint repo, branch anarsoul-wip-v3, could you please test it on your device?
(In reply to comment #16) > Patrick, I've merged your patch with few fixes into my libfprint repo, > branch anarsoul-wip-v3, could you please test it on your device? The scan does not stop. I will try to have a look at the changes in your branch and to do a bisect.
(In reply to comment #17) > (In reply to comment #16) > > Patrick, I've merged your patch with few fixes into my libfprint repo, > > branch anarsoul-wip-v3, could you please test it on your device? > > The scan does not stop. I will try to have a look at the changes in your > branch and to do a bisect. It works for me actually :) Are you sure that you're using anarsoul-wip-v3 branch? You've made a wrong assumption that dev_deactivate() will be called right after image is submitted. Driver should restart imaging loop if dev_deactivate() wasn't called. And driver should not report any errors on deactivation
(In reply to comment #17) > (In reply to comment #16) > > Patrick, I've merged your patch with few fixes into my libfprint repo, > > branch anarsoul-wip-v3, could you please test it on your device? > > The scan does not stop. I will try to have a look at the changes in your > branch and to do a bisect. Btw, with my latest changes it does 5 scans for enroll.
Actually, it works. Thanks a lot for the modifications! (First time I did not pay attention to the 5 enrollments). I am looking forward to see this in the next release ;)
(In reply to comment #20) > Actually, it works. Thanks a lot for the modifications! (First time I did > not pay attention to the 5 enrollments). I am looking forward to see this in > the next release ;) Thanks for a driver and for testing :)
Created attachment 77033 [details] [review] 0010-Add-EgisTec-ES603-driver.patch
Comment on attachment 77033 [details] [review] 0010-Add-EgisTec-ES603-driver.patch Review of attachment 77033 [details] [review]: ----------------------------------------------------------------- There's no need to detail the changes you've made yourself in the commit message. "With additional fixes from..." is good enough for you to be labelled a co-author. Is the code available somewhere for these? 29 * Extra features not present in this driver (ask the author for code) 30 * Tuning of DTVRT for contact detection 31 * Contact detection via capacitance 32 * Capture mode using assembled frames (usually better quality) Looks good to commit!
(In reply to comment #23) > Comment on attachment 77033 [details] [review] [review] > 0010-Add-EgisTec-ES603-driver.patch > > Review of attachment 77033 [details] [review] [review]: > ----------------------------------------------------------------- > > There's no need to detail the changes you've made yourself in the commit > message. > > "With additional fixes from..." is good enough for you to be labelled a > co-author. Since changes are minor I'll just omit this message > Is the code available somewhere for these? > > 29 * Extra features not present in this driver (ask the author for > code) > 30 * Tuning of DTVRT for contact detection > 31 * Contact detection via capacitance > 32 * Capture mode using assembled frames (usually better quality) Later, let's merge initial working version > Looks good to commit!
(In reply to comment #23) > There's no need to detail the changes you've made yourself in the commit > message. > > "With additional fixes from..." is good enough for you to be labelled a > co-author. Sure, fine with me. > Is the code available somewhere for these? > > 29 * Extra features not present in this driver (ask the author for > code) > 30 * Tuning of DTVRT for contact detection > 31 * Contact detection via capacitance > 32 * Capture mode using assembled frames (usually better quality) It is available here (and kind of documented in my personal homepage): https://code.google.com/p/etes603 As Vasily mentioned, let's integrate it first. Actually, for libfprint I think this is not required. Contact detection saves energy only. With assembled frames, you get a better quality but I think this is not necessary since the current capture gives already good results.
(In reply to comment #25) > (In reply to comment #23) <snip> > > Is the code available somewhere for these? > > > > 29 * Extra features not present in this driver (ask the author for > > code) > > 30 * Tuning of DTVRT for contact detection > > 31 * Contact detection via capacitance > > 32 * Capture mode using assembled frames (usually better quality) > > It is available here (and kind of documented in my personal homepage): > https://code.google.com/p/etes603 > > As Vasily mentioned, let's integrate it first. It wasn't really a question of whether to integrate that right now, but "ask me for code" isn't useful when code repositories are easily available, and we want to avoid the code not being available when it's needed (say, you're on holidays for a month and somebody wants to implement part of this).
(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #23) > <snip> > > > Is the code available somewhere for these? > > > > > > 29 * Extra features not present in this driver (ask the author for > > > code) > > > 30 * Tuning of DTVRT for contact detection > > > 31 * Contact detection via capacitance > > > 32 * Capture mode using assembled frames (usually better quality) > > > > It is available here (and kind of documented in my personal homepage): > > https://code.google.com/p/etes603 > > > > As Vasily mentioned, let's integrate it first. > > It wasn't really a question of whether to integrate that right now, but "ask > me for code" isn't useful when code repositories are easily available, and > we want to avoid the code not being available when it's needed (say, you're > on holidays for a month and somebody wants to implement part of this). Yep sure. I would remove all these lines...
Created attachment 79746 [details] [review] 0010-Add-EgisTec-ES603-driver.patch
Merged into libfprint master
I tested with the current trunk and it works fine on my machine. Thanks for the commit! Waiting for the release now :)
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.