Bug 47198

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 :)