feat: PN532 NFC reader support#48
Conversation
Add Adafruit PN532 as a second NFC reader option alongside PN5180. NVS key `nfc_reader` selects which reader initializes at boot (one-binary model). Both readers implement NFCConnectionI — zero changes to NFCManager or tag classification. - HardwareNFCConnectionPN532: full NFCConnectionI impl (ISO14443A only) - openprinttag_adafruit_pn532.h: HAL adapter for Adafruit library - Fix abstraction leaks: getReaderInfo() + logDiagnostics() virtual methods replace static_cast<HardwareNFCConnection*> in NFCManager - NVS config + web UI dropdown + installer prompt - Review fixes: firmware version parsing, UID verification in reactivateTag, GCode digit validation, memory cleanup on init failure, config validation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 8 seconds. ⌛ 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 (1)
📝 WalkthroughWalkthroughAdds PN532 NFC reader support (Adafruit PN532 HAL and HardwareNFCConnectionPN532), makes NFC diagnostics reader‑agnostic, adds runtime NFC reader selection and config/UI, introduces keypad GPIO definitions and ENABLE_KEYPAD flag, tightens ApplicationManager HTTP timeouts and toolNumber validation, and adds Adafruit PN532 dependency. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant NFC as HardwareNFCConnectionPN532
participant HAL as openprinttag HAL
participant Device as Adafruit_PN532
participant SPI as SPI Bus
App->>NFC: detectTag()
activate NFC
NFC->>Device: readPassiveTargetID(timeout)
Device->>SPI: SPI exchange (ISO14443A)
SPI-->>Device: ATQA, SAK, UID
Device-->>NFC: UID, length
NFC->>NFC: store UID/ATQA/SAK
deactivate NFC
NFC-->>App: UID present
App->>NFC: readISO14443Pages(start,pageCount,buf)
activate NFC
NFC->>HAL: getHal()
loop per page
App->>HAL: read_page(page) (via callback)
HAL->>Device: mifareultralight_ReadPage()
Device->>SPI: SPI read
alt success
HAL-->>App: OPT_OK + data
else failure
NFC->>NFC: reactivateTag()
NFC->>Device: readPassiveTargetID()
NFC->>HAL: retry read_page()
end
end
deactivate NFC
NFC-->>App: bytes read
App->>NFC: writeISO14443Pages(start,pageCount,data)
activate NFC
loop per page
NFC->>HAL: write_page(page,data)
HAL->>Device: mifareultralight_WritePage()
Device->>SPI: SPI write
alt success
HAL-->>NFC: OPT_OK
else retry up to 3 times (reactivateTag between attempts)
NFC->>Device: readPassiveTargetID()
end
NFC->>NFC: vTaskDelay()
end
deactivate NFC
NFC-->>App: write result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/TroubleshootingHTML.h (1)
252-252:⚠️ Potential issue | 🟡 MinorError handler still references hardcoded "PN5180".
The
catchblock on line 252 still uses'NFC Reader (PN5180)'in thenamesobject, which is inconsistent with the reader-agnostic approach. This should be updated to just'NFC Reader'to match line 234.Proposed fix
- const names = {wifi:'WiFi', mqtt:'MQTT Broker', spoolman:'Spoolman', nfc:'NFC Reader (PN5180)', heap:'Memory'}; + const names = {wifi:'WiFi', mqtt:'MQTT Broker', spoolman:'Spoolman', nfc:'NFC Reader', heap:'Memory'};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TroubleshootingHTML.h` at line 252, Update the hardcoded NFC reader label in the error handler: change the entry in the names object that currently uses 'NFC Reader (PN5180)' to the generic 'NFC Reader' so it matches the reader-agnostic label used elsewhere (e.g., the other occurrence at line ~234); edit the const names = {wifi:'WiFi', mqtt:'MQTT Broker', spoolman:'Spoolman', nfc:'NFC Reader (PN5180)', heap:'Memory'} to use nfc:'NFC Reader' instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/BoardPins.h`:
- Around line 65-72: Document that PIN_KEYPAD_ROW2 and PIN_KEYPAD_ROW3 (GPIO 16
and 17) can conflict with PSRAM on some ESP32-WROOM/WROVER modules and add a
short note next to the keypad pin definitions explaining this; instruct users to
avoid 16/17 on PSRAM-equipped boards and suggest alternative GPIOs (for example
use non-PSRAM pins such as GPIOs 2, 4, 12, 13, 14, 25-27, etc.), and optionally
recommend providing a comment or preprocessor/README hint to select alternate
PIN_KEYPAD_ROW* values when PSRAM/ESP32-WROVER is present.
In `@lib/openprinttag/openprinttag_adafruit_pn532.h`:
- Around line 14-16: The extern "C" linkage block around the header is
unnecessary and misleading because the header only declares static inline
functions that use C++ constructs (casts to Adafruit_PN532*) and therefore have
no external linkage; either remove the extern "C" { ... } / `#endif` pair (the
block beginning at the top of the file and the matching closing at lines near
74-76) or keep it but add a clear comment above it stating it is only for
documentation and does not affect the static inline functions; update references
to the static inline helpers (the inline functions that cast to Adafruit_PN532*)
accordingly so there’s no confusion about C vs C++ linkage.
In `@platformio.ini`:
- Line 27: The dependency "adafruit/Adafruit PN532@1.3.4" is pinned to an exact
version while other deps use semver-compatible ranges; update the entry (the
adafruit/Adafruit PN532 dependency line) to use a caret-prefixed range, e.g.,
change "@1.3.4" to "@^1.3.4", so it matches the project's versioning convention
and allows non-breaking updates.
In `@src/ApplicationManager.cpp`:
- Line 1000: The mutex and HTTP timeouts in ApplicationManager.cpp are much
shorter than the reference constant SpoolmanManager::HTTP_MUTEX_TIMEOUT; update
the call that uses g_httpMutex and xSemaphoreTake(pdMS_TO_TICKS(3000)) to use
SpoolmanManager::HTTP_MUTEX_TIMEOUT (or another shared constant) and similarly
align the HTTP connect/operation timeouts (1000ms/2000ms) to the same shared
timeout or document why shorter values are required for this code path (also
apply the same change where similar calls occur around the xSemaphoreTake/HTTP
setup later in the file).
In `@src/HardwareNFCConnectionPN532.cpp`:
- Around line 25-29: The allocation check after "pn532_ = new
Adafruit_PN532(PIN_PN532_SS, &SPI);" is unreliable across platforms because
plain new may throw instead of returning nullptr; to make the null-check
meaningful and portable, allocate with the nothrow variant (use std::nothrow)
when creating the Adafruit_PN532 instance and keep the existing if (!pn532_)
{...} error path, and ensure you include the <new> header; alternatively, if you
prefer throwing semantics, remove the nullptr check and wrap the allocation in a
try/catch for std::bad_alloc around the Adafruit_PN532 construction in
HardwareNFCConnectionPN532.cpp.
- Around line 64-86: Both reset() and hardwareReset() call pn532_->SAMConfig()
without checking its result; update both to verify SAMConfig's return value and
handle failures: for hardwareReset(), after pn532_->SAMConfig() check the bool
return and return false (and log an error) when it fails; for reset() (currently
void) either change its signature to return bool and return false on SAMConfig
failure (and true on success) or keep void but at minimum log an error and set
the object's initialized/error state so callers know initialization failed;
reference HardwareNFCConnectionPN532::reset,
HardwareNFCConnectionPN532::hardwareReset and the pn532_->SAMConfig() call when
making the change and update any callers if you change reset()'s return type.
- Around line 8-11: The code directly references the Adafruit PN532 internal
array pn532_packetbuffer[] which is fragile; update the file header to document
the minimum tested Adafruit PN532 library version (e.g., "Requires Adafruit
PN532 >= 1.3.4") and replace blind accesses with runtime checks: before reading
ATQA/SAK validate that pn532_packetbuffer exists and has the expected
length/indices (check buffer size >= 12 and that expected response offsets are
within bounds) and add assertions or guarded error-returns if the checks fail;
alternatively, where possible switch to public API calls instead of reading
pn532_packetbuffer directly (search for pn532_packetbuffer usages in
HardwareNFCConnectionPN532.cpp and add the described guards or API
replacements).
In `@src/WebServerManager.cpp`:
- Around line 529-531: The single-statement if that normalizes nfcVal should be
made explicit with braces for clarity and consistency: in the block where nfcVal
is set from doc (the code using nfcVal, strcmp(nfcVal, "pn532") and assigning
update.nfc_reader), wrap the body of the if (strcmp(nfcVal, "pn532") != 0)
statement in { } so the fallback assignment nfcVal = "pn5180"; is enclosed in
braces.
---
Outside diff comments:
In `@src/TroubleshootingHTML.h`:
- Line 252: Update the hardcoded NFC reader label in the error handler: change
the entry in the names object that currently uses 'NFC Reader (PN5180)' to the
generic 'NFC Reader' so it matches the reader-agnostic label used elsewhere
(e.g., the other occurrence at line ~234); edit the const names = {wifi:'WiFi',
mqtt:'MQTT Broker', spoolman:'Spoolman', nfc:'NFC Reader (PN5180)',
heap:'Memory'} to use nfc:'NFC Reader' instead.
🪄 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: 90837597-529f-4031-b99d-d421a9e070ca
📒 Files selected for processing (19)
.gitignoreinclude/BoardPins.hinclude/UserConfig.example.hlib/openprinttag/openprinttag_adafruit_pn532.hplatformio.inisrc/ApplicationManager.cppsrc/ConfigHTML.hsrc/ConfigurationManager.cppsrc/ConfigurationManager.hsrc/HardwareNFCConnection.cppsrc/HardwareNFCConnection.hsrc/HardwareNFCConnectionPN532.cppsrc/HardwareNFCConnectionPN532.hsrc/NFCConnectionI.hsrc/NFCManager.cppsrc/NFCManager.hsrc/TroubleshootingHTML.hsrc/WebServerManager.cppsrc/main.cpp
| // 3x4 Matrix Keypad (ENABLE_KEYPAD) | ||
| #define PIN_KEYPAD_ROW1 15 | ||
| #define PIN_KEYPAD_ROW2 16 | ||
| #define PIN_KEYPAD_ROW3 17 | ||
| #define PIN_KEYPAD_ROW4 18 | ||
| #define PIN_KEYPAD_COL1 19 | ||
| #define PIN_KEYPAD_COL2 21 | ||
| #define PIN_KEYPAD_COL3 5 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document potential GPIO conflicts on PSRAM-equipped ESP32-WROOM modules.
GPIO 16 and 17 (PIN_KEYPAD_ROW2, PIN_KEYPAD_ROW3) are used for PSRAM interface on ESP32-WROVER and some ESP32-WROOM-32D modules. Users with PSRAM-equipped boards may encounter conflicts.
Consider adding a note similar to the S3 section:
// 3x4 Matrix Keypad (ENABLE_KEYPAD)
+ // NOTE: GPIO 16/17 conflict with PSRAM on ESP32-WROVER modules.
+ // For PSRAM builds, remap ROW2/ROW3 to different pins.
`#define` PIN_KEYPAD_ROW1 15📝 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.
| // 3x4 Matrix Keypad (ENABLE_KEYPAD) | |
| #define PIN_KEYPAD_ROW1 15 | |
| #define PIN_KEYPAD_ROW2 16 | |
| #define PIN_KEYPAD_ROW3 17 | |
| #define PIN_KEYPAD_ROW4 18 | |
| #define PIN_KEYPAD_COL1 19 | |
| #define PIN_KEYPAD_COL2 21 | |
| #define PIN_KEYPAD_COL3 5 | |
| // 3x4 Matrix Keypad (ENABLE_KEYPAD) | |
| // NOTE: GPIO 16/17 conflict with PSRAM on ESP32-Wrover modules. | |
| // For PSRAM builds, remap ROW2/ROW3 to different pins. | |
| `#define` PIN_KEYPAD_ROW1 15 | |
| `#define` PIN_KEYPAD_ROW2 16 | |
| `#define` PIN_KEYPAD_ROW3 17 | |
| `#define` PIN_KEYPAD_ROW4 18 | |
| `#define` PIN_KEYPAD_COL1 19 | |
| `#define` PIN_KEYPAD_COL2 21 | |
| `#define` PIN_KEYPAD_COL3 5 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/BoardPins.h` around lines 65 - 72, Document that PIN_KEYPAD_ROW2 and
PIN_KEYPAD_ROW3 (GPIO 16 and 17) can conflict with PSRAM on some
ESP32-WROOM/WROVER modules and add a short note next to the keypad pin
definitions explaining this; instruct users to avoid 16/17 on PSRAM-equipped
boards and suggest alternative GPIOs (for example use non-PSRAM pins such as
GPIOs 2, 4, 12, 13, 14, 25-27, etc.), and optionally recommend providing a
comment or preprocessor/README hint to select alternate PIN_KEYPAD_ROW* values
when PSRAM/ESP32-WROVER is present.
| #ifdef __cplusplus | ||
| extern "C" { | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
extern "C" linkage is unnecessary for static inline functions.
These are static inline functions that won't have external linkage. The extern "C" block has no effect on them and could be misleading since the function bodies use C++ constructs (Adafruit_PN532* cast). Consider removing the extern "C" guards or, if kept for documentation purposes, add a comment clarifying their purpose.
Also applies to: 74-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/openprinttag/openprinttag_adafruit_pn532.h` around lines 14 - 16, The
extern "C" linkage block around the header is unnecessary and misleading because
the header only declares static inline functions that use C++ constructs (casts
to Adafruit_PN532*) and therefore have no external linkage; either remove the
extern "C" { ... } / `#endif` pair (the block beginning at the top of the file and
the matching closing at lines near 74-76) or keep it but add a clear comment
above it stating it is only for documentation and does not affect the static
inline functions; update references to the static inline helpers (the inline
functions that cast to Adafruit_PN532*) accordingly so there’s no confusion
about C vs C++ linkage.
| knolleary/PubSubClient@^2.8 | ||
| codewitch-honey-crisis/htcw_json@^0.2.5 | ||
| adafruit/Adafruit NeoPixel | ||
| adafruit/Adafruit PN532@1.3.4 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
What is the latest version of Adafruit PN532 Arduino library?
💡 Result:
The latest version of the Adafruit PN532 Arduino library is 1.3.4, released on 2025-02-18.
Citations:
- 1: https://github.com/adafruit/Adafruit-PN532
- 2: https://www.arduinolibraries.info/libraries/adafruit-pn532
- 3: http://arduinolibraries.info/libraries/adafruit-pn532
🏁 Script executed:
# Read the platformio.ini file to see all dependencies and their version pinning styles
cat platformio.iniRepository: SpoolSense/spoolsense_scanner
Length of output: 1187
Version pinning is inconsistent with other dependencies.
Most dependencies use ^ prefix for semver-compatible updates (e.g., ^1.1.4, ^7.0.0), but Adafruit PN532@1.3.4 is pinned to an exact version. While 1.3.4 is the current latest version, consider using @^1.3.4 for consistency with the rest of the dependency list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platformio.ini` at line 27, The dependency "adafruit/Adafruit PN532@1.3.4" is
pinned to an exact version while other deps use semver-compatible ranges; update
the entry (the adafruit/Adafruit PN532 dependency line) to use a caret-prefixed
range, e.g., change "@1.3.4" to "@^1.3.4", so it matches the project's
versioning convention and allows non-breaking updates.
|
|
||
| extern SemaphoreHandle_t g_httpMutex; | ||
| if (g_httpMutex && xSemaphoreTake(g_httpMutex, pdMS_TO_TICKS(5000)) != pdTRUE) { | ||
| if (g_httpMutex && xSemaphoreTake(g_httpMutex, pdMS_TO_TICKS(3000)) != pdTRUE) { |
There was a problem hiding this comment.
HTTP timeout values are significantly shorter than other HTTP operations.
The mutex timeout (3000ms) and HTTP timeouts (1000ms connect, 2000ms operation) are much shorter than SpoolmanManager::HTTP_MUTEX_TIMEOUT which is 10000ms. This inconsistency could cause premature failures if the network or Moonraker is slow. Consider aligning these values or documenting why shorter timeouts are appropriate for this specific use case.
Proposed timeout alignment
- if (g_httpMutex && xSemaphoreTake(g_httpMutex, pdMS_TO_TICKS(3000)) != pdTRUE) {
+ if (g_httpMutex && xSemaphoreTake(g_httpMutex, pdMS_TO_TICKS(5000)) != pdTRUE) {- http.setConnectTimeout(1000);
- http.setTimeout(2000);
+ http.setConnectTimeout(3000);
+ http.setTimeout(5000);Also applies to: 1017-1018
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ApplicationManager.cpp` at line 1000, The mutex and HTTP timeouts in
ApplicationManager.cpp are much shorter than the reference constant
SpoolmanManager::HTTP_MUTEX_TIMEOUT; update the call that uses g_httpMutex and
xSemaphoreTake(pdMS_TO_TICKS(3000)) to use SpoolmanManager::HTTP_MUTEX_TIMEOUT
(or another shared constant) and similarly align the HTTP connect/operation
timeouts (1000ms/2000ms) to the same shared timeout or document why shorter
values are required for this code path (also apply the same change where similar
calls occur around the xSemaphoreTake/HTTP setup later in the file).
| // Adafruit_PN532 uses a file-scope global packet buffer. After readPassiveTargetID, | ||
| // the InListPassiveTarget response populates it with ATQA (bytes 9-10) and SAK (byte 11). | ||
| extern byte pn532_packetbuffer[]; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Adafruit PN532 library pn532_packetbuffer API stability version 1.3.4
💡 Result:
The Adafruit PN532 Arduino library (https://github.com/adafruit/Adafruit-PN532) version 1.3.4, released on 2025-02-18, uses an internal pn532_packetbuffer array in its implementation for handling PN532 command/response packets, as evidenced by historical code references and usage in functions like ntag2xx_ReadPage. This buffer is not exposed as a public API class or method (no PN532_PacketBuffer class or public accessor found in Adafruit_PN532.h). The library API consists of high-level functions like SAMConfig, getFirmwareVersion, readPassiveTargetID, etc. No breaking changes to packet handling are noted in recent releases (1.3.x series focuses on minor updates like CI and version bumps); it remains stable and backward-compatible with prior versions (e.g., 1.2.x). For official documentation, refer to in-library examples and Doxygen-generated docs.
Citations:
- 1: https://github.com/adafruit/Adafruit-PN532
- 2: https://www.arduinolibraries.info/libraries/adafruit-pn532
- 3: https://github.com/adafruit/Adafruit-PN532/blob/master/Adafruit_PN532.h
- 4: ntag2xx_ReadPage only reads 4 bytes at a time adafruit/Adafruit-PN532#40
Accessing non-public library internals creates maintenance risk.
The pn532_packetbuffer[] is an internal implementation detail of the Adafruit PN532 library, not part of the public API. Relying on it directly couples this code to the library's internal structure, which could break if the buffer layout changes, is renamed, or made static.
Document the minimum library version required (e.g., 1.3.4+) and consider adding assertions to validate buffer indices at runtime.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 10-10: variable 'pn532_packetbuffer' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HardwareNFCConnectionPN532.cpp` around lines 8 - 11, The code directly
references the Adafruit PN532 internal array pn532_packetbuffer[] which is
fragile; update the file header to document the minimum tested Adafruit PN532
library version (e.g., "Requires Adafruit PN532 >= 1.3.4") and replace blind
accesses with runtime checks: before reading ATQA/SAK validate that
pn532_packetbuffer exists and has the expected length/indices (check buffer size
>= 12 and that expected response offsets are within bounds) and add assertions
or guarded error-returns if the checks fail; alternatively, where possible
switch to public API calls instead of reading pn532_packetbuffer directly
(search for pn532_packetbuffer usages in HardwareNFCConnectionPN532.cpp and add
the described guards or API replacements).
| pn532_ = new Adafruit_PN532(PIN_PN532_SS, &SPI); | ||
| if (!pn532_) { | ||
| Serial.println("PN532: Failed to allocate"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Null check after new may be redundant.
In standard C++, new throws std::bad_alloc on allocation failure rather than returning nullptr. However, some embedded toolchains (including ESP32 Arduino) may define new with nothrow semantics. This defensive check is acceptable for portability but may never trigger depending on the platform configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HardwareNFCConnectionPN532.cpp` around lines 25 - 29, The allocation
check after "pn532_ = new Adafruit_PN532(PIN_PN532_SS, &SPI);" is unreliable
across platforms because plain new may throw instead of returning nullptr; to
make the null-check meaningful and portable, allocate with the nothrow variant
(use std::nothrow) when creating the Adafruit_PN532 instance and keep the
existing if (!pn532_) {...} error path, and ensure you include the <new> header;
alternatively, if you prefer throwing semantics, remove the nullptr check and
wrap the allocation in a try/catch for std::bad_alloc around the Adafruit_PN532
construction in HardwareNFCConnectionPN532.cpp.
| const char* nfcVal = doc["nfc_reader"] | "pn5180"; | ||
| if (strcmp(nfcVal, "pn532") != 0) nfcVal = "pn5180"; // only allow known values | ||
| strncpy(update.nfc_reader, nfcVal, sizeof(update.nfc_reader) - 1); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Input validation is effective but consider adding braces for clarity.
The validation logic correctly normalizes unknown values to "pn5180", preventing invalid reader configurations. Per static analysis, consider wrapping the single-statement if body in braces for consistency:
- if (strcmp(nfcVal, "pn532") != 0) nfcVal = "pn5180"; // only allow known values
+ if (strcmp(nfcVal, "pn532") != 0) {
+ nfcVal = "pn5180"; // only allow known values
+ }📝 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.
| const char* nfcVal = doc["nfc_reader"] | "pn5180"; | |
| if (strcmp(nfcVal, "pn532") != 0) nfcVal = "pn5180"; // only allow known values | |
| strncpy(update.nfc_reader, nfcVal, sizeof(update.nfc_reader) - 1); | |
| const char* nfcVal = doc["nfc_reader"] | "pn5180"; | |
| if (strcmp(nfcVal, "pn532") != 0) { | |
| nfcVal = "pn5180"; // only allow known values | |
| } | |
| strncpy(update.nfc_reader, nfcVal, sizeof(update.nfc_reader) - 1); |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 529-529: variable 'nfcVal' is not initialized
(cppcoreguidelines-init-variables)
[warning] 530-530: 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/WebServerManager.cpp` around lines 529 - 531, The single-statement if
that normalizes nfcVal should be made explicit with braces for clarity and
consistency: in the block where nfcVal is set from doc (the code using nfcVal,
strcmp(nfcVal, "pn532") and assigning update.nfc_reader), wrap the body of the
if (strcmp(nfcVal, "pn532") != 0) statement in { } so the fallback assignment
nfcVal = "pn5180"; is enclosed in braces.
CodeRabbit finding — SAMConfig() failures were silently ignored in reset() and hardwareReset(). Now logs on soft reset failure and returns false on hard reset failure.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/HardwareNFCConnectionPN532.cpp (1)
64-69:⚠️ Potential issue | 🟠 MajorDrop
ready_on reset failures.Lines 64-69 and 73-87 detect re-init failures, but they never clear
ready_, andhardwareReset()still returnstruewhenpn532_is null. That leaves the rest of the stack free to keep treating the reader as healthy after a failed recovery. Setready_ = falsebefore re-init starts, returnfalsewhen there is no instance to reset, and only restoreready_after the firmware probe andSAMConfig()both succeed.Suggested fix
void HardwareNFCConnectionPN532::reset() { - if (pn532_) { - pn532_->begin(); - if (!pn532_->SAMConfig()) { - Serial.println("PN532: SAMConfig failed during reset"); - } + ready_ = false; + if (!pn532_) { + return; } + + pn532_->begin(); + if (!pn532_->SAMConfig()) { + Serial.println("PN532: SAMConfig failed during reset"); + return; + } + + ready_ = true; } bool HardwareNFCConnectionPN532::hardwareReset() { + ready_ = false; + if (!pn532_) { + return false; + } + // Toggle RST pin for hardware reset pinMode(PIN_PN532_RST, OUTPUT); digitalWrite(PIN_PN532_RST, LOW); @@ - if (pn532_) { - pn532_->begin(); - uint32_t ver = pn532_->getFirmwareVersion(); - if (!ver) return false; - if (!pn532_->SAMConfig()) return false; - } - return true; + pn532_->begin(); + uint32_t ver = pn532_->getFirmwareVersion(); + if (!ver) { + return false; + } + if (!pn532_->SAMConfig()) { + return false; + } + + ready_ = true; + return true; }Also applies to: 73-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HardwareNFCConnectionPN532.cpp` around lines 64 - 69, In HardwareNFCConnectionPN532::reset(), clear the ready_ flag immediately before attempting re-init (set ready_ = false), and if pn532_ is null return false to indicate failure; then perform pn532_->begin(), probe firmware version and call pn532_->SAMConfig(), and only restore ready_ = true after both the firmware probe and SAMConfig() succeed; apply the same ready_/return-false logic to the similar re-init path in the later block (lines ~73-87) and ensure hardwareReset() observes the updated return value when pn532_ is null.
🤖 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/HardwareNFCConnectionPN532.cpp`:
- Around line 196-199: getReaderInfo currently always writes a firmware string
using fwMajor_/fwMinor_, which can produce a bogus "PN532 v0.0" after init
failure; update HardwareNFCConnectionPN532::getReaderInfo to check the same
ready_ flag used elsewhere (e.g., where uninitialized is handled in the 202–206
block) and, when !ready_, write a clear uninitialized/disabled message (or an
empty/explicitly prefixed status) into buf instead of the firmware version so
diagnostics match the readiness check.
- Around line 151-156: The current multi-page read logic in
HardwareNFCConnectionPN532.cpp can return a partial bytesRead on mid-read
errors; change it so any failed pn532_->mifareultralight_ReadPage or failed
reactivateTag leads to returning 0 (per NFCConnectionI.h). Specifically, in the
block that calls pn532_->mifareultralight_ReadPage(page, pageBuf) and then
reactivateTag(), replace returning bytesRead on failure with returning 0 so
callers never see truncated data; ensure all early-failure paths in the read
routine (including the case where reactivateTag() itself fails) return 0 instead
of the current bytesRead.
---
Duplicate comments:
In `@src/HardwareNFCConnectionPN532.cpp`:
- Around line 64-69: In HardwareNFCConnectionPN532::reset(), clear the ready_
flag immediately before attempting re-init (set ready_ = false), and if pn532_
is null return false to indicate failure; then perform pn532_->begin(), probe
firmware version and call pn532_->SAMConfig(), and only restore ready_ = true
after both the firmware probe and SAMConfig() succeed; apply the same
ready_/return-false logic to the similar re-init path in the later block (lines
~73-87) and ensure hardwareReset() observes the updated return value when pn532_
is null.
🪄 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: b4d419af-a8c4-4c0f-8792-558dd74239cf
📒 Files selected for processing (1)
src/HardwareNFCConnectionPN532.cpp
| // Try read, reactivate tag on failure and retry once | ||
| if (!pn532_->mifareultralight_ReadPage(page, pageBuf)) { | ||
| if (!reactivateTag()) return bytesRead; | ||
| if (!pn532_->mifareultralight_ReadPage(page, pageBuf)) { | ||
| return bytesRead; | ||
| } |
There was a problem hiding this comment.
Return 0 on any failed multi-page read.
src/NFCConnectionI.h documents this API as returning 0 on failure, but Lines 153-156 return a partial byte count after a mid-read error. That can surface truncated NTAG/OpenPrintTag data as a successful read to callers that only check for non-zero.
Suggested fix
if (!pn532_->mifareultralight_ReadPage(page, pageBuf)) {
- if (!reactivateTag()) return bytesRead;
+ if (!reactivateTag()) return 0;
if (!pn532_->mifareultralight_ReadPage(page, pageBuf)) {
- return bytesRead;
+ return 0;
}
}📝 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.
| // Try read, reactivate tag on failure and retry once | |
| if (!pn532_->mifareultralight_ReadPage(page, pageBuf)) { | |
| if (!reactivateTag()) return bytesRead; | |
| if (!pn532_->mifareultralight_ReadPage(page, pageBuf)) { | |
| return bytesRead; | |
| } | |
| // Try read, reactivate tag on failure and retry once | |
| if (!pn532_->mifareultralight_ReadPage(page, pageBuf)) { | |
| if (!reactivateTag()) return 0; | |
| if (!pn532_->mifareultralight_ReadPage(page, pageBuf)) { | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HardwareNFCConnectionPN532.cpp` around lines 151 - 156, The current
multi-page read logic in HardwareNFCConnectionPN532.cpp can return a partial
bytesRead on mid-read errors; change it so any failed
pn532_->mifareultralight_ReadPage or failed reactivateTag leads to returning 0
(per NFCConnectionI.h). Specifically, in the block that calls
pn532_->mifareultralight_ReadPage(page, pageBuf) and then reactivateTag(),
replace returning bytesRead on failure with returning 0 so callers never see
truncated data; ensure all early-failure paths in the read routine (including
the case where reactivateTag() itself fails) return 0 instead of the current
bytesRead.
| void HardwareNFCConnectionPN532::getReaderInfo(char* buf, size_t len) const { | ||
| if (buf && len > 0) { | ||
| snprintf(buf, len, "PN532 v%d.%d", fwMajor_, fwMinor_); | ||
| } |
There was a problem hiding this comment.
Keep getReaderInfo() consistent with the readiness check.
Lines 196-199 always emit a firmware version, while Lines 202-206 already treat !ready_ as uninitialized. That can leave the status/diagnostics path reporting a bogus PN532 v0.0-style string after init failure.
Suggested fix
void HardwareNFCConnectionPN532::getReaderInfo(char* buf, size_t len) const {
if (buf && len > 0) {
+ if (!ready_) {
+ snprintf(buf, len, "PN532 (not initialized)");
+ return;
+ }
snprintf(buf, len, "PN532 v%d.%d", fwMajor_, fwMinor_);
}
}Also applies to: 202-206
🧰 Tools
🪛 Clang (14.0.6)
[warning] 196-196: method 'getReaderInfo' can be made static
(readability-convert-member-functions-to-static)
🪛 Cppcheck (2.20.0)
[style] 196-196: The function 'getReaderInfo' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HardwareNFCConnectionPN532.cpp` around lines 196 - 199, getReaderInfo
currently always writes a firmware string using fwMajor_/fwMinor_, which can
produce a bogus "PN532 v0.0" after init failure; update
HardwareNFCConnectionPN532::getReaderInfo to check the same ready_ flag used
elsewhere (e.g., where uninitialized is handled in the 202–206 block) and, when
!ready_, write a clear uninitialized/disabled message (or an empty/explicitly
prefixed status) into buf instead of the firmware version so diagnostics match
the readiness check.
1 participant
Summary
nfc_readerselects which reader initializes at boot (one-binary model)NFCConnectionI— zero changes to NFCManager or tag classificationstatic_castto concrete type replaced with virtual methods)Changes
HardwareNFCConnectionPN532.h/.cpp— full NFCConnectionI implementationlib/openprinttag/openprinttag_adafruit_pn532.h— HAL adapter for Adafruit librarygetReaderInfo()andlogDiagnostics()virtual methodsstatic_cast<HardwareNFCConnection*>leaksnfc_readerNVS key + accessor/api/status+ config APIReview fixes applied
reactivateTag()(prevent cross-tag corruption on swap)sendAssignSpool()(injection prevention)begin()failure pathssendAssignSpoolHTTP timeouts (1s/2s stopgap for loop blocking)Test plan
pio run -e esp32s3zerocompiles cleanpio run -e esp32devcompiles cleannfc_reader=pn5180, verify existing tag read/write (regression)nfc_reader=pn532, test NTAG215 read/write (hardware test)/configpage shows NFC reader dropdown/api/statusshowsnfc.readerstringSummary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores