feat(readers): forward go-pn532 debug output to Core log - #1107
Merged
Conversation
App-written NTAG213 tags (#1098, #1106) read on Core as a validly-formatted tag with an empty NDEF message (messageNil=false, zero records = an NDEF TLV of length 0). go-pn532 already prints the raw bytes it reads from the tag via Debugf, but Core never captured that output, so the decisive bytes never reached the log. Add a zerolog adapter in the PN532 reader that forwards go-pn532's debug lines to log.Debug(), installed once when the existing debug_logging config is on. Pairs with go-pn532's new SetDebugWriter hook. Includes temporary local wiring to build against a sibling go-pn532 checkout (go.mod replace + docker mount); see PR description for removal.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PN532 dependency is updated to v0.23.0. Reader initialization conditionally routes PN532 debug output through Core’s zerolog logger, installing the writer once and testing line handling and repeated installation behavior. ChangesPN532 debug logging
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Drops the local ../go-pn532 replace directive and the sibling-checkout mount in the Docker build task, both of which existed only because SetDebugWriter was unreleased.
wizzomafizzo
marked this pull request as ready for review
July 25, 2026 12:06
wizzomafizzo
changed the title
Member
Author
|
@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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/readers/pn532/debug.go`:
- Around line 22-58: Add tests for zerologDebugWriter.Write covering newline
splitting, blank-line filtering, CRLF trimming, and fragmented writes while
verifying each non-empty line is logged at debug level with the pn532 source.
Add a test for enablePN532DebugLogging confirming pn532.SetDebugWriter is
installed only once, using isolated sync.Once state or equivalent test setup.
- Around line 40-48: Update zerologDebugWriter.Write to buffer trailing partial
PN532 debug lines across calls, emitting only complete newline-terminated lines
while preserving the existing trimming, empty-line filtering, and return values.
Add a test covering a message split across multiple Write calls and verify it
logs as one complete line.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e782c8e-6fef-4799-8316-73fb9bcb1dd0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modpkg/readers/pn532/debug.gopkg/readers/pn532/pn532.go
Comment on lines
+22
to
+58
| import ( | ||
| "strings" | ||
| "sync" | ||
|
|
||
| pn532 "github.com/ZaparooProject/go-pn532" | ||
| "github.com/rs/zerolog/log" | ||
| ) | ||
|
|
||
| // debugWriterOnce ensures the go-pn532 debug writer is only installed once, | ||
| // even if multiple PN532 readers are opened. | ||
| var debugWriterOnce sync.Once | ||
|
|
||
| // zerologDebugWriter forwards go-pn532's always-on debug output into Core's | ||
| // zerolog logger at debug level. go-pn532 writes the exact bytes it reads from | ||
| // tags (e.g. "NTAG readNDEFHeader: user data = [...]") through this path, which | ||
| // is otherwise never captured because Core creates no session log file. | ||
| type zerologDebugWriter struct{} | ||
|
|
||
| func (zerologDebugWriter) Write(p []byte) (int, error) { | ||
| for line := range strings.Lines(string(p)) { | ||
| line = strings.TrimRight(line, "\r\n") | ||
| if line == "" { | ||
| continue | ||
| } | ||
| log.Debug().Str("src", "pn532").Msg(line) | ||
| } | ||
| return len(p), nil | ||
| } | ||
|
|
||
| // enablePN532DebugLogging routes go-pn532 debug output into Core's logger when | ||
| // debug logging is enabled in the config. This is how raw NDEF/TLV byte dumps | ||
| // reach Core's log for diagnosing tag read failures. | ||
| func enablePN532DebugLogging() { | ||
| debugWriterOnce.Do(func() { | ||
| pn532.SetDebugWriter(zerologDebugWriter{}) | ||
| }) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add tests for the new PN532 logging code.
Cover line splitting, blank-line filtering, CRLF handling, fragmented writes, and the once-only installation behavior. As per coding guidelines, **/*.go: Write tests for all new code — see TESTING.md and pkg/testing/README.md.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/readers/pn532/debug.go` around lines 22 - 58, Add tests for
zerologDebugWriter.Write covering newline splitting, blank-line filtering, CRLF
trimming, and fragmented writes while verifying each non-empty line is logged at
debug level with the pn532 source. Add a test for enablePN532DebugLogging
confirming pn532.SetDebugWriter is installed only once, using isolated sync.Once
state or equivalent test setup.
Source: Coding guidelines
Comment on lines
+40
to
+48
| func (zerologDebugWriter) Write(p []byte) (int, error) { | ||
| for line := range strings.Lines(string(p)) { | ||
| line = strings.TrimRight(line, "\r\n") | ||
| if line == "" { | ||
| continue | ||
| } | ||
| log.Debug().Str("src", "pn532").Msg(line) | ||
| } | ||
| return len(p), nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target file ==\n'
git ls-files 'pkg/readers/pn532/*'
printf '\n== outline/debug.go ==\n'
ast-grep outline pkg/readers/pn532/debug.go --view expanded || true
printf '\n== show debug.go ==\n'
cat -n pkg/readers/pn532/debug.go
printf '\n== search pn532 tests/references ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'pn532|strings\.Lines|enablePN532DebugLogging|zerologDebugWriter' .
printf '\n== go version hints ==\n'
rg -n --hidden --glob 'go.mod' '^go [0-9]+\.[0-9]+' .Repository: ZaparooProject/zaparoo-core
Length of output: 37788
Buffer partial PN532 debug lines. Write can receive fragments of one logical message, so this will log truncated lines when a dump is split across calls. Keep the trailing partial line until the next write/newline, and add a split-write test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/readers/pn532/debug.go` around lines 40 - 48, Update
zerologDebugWriter.Write to buffer trailing partial PN532 debug lines across
calls, emitting only complete newline-terminated lines while preserving the
existing trimming, empty-line filtering, and return values. Add a test covering
a message split across multiple Write calls and verify it logs as one complete
line.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Tests line splitting, blank-line filtering, CRLF trimming, unterminated chunks, and the io.Writer return contract, plus that repeat calls to enablePN532DebugLogging do not reinstall the writer. Also records why Write does not buffer across calls: go-pn532 emits each message in one Fprintf, so a partial line never arrives.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
App-written NTAG213 tags don't launch on Core (#1098, #1106). Across the attached logs the failing
scans show a validly-formatted NTAG213 (valid Capability Container →
variant=NTAG213,hasNDEF=true) whose NDEF message is empty:messageNil=falsewith zero records, which is emittedonly when the NDEF TLV has a length byte of 0 (
03 00). The record the app intended to write is notwhere Core reads it.
go-pn532 already prints the exact bytes it reads from a tag (
readNDEFHeader: user data = [...]) viaDebugf, but Core never wired that output anywhere, so the decisive bytes are absent from every log.This PR captures them.
What
pkg/readers/pn532/debug.go: azerologDebugWriterthat forwards each go-pn532 debug line tolog.Debug().Str("src","pn532"), installed once viasync.Once.pkg/readers/pn532/pn532.go:Openinstalls it when the existingdebug_loggingconfig isenabled. No new config key.
go.mod: go-pn532v0.22.1→v0.23.0, which adds theSetDebugWriter(io.Writer)hook thiscalls (feat: add embeddable debug writer go-pn532#96).
With
debug_loggingon, a failing app-written tag and a working Core-written tag can be scanned andtheir byte dumps diffed to decide write-side vs read-side.
The branch previously built against a local sibling
../go-pn532checkout via areplacedirectiveand a matching bind-mount in
scripts/tasks/docker.yml. Both are gone now thatSetDebugWriterisreleased.
Scope
This adds the capture path only — it changes no read or write behaviour, and with
debug_loggingoff it costs nothing (the writer is never installed). It does not itself fix #1098 or #1106.
Diagnosing those still needs an on-device scan: a physically app-written failing tag read on the
MiSTer with
debug_loggingon. If the captured bytes show a genuine empty NDEF (03 00), the fix isapp-side (write read-back verification); if they show a valid record Core misreads, a read-side fix
lands in go-pn532.
Summary by CodeRabbit
New Features
Bug Fixes
Tests