DEV-1021 Classify MAX86xxx PPG LED-test failures instead of raising a raw NACK - #142
Merged
Merged
Conversation
… raw NACK
Verisense firmware b98c113c3 (DEV-973) made the MAX86xxx LED-test debug command
NACK when it cannot reach the PPG chip, where it previously ACKed
unconditionally. max86xxxLedTest() passed that refusal straight through as
"Device returned NACK command=0x70 property=0x9", which is the wrong shape of
error for this particular command: the LED test is judged by an operator looking
at the board, so a unit whose PPG bus is wedged lights nothing and an
unclassified refusal reads as "PPG LEDs dead" - scrapping a good board.
The NACK is ambiguous on the wire and cannot be made otherwise. The firmware's
debug dispatch guards the MAX86xxx branch with doesHwSupportPpg(), and the else
catching unrecognised debug commands calls the same sendNackGeneric()
(asm_payload_parse.c), so three causes produce a byte-identical NACK_GENERIC on
property 0x09: firmware too old to know command 0x0E, hardware with no PPG front
end, and the new one - a wedged PPG bus. The only usable discriminator is the
hardware revision the host already holds from the production config.
- New devices/verisense/ppgLedTest.ts: VerisensePpgLedTestError carrying a
machine-readable reason ('ppg-comms' | 'not-supported' | 'no-response' |
'unknown'), an operator-facing message, and the hardware's PPG support.
Follows the existing FactoryTestError pattern.
- max86xxxLedTest() re-throws every failure classified.
- An unknown hardware revision resolves to 'ppg-comms', not 'not-supported'.
The two misreadings are not symmetric: calling a comms fault "unsupported" is
what scraps a good board, while the reverse only puzzles an operator holding
one with no PPG. operatorMessage says the revision could not be established.
- The lookup reads the cached production config and never reads from the device:
it runs on a failure path, where a second round trip can turn a classified
failure into a timeout.
Consumers still need to act on this - a caller that ignores the reason is no
better off than before. verisense-device-console is handled separately under
DEV-1021; the Java driver / ASM_PC path the ticket also names is not.
Verified: 16 new unit tests; full suite 1877 passing; typecheck, eslint and
prettier clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
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
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
Verisense firmware
b98c113c3(DEV-973, now onverisense-firmwaremain) made the MAX86xxx LED-test debug command NACK when it cannot reach the PPG chip, where it previously ACKed unconditionally.max86xxxLedTest()passed that refusal straight through asDevice returned NACK command=0x70 property=0x9. That is the wrong shape of error for this particular command, because the LED test is judged by an operator looking at the board: a unit whose PPG bus is wedged lights nothing, so an unclassified refusal reads as "PPG LEDs dead" — and a good board gets scrapped for a fault it does not have.The constraint: the NACK is ambiguous on the wire
This is the part worth reviewing carefully, because it cannot be fixed in the SDK.
The firmware's debug dispatch guards the MAX86xxx branch with
doesHwSupportPpg(), and theelsecatching unrecognised debug commands calls the samesendNackGeneric()(asm_payload_parse.c). So three different causes produce a byte-identicalNACK_GENERICon property0x09:0x0E;max86xxx_ledTest()returned non-success, i.e. a wedged or unreachable PPG bus.Nothing in the reply separates them. The only usable discriminator is the hardware revision the host already holds from the production config.
What changed
src/devices/verisense/ppgLedTest.ts—VerisensePpgLedTestErrorcarrying a machine-readablereason('ppg-comms' | 'not-supported' | 'no-response' | 'unknown'), an operator-facing message, and the hardware's PPG support. Follows the existingFactoryTestErrorpattern.max86xxxLedTest()re-throws every failure classified.VerisensePpgLedTestError,isVerisensePpgLedTestError,classifyPpgLedTestFailure,resolveHardwarePpgSupport,VerisensePpgLedTestFailureReason.Two judgement calls worth a look:
'ppg-comms', not'not-supported'. The two misreadings are not symmetric: calling a comms fault "unsupported" is what scraps a good board, while the reverse only puzzles an operator holding one with no PPG.operatorMessagesays the revision could not be established.Scope
This is the SDK half only. A caller that ignores the
reasonis no better off than before.verisense-device-consolefollows in its own PR once this merges andvendor/is re-synced. It has a related live bug: itsisDebugHeaderNackError()matches any NACK on the debug property, so today a wedged PPG bus is logged as "unavailable on this firmware" with no toast at all.Verification
tests/verisense/ppg-led-test.test.tsnpm run typecheck,npm run lint, prettier — clean🤖 Generated with Claude Code