Skip to content
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

Add API to enforce ISO15693 mode #3885

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

aaronjamt
Copy link

@aaronjamt aaronjamt commented Sep 8, 2024

What's new

  • Adds a new API to the NFC parser to allow applications to enforce a specific parsing mode. Some apps, such as Picopass, will always use one of the modes, so disabling autodetection can help to prevent issues resulting from incorrectly autodetecting the state (i.e. due to noise). It also includes a way to re-enable autodetection, should that be useful in the future.

Verification

  • Build a modified version of the Picopass app which calls the new nfc_iso15693_force_1outof4 method and emulate a card. Scanning the card on the reader no longer causes the Flipper to crash when the reader has LF enabled.

Checklist (For Reviewer)

  • PR has description of feature/bug or link to Confluence/Jira task
  • Description contains actions to verify feature/bugfix
  • I've built this code, uploaded it to the device and verified feature/bugfix

@nvx
Copy link
Contributor

nvx commented Sep 8, 2024

A bit of background, picopass only ever uses 1of4 modulation not 1of256. As readers will never use 1of256 if noise ever makes it appear like a 1of256 SOF was sent it will hang there indefinitely as the reader will never send a 1of256 EOF causing #3343

It is probably worth also having a timeout (we know the maximum frame size, if we don't see the rest of the frame in this time we should reset the emulation state ready for the next frame) and fix what looks like a buffer overrun in the 1of256 code (flipperdevices/flipperzero-good-faps#105), but even with those two things fixed I still think it makes sense allowing emulation code to opt out of 1of256 when it's known it should never happen as accidentally interpreting something as the 1of256 SOF would still likely interrupt emulation until the timeout occurs which would almost certainly cause the reader to time out.

A quick look at the patch seems fine to me, but I'm not an expert on the HAL design.

@bettse
Copy link
Contributor

bettse commented Sep 17, 2024

This should be an easier one to review since it is just adding a new API and not changing how anything functions

aaronjamt added a commit to aaronjamt/Momentum-Firmware that referenced this pull request Sep 17, 2024
Willy-JL added a commit to Next-Flip/Momentum-Firmware that referenced this pull request Sep 17, 2024
* Add API to enforce ISO15693 mode
See flipperdevices/flipperzero-firmware#3885

* Revert symbols version

* Format

* Update changelog

---------

Co-authored-by: Willy-JL <[email protected]>
@@ -26,6 +26,7 @@ typedef enum {
struct Iso15693Parser {
Iso15693ParserState state;
Iso15693ParserMode mode;
bool detect_mode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this detect_mode be moved to the Iso15693ParserMode enum?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, so you're proposing to add two new modes like Iso15693ParserModeForced1OutOf4 and Iso15693ParserModeForced1OutOf256 in place of the boolean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but there is a more important thing, which I've missed previously. Putting protocol specific functions into nfc.c / nfc.h layer it's not very good idea, because nfc layer was designed to be a generic protocol free. I had a quick conversation with original nfc stack designer and he agreed on that.
Also, we came up to the idea that we need to investigate this problem deeper and try to fix this in iso15 parser. Currently I'm investigating this issue and will be in touch with you on the results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants