Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,18 @@
# Changelog

## [Unreleased]

### Changed
- **Atomic OpenPrintTag write** — /api/write-tag now builds a fresh CBOR tag and writes all fields in a single NFC pass (~5s vs ~25s). Eliminates sequential write drops, scan loop race conditions, and CBOR re-encoding overflow. Existing field values are preserved when not specified.

### Fixed
- Buffer overflow in mifareBlockWrite16 — cmd[1] wrote past 1-byte array (undefined behavior)
- Mutex-less tag_data reads — sendSpoolUpdatedMessage and processWriteQueue now hold tagMutex when reading currentSpool
- ESP.restart() during NFC write — scan task is now paused before restart to prevent tag corruption
- NFCScanTask stack bumped 6144→8192 bytes
Comment thread
coderabbitai[bot] marked this conversation as resolved.

---

## [1.5.5] - 2026-03-26

### Added
Expand Down
2 changes: 1 addition & 1 deletion lib/PN5180/PN5180ISO14443.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ bool PN5180ISO14443::mifareBlockRead(uint8_t blockno, uint8_t *buffer) {


uint8_t PN5180ISO14443::mifareBlockWrite16(uint8_t blockno, uint8_t *buffer) {
uint8_t cmd[1];
uint8_t cmd[2];
// Clear RX CRC
writeRegisterWithAndMask(CRC_RX_CONFIG, 0xFFFFFFFE);

Expand Down
152 changes: 145 additions & 7 deletions src/NFCManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ void NFCManager::startScanTask() {
xTaskCreatePinnedToCore(
scanTaskFunc,
"NFCScanTask",
6144,
8192,
this,
1,
&scanTaskHandle,
Expand Down Expand Up @@ -1315,9 +1315,11 @@ void NFCManager::processWriteQueue() {
suppressReDetectionUid_[0] = '\0';
batchHadSuppressSync_ = false;

// Send SpoolDetected to notify listeners (tests, MQTT, LCD) of final state.
// The suppress_spoolman_sync flag prevents Spoolman sync for write_spoolman_spool batches.
sendSpoolDetectedMessage(hadSuppressSync);
// Send SpoolDetected under mutex — sendSpoolDetectedMessage reads currentSpool.tag_data
if (xSemaphoreTake(tagMutex, pdMS_TO_TICKS(100)) == pdTRUE) {
sendSpoolDetectedMessage(hadSuppressSync);
xSemaphoreGive(tagMutex);
}
}
}
}
Expand Down Expand Up @@ -1630,6 +1632,139 @@ bool NFCManager::executeWrite(const NFCWriteRequest& request) {
return ok;
}

// Handle WRITE_ATOMIC — build complete CBOR map from sidecar fields, write once
if (request.type == NFCWriteType::WRITE_ATOMIC) {
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_;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

uint8_t cur_mat_type = 0; opt_get_material_type(&currentSpool.tag_data, &cur_mat_type);
uint8_t cur_color[4] = {255,255,255,255}; opt_get_primary_color(&currentSpool.tag_data, cur_color);
float cur_full_wt = 1000.0f; opt_get_actual_full_weight(&currentSpool.tag_data, &cur_full_wt);
float cur_consumed = 0.0f; opt_get_consumed_weight(&currentSpool.tag_data, &cur_consumed);
char cur_brand[33] = {0}; opt_get_brand_name(&currentSpool.tag_data, cur_brand, sizeof(cur_brand));
float cur_density = 0.0f; opt_get_density(&currentSpool.tag_data, &cur_density);
float cur_diameter = 0.0f; opt_get_filament_diameter(&currentSpool.tag_data, &cur_diameter);
char cur_mat_name[33] = {0}; opt_get_material_name(&currentSpool.tag_data, cur_mat_name, sizeof(cur_mat_name));
int32_t cur_sm_id = -1; opt_get_gp_spoolman_id(&currentSpool.tag_data, &cur_sm_id);
int16_t cur_min_pt = 0, cur_max_pt = 0, cur_pre_t = 0, cur_min_bt = 0, cur_max_bt = 0;
opt_get_min_print_temp(&currentSpool.tag_data, &cur_min_pt);
opt_get_max_print_temp(&currentSpool.tag_data, &cur_max_pt);
opt_get_preheat_temp(&currentSpool.tag_data, &cur_pre_t);
opt_get_min_bed_temp(&currentSpool.tag_data, &cur_min_bt);
opt_get_max_bed_temp(&currentSpool.tag_data, &cur_max_bt);
xSemaphoreGive(tagMutex);

// Build a FRESH tag from scratch — avoids CBOR re-encoding overflow
// that happens when updating fields in an existing map
opt_init(&writeScratchTag_);
opt_error_t err = opt_format_empty_tag(&writeScratchTag_, 312, 32);
if (err != OPT_OK) {
Serial.printf("NFCManager: WRITE_ATOMIC format failed: %s\n", opt_error_str(err));
atomicWriteFields_.pending = false;
return false;
}

// 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.


// Single-pass NFC write — no sequential drops, no scan loop race
Serial.println("NFCManager: WRITE_ATOMIC writing to NFC...");
opt_nfc_hal_t* hal = connection_->getHal();
err = opt_write_to_nfc(&writeScratchTag_, hal);
if (err != OPT_OK) {
Serial.printf("NFCManager: WRITE_ATOMIC NFC write failed: %s\n", opt_error_str(err));
return false;
}

// Verify with retries (same pattern as formatNewSpool)
const int maxRetries = 3;
for (int retry = 0; retry < maxRetries; retry++) {
vTaskDelay(pdMS_TO_TICKS(50 * (retry + 1)));
err = opt_read_from_nfc(&writeScratchTag_, hal, 0, 78);
if (err == OPT_OK) {
err = opt_parse_ndef(&writeScratchTag_);
if (err == OPT_OK) break;
}
Serial.printf("NFCManager: WRITE_ATOMIC verify retry %d: %s\n", retry + 1, opt_error_str(err));
}

// Commit to shared state under mutex
if (xSemaphoreTake(tagMutex, pdMS_TO_TICKS(100)) != pdTRUE) {
Serial.println("NFCManager: WRITE_ATOMIC succeeded but commit lock failed");
return false;
}
currentSpool.tag_data = writeScratchTag_;
currentSpool.tag_data_valid = true;
currentSpool.blank_tag_present = false;
currentSpool.kind = TagKind::OpenPrintTag;
addToRecentSpools();
sendSpoolDetectedMessage();
xSemaphoreGive(tagMutex);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

Serial.println("NFCManager: WRITE_ATOMIC complete");
return true;
}

// For non-FORMAT writes: take mutex to validate + modify in-memory data, then release for NFC I/O
if (xSemaphoreTake(tagMutex, pdMS_TO_TICKS(100)) != pdTRUE) {
Serial.println("NFCManager: executeWrite - could not acquire tagMutex");
Expand Down Expand Up @@ -1795,10 +1930,13 @@ void NFCManager::sendSpoolUpdatedMessage(uint32_t request_id, NFCWriteType type,
msg.payload.spoolUpdated.success = success;
msg.payload.spoolUpdated.suppress_sync = 0; // Default: allow sync

// Get remaining weight from tag data
// Get remaining weight from tag data under mutex to prevent torn reads
float remaining_grams = 0;
if (currentSpool.tag_data_valid) {
opt_get_remaining_weight(&currentSpool.tag_data, &remaining_grams);
if (xSemaphoreTake(tagMutex, pdMS_TO_TICKS(50)) == pdTRUE) {
if (currentSpool.tag_data_valid) {
opt_get_remaining_weight(&currentSpool.tag_data, &remaining_grams);
}
xSemaphoreGive(tagMutex);
}
msg.payload.spoolUpdated.kg_remaining = remaining_grams / 1000.0f;

Expand Down
36 changes: 36 additions & 0 deletions src/NFCManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,48 @@
#include "TigerTagParser.h"
#include "opentag3d_lib.h"

// Sidecar for WRITE_ATOMIC: filled by HTTP handler, consumed by scan task.
// All fields are optional — only those with has_* = true are applied.
struct AtomicWriteFields {
bool has_material_type = false;
uint8_t material_type = 0;
bool has_color = false;
uint8_t color[4] = {0};
bool has_initial_weight = false;
float initial_weight_g = 0;
bool has_consumed_weight = false;
float consumed_weight = 0;
bool has_brand_name = false;
char brand_name[33] = {0};
bool has_spoolman_id = false;
int32_t spoolman_id = 0;
bool has_density = false;
float density = 0;
bool has_diameter = false;
float diameter_mm = 0;
bool has_material_name = false;
char material_name[33] = {0};
bool has_min_print_temp = false;
int16_t min_print_temp = 0;
bool has_max_print_temp = false;
int16_t max_print_temp = 0;
bool has_preheat_temp = false;
int16_t preheat_temp = 0;
bool has_min_bed_temp = false;
int16_t min_bed_temp = 0;
bool has_max_bed_temp = false;
int16_t max_bed_temp = 0;
volatile bool pending = false;
};

class NFCManager {
public:
static NFCManager& getInstance();
bool begin(); // Init hardware + queues
void startScanTask(); // Start FreeRTOS scan task
bool enqueueWrite(const NFCWriteRequest& req); // Queue a write request
bool enqueueRawWrite(const NFCWriteRequest& req, const uint8_t* data, size_t dataSize);
void setAtomicWriteFields(const AtomicWriteFields& fields) { atomicWriteFields_ = fields; }
bool writeSpoolmanDataToTag(int32_t spoolman_id, const char* expected_spool_id = nullptr);
bool isRequestCompleted(uint32_t request_id); // Check if request done
void requestCurrentSpool(); // Clear dedup to resend current spool
Expand Down Expand Up @@ -107,6 +142,7 @@ class NFCManager {
bool rawWritePending_ = false;
bool writeRawTag();
opt_tag_t writeScratchTag_; // Reused by scan task write path to avoid large stack frames
AtomicWriteFields atomicWriteFields_; // Sidecar for WRITE_ATOMIC (filled by HTTP, consumed by scan task)

// Write queue (FreeRTOS)
QueueHandle_t writeQueue = nullptr;
Expand Down
1 change: 1 addition & 0 deletions src/NFCWriteTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ enum class NFCWriteType : uint8_t {
SET_MAX_BED_TEMP, // Set maximum bed temperature (°C)
WRITE_TIGERTAG, // Write 40-byte TigerTag binary to NTAG pages 4-13
WRITE_OPENTAG3D, // Write NDEF-wrapped OpenTag3D payload to NTAG pages
WRITE_ATOMIC, // Atomic single-pass write: build complete CBOR map, write once
};

struct NFCWriteRequest {
Expand Down
Loading