search

anarsoul / libfprint

libfprint with my WiP drivers
GNU Lesser General Public License v2.1
27 stars 10 forks source link

Async improvements #5

Closed pelleman closed 8 years ago

pelleman commented 9 years ago

Rewriting asynchronous routines. vfs5011.c almost works. aes* is broken. I have a vfs5011 and an aes2501 so that is what I will concentrate on. There is now a real thread! I've tried to mess as little at possible with the code. Unfortunately it's in a bad shape so a big rewrite is in place. Much work needs to be moved from the thread of libusb (callbacks) into the main loop and made sure everything is done in order. It would probably have been easier to keep the library synchronous and making an asynchronous wrapper but the work has already begun, no stopping now!

Signed-off-by: Per-Ola Gustavsson pelle@marsba.se

anarsoul commented 9 years ago

What's the reason of that change? Async API works fine for me. Could you describe pros and cons of current implementation and yours? Thanks!

pelleman commented 9 years ago

Since my main laptop have the aes5011 libfprint didn't work at all for me, I also see some weird things in the code after spending some time debuggning. This is some of my opinions about libfprint:

I've mainly looked at async.c drv.c vfs5011.c imgdrv.c, only a quick glance at aes2501.c and aeslib.c to find out they don't like my changes to drv.c. I'll focus on drv.c to make sure threads are used in a proper way and events are executed in correct order. My small changes are sloppy to say the least.

As a side note, type and variable naming is really unintuitive. I guess I'll have to live with that. It's stripped by the compiler anyway.

It's work in progress, i'll continue, must do the dishes first. Just giving a heads up.

anarsoul commented 9 years ago

The thread of the callers is hi-jacked in a unknown amount of time. It's stack is abused by recursive calls. It's not really asynchronous programming and should be changed! (The change is done and i consider it working, maybe should do a fork with this change.)

Completely depends on the driver. Most of drivers don't (as you call it ) hi-jack a thread for an unknown amount of time. As for recursive calls see next comment

The thread of libusb is hi-jacked the the same manner. The callbacks from libusb should return as fast as possible and before next call to libusb_submit_transfer(). As it is now libusb is basically calling itself recursively an unknown amount of times, the stack gets really crazy.

libusb has no it's own thread. Everything's done in mainloop. Again, it's up to driver what to do in the callback, most drivers just change the state. Not a much of cpu time, right? If you're doing recursive calls and not using FSM/SSM in a driver - it's a driver issue.

The behavior upon a stop request is unknown, two threads are likely to collide.

What threads? libfprint is completely single-threaded, everything is expected to go through main loop (it's possible to export FDs set to integrate into your main loop). But stop request behavior depends on the driver.

There is in inconsistencies in imgdrv.c, it's unclear what the driver is supposed to to after activating (wait for finger) and after certain failures (short read). Should dev_change_state() be mandatory? It's equal bugs and poor documentation I guess.

Well, all AES drivers (except aes1610 and aes3000 - I just don't have these devices) are in good shape. aes2501, aes2550/2810, aes1660 and aes2660 work just fine for me. Well, additional accuracy is required while scanning finger on aes1660, but it's due to small sensor size.

You're right about documentation, it's not documented what driver's supposed to do.

Anyway, please stop writing a code at the moment. Let's try to discuss what's wrong with current code (as I said, I see no issues with my devices, and I own a device for almost every driver in libfprint!). I really see no reason to introduce additional threads in libfprint, since I'd like to keep things as simple as possible. Multithreading in drivers is a no-go. Believe me, since I'm doing driver development as a job.

I'm opened for suggestions and discussions, so please join #fprint channel at irc.freenode.org.

anarsoul commented 9 years ago

Btw, I'm really happy that there's one more developer for libfprint project, sole development is such a boring thing :)

anarsoul commented 8 years ago

Won't merge, breaks everything except vfs5011.