Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add NDEF plugin #3923

Closed
wants to merge 1 commit into from
Closed

Add NDEF plugin #3923

wants to merge 1 commit into from

Conversation

bettse
Copy link
Contributor

@bettse bettse commented Sep 27, 2024

What's new

  • Bringing NDEF plugin from fork upstream

Verification

  • Install
  • Read NTAG21x or NTAGI2C... tag with NDEF content. See URL displayed in UI

Screenshot-20240927-155124
Photo of reading Minecraft Earth figure using build of this branch

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
Contributor

@RebornedBrain RebornedBrain left a comment

Choose a reason for hiding this comment

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

Tested this PR, works fine with simple NDEF blocks, but doesn't work with SmartPoster ndefs, (it's where type == 'Sp').
I tried to add SmartPoster support, you can check my patch
sp_ndef.patch
It works fine, but needs to be tested, because I didn't have enough time to test, so please try it

return;
}
switch(tnf) {
case 0x01: // NFC Forum well-known type [NFC RTD]
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to replace all these numbers with appropriate defines and remove comments at all.

// Flags and TNF
uint8_t flags_tnf = *cur++;
// Message Begin should only be set on first record
if(record_num++ && flags_tnf & (1 << 7)) break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simplify this logic with the help of bitfield struct?

// Parse as TLV (see docs above)
while(cur < end) {
switch(*cur++) {
case 0x03: { // NDEF message
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to replace all these numbers with appropriate defines and remove comments at all.

}

static void parse_ndef_uri(FuriString* str, const uint8_t* payload, uint32_t payload_len) {
// https://learn.adafruit.com/adafruit-pn532-rfid-nfc/ndef#uri-records-0x55-slash-u-607763
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, move all such comments out from the function bodies and place them in function description comments in front

@bettse
Copy link
Contributor Author

bettse commented Oct 2, 2024

@RebornedBrain This is bring brought over as-is from a customer firmware fork (not my original code), so I'd prefer not to make any changes except those the flipper devs feel are required. I've been in conversation with the original dev about further improvements (type 4 support) once it has been merged, and these other ideas would be welcome as well.

@bettse bettse closed this Oct 6, 2024
@bettse bettse deleted the add_ndef_plugin branch October 6, 2024 04:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants