Bug 64351 - New driver for AuthenTech AES3500
Summary: New driver for AuthenTech AES3500
Status: RESOLVED FIXED
Alias: None
Product: libfprint
Classification: Unclassified
Component: libfprint (show other bugs)
Version: unspecified
Hardware: Other Linux (All)
: medium enhancement
Assignee: Juvenn Woo
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-08 08:45 UTC by Juvenn Woo
Modified: 2013-09-05 14:37 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
git formatted diff against commit 7eafca7 (11.74 KB, patch)
2013-05-08 08:45 UTC, Juvenn Woo
Details | Splinter Review
lsusb output (2.04 KB, text/plain)
2013-05-08 08:50 UTC, Juvenn Woo
Details
Add standalone aes3500 (12.25 KB, patch)
2013-05-16 09:30 UTC, Juvenn Woo
Details | Splinter Review
Extract common routines AES3K from AES3500 and AES4000 (23.05 KB, patch)
2013-05-16 09:30 UTC, Juvenn Woo
Details | Splinter Review
Extract common routines from aes4000 (15.72 KB, patch)
2013-05-16 10:36 UTC, Juvenn Woo
Details | Splinter Review
Add aes3500 support (9.78 KB, patch)
2013-05-16 10:37 UTC, Juvenn Woo
Details | Splinter Review
Cleanup macros and comments for aes3k drivers (2.56 KB, patch)
2013-05-17 04:53 UTC, Juvenn Woo
Details | Splinter Review
0001 aes3k: extract common routines aes3k from aes4000 (15.80 KB, patch)
2013-05-21 05:49 UTC, Juvenn Woo
Details | Splinter Review
0002 aes3k: add aes3500 driver support (9.82 KB, patch)
2013-05-21 05:49 UTC, Juvenn Woo
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Juvenn Woo 2013-05-08 08:45:06 UTC
Created attachment 79017 [details] [review]
git formatted diff against commit 7eafca7

AES3500 08ff:5731
=================

The AES3500 possesses a *press-typed* sensor of dimension in 128x128. Which is
similiar to AES4000, except the later is in 96x96.

The driver is a deriative work of Daniel Drake's AES4000 driver, with a
few parameters tuned for AES3500. While there seems no problem scanning
fingerprint images with it, the verification rate is pretty low at the
moment.
Comment 1 Juvenn Woo 2013-05-08 08:50:06 UTC
Created attachment 79018 [details]
lsusb output
Comment 2 Vasily Khoruzhick 2013-05-08 09:10:35 UTC
There're a few differences between aes4000 and aes3500, could you extend aes4000 to support aes3500 devices?

