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

Tue Aug 23 21:24:40 UTC 2016

Thanks Ludovic, I will update the commit message.

On Tue, Aug 23, 2016 at 5:31 AM, Ludovic Rousseau <
ludovic.rousseau at gmail.com> 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 <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
>
> _______________________________________________
> Pcsclite-muscle mailing list
> Pcsclite-muscle at lists.alioth.debian.org
> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pcsclite-muscle/attachments/20160823/547c7572/attachment.html>