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

NFC: Auto-run reading scripts #1322

Closed
wants to merge 14 commits into from
Closed

Conversation

djsime1
Copy link
Contributor

@djsime1 djsime1 commented Jun 15, 2022

What's new

  • "Read card" has been renamed and will automatically run corresponding reading script.
  • Old functionality has been moved to reading scripts menu under "Generic NFC."
  • Improve navigation flow for MF Ultralight as proof of concept.
  • Replace MF Desfire menu with save button & fix scene states under MF Classic menu.
  • Move MF Ultralight menu into success scene.

Todo

  • Move remaining submenus into success scenes.
  • Apply same back/retry navigation to other card types.
  • Test saving with a real MF Desfire card (I don't have any).
  • Suppress success notification if returning from save/emulate menus.

Notes

This PR is meant to be a proof of concept for aforementioned changes. I'd like to know if automatically running reading scripts with the universal read option is better or worse than what happens currently. My motivation is that I've seen various posts of confused users who didn't realize to fully read a card they had to run the corresponding script.
I'd like some people to flash this and provide feedback whether or not these changes improve the flow of NFC scenes.

Verification

  • Build & Flash
  • Open NFC app and run the top "Read NFC Tag" option.
  • Tap any supported NFC tag to the device.
  • Corresponding reading app should automatically run.
  • Original functionality is under Reading Scripts > Read Generic NFC.

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

@djsime1
Copy link
Contributor Author

djsime1 commented Jun 15, 2022

Build artifacts for commit 2297d19:

@djsime1
Copy link
Contributor Author

djsime1 commented Jun 21, 2022

Build artifacts for commit 8a5a5e6:

@djsime1
Copy link
Contributor Author

djsime1 commented Jun 21, 2022

Now that #1313 has been merged, the MF Ultralight reading fail state can be triggered. Known bugs:

  • Card type can be incorrectly determined (Tested with EV1)
  • data_size is zero even when partial data is stored.
  • UI text can overflow to a new line and get cut off-screen.

@djsime1
Copy link
Contributor Author

djsime1 commented Jun 21, 2022

3 bugs fixed, nearing final review state. New todo:

  • Handle partial data saves.
  • Port MF UL Scene changes to other NFC types.
  • Remove unused code/files.

New build file coming once I get some of that done. In the mean time, here's a video demo of the changes:

1655849094_qFlipper.mp4

@djsime1
Copy link
Contributor Author

djsime1 commented Jun 23, 2022

Build artifacts for commit d4b9cb9:

@djsime1
Copy link
Contributor Author

djsime1 commented Jun 23, 2022

Added a new tag_size attribute to MF UL data, this hopefully allows saving/emulation to work normally with unread bytes being 0x00 instead of not present at all, breaking standards.

@djsime1 djsime1 marked this pull request as ready for review June 23, 2022 08:19
@djsime1
Copy link
Contributor Author

djsime1 commented Jun 23, 2022

I'm opening this for review, although I don't expect/recommend a merge. I know there's some work left to do, but I'd like a review from the team before continuing on.

@gornekich
Copy link
Member

Hello @djsime1! You did wonderful work, but we can't merge your PR...
Our designers just finished reworking NFC app, and I will start implementation soon. We also come to an idea to make auto reading NFC tags.
Soon we will make a channel in official Flipper Discord room, where we want to discuss new features to our repo with contributers. We will consider your ideas from this PR and you are welcome to suggest features in mentioned channel.

@djsime1
Copy link
Contributor Author

djsime1 commented Jun 27, 2022

Hello @djsime1! You did wonderful work, but we can't merge your PR... Our designers just finished reworking NFC app, and I will start implementation soon. We also come to an idea to make auto reading NFC tags. Soon we will make a channel in official Flipper Discord room, where we want to discuss new features to our repo with contributers. We will consider your ideas from this PR and you are welcome to suggest features in mentioned channel.

I'll revise and re-open once these changes have been implemented if this PR isn't made obsolete.

@djsime1 djsime1 closed this Jun 27, 2022
@djsime1
Copy link
Contributor Author

djsime1 commented Jul 29, 2022

Revising this (In a new PR) now that #1364 has been merged. Mainly focusing on Mifare UL read issues being reported to the user instead of silently failing. I'll probably add a way to add custom UL password keys like the MF Classic user dict. Additional keys could be derived via #1365 and #1471.

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.

2 participants