Bug 73762 - Egistec S801U support
Summary: Egistec S801U support
Status: RESOLVED MOVED
Alias: None
Product: libfprint
Classification: Unclassified
Component: libfprint (show other bugs)
Version: unspecified
Hardware: All Linux (All)
: medium enhancement
Assignee: libfprint-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 30287
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-18 13:29 UTC by Fabian Zaremba
Modified: 2018-05-31 08:56 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch against current git master to include support for Egistec S801U (41.39 KB, patch)
2014-01-18 13:29 UTC, Fabian Zaremba
Details | Splinter Review

Description Fabian Zaremba 2014-01-18 13:29:55 UTC
Created attachment 92330 [details] [review]
Patch against current git master to include support for Egistec S801U

I found a repository on github containing libfprint support for the Egistec S801U (ID 1c7a:0801 LighTuning Technology Inc. Fingerprint Reader):
https://github.com/lielfr/Libfprint-for-Egistec-S801U

However, as this seems to be based on a 0.4.x branch I rebased it on the current git repo.

I was able to get this working using an additional patch against libfprint imgdev.c found in Bug 30287 - https://bugs.freedesktop.org/show_bug.cgi?id=30287 - without this patch the device would not accept a second scan after the first failed but instead loop in verify_retry_scan until timeout.

You can find my patch against current git master (commit 35e356f6) attached.

Some parts of it are still a bit crude, like inclusion of the driver in core.c or libfprint/Makefile.am. Additionally, I just took the first available driver id 19.
Comment 1 Igor Gnatenko 2014-01-18 14:23:46 UTC
Comment on attachment 92330 [details] [review]
Patch against current git master to include support for Egistec S801U

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

+	&etss801u_driver,
^
according to the same code in libfprint I think should be
#ifdef ENABLE_ETSS801u
 	&etss801u_driver,
#endif
I thinks this ifdefs must be in  fp_internal.h, Makefile.am
Also for this must add some lines to configure.ac.

Why you are using diff ....  instead of git add/commit/format-patch ?
I think more better to use git.
Comment 2 Fabian Zaremba 2014-01-18 15:39:51 UTC
I only ported the original modifications, they are still a bit crude but working for me. I guess further clean up is required by someone who is more experienced.

I am personally quite new to VCS in general and git in particular - I didn't knew there were specific git command to be used for this, sry.
Hope my patch can still be useful, otherwise, I could try and make a git patch.
Comment 3 Igor Gnatenko 2014-01-18 16:04:54 UTC
(In reply to comment #2)
> I only ported the original modifications, they are still a bit crude but
> working for me. I guess further clean up is required by someone who is more
> experienced.
> 
> I am personally quite new to VCS in general and git in particular - I didn't
> knew there were specific git command to be used for this, sry.
> Hope my patch can still be useful, otherwise, I could try and make a git
> patch.
Okay. I'll prepare new patch.
Comment 4 Vasily Khoruzhick 2014-01-18 16:17:44 UTC
Comment on attachment 92330 [details] [review]
Patch against current git master to include support for Egistec S801U

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

- Formatting doesn't follow libfprint codestyle, we're using tabs, not spaces.
- Driver doesn't seem to be clean, too much magic numbers, actually I think that author abuses switch/case usage

See other comments below.

::: libfprint-git/libfprint/drivers/etss801u.c
@@ +135,5 @@
> +  /*WORKAROUND NEEDED, SOMETIME ALL FREEZES, RESET DEVICE AND REACTIVATE???*/
> +  libusb_free_transfer(transfer);
> +  fp_dbg("ERROR");
> +  fpi_imgdev_session_error(dev,-EIO);
> +  //return;

Please leave no commmented out code in a driver

@@ +147,5 @@
> +  fpi_imgdev_session_error(dev,-EPROTO);
> +  return;
> + }
> + libusb_free_transfer(transfer);
> + //Check poll cmd result

Please use C-style comments, i.e. /* and */

@@ +156,5 @@
> +  fp_dbg("Invalid result: %s",print_buf(br));
> +  fpi_imgdev_session_error(dev,-EPROTO);
> +  return;
> + }
> + //fp_dbg("0x%02hhX,0x%02hhX",ret_buf[0],ret_buf[1]);