Btw, please use git format-patch next time
Comment 3 Juvenn Woo 2013-05-08 09:20:29 UTC
(In reply to comment #2)
> There're a few differences between aes4000 and aes3500, could you extend
> aes4000 to support aes3500 devices?

A driver to support both devices? I'm not sure, but I may not be experienced
enough to do it. Will look into it, anyway.

> 
> Btw, please use git format-patch next time

Will do so, thanks.
Comment 4 Vasily Khoruzhick 2013-05-08 09:25:05 UTC
Take a look at aesx660.c, aes1660.c and aes2660.c

aesx660.c is common routines for aes1660 and aes2660 devices, and device-specific code is in aes1660.c and aes2660.c
Comment 5 Juvenn Woo 2013-05-08 11:28:48 UTC
(In reply to comment #4)
> Take a look at aesx660.c, aes1660.c and aes2660.c
> 
> aesx660.c is common routines for aes1660 and aes2660 devices, and
> device-specific code is in aes1660.c and aes2660.c

Thanks Vasily, for pointing me out the way. I'll figure out some time to do it.
Comment 6 Juvenn Woo 2013-05-16 09:30:05 UTC
Created attachment 79392 [details] [review]
Add standalone aes3500
Comment 7 Juvenn Woo 2013-05-16 09:30:56 UTC
Created attachment 79393 [details] [review]
Extract common routines AES3K from AES3500 and AES4000
Comment 8 Juvenn Woo 2013-05-16 09:39:33 UTC
(In reply to comment #4)
> Take a look at aesx660.c, aes1660.c and aes2660.c
> 
> aesx660.c is common routines for aes1660 and aes2660 devices, and
> device-specific code is in aes1660.c and aes2660.c

Hi Vasily,

Following your approach, I've extracted out common routines as aes3k, and with differences left in aes3500 and aes4000, respctively.

Please have a look, and tell me if anything's not good yet. Thanks!
Comment 9 Vasily Khoruzhick 2013-05-16 09:45:50 UTC
(In reply to comment #8)
> Hi Vasily,
> 
> Following your approach, I've extracted out common routines as aes3k, and
> with differences left in aes3500 and aes4000, respctively.
> 
> Please have a look, and tell me if anything's not good yet. Thanks!

Hi Juvenn,

please create 2 patches:

1st to extract common routines from aes4k,
2nd to add aes3k device support

I know that it's additional work, but it does not look clean to add code in 1st patch that will be removed in 2nd, it's redundant.

Thanks!
Comment 10 Juvenn Woo 2013-05-16 10:36:26 UTC
Created attachment 79398 [details] [review]
Extract common routines from aes4000
Comment 11 Juvenn Woo 2013-05-16 10:37:04 UTC
Created attachment 79399 [details] [review]
Add aes3500 support
Comment 12 Juvenn Woo 2013-05-16 10:41:04 UTC
Hi Vasily,

Just prepared new patches as you've outlined. Your feedback are welcome. Thanks!
Comment 13 Vasily Khoruzhick 2013-05-16 10:58:45 UTC
Comment on attachment 79398 [details] [review]
Extract common routines from aes4000

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

::: libfprint/drivers/aes4000.c
@@ +42,2 @@
>  
> +// FRAME_WIDTH * AES3K_FRAME_HEIGHT / 2

Substitute it as a macro value, or remove this comment.
And don't use c99 comments, use multiline
Comment 14 Vasily Khoruzhick 2013-05-16 10:59:52 UTC
Comment on attachment 79399 [details] [review]
Add aes3500 support

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

Looks good
Comment 15 Juvenn Woo 2013-05-17 04:53:44 UTC
Created attachment 79452 [details] [review]
Cleanup macros and comments for aes3k drivers

Hi Vasily,

Added this new patch to cleanup the macros and comments. Please review.

Thanks!
Comment 16 Juvenn Woo 2013-05-20 11:10:03 UTC
Hi Vasily,

Were you notified the new patch to cleanup the macros and comments? Let me know what you think.

Thanks!
Comment 17 Vasily Khoruzhick 2013-05-20 11:19:24 UTC
Comment on attachment 79452 [details] [review]
Cleanup macros and comments for aes3k drivers

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

Please fixup 1st and 2nd patches, don't create another one.
You can use git interactive rebase (git rebase -i) to apply fixup against earlier commits.

See also some comments:

::: libfprint/drivers/aes3500.c
@@ +46,2 @@
>  #define FRAME_WIDTH 	128
> +#define FRAME_SIZE	FRAME_WIDTH * AES3K_FRAME_HEIGHT / 2

Please use brackets around macro value

::: libfprint/drivers/aes4000.c
@@ +41,4 @@
>  
> +/* image size = FRAME_WIDTH x FRAME_WIDTH */
> +#define FRAME_WIDTH 	96
> +#define FRAME_SIZE	FRAME_WIDTH * AES3K_FRAME_HEIGHT / 2

Same as above

@@ +41,5 @@
>  
> +/* image size = FRAME_WIDTH x FRAME_WIDTH */
> +#define FRAME_WIDTH 	96
> +#define FRAME_SIZE	FRAME_WIDTH * AES3K_FRAME_HEIGHT / 2
> +#define FRAME_NUMBER	FRAME_WIDTH / AES3K_FRAME_HEIGHT

Same as above
Comment 18 Juvenn Woo 2013-05-21 05:49:12 UTC
Created attachment 79604 [details] [review]
0001 aes3k: extract common routines aes3k from aes4000
Comment 19 Juvenn Woo 2013-05-21 05:49:46 UTC
Created attachment 79605 [details] [review]
0002 aes3k: add aes3500 driver support
Comment 20 Juvenn Woo 2013-05-21 05:54:50 UTC
Hi Vasily,

Newly built 2 patches attached, please have a review.

And thanks for guiding me through to produce good code, this is my first C project. Anything that's not good enough, please let me konw. I'd be glad to fix them. 

Best,
Comment 21 Vasily Khoruzhick 2013-05-21 07:22:48 UTC
Comment on attachment 79604 [details] [review]
0001 aes3k: extract common routines aes3k from aes4000

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

Looks OK
Comment 22 Vasily Khoruzhick 2013-05-21 07:23:37 UTC
Comment on attachment 79605 [details] [review]
0002 aes3k: add aes3500 driver support

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

Looks OK
Comment 23 Vasily Khoruzhick 2013-09-05 14:37:19 UTC
Merged into libfprint master


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.