Thanks Ludovic, I will update the commit message. On Tue, Aug 23, 2016 at 5:31 AM, Ludovic Rousseau < [email protected]> wrote:
> Hello Ben, > > Thanks for the patches. > I made some comments already. > > What I would like is to have the detailed comment from your email in the > commit messages themselves. The commit messages will stay, your email will > disappear. > > You can get some documentation on how to format git commit messages > http://chris.beams.io/posts/git-commit/ > https://github.com/erlang/otp/wiki/Writing-good-commit-messages > > Can you update your PR to fix the problems? > Thanks > > 2016-08-23 1:34 GMT+02:00 Ben Mehlman <[email protected]>: > >> >> Hello all.. Ludovic, Frank Morgner.. any users of ifdnfc... >> >> So I am using ifdnfc for a project where I need to communicate with >> several pn532_serial readers, and I discovered that it was not able to work >> with multiple readers, and it also seemed to have some memory handling >> problems, outputting garbage characters when it shouldn't, etc. >> >> So I worked on it and was able to add support for multiple readers. And >> in the process of doing that, I found some coding issues I was able to >> resolve, and I added some other features that make things work more >> reliably. All of which I would like to give to the project. >> >> It ended up being a lot of changes, so I thought that it would be better >> for me to explain on this list what I did, so that people will understand >> them, and if some of my changes aren't ok it could be discussed and I could >> change them if necessary. >> >> What I found/ What I did: >> >> In IFDHCreateChannelByName, the ifd_devices entry was defaulting to >> index=0 for any device other than those configured with a "usb:" >> connstring. This resulted in a segfault if ifdnfc was loaded more than >> once for one of these devices. This was fixed so that each device gets its >> own ifd_devices entry, allowing multiple non-usb devices. >> >> For non-usb devices, the DEVICENAME in the pcscd configuration used to be >> ignored by ifdnfc. In order to do the nfc_open, ifdnfc-activate would >> query libnfc to get the connstring from the libnfc configuration. If there >> was more than one device in libnfc, a human interaction would have to occur >> to select the correct nfc device. None of this would work very well for >> multiple devices, especially the human interaction part. >> >> I changed it so that it now works with two different configuration >> styles, one compatible with the code as it exists today (so that I wouldn't >> break the environment of anyone currently using ifdnfc), and a new one that >> allows multiple devices and some other things: >> >> Old config (as it is today): >> >> FRIENDLYNAME "IFD-NFC" >> DEVICENAME /dev/null >> LIBPATH /usr/lib/pcsc/drivers/ifdnfc.bundle/Contents/Linux/libifdnfc >> .so.0.1.4 >> >> There can only be one ifdnfc device since there can be only one named >> "IFD-NFC". DEVICENAME is not supplied in this file for serial devices >> using the current code. In this case we use the old behavior of running >> ifdnfc-activate which checks with libnfc to get the nfc_connstring, then >> use it to open the device. If there's more than one device configured for >> libnfc, ifdnfc-activate will ask the user to choose a device. This is all >> the same as before. >> >> New config (with the changes I've made): >> >> FRIENDLYNAME "IFD-NFC-Reader1" >> DEVICENAME pn532_uart:/dev/ttyS0 >> LIBPATH /usr/lib/pcsc/drivers/ifdnfc.bundle/Contents/Linux/libifdnfc >> .so.0.1.4 >> >> FRIENDLYNAME "IFD-NFC-Reader2" >> DEVICENAME pn532_uart:/dev/ttyS1 >> LIBPATH /usr/lib/pcsc/drivers/ifdnfc.bundle/Contents/Linux/libifdnfc >> .so.0.1.4 >> >> ..and so on up to 10 readers (default) >> >> The devicename is now the nfc_connstring, so there's no more ambiguity as >> to which nfc device belongs to which pcsc reader, there's no need to query >> libnfc for the connstring, and the user is never asked to choose the nfc >> device. >> >> In the IFDHCreateChannelByName I look to see if the devicename passed >> looks like something that nfc_open would accept.. simply it checks to see >> if the name contains a colon. If so, we know this is the new configuration >> style. In this case, we try to nfc_open() the device. Whether we can >> successfully open the device or not, the connstring is copied for later >> use. It is not necessary to use ifdnfc-activate. >> >> If nfc_open() doesn't work when the channel is created.. for example if >> the reader isn't powered or connected, it is ok. To allow "hot plug" of >> serial devices, I added retry logic driven by the polling behavior of pcscd >> when used with the pn532_serial.. every 15 seconds, ifdnfc will attempt >> nfc_open() on unopened devices. This is done in IFDHICCPresence() >> which, at least for the pn532_uart.. since it is a polled device we get >> called every half second and have the opportunity to try again to nfc_open >> the device. >> >> ifdnfc-activate was significantly rearranged to allow control of multiple >> ifdnfc devices. As I said, using this configuration style, the driver will >> try to open all devices on startup. If this is all you need then there's >> no need to run ifdnfc-activate. But if you want to control them manually, >> to shut them down, switch to se mode, or if you want to check the status, >> you can do that the same as before, except that the commands will be >> applied to all readers named starting with "IFD-NFC". >> >> If you have multiple devices and want to control them individually rather >> than all at once, you can pass the name to ifdnfc-activate, eg: >> >> ifdnfc-activate off IFD-NFC-Reader1 >> >> This is actually a substring match so if you have your readers grouped by >> name you can control the group with a left hand match. >> >> There were some problems before where if ifdnfc-activate was used >> multiple times to stop and start the device it would leak devices and as >> well there were null termination issues in places with the string handling >> in the control messages that resulted in garbage output and core dumps at >> times. This was cleaned up and simplified to have a simple C structure >> passed back and forth replacing all the pointer manipulation in the >> existing message format. With the low volume of these transactions it was >> simply not worth it to fix the variable length message code. Fixed length >> messages are much safer and it was easier to add additional data to the >> response message for better status messages. There were also similar >> problems in the driver itself where for example the control structure for >> the Lun was tagged for reuse when a close command was sent by >> ifdnfc-activate.. but pcscd had not actually closed the channel. This led >> to a segfault if you tried to turn that reader back on again. I made it so >> that once the channel is allocated it is not deallocated until pcscd closes >> it. >> >> So it's now ok to run ifdnfc-activate any time you want to open and close >> devices, or to enable SE mode, and the output of ifdnfc-activate will tell >> you more about the status of each device than it used to: What mode it is >> in.. mode being what we ASKED it to do with ifdnfc-activate (inactive, >> active, or active se), and then what the actual status of the device is at >> this time: whether it is actually connected (nfc_open was successful), and >> whether it is actually in se mode. A lot of the other error outputs in >> ifdnfc-activate were improved and made more consistent. >> >> So that's about it. >> >> You can look at the code @ https://github.com/benmehlman/ >> ifdnfc/tree/serial-nfc-open-on-startup >> >> I am sorry that it is a bit hard to follow the individual changes.. I >> started out with a branch for each change I was going to make.. but what >> happened was, when I first started out, I thought I'd be making only a few >> small changes, and it wasn't until I got into it that I really understood >> what needed to be done, and by that time everything had been done in the >> wrong order and couldn't be easily separated anymore without my starting >> over from the beginning. >> >> However if anything I have done does not make sense please ask and I'll >> explain. >> >> Thanks! >> Ben Mehlman >> >> >> >> >> >> >> >> _______________________________________________ >> Pcsclite-muscle mailing list >> [email protected] >> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle >> > > > > -- > Dr. Ludovic Rousseau > > _______________________________________________ > Pcsclite-muscle mailing list > [email protected] > http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle >
_______________________________________________ Pcsclite-muscle mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
