Atomic single-pass OpenPrintTag write + bug fixes#38
Conversation
Replace batched field-by-field writes with single CBOR encode + opt_write_to_nfc(). Build fresh tag in memory to avoid CBOR re-encoding region overflow, preserving existing field values as defaults. Write time drops from ~25s to ~5s. Fix buffer overflow cmd[1]→cmd[2] in mifareBlockWrite16. Add mutex protection for tag_data reads in processWriteQueue. Pause scan task before ESP.restart() to prevent tag corruption. Bump NFCScanTask stack 6144→8192.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | Title clearly summarizes the main change: atomic single-pass writes replacing batched writes, plus bug fixes mentioned in objectives. |
| Description check | ✅ Passed | Description covers summary, changes, test plan with checkmarks, and links to 4 closed issues; follows template structure with meaningful detail. |
| Linked Issues check | ✅ Passed | Changes fully implement all 4 linked issues: atomic write in #19, buffer overflow fix in #25, mutex protection in #26, and scan task pause before restart in #27. |
| Out of Scope Changes check | ✅ Passed | All code changes are directly tied to the 4 linked objectives; no unrelated modifications detected in the changeset. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
fix/atomic-openprinttag-write
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 5-12: The markdown headings "### Changed" and "### Fixed" are
immediately followed by list items and trigger markdownlint MD022; insert a
single blank line after each of those subsection headings (the "### Changed" and
"### Fixed" lines) so there is an empty line before the first list item under
each heading, keeping the changelog lint-clean.
In `@src/NFCManager.cpp`:
- Around line 1690-1728: The code currently logs errors from opt_set_* calls
(including opt_set_material_class and each ATOMIC_SET) but continues and calls
opt_write_to_nfc(), breaking atomicity; change the flow to capture the first
non-OPT_OK return value from opt_set_material_class and each ATOMIC_SET call,
stop applying further fields, clear or leave atomicWriteFields_.pending set
appropriately, and return/propagate the error before calling opt_write_to_nfc();
reference the opt_set_material_class call, the ATOMIC_SET macro/uses,
atomicWriteFields_.pending, and opt_write_to_nfc() so the fix checks err after
each call, aborts on the first failure, and avoids performing the final write
when any setter fails.
- Around line 1637-1662: The code currently aliases the shared
atomicWriteFields_ via const AtomicWriteFields& f = atomicWriteFields_ while
still holding tagMutex and then releases the lock, allowing concurrent
setAtomicWriteFields() to mutate the shared payload; fix by making a by-value
snapshot while still holding the lock (e.g. AtomicWriteFields f =
atomicWriteFields_ or a properly named local copy) and then release tagMutex so
the in-flight request uses its own copy; ensure any clearing of
atomicWriteFields_.pending or other mutations that must be atomic remain
protected by the same mutex or are performed on the snapshot instead to avoid
races.
- Around line 1739-1762: The verify loop using maxRetries calls
opt_read_from_nfc and opt_parse_ndef into writeScratchTag_ but currently always
commits writeScratchTag_ into currentSpool even when verification ultimately
fails; update the code after the retry loop to check if err != OPT_OK and, if
so, log/return false without taking tagMutex or modifying currentSpool (or
alternatively preserve and commit a pre-read copy instead of the mutated
writeScratchTag_); reference the verify loop variables and functions
(maxRetries, opt_read_from_nfc, opt_parse_ndef, writeScratchTag_, currentSpool,
tagMutex) and ensure the commit to currentSpool only happens when verification
succeeded.
In `@src/WebServerManager.cpp`:
- Around line 1007-1019: The code sets fields.pending and calls
NFCManager::getInstance().setAtomicWriteFields(fields) but ignores the result of
NFCManager::getInstance().enqueueWrite(req); change enqueue handling so you
check its return value and only return HTTP 200 when enqueueWrite succeeds; on
failure (queue full) call fields.pending = false and call
setAtomicWriteFields(fields) to clear the sidecar, then respond with HTTP 503
via _server.send; ensure you reference the same NFCWriteRequest req,
NFCWriteType::WRITE_ATOMIC and the NFCManager::getInstance().enqueueWrite(...)
call when adding the conditional handling.
- Around line 952-962: The handler currently requires both remaining_g and
initial_weight_g to set has_consumed_weight, preventing partial updates when
only remaining_g is provided; change the logic so that AtomicWriteFields stores
remaining_g independently (add fields.has_remaining_weight and
fields.remaining_g) or compute consumed_weight later in executeWrite() using the
snapshotted full weight; update the parsing code that references
initial_weight_g/remaining_g to set the new remaining fields (or remove
consumed_weight setting here) and implement consumed weight calculation inside
executeWrite() using the stored snapshot and fields.remaining_g, ensuring
has_consumed_weight is derived there instead of only in the request parsing.
🪄 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: 9d0fdbd4-555a-4da5-a34b-4159a8fd5cb2
📒 Files selected for processing (6)
CHANGELOG.mdlib/PN5180/PN5180ISO14443.cppsrc/NFCManager.cppsrc/NFCManager.hsrc/NFCWriteTypes.hsrc/WebServerManager.cpp
| if (!atomicWriteFields_.pending) { | ||
| Serial.println("NFCManager: WRITE_ATOMIC - no atomic fields pending"); | ||
| return false; | ||
| } | ||
|
|
||
| // Validate and snapshot under mutex | ||
| if (xSemaphoreTake(tagMutex, pdMS_TO_TICKS(100)) != pdTRUE) { | ||
| Serial.println("NFCManager: WRITE_ATOMIC - could not acquire tagMutex"); | ||
| atomicWriteFields_.pending = false; | ||
| return false; | ||
| } | ||
| if (!currentSpool.tag_data_valid) { | ||
| xSemaphoreGive(tagMutex); | ||
| atomicWriteFields_.pending = false; | ||
| return false; | ||
| } | ||
| if (request.expected_spool_id[0] != '\0' && | ||
| strcmp(currentSpool.spool_id, request.expected_spool_id) != 0) { | ||
| Serial.printf("NFCManager: WRITE_ATOMIC rejected: expected %s but found %s\n", | ||
| request.expected_spool_id, currentSpool.spool_id); | ||
| xSemaphoreGive(tagMutex); | ||
| atomicWriteFields_.pending = false; | ||
| return false; | ||
| } | ||
| // Read defaults from the existing tag for any fields not provided | ||
| const AtomicWriteFields& f = atomicWriteFields_; |
There was a problem hiding this comment.
Copy the atomic payload by value before releasing the lock.
Line 1662 binds const AtomicWriteFields& f = atomicWriteFields_, so this code keeps aliasing the shared sidecar after xSemaphoreGive(tagMutex). A concurrent setAtomicWriteFields() from src/WebServerManager.cpp Line 1008 can overwrite the in-flight request's fields, and the unlocked pending checks/clears on Lines 1637, 1645, 1650, and 1658 race with that same writer. The queued request needs its own payload, or at minimum this path needs a dedicated mutex and a by-value snapshot.
🧰 Tools
🪛 Clang (14.0.6)
[note] 1637-1637: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1643-1643: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1648-1648: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1653-1653: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[warning] 1662-1662: variable name 'f' is too short, expected at least 3 characters
(readability-identifier-length)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NFCManager.cpp` around lines 1637 - 1662, The code currently aliases the
shared atomicWriteFields_ via const AtomicWriteFields& f = atomicWriteFields_
while still holding tagMutex and then releases the lock, allowing concurrent
setAtomicWriteFields() to mutate the shared payload; fix by making a by-value
snapshot while still holding the lock (e.g. AtomicWriteFields f =
atomicWriteFields_ or a properly named local copy) and then release tagMutex so
the in-flight request uses its own copy; ensure any clearing of
atomicWriteFields_.pending or other mutations that must be atomic remain
protected by the same mutex or are performed on the snapshot instead to avoid
races.
| // Set material class (required by spec) | ||
| opt_set_material_class(&writeScratchTag_, OPT_MATERIAL_CLASS_FFF); | ||
|
|
||
| // Apply each field: use provided value if has_*, else use existing tag default | ||
| #define ATOMIC_SET(name, call) do { \ | ||
| err = (call); \ | ||
| if (err != OPT_OK) { \ | ||
| Serial.printf("NFCManager: WRITE_ATOMIC " name " failed: %s\n", opt_error_str(err)); \ | ||
| } \ | ||
| } while(0) | ||
|
|
||
| ATOMIC_SET("material_type", opt_set_material_type(&writeScratchTag_, f.has_material_type ? f.material_type : cur_mat_type)); | ||
| ATOMIC_SET("color", opt_set_primary_color(&writeScratchTag_, f.has_color ? f.color : cur_color)); | ||
| ATOMIC_SET("initial_weight", opt_set_actual_full_weight(&writeScratchTag_, f.has_initial_weight ? f.initial_weight_g : cur_full_wt)); | ||
| ATOMIC_SET("consumed_weight",opt_set_consumed_weight(&writeScratchTag_, f.has_consumed_weight ? f.consumed_weight : cur_consumed)); | ||
| if (f.has_brand_name || cur_brand[0] != '\0') | ||
| ATOMIC_SET("brand_name", opt_set_brand_name(&writeScratchTag_, f.has_brand_name ? f.brand_name : cur_brand)); | ||
| if (f.has_density || cur_density > 0.0f) | ||
| ATOMIC_SET("density", opt_set_density(&writeScratchTag_, f.has_density ? f.density : cur_density)); | ||
| if (f.has_diameter || cur_diameter > 0.0f) | ||
| ATOMIC_SET("diameter", opt_set_filament_diameter(&writeScratchTag_, f.has_diameter ? f.diameter_mm : cur_diameter)); | ||
| if (f.has_material_name || cur_mat_name[0] != '\0') | ||
| ATOMIC_SET("material_name", opt_set_material_name(&writeScratchTag_, f.has_material_name ? f.material_name : cur_mat_name)); | ||
| if (f.has_min_print_temp || cur_min_pt != 0) | ||
| ATOMIC_SET("min_print_temp", opt_set_min_print_temp(&writeScratchTag_, f.has_min_print_temp ? f.min_print_temp : cur_min_pt)); | ||
| if (f.has_max_print_temp || cur_max_pt != 0) | ||
| ATOMIC_SET("max_print_temp", opt_set_max_print_temp(&writeScratchTag_, f.has_max_print_temp ? f.max_print_temp : cur_max_pt)); | ||
| if (f.has_preheat_temp || cur_pre_t != 0) | ||
| ATOMIC_SET("preheat_temp", opt_set_preheat_temp(&writeScratchTag_, f.has_preheat_temp ? f.preheat_temp : cur_pre_t)); | ||
| if (f.has_min_bed_temp || cur_min_bt != 0) | ||
| ATOMIC_SET("min_bed_temp", opt_set_min_bed_temp(&writeScratchTag_, f.has_min_bed_temp ? f.min_bed_temp : cur_min_bt)); | ||
| if (f.has_max_bed_temp || cur_max_bt != 0) | ||
| ATOMIC_SET("max_bed_temp", opt_set_max_bed_temp(&writeScratchTag_, f.has_max_bed_temp ? f.max_bed_temp : cur_max_bt)); | ||
| // Spoolman ID goes in aux region | ||
| int32_t sm_id = f.has_spoolman_id ? f.spoolman_id : cur_sm_id; | ||
| if (sm_id > 0) | ||
| ATOMIC_SET("spoolman_id", opt_set_gp_spoolman_id(&writeScratchTag_, sm_id)); | ||
| #undef ATOMIC_SET | ||
| atomicWriteFields_.pending = false; |
There was a problem hiding this comment.
Abort the atomic write when any field-apply step fails.
Every ATOMIC_SET(...) only logs an error and then continues to opt_write_to_nfc(), and opt_set_material_class() on Line 1691 is unchecked as well. That breaks the atomic contract: a rejected weight, name, or temperature can still produce a partially rewritten tag while the request reports success. Capture the first OPT_* failure and return before touching the tag.
🧰 Tools
🪛 Clang (14.0.6)
[note] 1701-1701: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1694-1694: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1701-1701: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1702-1702: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1694-1694: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1702-1702: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1703-1703: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1694-1694: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1703-1703: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1704-1704: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1694-1694: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1704-1704: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1705-1705: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1705-1705: +1
(clang)
[note] 1706-1706: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1707-1707: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1707-1707: +1
(clang)
[note] 1708-1708: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1709-1709: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1709-1709: +1
(clang)
[note] 1710-1710: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1711-1711: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1711-1711: +1
(clang)
[note] 1712-1712: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1713-1713: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1714-1714: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1715-1715: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1716-1716: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1717-1717: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1718-1718: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1719-1719: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1720-1720: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1721-1721: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1722-1722: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[note] 1725-1725: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 1726-1726: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 1696-1696: expanded from macro 'ATOMIC_SET'
(clang)
[warning] 1694-1694: function-like macro 'ATOMIC_SET' used; consider a 'constexpr' template function
(cppcoreguidelines-macro-usage)
[warning] 1705-1705: statement should be inside braces
(readability-braces-around-statements)
[warning] 1707-1707: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 1707-1707: statement should be inside braces
(readability-braces-around-statements)
[warning] 1709-1709: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 1709-1709: statement should be inside braces
(readability-braces-around-statements)
[warning] 1711-1711: statement should be inside braces
(readability-braces-around-statements)
[warning] 1713-1713: statement should be inside braces
(readability-braces-around-statements)
[warning] 1715-1715: statement should be inside braces
(readability-braces-around-statements)
[warning] 1717-1717: statement should be inside braces
(readability-braces-around-statements)
[warning] 1719-1719: statement should be inside braces
(readability-braces-around-statements)
[warning] 1721-1721: statement should be inside braces
(readability-braces-around-statements)
[warning] 1724-1724: variable 'sm_id' is not initialized
(cppcoreguidelines-init-variables)
[warning] 1725-1725: 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 1690 - 1728, The code currently logs errors
from opt_set_* calls (including opt_set_material_class and each ATOMIC_SET) but
continues and calls opt_write_to_nfc(), breaking atomicity; change the flow to
capture the first non-OPT_OK return value from opt_set_material_class and each
ATOMIC_SET call, stop applying further fields, clear or leave
atomicWriteFields_.pending set appropriately, and return/propagate the error
before calling opt_write_to_nfc(); reference the opt_set_material_class call,
the ATOMIC_SET macro/uses, atomicWriteFields_.pending, and opt_write_to_nfc() so
the fix checks err after each call, aborts on the first failure, and avoids
performing the final write when any setter fails.
Copy AtomicWriteFields by value before releasing mutex to prevent concurrent HTTP calls from overwriting in-flight write fields. Don't commit tag data to shared state if verification re-read fails — return false instead of silently committing potentially invalid data.
…dling, CHANGELOG lint Allow remaining_g without initial_weight_g by reading existing tag's full weight as base for consumed weight calculation. Return 503 and clear sidecar when write queue is full instead of silently returning success. Fix markdown lint: blank lines after subsection headings in CHANGELOG.
1 participant
Summary
opt_write_to_nfc(). Builds a fresh tag in memory to avoid CBOR re-encoding region overflow, preserving existing field values as defaults. Write time drops from ~25s to ~5s.cmd[1]→cmd[2]inmifareBlockWrite16()(stack corruption, undefined behavior)tag_datareads insendSpoolUpdatedMessageandprocessWriteQueueESP.restart()to prevent tag corruption during writeTest plan
Closes #19, closes #25, closes #26, closes #27
Summary by CodeRabbit
New Features
Bug Fixes
Documentation