periph/devices

MFRC522 - ReadUID() returns 5 bytes for a tag with a 4-byte UID

Closed this issue · 4 comments

brackendawson commented

Describe the bug
When reading the UID of a tag with a 4-byte UID ReadUID() returns 5 bytes.

To Reproduce
Steps to reproduce the behavior:

  1. Call ReadUID() and scan an NFC tag with a 4-byte UID.
  2. 5 bytes will be returned. Comparing to the tag's known UID, the fifth byte appears to be extraneous.

Expected behavior
4 bytes should be returned.

Platform (please complete the following information):

  • OS: Raspbian 10.9
  • Board: Gen 1 Raspberry Pi Model B
  • Device: MFRC522

Additional context
The problem appears to be here: https://github.com/periph/devices/blob/47f3fec45df8376070823f45f7624567267335a8/mfrc522/mfrc522.go#L404-414

Both antiColl() and antColll2() check the crc byte but then also return it. In the case of a 4-byte UID selectCard() then returns the whole slice including the CRC, oops.

I think in the case of a 7-byte NUID it will also be wrong, the code correctly strips byte 0 (0x88), then leaves the crc from antiColl(), then strips the crc from antiColl2(), meaning it will return 8-bytes but byte 4 will be extraneous.

maruel commented

My take: Add a new Opts.FixedUID bool so the default is kept false. Keep the old broken behavior for now. Remove the broken behavior and flag in v4.

  • 👍1
brackendawson commented

Having obtained a 7-byte tag and tested it their UIDs are read correctly, this only affects the 4-byte tags.

maruel commented

The more I think about it, the less I think it's necessary to keep compatibility.
Folks can just stay on an older version of the driver if they can't switch.

So if it's trivial to keep the bogus algo keep it, but if it's non-trivial it's not a big deal and feel free to not keep support.

brackendawson commented

It was pretty trivial. I'll make a PR in cmd too so that the example shows good UIDs. We should also open an issue under the v4 milestone to remove this.