feat: OpenSpool tag format support (#93)#95
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds OpenSpool support end-to-end: JSON parser and data type, TagKind and TFT/display tag type, NFC detection and write path (NDEF application/json), ApplicationManager dispatch, web writer UI and API, embedded logo asset, and navigation updates across embedded pages. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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: 5
🤖 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 1168-1177: The manual strcasecmp chain that sets s.material_type
from os.material should be replaced with the shared string-to-material mapper
used elsewhere in this file (call that existing mapper to convert os.material to
the enum) so you no longer silently default unknown OpenSpool names to PLA;
remove the if/else block around s.material_type, call the common mapper with
os.material to assign s.material_type, and ensure the mapper’s fallback for
unsupported names yields an explicit "unknown/none" value (e.g., 0 or
OPT_MATERIAL_TYPE_UNKNOWN) rather than treating them as PLA.
In `@src/OpenSpoolLogo.h`:
- Around line 4-7: The embedded asset bytes start with JPEG/JFIF but are
named/commented as PNG (symbol OPENSPOOL_LOGO_PNG and the comment mentioning GET
/img/openspool.png); fix this by either re-encoding the binary to a true PNG and
replacing the byte blob while keeping the names, or by renaming the asset and
route to JPEG: rename OPENSPOOL_LOGO_PNG to OPENSPOOL_LOGO_JPG, update the
comment and the GET route from /img/openspool.png to /img/openspool.jpg, and
ensure the request handler serves it with Content-Type image/jpeg (and update
any caches or tooling references accordingly).
In `@src/OpenSpoolParser.cpp`:
- Line 21: Normalize color_hex by stripping any leading '#' before copying into
out.color_hex: read the value from doc["color_hex"] (as currently done), remove
a leading '#' if present, then strncpy the normalized string into out.color_hex
ensuring you pass sizeof(out.color_hex)-1 and null-terminate; reference the
existing usage points strncpy(...) and out.color_hex and note
TFTManager::hexToRgb already strips '#', so perform the normalization here for
cleanliness.
- Line 11: The StaticJsonDocument allocated as StaticJsonDocument<256> in
OpenSpoolParser.cpp is too small for some payloads; replace and resize it to a
larger capacity (e.g., StaticJsonDocument<384> or StaticJsonDocument<512>) to
allow for longer brand names and ArduinoJson DOM overhead, updating the
StaticJsonDocument<256> declaration accordingly (search for the
StaticJsonDocument<256> symbol in OpenSpoolParser.cpp and change the template
argument to 384 or 512).
In `@src/WebServerManager.cpp`:
- Line 25: tagKindToString and handleApiStatus are not handling OpenSpool:
update tagKindToString to return a proper string for TagKind::OpenSpoolTag
(e.g., "OpenSpool") instead of falling through to "Unsupported", and extend
handleApiStatus to add an OpenSpool branch that recognizes OpenSpoolTag, parses
the OpenSpool-specific fields from the scan result (use the OpenSpool-related
types/fields introduced with OpenSpoolLogo.h), and populates the /api/status
response with those parsed fields so OpenSpool scans are reported instead of
"unsupported". Ensure to reference TagKind::OpenSpoolTag, tagKindToString(), and
handleApiStatus() when making the changes.
🪄 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: daf5c985-0555-4e69-b729-8dbe52d8a10f
📒 Files selected for processing (13)
src/ApplicationManager.cppsrc/DisplayI.hsrc/LandingHTML.hsrc/NFCManager.cppsrc/NFCManager.hsrc/NFCTypes.hsrc/OpenSpoolLogo.hsrc/OpenSpoolParser.cppsrc/OpenSpoolParser.hsrc/TFTManager.cppsrc/TFTManager.hsrc/WebServerManager.cppsrc/WebServerManager.h
| // Material type lookup | ||
| s.material_type = 0; | ||
| if (strcasecmp(os.material, "PLA") == 0) s.material_type = OPT_MATERIAL_TYPE_PLA; | ||
| else if (strcasecmp(os.material, "PETG") == 0) s.material_type = OPT_MATERIAL_TYPE_PETG; | ||
| else if (strcasecmp(os.material, "ABS") == 0) s.material_type = OPT_MATERIAL_TYPE_ABS; | ||
| else if (strcasecmp(os.material, "ASA") == 0) s.material_type = OPT_MATERIAL_TYPE_ASA; | ||
| else if (strcasecmp(os.material, "TPU") == 0) s.material_type = OPT_MATERIAL_TYPE_TPU; | ||
| else if (strcasecmp(os.material, "PA") == 0 || strcasecmp(os.material, "Nylon") == 0) s.material_type = OPT_MATERIAL_TYPE_PA6; | ||
| else if (strcasecmp(os.material, "PC") == 0) s.material_type = OPT_MATERIAL_TYPE_PC; | ||
|
|
There was a problem hiding this comment.
Don't silently map unsupported OpenSpool materials to PLA.
This branch recognizes far fewer material names than the TigerTag/OpenTag3D paths. Anything outside this list leaves material_type at its default value, so valid OpenSpool materials can be serialized downstream as PLA. This file already uses a shared string-to-material mapper later on; reusing that here would keep the formats aligned and avoid drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NFCManager.cpp` around lines 1168 - 1177, The manual strcasecmp chain
that sets s.material_type from os.material should be replaced with the shared
string-to-material mapper used elsewhere in this file (call that existing mapper
to convert os.material to the enum) so you no longer silently default unknown
OpenSpool names to PLA; remove the if/else block around s.material_type, call
the common mapper with os.material to assign s.material_type, and ensure the
mapper’s fallback for unsupported names yields an explicit "unknown/none" value
(e.g., 0 or OPT_MATERIAL_TYPE_UNKNOWN) rather than treating them as PLA.
| // OpenSpool logo PNG served at GET /img/openspool.png | ||
|
|
||
| static const uint8_t OPENSPOOL_LOGO_PNG[] PROGMEM = { | ||
| 0xff, 0xd8, 0xff, 0xe0, 0x00, 0x10, 0x4a, 0x46, 0x49, 0x46, 0x00, 0x01, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rename or re-encode this asset so the file type matches.
The blob starts with JPEG/JFIF bytes, but the comment and symbol still call it PNG. The handler currently compensates with image/jpeg, but the mismatch is brittle and easy to propagate into future route, cache, or tooling changes. Either convert the asset to actual PNG or rename the route/identifiers to .jpg.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenSpoolLogo.h` around lines 4 - 7, The embedded asset bytes start with
JPEG/JFIF but are named/commented as PNG (symbol OPENSPOOL_LOGO_PNG and the
comment mentioning GET /img/openspool.png); fix this by either re-encoding the
binary to a true PNG and replacing the byte blob while keeping the names, or by
renaming the asset and route to JPEG: rename OPENSPOOL_LOGO_PNG to
OPENSPOOL_LOGO_JPG, update the comment and the GET route from /img/openspool.png
to /img/openspool.jpg, and ensure the request handler serves it with
Content-Type image/jpeg (and update any caches or tooling references
accordingly).
|
|
||
| if (!json || len == 0) return false; | ||
|
|
||
| StaticJsonDocument<256> doc; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider increasing JSON document size for robustness.
StaticJsonDocument<256> may be tight for payloads with longer brand names. The example JSON is ~130 bytes raw, and ArduinoJson's DOM overhead can approach 2x. A 64-char brand name alone could push limits.
Consider increasing to StaticJsonDocument<384> or StaticJsonDocument<512> for safety margin.
♻️ Proposed fix
- StaticJsonDocument<256> doc;
+ StaticJsonDocument<384> doc;📝 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.
| StaticJsonDocument<256> doc; | |
| StaticJsonDocument<384> doc; |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 11-11: variable 'doc' is not initialized
(cppcoreguidelines-init-variables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenSpoolParser.cpp` at line 11, The StaticJsonDocument allocated as
StaticJsonDocument<256> in OpenSpoolParser.cpp is too small for some payloads;
replace and resize it to a larger capacity (e.g., StaticJsonDocument<384> or
StaticJsonDocument<512>) to allow for longer brand names and ArduinoJson DOM
overhead, updating the StaticJsonDocument<256> declaration accordingly (search
for the StaticJsonDocument<256> symbol in OpenSpoolParser.cpp and change the
template argument to 384 or 512).
| strncpy(out.protocol, protocol, sizeof(out.protocol) - 1); | ||
| strncpy(out.version, doc["version"] | "1.0", sizeof(out.version) - 1); | ||
| strncpy(out.material, doc["type"] | "", sizeof(out.material) - 1); | ||
| strncpy(out.color_hex, doc["color_hex"] | "", sizeof(out.color_hex) - 1); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider stripping leading # from color_hex for robustness.
The spec indicates color_hex should not include #, but external writers might include it. Downstream code (e.g., TFTManager::hexToRgb) already handles stripping #, but storing it normalized here would be cleaner.
♻️ Optional normalization
- strncpy(out.color_hex, doc["color_hex"] | "", sizeof(out.color_hex) - 1);
+ const char* rawHex = doc["color_hex"] | "";
+ if (rawHex[0] == '#') rawHex++; // strip leading #
+ strncpy(out.color_hex, rawHex, sizeof(out.color_hex) - 1);📝 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.
| strncpy(out.color_hex, doc["color_hex"] | "", sizeof(out.color_hex) - 1); | |
| const char* rawHex = doc["color_hex"] | ""; | |
| if (rawHex[0] == '#') rawHex++; // strip leading # | |
| strncpy(out.color_hex, rawHex, sizeof(out.color_hex) - 1); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenSpoolParser.cpp` at line 21, Normalize color_hex by stripping any
leading '#' before copying into out.color_hex: read the value from
doc["color_hex"] (as currently done), remove a leading '#' if present, then
strncpy the normalized string into out.color_hex ensuring you pass
sizeof(out.color_hex)-1 and null-terminate; reference the existing usage points
strncpy(...) and out.color_hex and note TFTManager::hexToRgb already strips '#',
so perform the normalization here for cleanliness.
| #include "OpenPrintTagLogo.h" | ||
| #include "TigerTagLogo.h" | ||
| #include "OpenTag3DLogo.h" | ||
| #include "OpenSpoolLogo.h" |
There was a problem hiding this comment.
Complete the /api/status plumbing for OpenSpool.
This file now adds OpenSpool-specific assets, but tagKindToString() still falls through to "Unsupported" for TagKind::OpenSpoolTag, and handleApiStatus() still has no OpenSpool branch. An OpenSpool scan will therefore come back from /api/status as unsupported with no parsed fields.
Also applies to: 106-106, 237-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WebServerManager.cpp` at line 25, tagKindToString and handleApiStatus are
not handling OpenSpool: update tagKindToString to return a proper string for
TagKind::OpenSpoolTag (e.g., "OpenSpool") instead of falling through to
"Unsupported", and extend handleApiStatus to add an OpenSpool branch that
recognizes OpenSpoolTag, parses the OpenSpool-specific fields from the scan
result (use the OpenSpool-related types/fields introduced with OpenSpoolLogo.h),
and populates the /api/status response with those parsed fields so OpenSpool
scans are reported instead of "unsupported". Ensure to reference
TagKind::OpenSpoolTag, tagKindToString(), and handleApiStatus() when making the
changes.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/NFCManager.cpp (1)
600-605:⚠️ Potential issue | 🟡 Minor
lastOpenSpoolValid_not cleared when tag is removed.When a tag is removed (lines 600-604),
lastTigerTagValid_andlastOpenTag3DValid_are cleared, butlastOpenSpoolValid_is not. This could cause stale OpenSpool data to persist.Proposed fix
lastSeenValid = false; lastTigerTagValid_ = false; lastOpenTag3DValid_ = false; + lastOpenSpoolValid_ = false; // Clear suppression if tag removed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NFCManager.cpp` around lines 600 - 605, When handling tag removal in the block that sets currentSpool.present = false (the same area that clears lastSeenValid, lastTigerTagValid_, and lastOpenTag3DValid_), also clear lastOpenSpoolValid_ so stale OpenSpool state is not retained; update the tag-removal sequence in NFCManager.cpp to set lastOpenSpoolValid_ = false alongside the other validity flags referencing currentSpool/lastTigerTagValid_/lastOpenTag3DValid_.src/LandingHTML.h (1)
17-28:⚠️ Potential issue | 🟡 MinorNavigation bar is missing the OpenSpool link.
The nav bar includes links to OpenPrintTag, TigerTag, and OpenTag3D writers, but OpenSpool is missing. The OpenSpoolWriterHTML.h shows OpenSpool in its nav (line 24), creating an inconsistency where users can navigate away from the OpenSpool writer but cannot navigate back to it from the landing page nav.
Proposed fix
<a href="/writer/tigertag">TigerTag</a> <a href="/writer/opentag3d">OpenTag3D</a> + <a href="/writer/openspool">OpenSpool</a> <a href="/register/uid">NFC+</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LandingHTML.h` around lines 17 - 28, Add the missing OpenSpool nav link to the landing page nav so it matches the writer pages: insert an anchor element similar to the existing writer links (e.g., <a href="/writer/openspool">OpenSpool</a>) into the <nav> block alongside OpenPrintTag, TigerTag and OpenTag3D in LandingHTML.h, using the same classes/formatting as the other writer links to maintain consistency with OpenSpoolWriterHTML.h.
♻️ Duplicate comments (3)
src/NFCManager.cpp (1)
1168-1177:⚠️ Potential issue | 🟠 MajorLimited material type mapping silently defaults unknowns to PLA.
The material type lookup only handles 7 materials (PLA, PETG, ABS, ASA, TPU, PA/Nylon, PC), while the OpenPrintTag library supports 42 material types. Any other material (e.g., PCTG, HIPS, PVB from the writer UI dropdown, or PP, PVA, PEEK, etc.) will silently default to
OPT_MATERIAL_TYPE_PLA(0). This misidentifies filament types downstream.Consider using the shared
materialTypeFromString()helper in ConversionUtils for consistency with TigerTag and OpenTag3D paths, or extend this mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NFCManager.cpp` around lines 1168 - 1177, The material mapping in NFCManager.cpp currently hardcodes seven materials and defaults unknowns to PLA (s.material_type = OPT_MATERIAL_TYPE_PLA); replace this ad-hoc lookup with the shared helper to ensure all 42 types are handled consistently—call ConversionUtils::materialTypeFromString(os.material) (or the equivalent materialTypeFromString function) to derive s.material_type, and remove or fallback the existing strcasecmp chain so unknown strings no longer silently map to PLA. Ensure you include the proper header for ConversionUtils and adjust any types if the helper returns a different enum/type.src/WebServerManager.cpp (2)
63-73:⚠️ Potential issue | 🟠 MajorMissing
OpenSpoolTagcase intagKindToString.The
tagKindToStringfunction does not handleTagKind::OpenSpoolTag, causing it to fall through to"Unsupported". This affects/api/statusresponses when an OpenSpool tag is detected.Proposed fix
static const char* tagKindToString(TagKind kind) { switch (kind) { case TagKind::OpenPrintTag: return "OpenPrintTag"; case TagKind::GenericUidTag: return "GenericUidTag"; case TagKind::TigerTag: return "TigerTag"; case TagKind::OpenTag3D: return "OpenTag3D"; case TagKind::BambuTag: return "BambuTag"; case TagKind::BlankTag: return "BlankTag"; + case TagKind::OpenSpoolTag: return "OpenSpool"; default: return "Unsupported"; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 63 - 73, The tagKindToString function is missing handling for TagKind::OpenSpoolTag so that OpenSpool tags fall through to "Unsupported"; update the switch in tagKindToString to add a case for TagKind::OpenSpoolTag that returns "OpenSpoolTag" (ensure the case label and returned string match the naming used for other TagKind cases) so /api/status reports the correct tag string.
1016-1173:⚠️ Potential issue | 🟠 Major
handleApiStatusmissing OpenSpool branch.The
/api/statushandler has specific branches for TigerTag, OpenTag3D, GenericUidTag, and OpenPrintTag, but no branch forOpenSpoolTag. When an OpenSpool tag is detected, the response will not include the parsed OpenSpool fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 1016 - 1173, handleApiStatus lacks handling for TagKind::OpenSpoolTag so when state.kind == TagKind::OpenSpoolTag the response omits parsed OpenSpool fields; add an else-if branch checking state.kind == TagKind::OpenSpoolTag (alongside the existing TagKind::TigerTag, TagKind::OpenTag3D, etc.) that calls NFCManager::getInstance().getLastOpenSpoolData(openSpoolStruct) (or the appropriate NFCManager getter), verifies the returned data is valid, creates a nested JSON object (e.g. doc.createNestedObject("openspool")), and populates it with the OpenSpool fields you expose (material, manufacturer, color_hex, remaining_g, diameter_mm, temps/dry info if present) mirroring the style and null/zero checks used in the other tag branches in WebServerManager::handleApiStatus.
🤖 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 420-464: Extract the nested OpenSpool NDEF detection block into a
helper function (e.g., bool checkOpenSpoolNdef(const uint8_t* pageData, size_t
bytesRead, size_t ndefStart, size_t headerSize, size_t typeLen, bool sr, /*out*/
OpenSpoolType& openSpoolData)) that encapsulates the MIME check for
"application/json", payload length computation (short vs long record), page
boundary handling using connection_->readISO14443Pages, buffer bounds checks,
and the call to parseOpenSpool; then replace the large nested block in
NFCManager.cpp with a single call that sets isOpenSpool =
checkOpenSpoolNdef(pageData, bytesRead, ndefStart, headerSize, typeLen, sr,
openSpoolData). Ensure the helper uses the same symbols (pageData, ndefStart,
headerSize, typeLen, sr, bytesRead, connection_->readISO14443Pages,
parseOpenSpool, openSpoolData) and preserves existing size limits and fallback
logic.
- Around line 1158-1162: The code uses memset(&s, 0, sizeof(s)) to clear the
AppMessage::payload.spoolDetected struct inside
NFCManager::sendOpenSpoolMessage; replace that with proper value-initialization
(e.g., assign an empty initializer or default-constructed instance to s) so
floating-point members like kg_remaining, density, diameter, initial_weight_g
are set to 0.0 safely and portably; update the function to use s = {} or s =
SpoolDetected{} (matching the actual struct/type name) instead of memset.
In `@src/OpenSpoolWriterHTML.h`:
- Around line 110-126: The Spoolman callback (renderSpoolmanPicker ->
callback(spool)) currently writes f.material directly into the `#type` select
which can create an invalid selection if the value doesn't match any option;
update the handler to check whether document.getElementById('type') already
contains an option with value === f.material and if not either create and append
a new <option> with that value (and a sensible label) before setting .value, or
fall back to a safe default/empty value and mark the field for manual review;
make this change in the callback that reads f.material so the select always ends
up with a valid option.
In `@src/WebServerManager.cpp`:
- Around line 245-248: The response in WebServerManager::handleOpenSpoolLogo is
sending OPENSPOOL_LOGO_PNG via _server.send_P but sets the Content-Type to
"image/jpeg"; update the MIME type to "image/png" so the PNG data
(OPENSPOOL_LOGO_PNG and OPENSPOOL_LOGO_PNG_SIZE) is served with the correct
Content-Type header (modify the _server.send_P call to use "image/png").
- Around line 1558-1563: The JSON is built by snprintf into jsonPayload using
raw user strings (protocol, version, type, color_hex, brand, min_temp,
max_temp), which allows injection; fix by not interpolating unsanitized values
directly—either validate/whitelist each input (e.g., only alphanumerics and a
few safe punctuation) or replace this snprintf construction with a proper JSON
builder (e.g., ArduinoJson::JsonDocument + serialize) or at minimum escape JSON
string characters before formatting (create an escapeJsonString helper and apply
it to protocol/version/type/color_hex/brand/min_temp/max_temp) so the produced
JSON is always well-formed and safe.
---
Outside diff comments:
In `@src/LandingHTML.h`:
- Around line 17-28: Add the missing OpenSpool nav link to the landing page nav
so it matches the writer pages: insert an anchor element similar to the existing
writer links (e.g., <a href="/writer/openspool">OpenSpool</a>) into the <nav>
block alongside OpenPrintTag, TigerTag and OpenTag3D in LandingHTML.h, using the
same classes/formatting as the other writer links to maintain consistency with
OpenSpoolWriterHTML.h.
In `@src/NFCManager.cpp`:
- Around line 600-605: When handling tag removal in the block that sets
currentSpool.present = false (the same area that clears lastSeenValid,
lastTigerTagValid_, and lastOpenTag3DValid_), also clear lastOpenSpoolValid_ so
stale OpenSpool state is not retained; update the tag-removal sequence in
NFCManager.cpp to set lastOpenSpoolValid_ = false alongside the other validity
flags referencing currentSpool/lastTigerTagValid_/lastOpenTag3DValid_.
---
Duplicate comments:
In `@src/NFCManager.cpp`:
- Around line 1168-1177: The material mapping in NFCManager.cpp currently
hardcodes seven materials and defaults unknowns to PLA (s.material_type =
OPT_MATERIAL_TYPE_PLA); replace this ad-hoc lookup with the shared helper to
ensure all 42 types are handled consistently—call
ConversionUtils::materialTypeFromString(os.material) (or the equivalent
materialTypeFromString function) to derive s.material_type, and remove or
fallback the existing strcasecmp chain so unknown strings no longer silently map
to PLA. Ensure you include the proper header for ConversionUtils and adjust any
types if the helper returns a different enum/type.
In `@src/WebServerManager.cpp`:
- Around line 63-73: The tagKindToString function is missing handling for
TagKind::OpenSpoolTag so that OpenSpool tags fall through to "Unsupported";
update the switch in tagKindToString to add a case for TagKind::OpenSpoolTag
that returns "OpenSpoolTag" (ensure the case label and returned string match the
naming used for other TagKind cases) so /api/status reports the correct tag
string.
- Around line 1016-1173: handleApiStatus lacks handling for
TagKind::OpenSpoolTag so when state.kind == TagKind::OpenSpoolTag the response
omits parsed OpenSpool fields; add an else-if branch checking state.kind ==
TagKind::OpenSpoolTag (alongside the existing TagKind::TigerTag,
TagKind::OpenTag3D, etc.) that calls
NFCManager::getInstance().getLastOpenSpoolData(openSpoolStruct) (or the
appropriate NFCManager getter), verifies the returned data is valid, creates a
nested JSON object (e.g. doc.createNestedObject("openspool")), and populates it
with the OpenSpool fields you expose (material, manufacturer, color_hex,
remaining_g, diameter_mm, temps/dry info if present) mirroring the style and
null/zero checks used in the other tag branches in
WebServerManager::handleApiStatus.
🪄 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: 5ed9692b-5682-4c90-88e3-43575511e22d
📒 Files selected for processing (6)
src/LandingHTML.hsrc/NFCManager.cppsrc/NFCWriteTypes.hsrc/OpenSpoolWriterHTML.hsrc/WebServerManager.cppsrc/WebServerManager.h
|
|
||
| // Check for OpenSpool (application/json + "protocol":"openspool") | ||
| if (!isOpenTag3D) { | ||
| const char* jsonMime = "application/json"; | ||
| size_t jsonMimeLen = strlen(jsonMime); | ||
| if (typeLen == jsonMimeLen && | ||
| memcmp(pageData + ndefStart + headerSize, jsonMime, jsonMimeLen) == 0) { | ||
| uint32_t osPayloadLen = 0; | ||
| if (sr) { | ||
| osPayloadLen = pageData[ndefStart + 2]; | ||
| } else { | ||
| osPayloadLen = ((uint32_t)pageData[ndefStart + 2] << 24) | | ||
| ((uint32_t)pageData[ndefStart + 3] << 16) | | ||
| ((uint32_t)pageData[ndefStart + 4] << 8) | | ||
| pageData[ndefStart + 5]; | ||
| } | ||
| uint16_t osOffset = ndefStart + headerSize + typeLen; | ||
| uint8_t osPayload[256] = {0}; | ||
| uint16_t osBytes = 0; | ||
|
|
||
| if (osOffset + osPayloadLen <= bytesRead) { | ||
| osBytes = (uint16_t)osPayloadLen; | ||
| if (osBytes > sizeof(osPayload)) osBytes = sizeof(osPayload); | ||
| memcpy(osPayload, pageData + osOffset, osBytes); | ||
| } else { | ||
| uint8_t osStartPage = 4 + (osOffset / 4); | ||
| uint16_t osPagesNeeded = (uint16_t)((osPayloadLen + 3) / 4) + 1; | ||
| if (osPagesNeeded > 50) osPagesNeeded = 50; | ||
| uint8_t osExtData[256] = {0}; | ||
| uint16_t osExtRead = connection_->readISO14443Pages( | ||
| osStartPage, (uint8_t)osPagesNeeded, osExtData, sizeof(osExtData)); | ||
| uint16_t osOffInPage = osOffset % 4; | ||
| if (osExtRead > osOffInPage) { | ||
| osBytes = osExtRead - osOffInPage; | ||
| if (osBytes > osPayloadLen) osBytes = (uint16_t)osPayloadLen; | ||
| if (osBytes > sizeof(osPayload)) osBytes = sizeof(osPayload); | ||
| memcpy(osPayload, osExtData + osOffInPage, osBytes); | ||
| } | ||
| } | ||
|
|
||
| if (osBytes > 0 && parseOpenSpool(osPayload, osBytes, openSpoolData)) { | ||
| isOpenSpool = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deep nesting increases cognitive complexity.
The OpenSpool detection logic adds another level of nesting (up to 13 levels per static analysis). Consider extracting the OpenSpool NDEF check into a helper function to match the structure of tigerTagCheckMagic and improve maintainability.
🧰 Tools
🪛 Clang (14.0.6)
[note] 422-422: +10, including nesting penalty of 9, nesting level increased to 10
(clang)
[note] 425-425: +11, including nesting penalty of 10, nesting level increased to 11
(clang)
[note] 428-428: +12, including nesting penalty of 11, nesting level increased to 12
(clang)
[note] 430-430: +1, nesting level increased to 12
(clang)
[note] 440-440: +12, including nesting penalty of 11, nesting level increased to 12
(clang)
[note] 444-444: +1, nesting level increased to 12
(clang)
[note] 452-452: +13, including nesting penalty of 12, nesting level increased to 13
(clang)
[note] 460-460: +12, including nesting penalty of 11, nesting level increased to 12
(clang)
[warning] 424-424: variable 'jsonMimeLen' is not initialized
(cppcoreguidelines-init-variables)
[warning] 427-427: variable 'osPayloadLen' is not initialized
(cppcoreguidelines-init-variables)
[warning] 428-428: if with identical then and else branches
(bugprone-branch-clone)
[note] 430-430: else branch starts here
(clang)
[warning] 436-436: variable 'osOffset' is not initialized
(cppcoreguidelines-init-variables)
[warning] 438-438: variable 'osBytes' is not initialized
(cppcoreguidelines-init-variables)
[warning] 445-445: variable 'osStartPage' is not initialized
(cppcoreguidelines-init-variables)
[warning] 446-446: variable 'osPagesNeeded' is not initialized
(cppcoreguidelines-init-variables)
[warning] 449-449: variable 'osExtRead' is not initialized
(cppcoreguidelines-init-variables)
[warning] 451-451: variable 'osOffInPage' is not initialized
(cppcoreguidelines-init-variables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NFCManager.cpp` around lines 420 - 464, Extract the nested OpenSpool NDEF
detection block into a helper function (e.g., bool checkOpenSpoolNdef(const
uint8_t* pageData, size_t bytesRead, size_t ndefStart, size_t headerSize, size_t
typeLen, bool sr, /*out*/ OpenSpoolType& openSpoolData)) that encapsulates the
MIME check for "application/json", payload length computation (short vs long
record), page boundary handling using connection_->readISO14443Pages, buffer
bounds checks, and the call to parseOpenSpool; then replace the large nested
block in NFCManager.cpp with a single call that sets isOpenSpool =
checkOpenSpoolNdef(pageData, bytesRead, ndefStart, headerSize, typeLen, sr,
openSpoolData). Ensure the helper uses the same symbols (pageData, ndefStart,
headerSize, typeLen, sr, bytesRead, connection_->readISO14443Pages,
parseOpenSpool, openSpoolData) and preserves existing size limits and fallback
logic.
| void NFCManager::sendOpenSpoolMessage(const char* uid, const OpenSpoolData& os) { | ||
| AppMessage msg; | ||
| msg.type = AppMessageType::SPOOL_DETECTED; | ||
| auto& s = msg.payload.spoolDetected; | ||
| memset(&s, 0, sizeof(s)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use member initializer instead of memset on struct containing floats.
memset on a struct containing floating-point members (kg_remaining, density, diameter, initial_weight_g) may produce invalid bit patterns on some platforms. While typically safe on ESP32/IEEE 754, explicit zero-initialization is cleaner.
Proposed fix
void NFCManager::sendOpenSpoolMessage(const char* uid, const OpenSpoolData& os) {
AppMessage msg;
msg.type = AppMessageType::SPOOL_DETECTED;
- auto& s = msg.payload.spoolDetected;
- memset(&s, 0, sizeof(s));
+ msg.payload.spoolDetected = {}; // Value-initialize all members
+ auto& s = msg.payload.spoolDetected;📝 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.
| void NFCManager::sendOpenSpoolMessage(const char* uid, const OpenSpoolData& os) { | |
| AppMessage msg; | |
| msg.type = AppMessageType::SPOOL_DETECTED; | |
| auto& s = msg.payload.spoolDetected; | |
| memset(&s, 0, sizeof(s)); | |
| void NFCManager::sendOpenSpoolMessage(const char* uid, const OpenSpoolData& os) { | |
| AppMessage msg; | |
| msg.type = AppMessageType::SPOOL_DETECTED; | |
| msg.payload.spoolDetected = {}; // Value-initialize all members | |
| auto& s = msg.payload.spoolDetected; |
🧰 Tools
🪛 Clang (14.0.6)
[note] 1158-1158: the definition seen here
(clang)
[warning] 1158-1158: method 'sendOpenSpoolMessage' can be made static
(readability-convert-member-functions-to-static)
[warning] 1158-1158: parameter name 'os' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 1161-1161: variable name 's' is too short, expected at least 3 characters
(readability-identifier-length)
🪛 Cppcheck (2.20.0)
[portability] 1162-1162: Using memset() on struct which contains a floating point number.
(memsetClassFloat)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NFCManager.cpp` around lines 1158 - 1162, The code uses memset(&s, 0,
sizeof(s)) to clear the AppMessage::payload.spoolDetected struct inside
NFCManager::sendOpenSpoolMessage; replace that with proper value-initialization
(e.g., assign an empty initializer or default-constructed instance to s) so
floating-point members like kg_remaining, density, diameter, initial_weight_g
are set to 0.0 safely and portably; update the function to use s = {} or s =
SpoolDetected{} (matching the actual struct/type name) instead of memset.
| if (typeof renderSpoolmanPicker === 'function') { | ||
| renderSpoolmanPicker('spoolmanPicker', function(spool) { | ||
| if (spool.filament) { | ||
| var f = spool.filament; | ||
| if (f.vendor && f.vendor.name) document.getElementById('brand').value = f.vendor.name; | ||
| if (f.material) document.getElementById('type').value = f.material; | ||
| if (f.color_hex) { | ||
| document.getElementById('color_hex').value = '#' + f.color_hex; | ||
| document.getElementById('colorPreview').style.background = '#' + f.color_hex; | ||
| } | ||
| if (f.settings_extruder_temp) { | ||
| document.getElementById('min_temp').value = f.settings_extruder_temp; | ||
| document.getElementById('max_temp').value = f.settings_extruder_temp; | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Spoolman picker callback may set invalid select option value.
When f.material from Spoolman doesn't match any <option> value in the #type select (e.g., "WOOD", "CARBON", or other materials not in the limited dropdown), the select will show no valid selection but the form may still submit. Consider adding the material as a custom option or validating the material type.
Proposed enhancement
if (f.material) document.getElementById('type').value = f.material;
+ // If material not in dropdown, add it as an option
+ var typeSelect = document.getElementById('type');
+ if (typeSelect.value !== f.material) {
+ var opt = document.createElement('option');
+ opt.value = f.material;
+ opt.textContent = f.material;
+ typeSelect.appendChild(opt);
+ typeSelect.value = f.material;
+ }🧰 Tools
🪛 Clang (14.0.6)
[error] 110-110: expected identifier or '('
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenSpoolWriterHTML.h` around lines 110 - 126, The Spoolman callback
(renderSpoolmanPicker -> callback(spool)) currently writes f.material directly
into the `#type` select which can create an invalid selection if the value doesn't
match any option; update the handler to check whether
document.getElementById('type') already contains an option with value ===
f.material and if not either create and append a new <option> with that value
(and a sensible label) before setting .value, or fall back to a safe
default/empty value and mark the field for manual review; make this change in
the callback that reads f.material so the select always ends up with a valid
option.
| void WebServerManager::handleOpenSpoolLogo() { | ||
| _server.sendHeader("Cache-Control", "public, max-age=86400"); | ||
| _server.send_P(200, "image/jpeg", reinterpret_cast<const char*>(OPENSPOOL_LOGO_PNG), OPENSPOOL_LOGO_PNG_SIZE); | ||
| } |
There was a problem hiding this comment.
Content-Type mismatch: serving PNG as JPEG.
The constant OPENSPOOL_LOGO_PNG (and OPENSPOOL_LOGO_PNG_SIZE) suggest this is a PNG image, but the Content-Type is set to image/jpeg. This will cause browsers to misinterpret the image format.
Proposed fix
void WebServerManager::handleOpenSpoolLogo() {
_server.sendHeader("Cache-Control", "public, max-age=86400");
- _server.send_P(200, "image/jpeg", reinterpret_cast<const char*>(OPENSPOOL_LOGO_PNG), OPENSPOOL_LOGO_PNG_SIZE);
+ _server.send_P(200, "image/png", reinterpret_cast<const char*>(OPENSPOOL_LOGO_PNG), OPENSPOOL_LOGO_PNG_SIZE);
}📝 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.
| void WebServerManager::handleOpenSpoolLogo() { | |
| _server.sendHeader("Cache-Control", "public, max-age=86400"); | |
| _server.send_P(200, "image/jpeg", reinterpret_cast<const char*>(OPENSPOOL_LOGO_PNG), OPENSPOOL_LOGO_PNG_SIZE); | |
| } | |
| void WebServerManager::handleOpenSpoolLogo() { | |
| _server.sendHeader("Cache-Control", "public, max-age=86400"); | |
| _server.send_P(200, "image/png", reinterpret_cast<const char*>(OPENSPOOL_LOGO_PNG), OPENSPOOL_LOGO_PNG_SIZE); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WebServerManager.cpp` around lines 245 - 248, The response in
WebServerManager::handleOpenSpoolLogo is sending OPENSPOOL_LOGO_PNG via
_server.send_P but sets the Content-Type to "image/jpeg"; update the MIME type
to "image/png" so the PNG data (OPENSPOOL_LOGO_PNG and OPENSPOOL_LOGO_PNG_SIZE)
is served with the correct Content-Type header (modify the _server.send_P call
to use "image/png").
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/WebServerManager.cpp (1)
1029-1115:⚠️ Potential issue | 🔴 CriticalOpenSpool-specific data branch is missing from
/api/statushandler, but suggested fix references non-existent method.OpenSpoolTag is recognized and parsed (lines 1033, 505), but the API status handler doesn't include parsed OpenSpool fields. The issue is real, but the suggested code won't compile:
NFCManager::getInstance().getLastOpenSpoolData()does not exist. NFCManager has the data stored inlastOpenSpool_andlastOpenSpoolValid_members, but lacks a public getter method matching the pattern used for TigerTag and OpenTag3D.To implement this properly: add a public getter method to NFCManager (e.g.,
bool getLastOpenSpoolData(OpenSpoolData& out)) mirroring the existinggetLastTigerTagData()andgetLastOpenTag3DData()methods, then add the OpenSpoolTag branch to the API handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 1029 - 1115, The API /api/status handler is missing the OpenSpoolTag branch and the suggested getter used in the review doesn't exist; add a public NFCManager::getLastOpenSpoolData(OpenSpoolData& out) method that returns lastOpenSpoolValid_ and copies lastOpenSpool_ into out (mirroring getLastTigerTagData/getLastOpenTag3DData), then update the TagKind handling in WebServerManager.cpp to add an else-if branch for state.kind == TagKind::OpenSpool that calls NFCManager::getInstance().getLastOpenSpoolData(...) and serializes the relevant OpenSpoolData fields into a nested "openspool" JSON object (similar style to the TigerTag and OpenTag3D branches).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/WebServerManager.cpp`:
- Around line 1029-1115: The API /api/status handler is missing the OpenSpoolTag
branch and the suggested getter used in the review doesn't exist; add a public
NFCManager::getLastOpenSpoolData(OpenSpoolData& out) method that returns
lastOpenSpoolValid_ and copies lastOpenSpool_ into out (mirroring
getLastTigerTagData/getLastOpenTag3DData), then update the TagKind handling in
WebServerManager.cpp to add an else-if branch for state.kind ==
TagKind::OpenSpool that calls
NFCManager::getInstance().getLastOpenSpoolData(...) and serializes the relevant
OpenSpoolData fields into a nested "openspool" JSON object (similar style to the
TigerTag and OpenTag3D branches).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fbe7dfa8-75ec-44fc-bf49-aacb13c5a035
📒 Files selected for processing (11)
src/ConfigHTML.hsrc/LandingHTML.hsrc/OpenPrintTagWriterHTML.hsrc/OpenSpoolWriterHTML.hsrc/OpenTag3DWriterHTML.hsrc/ReaderHTML.hsrc/TigerTagWriterHTML.hsrc/TroubleshootingHTML.hsrc/UIDRegistrationHTML.hsrc/UpdateHTML.hsrc/WebServerManager.cpp
1 participant
Summary
OpenSpoolParser.cpp/.h— parses NDEF JSON with"protocol":"openspool"application/jsonMIME type after OpenTag3D/img/openspool.pngroute serving logo from PROGMEMOpenSpool Tag Format
NTAG 215/216, NDEF record with
application/jsonMIME type:{"protocol":"openspool","version":"1.0","type":"PLA","color_hex":"FFAABB","brand":"Generic","min_temp":"220","max_temp":"240"}No writer included
OpenSpool has their own writer app. Landing page card links to reader.
Test Plan
pio run -e esp32devcompiles cleanpio run -e esp32s3zerocompiles cleanreview-remote scanner feature/openspool-supportpassedCloses #93
Summary by CodeRabbit