-
Notifications
You must be signed in to change notification settings - Fork 580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve Pn532 I2C performance by reducing response buffer size #2077
Conversation
@@ -26,7 +26,9 @@ namespace Iot.Device.Pn532 | |||
/// </summary> | |||
public class Pn532 : CardTransceiver, IDisposable | |||
{ | |||
private const int I2cMaxBuffer = 1024; | |||
// Host-controller frame size | |||
private const int MaxPacketData = 264; // maximum length of packet data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this should be left as an exercise to the reader (or a future PR author) to replace these fixed-size allocations with the (much smaller) maximum possible response sizes in each case, shouldn't the constant be public, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this constant could be public. After all, it comes from the data sheet.
The exercise for the reader is a little different. There are several places that simply assumed a maximun response of 1024 bytes, but the actual response will be much smaller . It is almost a 4x performance improvement to use MaxPacketData instead of 1024, but it would be better still for each of these to determine the actual maximum response size and use that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got that, but (blindly) changing it to the max size would already help, so if the constant is public, that's very easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. I did change them from 1024 to MaxPacketData
. They're all within this class, but I can make the constant public in case something else wants to take advantage of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly Patrick, it's more a property with this constant as initial value than having a public constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the question as you've been reading all this deeply recently, is: is there an interest to change that value? Or is this 264 + 12 the absolute maximum value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of the 264 total, the first byte of the command and response payload is the command code (which is incremented by one in the response), and then for InDataExchange the next byte is the target. I think the largest such payload is a command APDU, which is CLA,INS,P1,P2,Lc,Data,Le (where Data can be up to 255 bytes). [However, that's only 261]
When the length of the packet data exceeds 255 it uses the extended information frame, which is 12 bytes. (This includes the command code that is prefixed to the payload.)
So, in my understanding, the largest payload size will always be <= 264 and the largest total size will always be <= 276. The I2C buffer in ReadResponseI2C can be sized according to the expected data. It is easiest to just add 12 (for the extended information frame). It could be more precise and use the smaller normal information frame, but the difference is small enough at that point that it wasn't worth adding the extra logic to compute that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, we don't need to expose it. And it's all ok for this optimization. Thanks again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this additional optimization!
/azp run dotnet.iot |
Azure Pipelines successfully started running 1 pipeline(s). |
ReadResponseI2C
waits for the PN532 to be ready (usingIsReady
) and then does an_i2cDevice.Read
with a read buffer size ofMaxPacketData
(1024). I2C is slow, so this read takes a long time (I measured it as more than 100ms).The largest packet data size that the PN532 supports is 264 bytes. When wrapped in an extended information frame, this would require a total size of 276 bytes. However, many responses are much shorter than this. For instance, the response payload for reading a block of a Mifare Classic card is 16 bytes.
The second argument to
ReadResponseI2C
is aSpan<byte>
into which the response will be written. This PR changesReadResponseI2C
tostackalloc
a buffer for the I2C response based upon the payload size plus the additional bytes required for framing, rather than using a fixed size of 1024. This is much smaller and significantly improves the performance of the PN532 over I2C.There are a few places in the code where the caller of
ReadResponse
allocates a buffer of size 1024. The response cannot be larger than the maximum packet size of 264 bytes (as per the data sheet). Therefore, these are redefined as buffers of sizeMaxPacketData
(264). It is left as an exercise to the reader (or a future PR author) to replace these fixed-size allocations with the (much smaller) maximum possible response sizes in each case.Microsoft Reviewers: Open in CodeFlow