Support for multiple devices with ifdnfc, serial hotplug, and other improvements.

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Support for multiple devices with ifdnfc, serial hotplug, and other improvements.

Ben Mehlman

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.


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
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: Support for multiple devices with ifdnfc, serial hotplug, and other improvements.

Ludovic Rousseau
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.

Can you update your PR to fix the problems?
Thanks

2016-08-23 1:34 GMT+02:00 Ben Mehlman <[hidden email]>:

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.


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
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle



--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: Support for multiple devices with ifdnfc, serial hotplug, and other improvements.

Ben Mehlman
Thanks Ludovic, I will update the commit message.

On Tue, Aug 23, 2016 at 5:31 AM, Ludovic Rousseau <[hidden email]> 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.

Can you update your PR to fix the problems?
Thanks

2016-08-23 1:34 GMT+02:00 Ben Mehlman <[hidden email]>:

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.


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
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle



--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle


_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle