diff --git a/CHANGELOG.md b/CHANGELOG.md index 4eedbed..4f21f2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,20 @@ # 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 + +--- + ## [1.5.5] - 2026-03-26 ### Added diff --git a/lib/PN5180/PN5180ISO14443.cpp b/lib/PN5180/PN5180ISO14443.cpp index 9979fe5..b227ef6 100644 --- a/lib/PN5180/PN5180ISO14443.cpp +++ b/lib/PN5180/PN5180ISO14443.cpp @@ -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); diff --git a/src/NFCManager.cpp b/src/NFCManager.cpp index 41e5ab6..174dc99 100644 --- a/src/NFCManager.cpp +++ b/src/NFCManager.cpp @@ -146,7 +146,7 @@ void NFCManager::startScanTask() { xTaskCreatePinnedToCore( scanTaskFunc, "NFCScanTask", - 6144, + 8192, this, 1, &scanTaskHandle, @@ -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); + } } } } @@ -1630,6 +1632,144 @@ 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; + } + + // Copy sidecar by value and clear pending immediately — prevents a + // concurrent HTTP call from overwriting fields mid-write + AtomicWriteFields f = atomicWriteFields_; + atomicWriteFields_.pending = false; + + // Validate and snapshot under mutex + if (xSemaphoreTake(tagMutex, pdMS_TO_TICKS(100)) != pdTRUE) { + Serial.println("NFCManager: WRITE_ATOMIC - could not acquire tagMutex"); + return false; + } + if (!currentSpool.tag_data_valid) { + xSemaphoreGive(tagMutex); + 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); + return false; + } + // Read defaults from the existing tag for any fields not provided + uint8_t cur_mat_type = 0; opt_get_material_type(¤tSpool.tag_data, &cur_mat_type); + uint8_t cur_color[4] = {255,255,255,255}; opt_get_primary_color(¤tSpool.tag_data, cur_color); + float cur_full_wt = 1000.0f; opt_get_actual_full_weight(¤tSpool.tag_data, &cur_full_wt); + float cur_consumed = 0.0f; opt_get_consumed_weight(¤tSpool.tag_data, &cur_consumed); + char cur_brand[33] = {0}; opt_get_brand_name(¤tSpool.tag_data, cur_brand, sizeof(cur_brand)); + float cur_density = 0.0f; opt_get_density(¤tSpool.tag_data, &cur_density); + float cur_diameter = 0.0f; opt_get_filament_diameter(¤tSpool.tag_data, &cur_diameter); + char cur_mat_name[33] = {0}; opt_get_material_name(¤tSpool.tag_data, cur_mat_name, sizeof(cur_mat_name)); + int32_t cur_sm_id = -1; opt_get_gp_spoolman_id(¤tSpool.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(¤tSpool.tag_data, &cur_min_pt); + opt_get_max_print_temp(¤tSpool.tag_data, &cur_max_pt); + opt_get_preheat_temp(¤tSpool.tag_data, &cur_pre_t); + opt_get_min_bed_temp(¤tSpool.tag_data, &cur_min_bt); + opt_get_max_bed_temp(¤tSpool.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 + + // 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 — re-read tag to confirm write succeeded + bool verified = false; + 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) { verified = true; break; } + } + Serial.printf("NFCManager: WRITE_ATOMIC verify retry %d: %s\n", retry + 1, opt_error_str(err)); + } + if (!verified) { + Serial.println("NFCManager: WRITE_ATOMIC verification failed — not committing"); + return false; + } + + // Commit verified data 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); + + 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"); @@ -1795,10 +1935,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(¤tSpool.tag_data, &remaining_grams); + if (xSemaphoreTake(tagMutex, pdMS_TO_TICKS(50)) == pdTRUE) { + if (currentSpool.tag_data_valid) { + opt_get_remaining_weight(¤tSpool.tag_data, &remaining_grams); + } + xSemaphoreGive(tagMutex); } msg.payload.spoolUpdated.kg_remaining = remaining_grams / 1000.0f; diff --git a/src/NFCManager.h b/src/NFCManager.h index b6a5656..5b2d91f 100644 --- a/src/NFCManager.h +++ b/src/NFCManager.h @@ -14,6 +14,40 @@ #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(); @@ -21,6 +55,7 @@ class NFCManager { 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 @@ -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; diff --git a/src/NFCWriteTypes.h b/src/NFCWriteTypes.h index b6b9b05..e17cd35 100644 --- a/src/NFCWriteTypes.h +++ b/src/NFCWriteTypes.h @@ -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 { diff --git a/src/WebServerManager.cpp b/src/WebServerManager.cpp index 704affe..99b7982 100644 --- a/src/WebServerManager.cpp +++ b/src/WebServerManager.cpp @@ -533,9 +533,13 @@ void WebServerManager::handleApiPostConfig() { return; } - Serial.println("WebServerManager: Config saved, rebooting in 3 seconds..."); + Serial.println("WebServerManager: Config saved, rebooting..."); _server.send(200, "application/json", "{\"success\":true}"); - delay(3000); + + // Suspend scan task to prevent new NFC writes, then wait for any + // in-progress write to finish before restarting (#27) + NFCManager::getInstance().pauseScanTask(); + delay(500); ESP.restart(); } @@ -596,7 +600,8 @@ void WebServerManager::handleApiUploadFirmwareComplete() { } else { _server.send(200, "application/json", "{\"success\":true}"); Serial.println("OTA: Rebooting..."); - delay(1000); + // Scan task already paused during upload; wait for any in-flight NFC I/O (#27) + delay(500); ESP.restart(); } } @@ -930,108 +935,100 @@ void WebServerManager::handleApiWriteTag() { float remaining_g = doc["remaining_g"] | 0.0f; int32_t spoolman_id = doc["spoolman_id"] | -1; - float consumed_g = initial_weight_g - remaining_g; - if (consumed_g < 0.0f) consumed_g = 0.0f; - - uint32_t base_id = millis(); - NFCWriteRequest req; - memset(&req, 0, sizeof(req)); - req.suppress_sync = 1; // batch write — suppress individual Spoolman syncs - strncpy(req.expected_spool_id, uid, sizeof(req.expected_spool_id) - 1); + // Build atomic write fields — all provided fields written in a single NFC pass + AtomicWriteFields fields; + memset(&fields, 0, sizeof(fields)); - // 1. Material type (only if explicitly provided) if (doc.containsKey("material_type")) { - req.request_id = base_id; - req.type = NFCWriteType::CHANGE_FILAMENT_TYPE; - req.data.new_material_type = mat_type; - NFCManager::getInstance().enqueueWrite(req); + fields.has_material_type = true; + fields.material_type = mat_type; } - // 2. Initial weight (only if explicitly provided) - if (doc.containsKey("initial_weight_g")) { - req.request_id = base_id + 1; - req.type = NFCWriteType::SET_INITIAL_WEIGHT; - req.data.consumed_weight = initial_weight_g; - NFCManager::getInstance().enqueueWrite(req); + if (hasValidColor) { + fields.has_color = true; + memcpy(fields.color, color, 4); } - // 3. Color (only if explicitly provided and valid hex) - if (hasValidColor) { - req.request_id = base_id + 2; - req.type = NFCWriteType::CHANGE_COLOR; - memcpy(req.data.new_color, color, 4); - NFCManager::getInstance().enqueueWrite(req); + if (doc.containsKey("initial_weight_g")) { + fields.has_initial_weight = true; + fields.initial_weight_g = initial_weight_g; } - // 4. Manufacturer (only if explicitly provided and non-empty) - if (doc.containsKey("manufacturer") && mfr[0] != '\0') { - req.request_id = base_id + 3; - req.type = NFCWriteType::SET_BRAND_NAME; - strncpy(req.data.brand_name, mfr, sizeof(req.data.brand_name) - 1); - NFCManager::getInstance().enqueueWrite(req); + if (doc.containsKey("remaining_g")) { + // Use provided initial_weight_g, or read from existing tag if not specified + float base_weight = initial_weight_g; + if (!doc.containsKey("initial_weight_g")) { + CurrentSpoolState state; + if (NFCManager::getInstance().getCurrentSpoolState(state) && state.tag_data_valid) { + opt_get_actual_full_weight(&state.tag_data, &base_weight); + } + } + float consumed_g = base_weight - remaining_g; + if (consumed_g < 0.0f) consumed_g = 0.0f; + fields.has_consumed_weight = true; + fields.consumed_weight = consumed_g; } - // 5. Consumed weight (only if both remaining_g and initial_weight_g provided) - if (doc.containsKey("remaining_g") && doc.containsKey("initial_weight_g")) { - req.request_id = base_id + 4; - req.type = NFCWriteType::SET_CONSUMED_WEIGHT; - req.data.consumed_weight = consumed_g; - NFCManager::getInstance().enqueueWrite(req); + if (doc.containsKey("manufacturer") && mfr[0] != '\0') { + fields.has_brand_name = true; + strncpy(fields.brand_name, mfr, sizeof(fields.brand_name) - 1); } - // 6. Spoolman ID (optional) if (spoolman_id > 0) { - req.request_id = base_id + 5; - req.type = NFCWriteType::WRITE_SPOOLMAN_ID; - req.data.spoolman_id = spoolman_id; - NFCManager::getInstance().enqueueWrite(req); + fields.has_spoolman_id = true; + fields.spoolman_id = spoolman_id; } - // 7. Density (optional) float density = doc["density"] | 0.0f; if (density > 0.0f) { - req.request_id = base_id + 6; - req.type = NFCWriteType::SET_DENSITY; - req.data.float_value = density; - NFCManager::getInstance().enqueueWrite(req); + fields.has_density = true; + fields.density = density; } - // 8. Diameter (optional) float diameter_mm = doc["diameter_mm"] | 0.0f; if (diameter_mm > 0.0f) { - req.request_id = base_id + 7; - req.type = NFCWriteType::SET_DIAMETER; - req.data.float_value = diameter_mm; - NFCManager::getInstance().enqueueWrite(req); + fields.has_diameter = true; + fields.diameter_mm = diameter_mm; } - // 9. Custom material name (optional) const char* mat_name = doc["material_name"] | ""; if (mat_name[0] != '\0') { - req.request_id = base_id + 8; - req.type = NFCWriteType::SET_MATERIAL_NAME; - strncpy(req.data.material_name, mat_name, sizeof(req.data.material_name) - 1); - NFCManager::getInstance().enqueueWrite(req); + fields.has_material_name = true; + strncpy(fields.material_name, mat_name, sizeof(fields.material_name) - 1); } - // 10-14. Temperatures (optional, 0 = not set) - struct { const char* key; NFCWriteType type; uint32_t id; } temps[] = { - { "min_print_temp", NFCWriteType::SET_MIN_PRINT_TEMP, base_id + 9 }, - { "max_print_temp", NFCWriteType::SET_MAX_PRINT_TEMP, base_id + 10 }, - { "preheat_temp", NFCWriteType::SET_PREHEAT_TEMP, base_id + 11 }, - { "min_bed_temp", NFCWriteType::SET_MIN_BED_TEMP, base_id + 12 }, - { "max_bed_temp", NFCWriteType::SET_MAX_BED_TEMP, base_id + 13 }, + struct { const char* key; bool AtomicWriteFields::*has; int16_t AtomicWriteFields::*val; } temps[] = { + { "min_print_temp", &AtomicWriteFields::has_min_print_temp, &AtomicWriteFields::min_print_temp }, + { "max_print_temp", &AtomicWriteFields::has_max_print_temp, &AtomicWriteFields::max_print_temp }, + { "preheat_temp", &AtomicWriteFields::has_preheat_temp, &AtomicWriteFields::preheat_temp }, + { "min_bed_temp", &AtomicWriteFields::has_min_bed_temp, &AtomicWriteFields::min_bed_temp }, + { "max_bed_temp", &AtomicWriteFields::has_max_bed_temp, &AtomicWriteFields::max_bed_temp }, }; for (const auto& entry : temps) { int v = doc[entry.key] | 0; if (v != 0) { - req.request_id = entry.id; - req.type = entry.type; - req.data.temp_celsius = static_cast(v); - NFCManager::getInstance().enqueueWrite(req); + fields.*entry.has = true; + fields.*entry.val = static_cast(v); } } + fields.pending = true; + NFCManager::getInstance().setAtomicWriteFields(fields); + + // Enqueue single atomic write request + NFCWriteRequest req; + memset(&req, 0, sizeof(req)); + req.request_id = millis(); + req.type = NFCWriteType::WRITE_ATOMIC; + req.suppress_sync = 0; + strncpy(req.expected_spool_id, uid, sizeof(req.expected_spool_id) - 1); + + if (!NFCManager::getInstance().enqueueWrite(req)) { + NFCManager::getInstance().setAtomicWriteFields(AtomicWriteFields{}); // clear stale sidecar + sendError(503, "Write queue full"); + return; + } + _server.send(200, "application/json", "{\"success\":true}"); }