feat: M5Stack NFC Support - ST25R3916 (Grove I2C) reader - #294
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 4 reviews per rolling hour; 3 remain after this review. WalkthroughChangesST25R3916 I2C reader support
Sequence Diagram(s)sequenceDiagram
participant ConfigurationUI
participant WebServerManager
participant NfcManager
participant St25r3916Reader
participant ST25R3916
participant NFC_Tag
ConfigurationUI->>WebServerManager: Submit ST25R3916 and SDA/SCL settings
WebServerManager->>WebServerManager: Validate NFC pin ownership
NfcManager->>St25r3916Reader: Initialize with SDA/SCL pins
St25r3916Reader->>ST25R3916: Configure I2C, RF field, and interrupts
NfcManager->>St25r3916Reader: Begin tag discovery
St25r3916Reader->>NFC_Tag: Perform ISO14443 discovery and RATS
NfcManager->>St25r3916Reader: Exchange APDU
St25r3916Reader->>NFC_Tag: Exchange ISO-DEP frames
NFC_Tag-->>St25r3916Reader: Return APDU response
Poem
Merge Risk: 🔵 Low · up to The PR adds ST25R3916 support, but installation guidance still points users to PN532 wiring, which could lead to incorrect setup for the new reader. This is a bounded documentation issue that is mergeable with explicit owner follow-up. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
main/St25r3916Reader.cpp (1)
104-109: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClamp the derived frame waiting time.
fwiToMs(14)returns 81925 ms.exchangeApduusesm_fwtMsas the transceive timeout, andwaitInterruptpolls I2C in a tight loop for that whole period. A card that advertises a large FWI therefore blocks the NFC polling task for tens of seconds. Cap the value at a practical ceiling.♻️ Proposed clamp
uint32_t fwiToMs(uint8_t fwi) { if (fwi > 14) fwi = 14; uint32_t ms = 5; for (uint8_t i = 0; i < fwi; i++) ms *= 2; - return ms + 5; + ms += 5; + // Cap the wait: no Home Key transaction needs more than ~2 s, and a longer + // wait blocks the polling task on the I2C interrupt poll loop. + constexpr uint32_t kMaxFwtMs = 2000; + return ms > kMaxFwtMs ? kMaxFwtMs : ms; }main/NfcManager.cpp (1)
295-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the I2C pins before constructing the reader.
The type 1 branch rejects unset IRQ/VEN pins. The type 2 branch accepts unset pins, so an unconfigured device reaches
i2c_new_master_buswithsda_io_num = 255and reports only a driver argument error. Apply the same explicit check for consistent diagnostics.♻️ Proposed fix
} else if (m_nfcReaderType == 2) { + if (nfcGpioPins[0] == 255 || nfcGpioPins[1] == 255) { + ESP_LOGE(TAG, "ST25R3916 selected but SDA/SCL pins are unset"); + return false; + } // I2C: nfcGpioPins[0] = SDA, [1] = SCL. Entries [2]/[3] are unused. m_reader = std::make_unique<St25r3916Reader>(nfcGpioPins, m_ecpData); ESP_LOGI(TAG, "Using ST25R3916 reader (I2C)");
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81ec50ed-3f80-4de9-a925-d32aa7cf8dfc
📒 Files selected for processing (8)
README.mddata/src/lib/components/HardwareConfig.sveltemain/CMakeLists.txtmain/NfcManager.cppmain/St25r3916Reader.cppmain/include/NfcManager.hppmain/include/St25r3916Reader.hppmain/main.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
assess the relevance of the changes to the purpose of the PR and do not mention anything if they are relevant.
Files:
main/CMakeLists.txtmain/main.cppmain/include/NfcManager.hppmain/NfcManager.cppREADME.mdmain/include/St25r3916Reader.hppdata/src/lib/components/HardwareConfig.sveltemain/St25r3916Reader.cpp
🪛 LanguageTool
README.md
[grammar] ~70-~70: Ensure spelling is correct
Context: ...kHz is required - M5Stack document that SoftwareI2C latency is too high > for the chip's RF...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (11)
main/include/St25r3916Reader.hpp (1)
37-144: LGTM!main/St25r3916Reader.cpp (5)
125-241: LGTM!
328-344: LGTM!
451-524: LGTM!
528-647: LGTM!
651-793: LGTM!main/include/NfcManager.hpp (1)
104-107: LGTM!main/NfcManager.cpp (1)
231-243: LGTM!main/CMakeLists.txt (1)
3-3: LGTM!data/src/lib/components/HardwareConfig.svelte (1)
48-58: LGTM!Also applies to: 125-125, 148-151, 163-163, 173-205
README.md (1)
36-64: LGTM!Also applies to: 276-278, 301-302, 315-318
|
It looks good, i don't have the hardware to test it, but looking at it, i trust that the code is functional. Before i merge this, please look into testing the Attestation flow and implement the right procedure for this nfc chip to handle >256 bytes payloads / extended frames. sometimes provision might not occur or for some reason fail and attestation can fill in the gap, i haven't experienced this but other people did and haven't found the root cause. There's a debug command you can use to force the use of the Attestation flow to test, but first you need to press a key on reset to enable serial input (c2a8d01) and then enter |
|
I will work on addressing the comments, and update the pull request. I want to make sure I test everything again. |
Implements INfcReader for the ST25R3916, selectable as nfcReaderType = 2
alongside PN532 (0) and PN7160 (1). Developed against an M5Stack AtomS3 Lite
plus M5Stack Unit NFC (ST25R3916-AQWT, SKU U216) over Grove I2C -- a
solder-free combination.
The PN532 hides two layers in its own firmware that a bare transceiver does
not, so both are implemented here:
- RATS. InListPassiveTarget performs it internally; pollForTag() runs it
after anticollision when SAK indicates ISO-DEP, so a tag reported to the
caller is genuinely ready for exchangeApdu().
- ISO-DEP block framing. InDataExchange handles it in hardware; exchangeApdu()
does I-block framing, block-number toggling, S(WTX) waiting-time extension
and R(NAK) error recovery in software.
Two behaviours of this silicon are easy to get wrong and were found the hard
way:
- The received CRC is left in the FIFO. Taking the last two bytes of an APDU
response as SW1 SW2 yields the CRC instead; a SELECT that had actually
succeeded reported SW=0x5B62.
- ANTICOLLISION carries no CRC in either direction. With RX CRC checking
left on, a perfectly good UID+BCC response is flagged as a CRC error.
no_crc_rx (AUX bit 0x80) is toggled per exchange.
The Grove connector does not break out the IRQ pin, so every wait polls the
interrupt status registers over I2C. Measured on hardware, this is comfortable:
oscillator stable (I_osc) 1197 us
ISO14443-A response (I_rxe) ~600 us
SELECT AID 15 ms
Auth0 (112 B cmd, 1 WTX round) 76 ms
control flow 15 ms
full FAST-flow authentication 130-160 ms
ECP is transmitted with CMD_TRANSMIT_WITHOUT_CRC and the software CRC-A that
NfcManager already appends; no transparent or stream mode is required.
Also in this commit, because they are part of making the reader usable:
- NfcManager no longer claims four SPI pins for an I2C reader. Only
nfcGpioPins[0] (SDA) and [1] (SCL) are acquired, under new SDA/SCL pin
functions. Claiming MISO/MOSI reserved two arbitrary GPIOs under
misleading names, which matters on a board like the AtomS3 Lite that
exposes only a handful.
- The startup reader-type log was a two-way ternary and printed "PN7160" for
any value other than 0, so a correctly configured ST25R3916 reported the
wrong hardware.
- The web UI offers ST25R3916 in the reader dropdown, relabels the first two
pin fields SDA/SCL and hides MISO/MOSI for it. Without the option the
select rendered blank and saving the page would have written a bogus
reader type.
Verified end to end against a real iPhone: ECP wakeup, anticollision, RATS,
SELECT A0000008580101 -> 9000, full FAST-flow authentication, lock state to
HomeKit and MQTT.
Known limitations: ISO-DEP chaining is not implemented (every Home Key command
observed so far fits the card's 256-byte FSC, but the attestation flow has not
been exercised); ANTICOLLISION is sent with NVB=0x20 only, so two devices in
the field at once are not resolved; RFAL's per-technology analog presets are
not applied, which may limit range.
Adds the ST25R3916 to the supported readers and documents an M5Stack pairing that needs no soldering, breadboard or jumper wires -- the AtomS3 Lite and the Unit NFC connect with the supplied Grove cable. That lowers the barrier for anyone who wants a working Home Key reader without building a circuit. Includes the exact settings the combination needs (reader type, SDA=2, SCL=1, optional NeoPixel on GPIO 35), measured authentication latency of 130-160 ms, and a note that the Grove connector omits the IRQ pin so hardware I2C at 400 kHz is required. Also records the real ESP-IDF requirement in the build instructions. The previous text linked to the generic install guide without naming a version, while idf_component.yml claimed 5.3.0 and the tree does not compile below 5.4. Project structure and component table updated to list the three reader backends rather than describing NfcManager as PN532-specific.
Three items raised in review of rednblkx#294. The startup log reported the wrong pins. NfcManager is handed nfcGpioPinsPresets[...].gpioPins whenever a preset is selected, but the ST25R3916 branch logged miscConfig.nfcGpioPins, so a device using a preset printed SDA/SCL that the reader was not using. The pins are now resolved once into activeNfcPins and that same value feeds both the log line and the NfcManager constructor, so the two cannot disagree. The reader type 2 branch accepted unset pins. Type 1 rejects an unset IRQ/VEN pair with a clear message; type 2 passed 255 straight through to i2c_new_master_bus(), which fails with a generic driver argument error that says nothing about configuration. Same explicit check added. README: "M5Stack document" -> "M5Stack documents".
Chaining
--------
exchangeApdu() assumed every command and response fitted the card's FSC.
Every Home Key exchange observed in the FAST flow does -- the largest is a
112 byte Auth0 command with an 87 byte response against FSC 256 -- but the
attestation flow does not, and that flow exists precisely to cover cases where
provisioning did not complete. An oversized APDU previously failed outright.
Both directions are now implemented:
- Transmit chaining splits the command into FSC-sized I-blocks, sets the M
bit on all but the last, waits for R(ACK) between chunks and toggles the
block number per chunk.
- Receive chaining reassembles a chained response, requesting each
continuation with R(ACK) carrying the received block number toggled, per
ISO 14443-4. S(WTX) is handled inside the chain, not just around it.
The block number is taken from the PCB the card actually sent rather than
assumed, so a chained response leaves the session aligned for the next
exchange. Responses are bounded at 4 KB so a misbehaving card cannot grow the
heap without limit.
Also removes an incorrect assumption: FSC bounds what the card is willing to
*receive*. Its own responses are bounded by the FSD advertised in RATS, so the
receive buffer is sized from the frame buffer rather than from FSC.
Review fixes
------------
Both RX waits could block for the full timeout after the response had already
arrived. readInterrupts() clears the status registers as it reads them, so a
poll that observed IRQ_TXE may have latched IRQ_RXE in the same read; the
following wait could then never see that bit and spun to its deadline. The
exchange still succeeded, which is why this showed up as latency rather than
failure. Both transceive() and shortFrame() now skip the RX wait when the bit
has already been seen.
waitInterrupt() used an absolute deadline. nowMs() wraps every ~49.7 days, and
a wrapped "nowMs() + timeoutMs" is already in the past, so every transceive
would fail for the length of that window. Now an unsigned elapsed delta.
shortFrame() inherited AUX_NO_CRC_RX from whatever ran before it. In
isTagStillPresent() that is an R(NAK) exchange with rxCrc = true, which clears
the bit, so the WUPA fallback had its reply checked against a CRC that ATQA
does not carry -- passing only because the error flags are ignored there. Now
set explicitly.
writeFifo() reserved 513 bytes of stack per call while exchangeApdu() already
held tx[264] and rx[264] on the same stack, on a path that also runs on the
auth precompute task. Bounded to the largest frame a caller can actually build.
fwiToMs() was unclamped: FWI 14 yields 81925 ms, and with no IRQ line the wait
is a tight I2C poll, so a card advertising a large FWI would occupy the polling
task for over a minute. Capped at 2000 ms; a card that legitimately needs
longer says so with S(WTX). Measured against a real Home Key card the
advertised FWT is 645 ms, well under the cap.
Verified on hardware after these changes: FAST flow authentication in 166 ms
(SELECT 17 ms, Auth0 112 B command with one WTX round 81 ms, control flow
14 ms), lock state propagated to HomeKit and MQTT, precompute task headroom
3984 of 8192 bytes.
256b695 to
61a418a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
main/St25r3916Reader.cpp (2)
331-343: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider yielding inside the interrupt poll loop.
waitInterruptpolls without any yield. Each iteration issues four I2C reads, so the loop is rate-limited but never blocks. With the FWT cap the loop can hold the CPU for up to 2000 ms. If several long waits run back to back, the idle task on that core can starve and the task watchdog can fire. A shortvTaskDelay(1)after the first few iterations keeps the fast path fast and lets other tasks run during long waits.♻️ Proposed refactor
const uint32_t start = nowMs(); uint32_t seen = 0; + uint32_t spins = 0; do { seen |= readInterrupts(); if (seen & mask) break; + // Spin for the common sub-millisecond case, then yield so long waits + // do not starve the idle task. + if (++spins > 8) vTaskDelay(1); } while ((nowMs() - start) < timeoutMs); return seen;
883-886: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead comparison in the chaining PCB check.
PCB_TYPE_MASKis0xC0andPCB_I_BLOCKis0x02.cpcb & PCB_TYPE_MASKcan never equal0x02, so the second comparison is always true. The check is still correct because I-blocks carry type bits00, but the dead term suggests an intent that the code does not implement.♻️ Proposed refactor
- if ((cpcb & PCB_TYPE_MASK) != 0x00 && (cpcb & PCB_TYPE_MASK) != PCB_I_BLOCK) { + // I-blocks are the only PCB type with bits 7:6 == 00. + if ((cpcb & PCB_TYPE_MASK) != 0x00) { ESP_LOGW(TAG, "unexpected PCB 0x%02X during receive chaining", cpcb); return false; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d048341-c02a-4e34-bdf9-75b114d5d61f
📒 Files selected for processing (8)
README.mddata/src/lib/components/HardwareConfig.sveltemain/CMakeLists.txtmain/NfcManager.cppmain/St25r3916Reader.cppmain/include/NfcManager.hppmain/include/St25r3916Reader.hppmain/main.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- main/CMakeLists.txt
- main/NfcManager.cpp
- main/main.cpp
- main/include/NfcManager.hpp
- data/src/lib/components/HardwareConfig.svelte
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
assess the relevance of the changes to the purpose of the PR and do not mention anything if they are relevant.
Files:
main/include/St25r3916Reader.hppmain/St25r3916Reader.cppREADME.md
🪛 LanguageTool
README.md
[grammar] ~70-~70: Ensure spelling is correct
Context: ...Hz is required - M5Stack documents that SoftwareI2C latency is too high > for the chip's RF...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (15)
README.md (6)
36-42: LGTM!
44-58: LGTM!
62-71: LGTM!
276-278: LGTM!
301-302: LGTM!
315-318: LGTM!main/include/St25r3916Reader.hpp (1)
37-144: LGTM!main/St25r3916Reader.cpp (8)
11-121: LGTM!
135-236: LGTM!
255-314: LGTM!
347-487: LGTM!
491-564: LGTM!
568-687: LGTM!
691-724: LGTM!Also applies to: 837-943
242-251: 🩺 Stability & AvailabilityNo change needed for failed
healthCheck.
NfcManager::pollingTask()enters the reconnection loop whenm_reader->healthCheck()returnsfalseand callsNfcManager::initializeReader()until reconnection succeeds.
Two problems from a single counter, both raised in review. nakRetries was declared once per exchangeApdu() call and never cleared, so the budget was consumed across the whole transaction rather than per recovery. With chaining that is materially worse than before: two recoverable errors while sending the first chunk of a chained command left no recovery at all for any later frame -- and later frames are exactly where errors accumulate. The same counter also doubled as "the frame I am waiting for is a reply to an R(NAK)". It stayed non-zero after a recovery succeeded, leaving the resend-on-R-block branch armed for the rest of the exchange. A subsequent unexpected R-block was then read as an answer to an R(NAK) that had already been resolved, so the driver resent the I-block instead of failing, and could keep doing so until the 8-attempt cap. Those are two different questions, so they are now two variables. awaitingNakReply tracks only what we are waiting for and is cleared as soon as it is acted on; nakRetries is the budget and is reset whenever a frame arrives intact -- on a mid-chain R(ACK) and on a good response I-block. Also aligns the README reader-type value with the string the web UI actually shows, so the documented option can be selected verbatim: ST25R3916 (I2C).
|
Thanks — both requests are done, and I have test results plus one finding you may care about. ISO-DEP chaining
The block number is taken from the PCB the card actually sent rather than assumed, so a chained response leaves the session aligned for the next exchange. Responses are bounded at 4 KB. I also removed an incorrect assumption: FSC bounds what the card is willing to receive. Its own responses are bounded by the FSD we advertise in RATS, so the receive buffer is sized from the frame buffer, not from FSC. Attestation testingForced the attestation flow and tapped repeatedly. Over roughly 8 minutes:
A 723-byte attestation package reassembled from 255 + 255 + 217 across chained reads. Before this change that exchange failed. FAST flow is unchanged and re-verified: 166 ms end to end (SELECT 17 ms, Auth0 112 B with one WTX round 81 ms, control flow 14 ms), lock state propagating to HomeKit and MQTT. A crash in the attestation flow — root-caused, filed separatelyTesting attestation surfaced a real crash, and it turned out not to be in this Short version: The trigger is a clean, correctly-reported timeout on a 4-byte command:
I audited all seven This is reader-independent: any Ruled out along the way: heap (min-ever 56188 bytes free), NFC task stack Review commentsAll CodeRabbit items addressed:
And the two from the round after my push, both correct:
|
|
Thanks for fixing the attestation flow, I’ll review this and will let you know if anything else is needed. |
|
And apologies for the flurry of PRs. I am trying to make this as robust as possible, and so far the results of the patched implementation are very stable. I can trigger an incomplete tap by quickly moving the phone over the sensor, but the esp32 does not crash anymore. |
…or NFC-owned
Two narrow rules in WebServerManager::validateRequest fix the bug where
saving the misc config on an AtomS3 Lite installer (or any board where
NfcManager currently holds ST25R3916 or PN7160 pins) would reject the
whole form with "GPIO 2 for nfcGpioPins already owned by I2C_SDA" even
when the user changed only bootLogKb:
1. If an incoming pin scalar (or array element) equals the currently-
persisted value, skip the ownership check. Nothing new is being
acquired.
2. For an NFC-related pin (nfcIrqPin, nfcVenPin, nfcGpioPins), an
owner that is itself one of NfcManager's own eight lease names
(SPI2_SS/SCK/MISO/MOSI, I2C_SDA/SCL, NFC_IRQ/VEN) is accepted --
the misc-config save always reboots on success, which releases the
old NfcManager's leases before the new one is constructed on the
next boot. Unrelated owners (Ethernet SPI, HardwareManager action
pins, HomeSpan status/control, etc.) still reject.
The decision is factored into a tiny host-testable helper
(main/include/misc_config_nfc_pin_check.hpp, 26 effective code lines)
so the four required cases -- unchanged full form, atomic per-pin
replacement, unrelated-subsystem conflict, ETH_SPI not exempted -- are
covered by six focused tests. Existing type/GPIO validation is
retained; the ethSpiConfig substring-SPI exemption is unchanged (it
governs ETH-vs-ETH SPI swaps and is not reachable for NFC keys).
Host tests: 6 new + 19 existing = 25/25. Both firmware variants build.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d3b04439-df4d-4aa1-9a15-7af81afeb6b8
📒 Files selected for processing (2)
main/WebServerManager.cppmain/include/misc_config_nfc_pin_check.hpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
assess the relevance of the changes to the purpose of the PR and do not mention anything if they are relevant.
Files:
main/include/misc_config_nfc_pin_check.hppmain/WebServerManager.cpp
🔇 Additional comments (1)
main/WebServerManager.cpp (1)
1221-1224: 🎯 Functional CorrectnessNo change needed.
std::string::containsis supported on the ESP-IDF C++ toolchain used by this project.
|
hey, don’t worry about the PRs, I want the same thing as you and I’m aware this isn’t nowhere near a bug-free codebase, so any help is appreciated. As long as a PR is adding something genuinely useful and is well written, it is more than welcomed, however many there may be. |
Conflict: main/main.cpp NfcManager construction Origin/main moved core managers to static storage (readerDataManager is now `ReaderDataManager readerDataManager;` instead of `std::unique_ptr<ReaderDataManager>`) while the PR side extracted the `activeNfcPins` reference so the log line and the NfcManager argument use the same resolved pin array. Resolution: pass readerDataManager by value (origin/main form) and keep activeNfcPins (PR-side cleanup).
|
If you are happy with this PR it should be good to merge. |
Three items raised in review of rednblkx#294. The startup log reported the wrong pins. NfcManager is handed nfcGpioPinsPresets[...].gpioPins whenever a preset is selected, but the ST25R3916 branch logged miscConfig.nfcGpioPins, so a device using a preset printed SDA/SCL that the reader was not using. The pins are now resolved once into activeNfcPins and that same value feeds both the log line and the NfcManager constructor, so the two cannot disagree. The reader type 2 branch accepted unset pins. Type 1 rejects an unset IRQ/VEN pair with a clear message; type 2 passed 255 straight through to i2c_new_master_bus(), which fails with a generic driver argument error that says nothing about configuration. Same explicit check added. README: "M5Stack document" -> "M5Stack documents".
7e24a5e to
90f284c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d3c70ed-1be9-4e39-a738-f2ac3bea4ace
📒 Files selected for processing (9)
README.mdcomponents/DigitalDoorKeymain/HomeKitLock.cppmain/NfcManager.cppmain/ReaderDataManager.cppmain/WebServerManager.cppmain/include/NfcManager.hppmain/include/ReaderDataManager.hppmain/main.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- main/include/NfcManager.hpp
- main/NfcManager.cpp
- main/main.cpp
- main/WebServerManager.cpp
Included review availability: Your plan includes up to 4 reviews per rolling hour; 3 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
assess the relevance of the changes to the purpose of the PR and do not mention anything if they are relevant.
Files:
components/DigitalDoorKeymain/ReaderDataManager.cppmain/include/ReaderDataManager.hppmain/HomeKitLock.cppREADME.md
🪛 LanguageTool
README.md
[grammar] ~72-~72: Ensure spelling is correct
Context: ...Hz is required - M5Stack documents that SoftwareI2C latency is too high > for the chip's RF...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (8)
components/DigitalDoorKey (1)
1-1: LGTM!main/include/ReaderDataManager.hpp (1)
91-97: LGTM!main/ReaderDataManager.cpp (2)
7-8: LGTM!Also applies to: 270-273
307-333: LGTM!main/HomeKitLock.cpp (3)
431-431: LGTM!
575-580: LGTM!
590-625: LGTM!README.md (1)
32-37: LGTM!Also applies to: 42-73, 131-134, 282-284, 307-308, 321-324
e289b38 to
90f284c
Compare
- Delete the single-use header `misc_config_nfc_pin_check.hpp`. - Implement `decideNfcPin()` directly in `WebServerManager.cpp` using boolean returns instead of custom namespaces and enum states. - Utilize `std::any_of` to verify if the incoming pin's owner matches defined NFC leases. - Adapt scalar and array-based pin validation checks in `validateRequest()` to use the simplified helper interface.
- Relocate the M5Stack AtomS3 Lite and Unit NFC (ST25R3916) documentation from `README.md` to `docs/content/prerequisites.md`. - Update the lists of supported NFC reader modules to include the ST25R3916 I2C interface details.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50653b66-f707-459d-88df-fa4224d41756
📒 Files selected for processing (3)
README.mddocs/content/prerequisites.mdmain/WebServerManager.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- main/WebServerManager.cpp
Included review availability: Your plan includes up to 4 reviews per rolling hour; 1 remains after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
assess the relevance of the changes to the purpose of the PR and do not mention anything if they are relevant.
Files:
docs/content/prerequisites.mdREADME.md
🧠 Learnings (1)
📚 Learning: 2026-08-04T03:08:04.811Z
Learnt from: mullender
Repo: rednblkx/HomeKey-ESP32 PR: 298
File: docs/coredump.md:67-69
Timestamp: 2026-08-04T03:08:04.811Z
Learning: When documenting authenticated HTTP endpoints, explain that the `curl -u webUsername:webPassword` option is required only when `webAuthEnabled` is true. When web authentication is disabled, users must omit the Basic Authentication credentials because `WebServerManager::basicAuth()` rejects missing credentials only when authentication is enabled.
Applied to files:
docs/content/prerequisites.md
🪛 LanguageTool
docs/content/prerequisites.md
[grammar] ~142-~142: Ensure spelling is correct
Context: ...Hz is required - M5Stack documents that SoftwareI2C latency is too high > for the chip's RF...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (3)
README.md (2)
38-41: Keep the installation step reader-neutral.The new prerequisite list names PN532, PN7160, and ST25R3916, but Line [78] still tells every user to wire a PN532. Update that step to reference the selected reader or provide reader-specific wiring instructions, including the ST25R3916 I2C SDA/SCL settings.
253-255: LGTM!Also applies to: 278-279, 292-295
docs/content/prerequisites.md (1)
81-83: LGTM!Also applies to: 114-143
|
@mullender did some tweaks, please take a look and let me know if you have any thoughts. i can't test the driver but based on the code and your comments, seems to be alright. the webserver changes are fine for now, i need to overhaul the validation flow for the API anyway so it's good enough to merge. |
|
These changes look like improvements to me. thank you for doing that! |
…es in README - Replace hardware-specific mentions of "PN532" with "NFC module" across the introduction, wiring steps, and Mermaid diagram. - Add notes to the prerequisites list specifying that the PN7160 (SPI) and ST25R3916 (I2C) backends are available in the development release.
- Update the sub-heading under the NFC reader modules section to include PN7161 and ST25R3916. - Format the ST25R3916 list entry to match the bold title style used for other controllers.
|
thanks, all good, will merge. |
2 participants
This PR was created with AI, and reviewed and verified by me before submission.
feat: ST25R3916 (I2C) reader backend + solder-free M5Stack build
Implements
INfcReaderfor the ST25R3916, selectable asnfcReaderType = 2alongside PN532 (0) and PN7160 (1).
Developed against an M5Stack AtomS3 Lite + M5Stack Unit NFC
(ST25R3916-AQWT, SKU U216) connected over Grove I2C. No soldering, no
breadboard, no jumper wires — which is the main reason this is worth having:
it makes a working Home Key reader available for people who don't want to
build a circuit.
What the driver has to do that the PN532 hid
The PN532 implements two layers in its own firmware that a bare transceiver
does not, so both are implemented here in software:
InListPassiveTargetdoes this internally.pollForTag()runs itafter anticollision when SAK indicates ISO-DEP, so a tag handed to the caller
is genuinely ready for
exchangeApdu().InDataExchangedoes this in hardware.exchangeApdu()implements I-block framing, block-number toggling, S(WTX)waiting-time extension and R(NAK) error recovery.
Two silicon behaviours found the hard way
response as SW1 SW2 yields the CRC instead — a SELECT that had actually
succeeded reported
SW=0x5B62.left on, a perfectly good UID+BCC response is flagged as a CRC error.
no_crc_rx(AUX bit0x80) is toggled per exchange.No IRQ pin, and that turns out to be fine
The Grove connector doesn't break out IRQ, so every wait polls the interrupt
status registers over I2C. Measured on hardware:
I_osc)I_rxe)ECP is transmitted with
CMD_TRANSMIT_WITHOUT_CRCplus the software CRC-A thatNfcManageralready appends — no transparent or stream mode required, and noRFAL dependency.
Also in scope, because the reader is unusable without them
NfcManagerno longer claims four SPI pins for an I2C reader. OnlynfcGpioPins[0](SDA) and[1](SCL) are acquired, under new SDA/SCL pinfunctions. Claiming MISO/MOSI reserved two arbitrary GPIOs under misleading
names — which matters on a board like the AtomS3 Lite that exposes only a
handful.
PN7160for anyvalue other than 0, so a correctly configured ST25R3916 reported the wrong
hardware.
fields SDA/SCL and hides MISO/MOSI for it. Without the option the select
rendered blank and saving the page would have written a bogus reader type.
SDA=2, SCL=1, optional NeoPixel on GPIO 35), and the component table updated
to describe three reader backends rather than calling
NfcManagerPN532-specific.
Testing
Verified end to end against a real iPhone: ECP wakeup → anticollision → RATS →
SELECT A0000008580101→9000→ full FAST-flow authentication → lock statepropagated to HomeKit and MQTT.
The backend also has host-side unit tests (33 cases against a fake chip)
covering CRC trimming, block-number toggling, WTX handling and NAK recovery.
They are not included here as the repo has no host test harness — happy to add
them under whatever layout you'd prefer.
Clean build on ESP-IDF v5.5.4 (gcc 14.2.0), esp32s3 target.
ISO-DEP chaining
Commands and responses larger than the card's FSC are split and reassembled:
bit on all but the last, waits for R(ACK) between chunks and toggles the
block number per chunk.
continuation with R(ACK) carrying the received block number toggled, per
ISO 14443-4. S(WTX) is handled inside the chain, not only around it.
The block number is taken from the PCB the card actually sent rather than
assumed, so a chained response leaves the session aligned. Responses are
bounded at 4 KB.
FSC bounds what the card is willing to receive; its own responses are bounded
by the FSD advertised in RATS, so the receive buffer is sized from the frame
buffer rather than from FSC.
Exercised via the attestation flow on hardware: a 723-byte attestation package
reassembled from 255 + 255 + 217 bytes across chained reads.
20 successful attestation authentications over the test run.
Known limitations
NVB=0x20only, so two devices in the field atonce are not resolved.
Summary by CodeRabbit
New Features
Documentation