[Pcsclite-muscle] Support for multiple devices with ifdnfc, serial hotplug, and other improvements.

Tue Aug 23 02:31:39 PDT 2016

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 <ben.mehlman at sweetsams.com>:

>
> 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
> Pcsclite-muscle at lists.alioth.debian.org
> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
>

-- 
 Dr. Ludovic Rousseau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pcsclite-muscle/attachments/20160823/c131a429/attachment.html>