Hi Konstantin, Could you please squash your driver into a single commit (use git rebase -i) for easier review? Please leave core changes as a separate commit.
Thanks! Regards, Vasily On Thu, Aug 6, 2015 at 11:34 AM, Vasily Khoruzhick <anars...@gmail.com> wrote: > Hi, > > I'm quite busy atm and I don't know when I get some time to take a look at it. > > Regards, > Vasily > > On Fri, Jul 31, 2015 at 5:52 AM, Константин Семенов <i...@zem7.ru> wrote: >> Hello, >> >> I tested my driver for two weeks and I am completely satisfied with its >> work. I guess that it doesn't need more image and protocol improvements. I >> formatted the source and removed debug functions and waiting for future >> review. >> >> Thanks >> >> 2015-07-16 1:21 GMT+04:00 Константин Семенов <i...@zem7.ru>: >>> >>> Hello Vasily, >>> >>> The first and the second issues are easy to fix, but I will do it when >>> everything else is ok. >>> >>> About the third point: fpi_im_resize can only increase the size of image, >>> so I wrote similar function fpi_img_downscale in pixman.c. >>> >>> Fourth part was the hardest one: I actually don't understand, how does >>> verifying process work in libfprint. In fprint_demo and examples >>> dev_deactivate is called after every fingerprint. But in fprintd one loop is >>> used for all scans. However, after entering >>> IMG_ACQUIRE_STATE_AWAIT_FINGER_OFF, state could be changed to >>> IMG_ACQUIRE_STATE_AWAIT_FINGER_ON only if action is enroll! I wrote a small >>> stub to fix it, but this thing needs more attention because of identify and >>> capture actions. >>> >>> About transfer cancellation: I checked it plenty of times when was playing >>> with fprint_demo, it works fine. >>> >>> I found a corner case which appears very rarely(e.g. when finger is on the >>> sensor for about a minute). It seems that driver doesn't work properly in >>> this case. Moreover, restarting fprintd and fprint_demo doesn't help, only >>> rebooting. I suggest to use libusb_reset_device in this case. >>> >>> Thanks >>> >>> 2015-07-14 21:25 GMT+04:00 Vasily Khoruzhick <anars...@gmail.com>: >>>> >>>> Hi Konstantin, >>>> >>>> Thank you for your work! >>>> >>>> I've taken only a brief look, will do a proper review on weekend, >>>> here're few issues (however I suppose not all): >>>> >>>> 1) You use formatting that differs from other libfprint. Please note >>>> that libfprint uses linux kernel coding style, see [1] for details, >>>> you can use script/Lindent script from Linux kernel sources to >>>> reformat your code. >>>> 2) Be ready to drop all debugging stuff (like one that wrapped into >>>> #ifdef VFS_DEBUG_IMAGE) before preparing a patch to submit >>>> 3) Drop your simple scaling, use libfprint functions, see fpi_im_resize() >>>> 4) I'm not very happy with [2]. Could you try to fix it without ugly >>>> hacks? Btw, how does it pass enroll which does 5 scans in row? >>>> >>>> Please also make sure that cancellation works well, i.e. try to press >>>> "cancel" button in fprint_demo while it waits for finger and ensure >>>> that it cancels scan properly >>>> and further scans for verification/enrollment work well. >>>> >>>> [1] https://www.kernel.org/doc/Documentation/CodingStyle >>>> [2] >>>> https://github.com/zemen/libfprint/blob/master/libfprint/drivers/vfs0050.c#L440 >>>> >>>> Regards, >>>> Vasily >>>> >>>> On Mon, Jul 13, 2015 at 12:30 PM, Константин Семенов <i...@zem7.ru> wrote: >>>> > Hello Vasily, >>>> > >>>> > I have updated my driver and now it satisfies all 4 requirements. Could >>>> > you >>>> > please check it again? >>>> > >>>> > The repo is still here: https://github.com/zemen/libfprint >>>> > >>>> > I will test it for several days and close some of TODO's. >>>> > >>>> > 2015-07-10 7:29 GMT+04:00 Vasily Khoruzhick <anars...@gmail.com>: >>>> >> >>>> >> Hi Konstantin, >>>> >> >>>> >> On Thu, Jul 9, 2015 at 5:31 PM, Константин Семенов <i...@zem7.ru> wrote: >>>> >> > Hello, >>>> >> > >>>> >> > Recently I have managed to write working Validity VFS0050 driver >>>> >> > with >>>> >> > some >>>> >> > help of payden's source on github >>>> >> > (https://github.com/payden/libfprint). >>>> >> > I >>>> >> > made it from scratch, sniffing USB packets from windows' driver via >>>> >> > USBLyzer. Now it works very well for me without any crashes and with >>>> >> > low >>>> >> > false negatives. >>>> >> > >>>> >> > In a few days I will add USB errors checkings and cut out all debug >>>> >> > parts. >>>> >> > >>>> >> > Could you please take a look on it and if everything is OK add the >>>> >> > driver to >>>> >> > the library? >>>> >> >>>> >> 1) You should use asynchronous libusb API. Using synchronous API is >>>> >> not acceptable. >>>> >> 2) Please try to avoid using libusb_device_reset(). Find out how >>>> >> Windows driver deinitializes the device >>>> >> 3) Move functions from header into *c file >>>> >> >>>> >> > My repo is here: https://github.com/zemen/libfprint >>>> >> >>>> >> 4) Last but not least - you have to prepare proper git-formatted >>>> >> patch. Clone libfprint git repo instead of creating new one, add your >>>> >> changes, commit, push to github. >>>> >> >>>> >> Regards, >>>> >> Vasily >>>> >> >>>> >> P.S. Please keep ML in CC. >>>> >> >>>> >> > -- >>>> >> > Kind regards, >>>> >> > Konstantin Semenov >>>> >> > >>>> >> > _______________________________________________ >>>> >> > fprint mailing list >>>> >> > fprint@lists.freedesktop.org >>>> >> > http://lists.freedesktop.org/mailman/listinfo/fprint >>>> >> > >>>> >> _______________________________________________ >>>> >> fprint mailing list >>>> >> fprint@lists.freedesktop.org >>>> >> http://lists.freedesktop.org/mailman/listinfo/fprint >>>> > >>>> > >>>> > >>>> > >>>> > -- >>>> > Kind regards, >>>> > Konstantin Semenov >>> >>> >>> >>> >>> -- >>> Kind regards, >>> Konstantin Semenov >> >> >> >> >> -- >> Kind regards, >> Konstantin Semenov _______________________________________________ fprint mailing list fprint@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/fprint
