miguelbalboa / rfid

Arduino RFID Library for MFRC522
The Unlicense
2.77k stars 1.44k forks source link

Fix detection of ISO/IEC 14443 and ISO/IEC 18092 cards #486

Closed wojtekka closed 5 years ago

wojtekka commented 5 years ago

Information about SAK coding in both ISO/IEC 14443-3 and ISO/IEC 18092 shows that most of the bits are "don't care", so the value should not be compared as whole but bitmasked instead:

bit 8 bit 7 bit 6 bit 5 bit 4 bit 3 bit 2 bit 1 Meaning
x x x x x 1 x x UID not complete
x x 1 x x 0 x x UID complete, compliant with ISO/IEC 14443-4
x x 0 x x 0 x x UID complete, not compliant ISO/IEC 14443-4
x 1 x x x 0 x x UID complete, compliant with ISO/IEC 18092
x 0 x x x 0 x x UID complete, not compliant ISO/IEC 18092
Q A
Bug fix? yes
New feature? no
Doc update? no
BC breaks? no
Deprecations? no
Fixed tickets N/A
Rotzbua commented 5 years ago

I think your information is a bit outdated.

As reference use: https://www.nxp.com/docs/en/application-note/AN10833.pdf Rev. 3.6—11 July 2016

There are some differences like:

3.2 Coding of Select Acknowledge (SAK) [...] UID not complete 00000100

wojtekka commented 5 years ago

I'm referring to ISO/IEC standard, you're referring to an application note. Sure, NXP is not just some random manufacturer but in case of "UID not complete" it looks like an oversimplification.

Anyway, after looking at PICC_Select method, I see that:

If you want me to leave the original PICC_TYPE_NOT_COMPLETE condition, I'd be happy to do that. That was just a "by-the-way" cleanup attempt. I'm more interested in proper handling of some the ISO 14443-4 cards I have.

Rotzbua commented 5 years ago

uid->sak will not be set if UID is not complete, so PICC_GetType should never see this bit after all.

Correct, but variable uid is public, so it could be set.

Sure, NXP is not just some random manufacturer but in case of "UID not complete" it looks like an oversimplification.

Could be that they simplified their application notes, but it is documented. Please add iso standard + year + chapter to your implementation, so we can avoid that it is changed back.

Some other notes:

wojtekka commented 5 years ago

I added excerpts from ISO/IEC 14443-3:2016 and ISO/IEC 18092:2013 in the comments. I also modified MFRC522Extended::PICC_GetType() to use MFRC522::PICC_GetType() to avoid redundancy.

There's annex B in ISO/IEC 18092:2013 which describes SAK algorithm:

That's why I'm checking those two bits before detecting MIFARE type. Note that none of the MIFARE SAKs described in AN10833 have bits 6 and 7 set except those that actually are ISO/IEC 14443-4 compliant. That's why MFRC522Extended::PICC_GetType() has an additional condition for MIFARE DESFire.

As for checking if bit 3 is 0, the first condition checks if it's 1 and then returns so the rest of the method is executed only if bit 3 is 0. I can change it to if/else if it increases readability.