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

[FL-3661] Troika layout fixes #3365

Merged
merged 9 commits into from
Feb 6, 2024
Merged

[FL-3661] Troika layout fixes #3365

merged 9 commits into from
Feb 6, 2024

Conversation

Astrrra
Copy link
Member

@Astrrra Astrrra commented Jan 15, 2024

What's new

  • Support for different Troika layouts and sub-layouts (currently, layout 0xE and sub-layouts 0x3 and 0x5 are supported). If the parser encounters an unsupported layout/sub-layout, it will exit to avoid showing garbage data.
  • Fixes for 4K Troika cards (see Verification section for more info)
  • Enabling Debug in system settings will result in the parser now showing additional data in results: layout, sub-layout, and data block number. It will also disable the check whether layout is supported or not to simplify diagnosing unknown cards that don't work correctly.

Verification

  • Check parsing of 1K and 4K cards with layout 0xE and sub-layouts 0x3 and 0x5 (4 variations in total).
  • 4K Troika cards seem to have had a bug with the wrong data block being used, at least all the 4K cards I have access to (with the same layout/sub-layout) have a different data block. This makes me a bit suspicious, therefore further experimentation is required on a bigger sample size:
    • Check all available 4K Troika cards with Debug disabled, verify that they either show correct data or aren't parsed at all (due to yet unsupported layout). If any of the cards gets parsed but shows incorrect data, please provide a full dump of the card for further testing. Ideally, that shouldn't happen.
    • Check all available 4K Troika cards with Debug enabled. All the cards should trigger the parser and show some results, possibly wrong. Note all the cards with wrong results and provide dumps of them as well, this will help with support for more layouts. This is OK and expected, as we don't yet support all the layouts.

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

Copy link

github-actions bot commented Jan 15, 2024

Compiled f7 firmware for commit f1e4fff2:

Copy link

github-actions bot commented Jan 15, 2024

PVS-Studio report for commit e607abb8:

@Astrrra Astrrra requested a review from gsurkov as a code owner January 23, 2024 18:53
@gornekich gornekich self-assigned this Jan 23, 2024
gornekich
gornekich previously approved these changes Jan 24, 2024
@gornekich gornekich mentioned this pull request Jan 25, 2024
3 tasks
@hedger hedger added the NFC NFC-related label Feb 6, 2024
gornekich
gornekich previously approved these changes Feb 6, 2024
// In layout 0x5 balance in bits 165:185 ( from sector start, length 20).

if(layout == TroikaLayoutE && sub_layout == TroikaSublayout3) {
const uint8_t* temp_ptr = &data->block[start_block_num + 1].data[7];
Copy link
Member

Choose a reason for hiding this comment

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

use a common function in both branches?


TroikaLayout result = TroikaLayoutUnknown;
switch(layout) {
case 0x2:
Copy link
Member

Choose a reason for hiding this comment

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

pointless - enum values are equal to switch cases, we can use them as-is.
So, just check for known values and cast to enum, return Unknown otherwise

@hedger hedger merged commit e078296 into dev Feb 6, 2024
9 checks passed
@hedger hedger deleted the astra/3661-troika-fix branch February 6, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NFC NFC-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants