feat: NTAG variant detection via GET_VERSION (#22)#119
Closed
sjordan0228 wants to merge 1 commit into
Closed
Conversation
Sends GET_VERSION (0x60) during tag classification to identify exact NTAG model (213/215/216, Ultralight EV1). Write paths now validate tag capacity before writing — rejects writes that exceed page count. Variant exposed in /api/status JSON.
📝 WalkthroughWalkthroughThis PR implements NTAG GET_VERSION (0x60) command support across the NFC connection layer, introduces variant classification for NTAG models, tracks variant information in tag scan results, and enforces write-capacity checks to prevent exceeding tag storage limits. Changes
Sequence DiagramsequenceDiagram
actor Client
participant WebServer as WebServerManager
participant NFCManager
participant HardwareConn as HardwareNFCConnection
participant Device as NFC Device
Client->>WebServer: GET /api/status
WebServer->>NFCManager: query current tag state
NFCManager->>NFCManager: check if tag needs<br/>variant detection
alt Tag Present & Variant Unknown
NFCManager->>HardwareConn: ntagGetVersion()
HardwareConn->>Device: send 0x60 command
Device-->>HardwareConn: version response (8 bytes)
HardwareConn-->>NFCManager: return version data
NFCManager->>NFCManager: mapStorageByte(version[6])<br/>to NtagVariant
NFCManager->>NFCManager: store variant in<br/>currentSpool
end
WebServer->>NFCManager: get current spool<br/>with variant
NFCManager-->>WebServer: return state +<br/>variant info
alt Variant Known
WebServer->>WebServer: add ntag_variant<br/>& ntag_pages to JSON
end
WebServer-->>Client: JSON with variant<br/>metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NFCTypes.h (1)
57-76: 🧹 Nitpick | 🔵 TrivialConsider adding default member initializers for new
variantfields.The
variantfield inTagScanResultandCurrentSpoolStatelacks a default initializer. WhileclassifyTag()explicitly setsresult.variant = NtagVariant::Unknown(line 1234 in NFCManager.cpp), aggregate initialization ormemsetelsewhere may leavevariantuninitialized. Adding a default ensures safety.🛡️ Suggested defensive initialization
struct TagScanResult { TagProtocol protocol; TagKind kind; - NtagVariant variant; + NtagVariant variant = NtagVariant::Unknown; char uid_hex[17]; // null-terminated UID hex string (up to 8 bytes = 16 hex chars) bool present; bool tag_data_valid; }; struct CurrentSpoolState { bool present; bool blank_tag_present; // Deprecated: use kind == TagKind::BlankTag instead TagKind kind; - NtagVariant variant; + NtagVariant variant = NtagVariant::Unknown; char spool_id[17];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NFCTypes.h` around lines 57 - 76, The new NtagVariant member named variant in struct TagScanResult and struct CurrentSpoolState can remain uninitialized during aggregate initialization or memset; update both structs (TagScanResult::variant and CurrentSpoolState::variant) to include a default member initializer (e.g., = NtagVariant::Unknown) so the variant always has a known value even before classifyTag() runs; modify the declarations of TagScanResult and CurrentSpoolState to set the default for variant accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NFCManager.cpp`:
- Line 490: currentSpool.variant is only set for Bambu and ISO14443A paths but
never cleared on ISO15693/blank/remove or other non-NTAG transitions, leaving
stale ntag_variant/ntag_pages in status; update the state transition code for
ISO15693 handling and the blank/remove flows (where currentSpool.variant is not
overwritten) to explicitly clear/reset currentSpool.variant (and related NTAG
fields if present) whenever the tag type is not NTAG so the status can't retain
stale values—look for assignments to currentSpool.variant in the Bambu/ISO14443A
code paths and add matching reset logic in the ISO15693, blank, and remove
handlers in NFCManager.cpp.
- Around line 1611-1616: The capacity check in NFCManager::checkWriteCapacity
incorrectly compares an absolute startPage to a usable-page count from
ntagUsablePages(currentSpool.variant); change the logic to compute remaining
usable pages from the absolute startPage and ensure pageCount fits into that
remainder. Specifically, in checkWriteCapacity use the usable-pages value as a
total and verify startPage is within bounds and then assert pageCount <=
(maxPages - startPage) (or equivalently startPage + pageCount <= maxPages) so
writes near the upper boundary aren’t wrongly rejected; keep the same
Serial.printf message using ntagVariantName(currentSpool.variant) for context.
Ensure checks still handle maxPages == 0 and invalid startPage values.
---
Outside diff comments:
In `@src/NFCTypes.h`:
- Around line 57-76: The new NtagVariant member named variant in struct
TagScanResult and struct CurrentSpoolState can remain uninitialized during
aggregate initialization or memset; update both structs (TagScanResult::variant
and CurrentSpoolState::variant) to include a default member initializer (e.g., =
NtagVariant::Unknown) so the variant always has a known value even before
classifyTag() runs; modify the declarations of TagScanResult and
CurrentSpoolState to set the default for variant accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a10a32c9-3901-4fb7-a088-280d6850c6f2
📒 Files selected for processing (9)
src/HardwareNFCConnection.cppsrc/HardwareNFCConnection.hsrc/HardwareNFCConnectionPN532.cppsrc/HardwareNFCConnectionPN532.hsrc/NFCConnectionI.hsrc/NFCManager.cppsrc/NFCManager.hsrc/NFCTypes.hsrc/WebServerManager.cpp
| currentSpool.uid_length = uidLength; | ||
| currentSpool.present = true; | ||
| currentSpool.blank_tag_present = false; | ||
| currentSpool.variant = scan.variant; |
There was a problem hiding this comment.
Reset variant on all non-NTAG state transitions to avoid stale API data.
currentSpool.variant is assigned for Bambu and generic ISO14443A paths, but it is not consistently cleared in ISO15693/blank/remove flows. That can leave stale ntag_variant / ntag_pages in status after tag type changes.
Suggested fix
@@ bool NFCManager::readAndParseTag(uint8_t* uid, uint8_t uid_length) {
currentSpool.present = true;
currentSpool.tag_data_valid = true;
+ currentSpool.variant = NtagVariant::Unknown;
@@ if (!readOk) {
currentSpool.blank_tag_present = true;
currentSpool.kind = TagKind::BlankTag;
+ currentSpool.variant = NtagVariant::Unknown;
@@ else { // No tag detected - clear state
currentSpool.present = false;
currentSpool.blank_tag_present = false;
+ currentSpool.variant = NtagVariant::Unknown;
@@ bool NFCManager::scanOnce() {
currentSpool.blank_tag_present = true;
currentSpool.kind = TagKind::BlankTag;
+ currentSpool.variant = NtagVariant::Unknown;
@@ else { // scanOnce no tag
currentSpool.present = false;
currentSpool.blank_tag_present = false;
+ currentSpool.variant = NtagVariant::Unknown;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NFCManager.cpp` at line 490, currentSpool.variant is only set for Bambu
and ISO14443A paths but never cleared on ISO15693/blank/remove or other non-NTAG
transitions, leaving stale ntag_variant/ntag_pages in status; update the state
transition code for ISO15693 handling and the blank/remove flows (where
currentSpool.variant is not overwritten) to explicitly clear/reset
currentSpool.variant (and related NTAG fields if present) whenever the tag type
is not NTAG so the status can't retain stale values—look for assignments to
currentSpool.variant in the Bambu/ISO14443A code paths and add matching reset
logic in the ISO15693, blank, and remove handlers in NFCManager.cpp.
Comment on lines
+1611
to
+1616
| bool NFCManager::checkWriteCapacity(uint8_t startPage, uint8_t pageCount, const char* writeType) { | ||
| uint16_t maxPages = ntagUsablePages(currentSpool.variant); | ||
| if (maxPages == 0) return true; // Unknown variant — skip check | ||
| if (startPage + pageCount > maxPages) { | ||
| Serial.printf("NFCManager: %s rejected — needs %d pages (start=%d), tag has %d (%s)\n", | ||
| writeType, pageCount, startPage, maxPages, ntagVariantName(currentSpool.variant)); |
There was a problem hiding this comment.
Capacity boundary check uses the wrong frame of reference.
startPage + pageCount > maxPages mixes absolute page index with usable-page count. If ntagUsablePages() returns count from page 4 (e.g., 45 on NTAG213), valid writes near boundary are incorrectly rejected.
Suggested fix
bool NFCManager::checkWriteCapacity(uint8_t startPage, uint8_t pageCount, const char* writeType) {
- uint16_t maxPages = ntagUsablePages(currentSpool.variant);
- if (maxPages == 0) return true; // Unknown variant — skip check
- if (startPage + pageCount > maxPages) {
+ const uint16_t usablePages = ntagUsablePages(currentSpool.variant);
+ if (usablePages == 0) {
+ return true; // Unknown variant — skip check
+ }
+
+ constexpr uint16_t kFirstUsablePage = 4;
+ const uint16_t writeEndExclusive = static_cast<uint16_t>(startPage) + pageCount;
+ const uint16_t usableEndExclusive = kFirstUsablePage + usablePages;
+ if (writeEndExclusive > usableEndExclusive) {
Serial.printf("NFCManager: %s rejected — needs %d pages (start=%d), tag has %d (%s)\n",
- writeType, pageCount, startPage, maxPages, ntagVariantName(currentSpool.variant));
+ writeType, pageCount, startPage, usablePages, ntagVariantName(currentSpool.variant));
return false;
}
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool NFCManager::checkWriteCapacity(uint8_t startPage, uint8_t pageCount, const char* writeType) { | |
| uint16_t maxPages = ntagUsablePages(currentSpool.variant); | |
| if (maxPages == 0) return true; // Unknown variant — skip check | |
| if (startPage + pageCount > maxPages) { | |
| Serial.printf("NFCManager: %s rejected — needs %d pages (start=%d), tag has %d (%s)\n", | |
| writeType, pageCount, startPage, maxPages, ntagVariantName(currentSpool.variant)); | |
| bool NFCManager::checkWriteCapacity(uint8_t startPage, uint8_t pageCount, const char* writeType) { | |
| const uint16_t usablePages = ntagUsablePages(currentSpool.variant); | |
| if (usablePages == 0) { | |
| return true; // Unknown variant — skip check | |
| } | |
| constexpr uint16_t kFirstUsablePage = 4; | |
| const uint16_t writeEndExclusive = static_cast<uint16_t>(startPage) + pageCount; | |
| const uint16_t usableEndExclusive = kFirstUsablePage + usablePages; | |
| if (writeEndExclusive > usableEndExclusive) { | |
| Serial.printf("NFCManager: %s rejected — needs %d pages (start=%d), tag has %d (%s)\n", | |
| writeType, pageCount, startPage, usablePages, ntagVariantName(currentSpool.variant)); | |
| return false; | |
| } | |
| return true; | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 1611-1611: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 1611-1611: method 'checkWriteCapacity' can be made static
(readability-convert-member-functions-to-static)
[warning] 1611-1611: 2 adjacent parameters of 'checkWriteCapacity' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 1611-1611: the first parameter in the range is 'startPage'
(clang)
[note] 1611-1611: the last parameter in the range is 'pageCount'
(clang)
[warning] 1612-1612: variable 'maxPages' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1613-1613: statement should be inside braces
(readability-braces-around-statements)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NFCManager.cpp` around lines 1611 - 1616, The capacity check in
NFCManager::checkWriteCapacity incorrectly compares an absolute startPage to a
usable-page count from ntagUsablePages(currentSpool.variant); change the logic
to compute remaining usable pages from the absolute startPage and ensure
pageCount fits into that remainder. Specifically, in checkWriteCapacity use the
usable-pages value as a total and verify startPage is within bounds and then
assert pageCount <= (maxPages - startPage) (or equivalently startPage +
pageCount <= maxPages) so writes near the upper boundary aren’t wrongly
rejected; keep the same Serial.printf message using
ntagVariantName(currentSpool.variant) for context. Ensure checks still handle
maxPages == 0 and invalid startPage values.
Contributor
Author
|
Superseded by feature/ntag-variant-v2 (merged directly to dev — original branch conflicted with refactor) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/api/statusJSON (ntag_variant,ntag_pages)Changes
NtagVariantenum +ntagUsablePages()/ntagVariantName()helpers in NFCTypes.hntagGetVersion()on NFCConnectionI interface, implemented for both PN5180 and PN532checkWriteCapacity()guard on TigerTag, OpenTag3D, and OpenSpool write pathsCurrentSpoolStateandTagScanResultTest plan
/api/statusincludesntag_variantandntag_pagesfieldsSummary by CodeRabbit
Release Notes