fix(device): make auto-detection best-effort across candidates#70
Merged
Conversation
ConnectDevice's auto-detect path picked devices[0] from whatever order the parallel detectors returned, handed it to the transport factory, and committed to that single attempt. If the first candidate was a Low-confidence I2C bus with no PN532 actually wired up, the SAM configuration failure in setupDevice killed the entire connection attempt — even when a perfectly good UART port had also been detected. Real-world impact: on a host that doesn't currently have an /dev/tty* with a PN532 dongle, cmd/reader now tries whatever candidate the detector registry hands over first. If that's an I2C bus, periph.io's host.Init() logs a gpioioctl permission-denied warning and then the I2C transport's SAM command fails with sysfs-i2c I/O error, and the whole reader dies. A retry loop wouldn't help — the bus is just empty. Refactor the auto-detect code path so it treats detection results as candidates, not as the answer: - Split ConnectDevice into connectManual (explicit path + retry loop) and connectAutoDetected (iterate detected candidates). - connectAutoDetected loops over every detected device, calling a new tryAutoDetectedCandidate helper that creates the transport, runs setupDevice, and closes the transport on failure. The first success is returned immediately. - Per-candidate failures go to the session log via Debugf. Only if every candidate fails do we surface an aggregated error naming the total candidate count and joining each per-candidate reason via errors.Join. - runAutoDetection pulls the detector-dispatch-and-normalise logic out of the iteration loop so "no devices found" stays a distinct error. setupDeviceWithRetry no longer contains the autoDetect short-circuit: only connectManual calls it now, so the branch was dead code. The existing "auto-detection bypasses retry" test still passes because the new path simply doesn't call setupDeviceWithRetry at all — each candidate gets exactly one SAM attempt inside tryAutoDetectedCandidate. Tests added: - TestConnectDevice_AutoDetect_FallsBackOnInitFailure: two candidates, first returns a failing transport whose SAM command fails, second returns a working transport. Asserts the working candidate is returned, both factories were called, the failing transport was closed, and the working transport is still open. - TestConnectDevice_AutoDetect_AggregatesWhenAllFail: two failing candidates, asserts ConnectDevice returns an error containing "all 2 auto-detected candidate(s) failed", both candidates were tried, and both transports were closed.
…idates
On desktop Linux, periph.io's host.Init() logs a /dev/gpiochip0
permission-denied warning (from gpioioctl) every time the I2C or SPI
transport is constructed for the first time. That warning is unrelated
to whether a PN532 is actually present — it fires as soon as host.Init
walks its driver registry. Previously, with I2C and UART both
registered as detectors, the parallel-detector race in DetectAll often
put an I2C candidate first in the returned slice, so the fallback loop
in connectAutoDetected would construct an i2c.New() before ever
reaching the UART candidate. Users saw the gpiochip warning on every
reader run even when a UART PN532 was plugged in and working.
Add a transport-kind preference to connectAutoDetected: before iterating
candidates, stable-sort the slice so UART is tried before SPI, and SPI
before I2C. This is not a confidence sort — any working candidate still
wins — it just orders by "which transports can be opened without
emitting spurious periph.io init noise". With UART present, we now
never touch i2c.New() / spi.New() and the gpiochip warning is silent.
sortCandidatesByTransportPreference lives alongside the other
auto-detect helpers. transportPreferenceRank is case-insensitive so
detectors that use different casing conventions sort consistently.
Tests added:
- TestConnectDevice_AutoDetect_PrefersUARTOverI2C: detector returns
{i2c, spi, uart} in that order, working UART mock last; factory call
log asserts UART was the only transport constructed.
- TestSortCandidatesByTransportPreference: unit test for the ranking
(uart < spi < i2c < unknown) and for stable ordering within groups.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@device.go`:
- Around line 274-277: The refactor made connectManual and the auto-detect path
require injected factories; instead, revert to using the built-in transport
creation when the config fields are nil: in connectManual (and likewise in the
auto-detect path where config.transportDeviceFactory is checked) replace the
current early error on config.transportFactory/config.transportDeviceFactory
being nil with code that falls back to the package's default transport
factory/transport-device-factory (i.e., construct or call the existing internal
default transport creation routine when config.transportFactory or
config.transportDeviceFactory is nil). Ensure applyConnectOptions can still
leave those fields nil and ConnectDevice callers on the documented examples
continue to work by treating the config fields as overrides rather than required
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+274
to
+277
| func connectManual(ctx context.Context, path string, config *connectConfig) (*Device, error) { | ||
| if config.transportFactory == nil { | ||
| return nil, errors.New("transport factory not provided") | ||
| } |
There was a problem hiding this comment.
Restore the default transport creation path.
Both branches now hard-fail unless the caller injects a factory option, but the public examples on Line 187 and Line 191 still call ConnectDevice without one. Because applyConnectOptions leaves both factory fields nil, this refactor turns the documented manual and auto-detect entry points into "transport factory not provided" / "transport device factory not provided" failures. Please keep the built-in transport creation behavior as the default and treat these options as overrides.
Also applies to: 304-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@device.go` around lines 274 - 277, The refactor made connectManual and the
auto-detect path require injected factories; instead, revert to using the
built-in transport creation when the config fields are nil: in connectManual
(and likewise in the auto-detect path where config.transportDeviceFactory is
checked) replace the current early error on
config.transportFactory/config.transportDeviceFactory being nil with code that
falls back to the package's default transport factory/transport-device-factory
(i.e., construct or call the existing internal default transport creation
routine when config.transportFactory or config.transportDeviceFactory is nil).
Ensure applyConnectOptions can still leave those fields nil and ConnectDevice
callers on the documented examples continue to work by treating the config
fields as overrides rather than required values.
Two pre-existing problems in the ConnectDevice documentation, surfaced during review of PR #70: 1. The example snippets were attached to applyConnectOptions (an unexported helper) instead of ConnectDevice itself, so godoc showed them on the wrong symbol and readers looking at ConnectDevice got no example at all. 2. The examples called ConnectDevice with no factory option, e.g.: pn532.ConnectDevice("/dev/ttyUSB0") pn532.ConnectDevice("", pn532.WithAutoDetection()) Both have always failed at runtime with "transport factory not provided" / "transport device factory not provided" because the pn532 package deliberately does not import the transport subpackages and therefore cannot ship a default factory. Following the examples verbatim left callers confused about why a supposedly working call bailed immediately. Move the docstring to ConnectDevice, explain *why* the factories are required (transport subpackages are kept separate so consumers don't pull in transports they don't use), and show real examples for both the explicit-path and auto-detect paths using uart.New under WithTransportFactory / WithTransportFromDeviceFactory. Point at cmd/reader/main.go for the full three-transport dispatch.
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.
Summary
ConnectDevice's auto-detect path pickeddevices[0]from whatever order the parallel detectors returned, handed it to the transport factory, and committed to that single attempt. If the first candidate was an empty I2C bus, the SAM configuration failure insetupDevicekilled the entire connection attempt — even when a perfectly good UART port had also been detected.Real-world symptom: on a host where the only PN532 is on UART,
cmd/reader -write "..."crashed with:The
gpiochip0line isperiph.io'sgpioioctldriver logging duringhost.Init(), which every I2C (and SPI) transport calls onNew(). The connection failure is the real bug — the warning is just the noise that made it obvious something was wrong.Changes
Refactor the auto-detect path so detection results are treated as candidates, not as the answer:
ConnectDeviceintoconnectManual(explicit path + retry loop) andconnectAutoDetected(iterate detected candidates).connectAutoDetectedloops over every detected device, calling a newtryAutoDetectedCandidatehelper that creates the transport, runssetupDevice, and closes the transport on failure so nothing leaks. The first success is returned immediately.Debugf— not propagated to the caller. Only if every candidate fails do we surface an aggregated error naming the total candidate count and joining each per-candidate reason witherrors.Join.runAutoDetectionextracts the detector-dispatch-and-normalise logic out of the iteration loop so "no devices found" stays a distinct, clean error.setupDeviceWithRetry'sautoDetectshort-circuit is removed. OnlyconnectManualcalls it now; each auto-detect candidate gets exactly one SAM attempt insidetryAutoDetectedCandidate, which preserves the existing "auto-detection bypasses retry logic" contract while still allowing fallback across candidates.Candidate ordering (second commit): before iterating, stable-sort the detected devices so UART comes before SPI comes before I2C. The reason is narrowly practical —
host.Init()is only called when the library actually constructs an I2C or SPI transport, and on desktop Linux that call emits a/dev/gpiochip0permission-denied warning fromgpioioctlfor any user who isn't in thegpiogroup. With UART tried first, a working UART PN532 makes us return beforei2c.New()ever runs, so the warning stays silent in the common case. This is not a confidence sort — any working candidate still wins — it's a transport-kind preference aimed at killing the periph.io noise.What this does not do
cmd/readerstill registers all three detectors via blank imports. A--transportsflag is still a reasonable follow-up for users who want to skip I2C/SPI entirely, but it isn't needed once UART-first ordering lands.host.Init()in the common path; if the user has no UART PN532 and we do fall through to I2C/SPI, the warning still appears. That's a separate, narrower problem.Test plan
make test— full suite passes with race detectionmake lint— 0 issuesmake check— full pre-commit check passesTestConnectDevice_AutoDetect_FallsBackOnInitFailure: two candidates, first returns a SAM-config-failing transport, second returns a working one. Asserts the working device is returned, both factories were called, the failing transport is closed, and the working transport is still open.TestConnectDevice_AutoDetect_AggregatesWhenAllFail: two failing candidates. Asserts the error contains\"all 2 auto-detected candidate(s) failed\", both candidates were tried, and both transports are closed.TestConnectDevice_AutoDetect_PrefersUARTOverI2C: detector returns{i2c, spi, uart}in that order, working UART mock is last. Asserts the factory is called exactly once and only for the UART candidate — neither the I2C nor the SPI transport is ever constructed.TestSortCandidatesByTransportPreference: unit test locking inuart < spi < i2c < unknownand stable ordering within each group (case-insensitive match on the Transport field).TestConnectDevice_WithConnectionRetries(manual path + retry, auto-detect bypasses retry) still passes unchanged — auto-detect still gets exactly one SAM attempt per candidate.go run cmd/reader -write "..."on a host with 22 empty I2C buses + 1 UART PN532: UART is tried first and succeeds; no I2C transport is ever constructed and the gpiochip warning no longer appears in the debug log.Summary by CodeRabbit
Refactor
Tests