Same

@@ +166,5 @@
> +  Sometime ret_buf[0] equals 0x82 at the first poll rounds, but
> +  really no finger on the scanner
> +  */ 
> +   fp_dbg("POLL WORKAROUND");
> +   goto _st;

I'm not so happy with goto-s in code, the only exception is error path, which is not the case here.

@@ +170,5 @@
> +   goto _st;
> +  }
> +  //Finger detected
> +  //fp_dbg("FINGER DETECTED!");
> +  //fp_dbg("BUF: %s",print_buf(ret_buf));

No commented out blocks are allowed

@@ +194,5 @@
> + libusb_fill_bulk_transfer(transfer,dev->udev,EP_OUT,(char*)&cmd_buf,31,et_write_poll_cb,adata,BULK_TIMEOUT);
> + //Wait before write next poll
> + tv.tv_sec=0;
> + tv.tv_usec=700;
> + select(0,NULL,NULL,NULL,&tv);

Use fpi_timeout_add

@@ +228,5 @@
> +{
> + memset(cmd,0,sizeof(*cmd));
> + switch(stage)
> + {
> +  case 1:

What do these magic numbers mean?

@@ +453,5 @@
> + if(!transfer) return -ENOMEM;
> + libusb_fill_bulk_transfer(transfer,dev->udev,EP_OUT,(char*)&cmd_buf,31,et_write_enroll_cb,&adata,BULK_TIMEOUT);
> + switch(dinit->stage)
> + {
> +  case 5:

No magic please

@@ +549,5 @@
> + int rlen;
> + 
> + switch(dinit->stage)
> + {
> +  case 2:

Same here

@@ +842,5 @@
> +}
> +
> +void et_assemble_image(struct fpi_img_dev* dev)
> +{
> + //size_t sz=(61440/4)*8;

No commented-out blocks

@@ +1177,5 @@
> +  cmd->flag4=0xB0;
> +  cmd->flag5=0x100;
> +  break;
> +  
> +  case 59:

Too much magic numbers :(

@@ +1347,5 @@
> + }
> + return r;
> +}
> +
> +char* print_cmd_buf()

Is it some debug stuff?

@@ +1358,5 @@
> + for(i=8;i<26;i++)
> + {
> +  if(i!=8)
> +  {
> +   sprintf(p,",");

sprintf is not safe.

@@ +1560,5 @@
> +
> +   /* Activation process of SS801U is not understandable at
> +   the moment. Simply repeats captured protocol.
> +   And, I don't know how to enter in initial state without device reset
> +    */

That's ugly. Could driver author experiment a bit to make it work without device reset?

::: libfprint-git/libfprint/drivers/etss801u.h
@@ +71,5 @@
> +waiting for finger*/
> +#define ET_ACTIVATE_STATES 118
> +#define ET_CAPTURE_STATES 82
> +
> +//FOR DEBUG, MUST BE REMOVED!

so do it.

::: libfprint-git/libfprint/Makefile.am
@@ +178,4 @@
>  DRIVER_SRC += $(ETES603_SRC)
>  endif
>  
> +DRIVER_SRC += $(ETSS801U_SRC)

I guess some configure.ac modifications are missing? We want any driver to be optional.
Comment 5 Chad-Kroeger 2014-10-21 20:44:50 UTC
Hello,

I was able to ./configure, apply both patches, make and make install the driver using the files that Fabian Zaremba provided.

I own an Acer Aspire 5943G which includes a
lsusb: 1c7a:0801 LighTuning Technology Inc. Fingerprint Reader

Sadly, even after a restart, I was not able to use the reader as Fingerprint GUI and fprint-demo both state that no device was recognized.

What am I doing wrong? Thanks for any help!
Comment 6 Bastien Nocera 2014-10-21 21:55:07 UTC
(In reply to Chad-Kroeger from comment #5)
> What am I doing wrong? Thanks for any help!

Don't change the bug status.
Comment 7 Fabian Zaremba 2014-10-22 19:21:08 UTC
(In reply to Chad-Kroeger from comment #5)
> Hello,
> 
> I was able to ./configure, apply both patches, make and make install the
> driver using the files that Fabian Zaremba provided.

Where did you get your files from? I did not provide any files, only links to the original patched github repository and a pointer to the git commit against my patch was done.

If you are using the github repository, this is an old, pre-patched version of fprint. You don't need to patch this any further.

If you are checking out the fprint git repository, HEAD commit 35e356f6) you can apply the two patches and build libfprint afterwards.

> What am I doing wrong? Thanks for any help!

Please do not mail people privately with or for more information, stay on Bugzilla for common information availability.
As you stated in your e-mail you are using Ubuntu, you should try to use the Debian packaging workflow to build your patched libfprint. Trying to install with "make install" in a package managed environment is almost always a bad idea.
Comment 8 Chad-Kroeger 2014-10-22 19:49:48 UTC
Hello and thanks for responding!

I got my files from here:
https://cloud.nf-design.eu/d/3a191bd2b2/

As I could not compile the files from https://github.com/lielfr/Libfprint-for-Egistec-S801U, I used the ones from above.

I did the following:

./configure
patch -p1 < s801u.patch
patch -p1 < verifyfix.patch

make
make install

reboot

I'm still new to ubuntu. You said I should use the "Debian packaging workflow to build your patched libfprint". How can I do that?
Fingerprint-Gui states no device detected, so something is still not working...
Comment 9 Fabian Zaremba 2014-10-22 20:50:21 UTC
(In reply to Chad-Kroeger from comment #8)
> I got my files from here:
> https://cloud.nf-design.eu/d/3a191bd2b2/

Okay, this collection should work as intended (at least it does for me).

> I'm still new to ubuntu. You said I should use the "Debian packaging
> workflow to build your patched libfprint". How can I do that?

Basically, you should not use "make install" to install software on your system (it breaks dependency management and file tracking of apt/dpkg).
If you also installed the Ubuntu package version of libfprint, you will now have a broken package management system.
Please try reading up on Ubuntu wikis, manuals and help forums. The provided keywords should be enough for a quick google search.
If you have further questions on packaging for your distro, try asking in e.g. "AskUbuntu". I'm afraid I can't help you here because I'm using Arch Linux and this is off topic for this libfprint/fprintd Bugzilla.

One (last) hint for you:
Since current Ubuntu libfprint version roughly matches the git master from January, you can try to apply this workflow: http://pascal.nextrem.ch/2010/05/06/build-ubuntudebian-packages-from-source-and-apply-a-patch/
But remember to try and clean up everything you previously installed first.
Comment 10 Chad-Kroeger 2014-10-28 21:58:03 UTC
Thanks, I got it working now!

I created a deb and since it might help others, here is the link (hope it works):
https://drive.google.com/file/d/0B-jxNcxMw-Z9VzFqYklHWlNOSFE/view?usp=sharing_eid&invite=CIGywrwE

Besides this: It is a known bug, but password can only be entered after the fingerprint verification timed out.
Furthermore, Fingerprint-gui is not working with this driver, only fprint-demo.
In Fingerprint-Gui, I can scan the finger once but not for the second, third,... time. In fprint-demo this is working. It should be working since I also applied the verifyfix-patch.
Comment 11 Fabian Zaremba 2014-10-29 13:59:22 UTC
(In reply to Chad-Kroeger from comment #10)
> Thanks, I got it working now!

Great, glad I could point you in the right direction.

> Besides this: It is a known bug, but password can only be entered after the
> fingerprint verification timed out.

This is due to PAM behaviour (running pam modules one after another) and can't be changed.

> Furthermore, Fingerprint-gui is not working with this driver, only
> fprint-demo.
> In Fingerprint-Gui, I can scan the finger once but not for the second,
> third,... time. In fprint-demo this is working.

I see the same behaviour on my system. You can however enroll your fingerprints using fprint-demo and then copy the corresponding files from ~/.fprint/prints to /var/lib/fprint/ (be careful to check the directory structure).
Comment 12 Vasily Khoruzhick 2015-09-09 17:25:34 UTC
@Fabian, do you plan to address issues I pointed out in the review?
Comment 13 GitLab Migration User 2018-05-31 08:56:29 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/libfprint/libfprint/issues/57.


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.