feat(#13): TigerTag partial write — update only changed pages#164
Merged
Conversation
Pre-read pages 4–13, diff at 4-byte page granularity, write only contiguous runs of changed pages (max 5 runs for 10 alternating pages). No-op when nothing changed. Falls back to full 10-page write if the pre-read returns fewer than 40 bytes. TigerTag uses a fixed binary layout across pages 4–13, so byte offsets never shift — page-granular partial writes are safe. Preserves original log output, forceRescan, and return-value semantics.
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🤖 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`:
- Around line 1812-1829: The partial-run write loop in NFCManager (using
connection_->writeISO14443Pages and request.data.tigertag_data) can leave a
mixed/partial TigerTag if a later run fails; modify the logic so that on any run
failure you attempt a full-write fallback of pages 4–13 (all 10 pages) using the
original tigertag_data, then read back the full 40 bytes (pages 4–13) via
connection_->readISO14443Pages (or the existing read method) to verify the
contents match request.data.tigertag_data before reporting success; ensure
failure paths still log via LogBuffer::getInstance().logPrintf and
Serial.printf, call forceRescan() only after verified success, and return false
if verification fails.
🪄 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: 453cc35b-2950-4b56-a061-d182af3f8d36
📒 Files selected for processing (1)
src/NFCManager.cpp
Comment on lines
+1812
to
+1829
| uint8_t totalPages = 0; | ||
| for (uint8_t r = 0; r < runCount; ++r) { | ||
| const PageRun& run = runs[r]; | ||
| const uint8_t* data = request.data.tigertag_data + (run.startPage - 4) * 4; | ||
| if (!connection_->writeISO14443Pages(run.startPage, run.pageCount, data, run.pageCount * 4)) { | ||
| Serial.printf("NFCManager: WRITE_TIGERTAG failed at run %u (page %u, %u pages)\n", | ||
| r, run.startPage, run.pageCount); | ||
| LogBuffer::getInstance().logPrintf("Write TigerTag: FAILED\n"); | ||
| return false; | ||
| } | ||
| totalPages += run.pageCount; | ||
| } | ||
|
|
||
| Serial.printf("NFCManager: WRITE_TIGERTAG wrote %u/10 pages in %u run(s)\n", totalPages, runCount); | ||
| LogBuffer::getInstance().logPrintf("Write TigerTag: OK (%u/10 pages, %u run%s)\n", | ||
| totalPages, runCount, runCount == 1 ? "" : "s"); | ||
| forceRescan(); | ||
| return true; |
There was a problem hiding this comment.
Add convergence/verification after partial TigerTag writes.
Line 1816 writes each changed run independently, but Line 1820 returns after a failed later run even if earlier runs already changed the tag. Also, Lines 1825-1829 report success without re-reading pages 4-13, so this can return success for an unverified or mixed TigerTag image. Add a full-write fallback on run failure and verify the final 40 bytes before returning true.
🐛 Proposed fix: converge on full write after run failure and verify read-back
uint8_t totalPages = 0;
+ bool needsFullWriteFallback = false;
for (uint8_t r = 0; r < runCount; ++r) {
const PageRun& run = runs[r];
const uint8_t* data = request.data.tigertag_data + (run.startPage - 4) * 4;
if (!connection_->writeISO14443Pages(run.startPage, run.pageCount, data, run.pageCount * 4)) {
Serial.printf("NFCManager: WRITE_TIGERTAG failed at run %u (page %u, %u pages)\n",
r, run.startPage, run.pageCount);
- LogBuffer::getInstance().logPrintf("Write TigerTag: FAILED\n");
- return false;
+ LogBuffer::getInstance().logPrintf("Write TigerTag: partial run failed, trying full write\n");
+ needsFullWriteFallback = true;
+ break;
}
totalPages += run.pageCount;
}
+
+ if (needsFullWriteFallback) {
+ if (!connection_->writeISO14443Pages(4, 10, request.data.tigertag_data, 40)) {
+ Serial.println("NFCManager: WRITE_TIGERTAG full-write fallback failed");
+ LogBuffer::getInstance().logPrintf("Write TigerTag: FAILED\n");
+ return false;
+ }
+ totalPages = 10;
+ }
+
+ uint8_t verify[40];
+ uint16_t verifyBytes = connection_->readISO14443Pages(4, 10, verify, sizeof(verify), true);
+ if (verifyBytes < 40 || memcmp(verify, request.data.tigertag_data, sizeof(verify)) != 0) {
+ Serial.printf("NFCManager: WRITE_TIGERTAG verify failed (%u bytes read)\n", verifyBytes);
+ LogBuffer::getInstance().logPrintf("Write TigerTag: VERIFY FAILED\n");
+ return false;
+ }
Serial.printf("NFCManager: WRITE_TIGERTAG wrote %u/10 pages in %u run(s)\n", totalPages, runCount);📝 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
| uint8_t totalPages = 0; | |
| for (uint8_t r = 0; r < runCount; ++r) { | |
| const PageRun& run = runs[r]; | |
| const uint8_t* data = request.data.tigertag_data + (run.startPage - 4) * 4; | |
| if (!connection_->writeISO14443Pages(run.startPage, run.pageCount, data, run.pageCount * 4)) { | |
| Serial.printf("NFCManager: WRITE_TIGERTAG failed at run %u (page %u, %u pages)\n", | |
| r, run.startPage, run.pageCount); | |
| LogBuffer::getInstance().logPrintf("Write TigerTag: FAILED\n"); | |
| return false; | |
| } | |
| totalPages += run.pageCount; | |
| } | |
| Serial.printf("NFCManager: WRITE_TIGERTAG wrote %u/10 pages in %u run(s)\n", totalPages, runCount); | |
| LogBuffer::getInstance().logPrintf("Write TigerTag: OK (%u/10 pages, %u run%s)\n", | |
| totalPages, runCount, runCount == 1 ? "" : "s"); | |
| forceRescan(); | |
| return true; | |
| uint8_t totalPages = 0; | |
| bool needsFullWriteFallback = false; | |
| for (uint8_t r = 0; r < runCount; ++r) { | |
| const PageRun& run = runs[r]; | |
| const uint8_t* data = request.data.tigertag_data + (run.startPage - 4) * 4; | |
| if (!connection_->writeISO14443Pages(run.startPage, run.pageCount, data, run.pageCount * 4)) { | |
| Serial.printf("NFCManager: WRITE_TIGERTAG failed at run %u (page %u, %u pages)\n", | |
| r, run.startPage, run.pageCount); | |
| LogBuffer::getInstance().logPrintf("Write TigerTag: partial run failed, trying full write\n"); | |
| needsFullWriteFallback = true; | |
| break; | |
| } | |
| totalPages += run.pageCount; | |
| } | |
| if (needsFullWriteFallback) { | |
| if (!connection_->writeISO14443Pages(4, 10, request.data.tigertag_data, 40)) { | |
| Serial.println("NFCManager: WRITE_TIGERTAG full-write fallback failed"); | |
| LogBuffer::getInstance().logPrintf("Write TigerTag: FAILED\n"); | |
| return false; | |
| } | |
| totalPages = 10; | |
| } | |
| uint8_t verify[40]; | |
| uint16_t verifyBytes = connection_->readISO14443Pages(4, 10, verify, sizeof(verify), true); | |
| if (verifyBytes < 40 || memcmp(verify, request.data.tigertag_data, sizeof(verify)) != 0) { | |
| Serial.printf("NFCManager: WRITE_TIGERTAG verify failed (%u bytes read)\n", verifyBytes); | |
| LogBuffer::getInstance().logPrintf("Write TigerTag: VERIFY FAILED\n"); | |
| return false; | |
| } | |
| Serial.printf("NFCManager: WRITE_TIGERTAG wrote %u/10 pages in %u run(s)\n", totalPages, runCount); | |
| LogBuffer::getInstance().logPrintf("Write TigerTag: OK (%u/10 pages, %u run%s)\n", | |
| totalPages, runCount, runCount == 1 ? "" : "s"); | |
| forceRescan(); | |
| return true; |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 1813-1813: loop variable name 'r' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 1819-1819: do not call c-style vararg functions
(cppcoreguidelines-pro-type-vararg)
[warning] 1826-1826: do not call c-style vararg functions
(cppcoreguidelines-pro-type-vararg)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NFCManager.cpp` around lines 1812 - 1829, The partial-run write loop in
NFCManager (using connection_->writeISO14443Pages and
request.data.tigertag_data) can leave a mixed/partial TigerTag if a later run
fails; modify the logic so that on any run failure you attempt a full-write
fallback of pages 4–13 (all 10 pages) using the original tigertag_data, then
read back the full 40 bytes (pages 4–13) via connection_->readISO14443Pages (or
the existing read method) to verify the contents match
request.data.tigertag_data before reporting success; ensure failure paths still
log via LogBuffer::getInstance().logPrintf and Serial.printf, call forceRescan()
only after verified success, and return false if verification fails.
If any per-run writeISO14443Pages call fails mid-sequence, earlier runs may have already committed, leaving the tag in a mixed state. On run failure now retry with a full 10-page rewrite using the original tigertag_data before returning — only call forceRescan on verified success, preserve FAILED log + return false if the fallback also fails.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/NFCManager.cpp (1)
1767-1837:⚠️ Potential issue | 🔴 CriticalVerify the final TigerTag image before reporting success.
The fallback paths and the normal partial-write success path still return success after write ACKs only. Re-read pages 4–13 and compare against
request.data.tigertag_databefore logging OK, callingforceRescan(), or returningtrue; otherwise a mixed or incorrectly written TigerTag can be reported as successful.🐛 Proposed fix: verify all write-success paths
bool NFCManager::executeTigerTagWrite(const NFCWriteRequest& request) { if (!validateWriteUid(request.expected_spool_id, "WRITE_TIGERTAG")) return false; if (!checkWriteCapacity(4, 10, "WRITE_TIGERTAG")) return false; + auto verifyTigerTagWrite = [this, &request]() -> bool { + uint8_t verify[40]; + uint16_t verifyBytes = connection_->readISO14443Pages(4, 10, verify, sizeof(verify), true); + if (verifyBytes < 40 || memcmp(verify, request.data.tigertag_data, sizeof(verify)) != 0) { + Serial.printf("NFCManager: WRITE_TIGERTAG verify failed (%u bytes read)\n", verifyBytes); + LogBuffer::getInstance().logPrintf("Write TigerTag: VERIFY FAILED\n"); + return false; + } + return true; + }; + // Pre-read current tag contents so we can write only the pages that actually changed. // TigerTag has a fixed binary layout across pages 4–13, so byte offsets never shift // and page-granular partial writes are safe. uint8_t current[40]; uint16_t bytesRead = connection_->readISO14443Pages(4, 10, current, sizeof(current), true); @@ - if (ok) { + if (ok && verifyTigerTagWrite()) { Serial.println("NFCManager: WRITE_TIGERTAG succeeded"); LogBuffer::getInstance().logPrintf("Write TigerTag: OK\n"); forceRescan(); } else { Serial.println("NFCManager: WRITE_TIGERTAG failed"); @@ LogBuffer::getInstance().logPrintf("Write TigerTag: run failed, full rewrite\n"); - if (connection_->writeISO14443Pages(4, 10, request.data.tigertag_data, 40)) { + if (connection_->writeISO14443Pages(4, 10, request.data.tigertag_data, 40) && + verifyTigerTagWrite()) { Serial.println("NFCManager: WRITE_TIGERTAG full rewrite succeeded"); LogBuffer::getInstance().logPrintf("Write TigerTag: OK (full rewrite)\n"); forceRescan(); return true; } @@ } totalPages += run.pageCount; } + if (!verifyTigerTagWrite()) { + return false; + } + Serial.printf("NFCManager: WRITE_TIGERTAG wrote %u/10 pages in %u run(s)\n", totalPages, runCount); LogBuffer::getInstance().logPrintf("Write TigerTag: OK (%u/10 pages, %u run%s)\n", totalPages, runCount, runCount == 1 ? "" : "s");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NFCManager.cpp` around lines 1767 - 1837, The write paths report success based only on ACKs; you must re-read pages 4–13 and verify the written image matches request.data.tigertag_data before logging OK, calling forceRescan(), or returning true. After any successful connection_->writeISO14443Pages call (the full-write fallback and after all run writes), perform the same pre-read used earlier (the read that produced bytesRead/current) into a 40-byte verify buffer, compare memcmp(verify, request.data.tigertag_data, 40), and only if equal proceed to LogBuffer::getInstance().logPrintf("Write TigerTag: OK..."), forceRescan(), and return true; otherwise log FAILED and return false. Ensure the verification is applied to both the full-rewrite branch and the normal multi-run success path (after totalPages/runCount logging).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/NFCManager.cpp`:
- Around line 1767-1837: The write paths report success based only on ACKs; you
must re-read pages 4–13 and verify the written image matches
request.data.tigertag_data before logging OK, calling forceRescan(), or
returning true. After any successful connection_->writeISO14443Pages call (the
full-write fallback and after all run writes), perform the same pre-read used
earlier (the read that produced bytesRead/current) into a 40-byte verify buffer,
compare memcmp(verify, request.data.tigertag_data, 40), and only if equal
proceed to LogBuffer::getInstance().logPrintf("Write TigerTag: OK..."),
forceRescan(), and return true; otherwise log FAILED and return false. Ensure
the verification is applied to both the full-rewrite branch and the normal
multi-run success path (after totalPages/runCount logging).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1818dd62-5acb-42d9-b5bb-b0f0caacf230
📒 Files selected for processing (1)
src/NFCManager.cpp
Contributor
Author
|
CodeRabbit's follow-up suggestion (readback-verify on both write paths) tracked as #167 for future consideration. Rationale for not addressing in this PR: parity with the pre-existing single-write path (which also didn't verify), ~50–80 ms added to every success path, and spurious failures from PN5180 transceiver-state issues (#20) would need distinguishing from real persistence failures. Merging as-is. |
This was referenced Apr 20, 2026
roomonthethird
pushed a commit
to roomonthethird/spoolsense_scanner
that referenced
this pull request
Apr 22, 2026
Bumps FIRMWARE_VERSION from 1.7.2 → 1.7.3. CHANGELOG.md: - Adds [1.7.3] entry documenting SpoolSense#163 WiFi keep-awake, SpoolSense#165 case files, SpoolSense#164/SpoolSense#13 TigerTag partial writes, SpoolSense#159/SpoolSense#162 openprinttag rename. - Backfills missing [1.7.2] entry (Bambu MIFARE Classic reading, SpoolSense#24). - Backfills missing [1.7.1] entry (writer enrichment + bug fixes, SpoolSense#130/SpoolSense#101/SpoolSense#128/SpoolSense#151 and platform pin).
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
forceRescan, and return-value semantics.Test plan
Summary by CodeRabbit