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

NfcDict Refactoring #3271

Merged
merged 4 commits into from
Dec 18, 2023
Merged

NfcDict Refactoring #3271

merged 4 commits into from
Dec 18, 2023

Conversation

v0lp3
Copy link
Contributor

@v0lp3 v0lp3 commented Dec 5, 2023

What's new

I refactored the NfcDict structure to make it more generic, so now we can use it with other applications like RFID (for example if we implement dictionary attack on T5577's password). The NfcDict was renamed KeysList and was moved into toolbox, therefore MfUserDict wraps this structure.

Verification

Test NFC application.

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

targets/f7/api_symbols.csv Outdated Show resolved Hide resolved
@hedger hedger added NFC NFC-related Applications Non-core applications New Feature Contains an IMPLEMENTATION of a new feature labels Dec 6, 2023
@skotopes
Copy link
Member

skotopes commented Dec 7, 2023

@v0lp3 also keep in mind that there are other apps that using nfc dict (picopass) that is going to be broken by those changes

@v0lp3
Copy link
Contributor Author

v0lp3 commented Dec 7, 2023

@v0lp3 also keep in mind that there are other apps that using nfc dict (picopass) that is going to be broken by those changes

I see. How about we retain the nfc_dict.* files or replace functions in nfc_dict.h with inlines to ensure compatibility?

@skotopes
Copy link
Member

skotopes commented Dec 7, 2023

@v0lp3 good question, let's wait for @gornekich comment on this PR.

Copy link
Member

@gornekich gornekich left a comment

Choose a reason for hiding this comment

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

Please, rename KeysList to KeysDict and rework nfc application with KeysDict. No need to support old symbols. I will rework picopass app to use new KeysDict by my self.

So, the scope of this PR should be moving NfcDict to KeysDict in toolbox

applications/main/nfc/scenes/nfc_scene_config.h Outdated Show resolved Hide resolved
@v0lp3
Copy link
Contributor Author

v0lp3 commented Dec 13, 2023

Please, rename KeysList to KeysDict and rework nfc application with KeysDict. No need to support old symbols. I will rework picopass app to use new KeysDict by my self.

So, the scope of this PR should be moving NfcDict to KeysDict in toolbox

Ok done

@gornekich
Copy link
Member

gornekich commented Dec 14, 2023

@v0lp3 please, fix unit tests.
Run ./fbt FIRMWARE_APP_SET=unit_tests to build firmware with unit tests. Then connect to cli via ./fbt cli, run unit_tests nfc and make sure all tests are passed

@v0lp3
Copy link
Contributor Author

v0lp3 commented Dec 14, 2023

@v0lp3 please, fix unit tests. Run ./fbt FIRMWARE_APP_SET=unit_tests to build firmware with unit tests. Then connect to cli via ./fbt cli, run unit_tests nfc and make sure all tests are passed

How can i configure a mock flipper zero device?

@gornekich
Copy link
Member

@v0lp3 please, fix unit tests. Run ./fbt FIRMWARE_APP_SET=unit_tests to build firmware with unit tests. Then connect to cli via ./fbt cli, run unit_tests nfc and make sure all tests are passed

How can i configure a mock flipper zero device?

Don't understand your problem. Do you have flipper to test your changes?

@v0lp3
Copy link
Contributor Author

v0lp3 commented Dec 14, 2023

@v0lp3 please, fix unit tests. Run ./fbt FIRMWARE_APP_SET=unit_tests to build firmware with unit tests. Then connect to cli via ./fbt cli, run unit_tests nfc and make sure all tests are passed

How can i configure a mock flipper zero device?

Don't understand your problem. Do you have flipper to test your changes?

I was wondering if it is a problem to run unit tests with PR GitHub actions. I read that the device used in the actions is mock and I was asking you if it would be possible to replicate the configuration in my own environment.

lib/nfc/protocols/mf_classic/mf_classic.h Outdated Show resolved Hide resolved
lib/nfc/protocols/mf_classic/mf_classic.h Outdated Show resolved Hide resolved
lib/toolbox/keys_dict.c Outdated Show resolved Hide resolved
lib/toolbox/keys_dict.c Outdated Show resolved Hide resolved
lib/toolbox/keys_dict.c Outdated Show resolved Hide resolved
@gornekich
Copy link
Member

I was wondering if it is a problem to run unit tests with PR GitHub actions. I read that the device used in the actions is mock and I was asking you if it would be possible to replicate the configuration in my own environment.

I see that you fixed unit tests. Running locally unit_tests in CLI is equivalent to CI tests

Copy link
Member

@gornekich gornekich left a comment

Choose a reason for hiding this comment

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

Well done, thanks!

@skotopes skotopes merged commit 7642d67 into flipperdevices:dev Dec 18, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Applications Non-core applications New Feature Contains an IMPLEMENTATION of a new feature NFC NFC-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants