From c322504e10f322fa57f3c8689a7590528c201626 Mon Sep 17 00:00:00 2001 From: Mark Nolan Date: Tue, 15 Sep 2026 16:26:20 +0100 Subject: [PATCH] DEV-1021 Classify MAX86xxx PPG LED-test failures instead of raising a 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 --- CHANGELOG.md | 8 + src/devices/verisense/VerisenseClient.ts | 40 ++++- src/devices/verisense/ppgLedTest.ts | 180 +++++++++++++++++++++++ src/index.ts | 7 + tests/verisense/ppg-led-test.test.ts | 134 +++++++++++++++++ 5 files changed, 368 insertions(+), 1 deletion(-) create mode 100644 src/devices/verisense/ppgLedTest.ts create mode 100644 tests/verisense/ppg-led-test.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9884526..392bca6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,6 +102,14 @@ This project follows [Semantic Versioning](https://semver.org/). ### Changed +- **`max86xxxLedTest()` now rejects with a classified `VerisensePpgLedTestError`** instead of the raw transport error, and `classifyPpgLedTestFailure`, `resolveHardwarePpgSupport`, `isVerisensePpgLedTestError` and the `VerisensePpgLedTestFailureReason` type are exported alongside it (DEV-1021). + + Verisense firmware `b98c113c3` (DEV-973) made this debug command NACK when it cannot reach the PPG chip, where it previously ACKed unconditionally. That matters more than a routine error would, 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. + + **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 firmware too old to know command `0x0E`, hardware carrying no PPG front end, and a wedged PPG bus all produce a byte-identical `NACK_GENERIC` on property `0x09`. Nothing in the reply separates them. The only usable discriminator is the hardware revision the host already holds from the production config, and that is what `reason` is derived from: `'ppg-comms'` on hardware known to carry a MAX86xxx, `'not-supported'` on hardware known not to, plus `'no-response'` for a timeout and `'unknown'` for anything else. + + An **unknown** 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 merely puzzles an operator holding one that has no PPG — so this fails loud and says in `operatorMessage` that the revision could not be established. For the same reason the lookup reads the client's 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. + - **`'uSiemens'` is now `'uS'`**, matching Java's `U_SIEMENS`. It appears in the streamed GSR field's unit and in the SD-log channel table; the only consumer is a CSV units row, and none of the demos parsed it. - **Raw fields carry `'no_units'` rather than `null`.** A units row with an empty cell reads as "the unit was not recorded"; this reads as "there is no unit", and those are different facts about a column. Java makes the same distinction with the same word. `TIMESTAMP` keeps `'ticks'`. diff --git a/src/devices/verisense/VerisenseClient.ts b/src/devices/verisense/VerisenseClient.ts index 9e52fbb..a93ef1e 100644 --- a/src/devices/verisense/VerisenseClient.ts +++ b/src/devices/verisense/VerisenseClient.ts @@ -25,6 +25,7 @@ import { SC_GLOBAL_HEADER_BYTES, type CalibrationSet, } from './calibration.js'; +import { classifyPpgLedTestFailure, resolveHardwarePpgSupport } from './ppgLedTest.js'; import { buildHeader, buildMessage, @@ -1972,8 +1973,45 @@ export class VerisenseBleDevice extends BaseShimmerClient { await this.sendDebugCommand(DEBUG_COMMAND_ID.LED_TEST, [ledIndex & 0xff]); } + /** + * Run the MAX86xxx PPG LED test — `start` lights the PPG LEDs, `!start` + * turns them back off. + * + * Since DEV-973 (firmware commit `b98c113c3`) the device NACKs this command + * when it cannot talk to the PPG chip, where it previously ACKed + * unconditionally. A rejection therefore no longer means "unsupported": on + * hardware known to carry a MAX86xxx it means the PPG bus is wedged, and the + * LEDs are unlit *because the test never ran*. Callers must not present that + * to an operator as a dead-LED fault — see {@link classifyPpgLedTestFailure} + * for why the NACK cannot be disambiguated from the reply alone. + * + * @throws {@link VerisensePpgLedTestError} tagged with a `reason` — every + * failure of this command is re-thrown classified. + */ async max86xxxLedTest(start: boolean): Promise { - await this.sendDebugCommand(DEBUG_COMMAND_ID.MAX86XXX_LED_TEST, [start ? 0x01 : 0x00]); + try { + await this.sendDebugCommand(DEBUG_COMMAND_ID.MAX86XXX_LED_TEST, [start ? 0x01 : 0x00]); + } catch (e) { + throw classifyPpgLedTestFailure(e, { + hardwarePpgSupport: this._cachedHardwarePpgSupport(), + }); + } + } + + /** + * PPG support of the connected hardware, from the production config already + * cached on this client. Deliberately does not read from the device: this is + * called on a failure path where the unit may be in a bad state, and a + * second round-trip could turn one classified failure into a timeout. + */ + private _cachedHardwarePpgSupport(): boolean | null { + const blob = this.productionConfig; + if (!blob?.length || this._isErasedBlob(blob)) return null; + try { + return resolveHardwarePpgSupport(parseProductionConfigPayload(blob)); + } catch { + return null; + } } async startPowerProfilerTest(): Promise { diff --git a/src/devices/verisense/ppgLedTest.ts b/src/devices/verisense/ppgLedTest.ts new file mode 100644 index 0000000..fd511df --- /dev/null +++ b/src/devices/verisense/ppgLedTest.ts @@ -0,0 +1,180 @@ +import { getVerisenseHardwareSensorSupport } from './hardwareModels.js'; + +/** + * Why the MAX86xxx PPG LED test did not run. + * + * The distinction that matters on a factory line is `ppg-comms` versus + * everything else: the LED test is judged by an operator looking at the board, + * so a unit whose PPG bus is wedged lights no LEDs and reads as "PPG LEDs + * dead". That misdiagnosis scraps a good board, which is what DEV-973 changed + * the firmware to prevent and what this classification surfaces to the host. + */ +export type VerisensePpgLedTestFailureReason = + /** + * The device refused the command and the connected hardware is known to + * carry a MAX86xxx, so the firmware reached the LED test and it failed — + * a PPG comms failure (wedged or unreachable PPG I2C bus). + * + * Also used when the hardware revision is unknown: failing loud is the safe + * direction here, because reporting "unsupported" on a real comms fault is + * what leads to a good board being scrapped. + */ + | 'ppg-comms' + /** + * The device refused the command and the connected hardware carries no PPG + * front end, so the firmware never reached the LED test. Not a unit fault. + */ + | 'not-supported' + /** No reply within the command timeout — link problem, not a PPG verdict. */ + | 'no-response' + /** Anything else the transport raised. */ + | 'unknown'; + +/** + * A MAX86xxx PPG LED-test failure, tagged with a machine-readable reason. + * + * Mirrors the {@link FactoryTestError} pattern: callers switch on `reason` + * rather than pattern-matching a message string. + */ +export class VerisensePpgLedTestError extends Error { + readonly reason: VerisensePpgLedTestFailureReason; + /** + * Whether the connected hardware is known to carry a PPG front end: + * `true`/`false` when the production config was readable, `null` when the + * hardware revision could not be established. + */ + readonly hardwarePpgSupport: boolean | null; + /** Operator-facing wording, safe to put straight into a toast or a log. */ + readonly operatorMessage: string; + /** The underlying transport error, when there was one. */ + readonly cause?: unknown; + + constructor( + reason: VerisensePpgLedTestFailureReason, + operatorMessage: string, + hardwarePpgSupport: boolean | null, + cause?: unknown, + ) { + super(operatorMessage); + this.name = 'VerisensePpgLedTestError'; + this.reason = reason; + this.hardwarePpgSupport = hardwarePpgSupport; + this.operatorMessage = operatorMessage; + this.cause = cause; + } +} + +/** Type guard for {@link VerisensePpgLedTestError}. */ +export function isVerisensePpgLedTestError(e: unknown): e is VerisensePpgLedTestError { + return e instanceof VerisensePpgLedTestError; +} + +/** + * Whether an error raised by the command path is the device NACKing a debug + * command. Matches the three NACK opcodes (0x50 bad-header-command, 0x60 + * bad-header-property, 0x70 generic) on the DEBUG_COMMAND property (0x9), + * which is how `validatePendingResponse` renders a refusal. + */ +function isDebugNackError(message: string): boolean { + return /NACK command=0x(?:50|60|70) property=0x9/i.test(message); +} + +/** Whether the error is the command path's own timeout. */ +function isTimeoutError(message: string): boolean { + return /Request timeout/i.test(message); +} + +/** + * Classify a failure of the MAX86xxx LED-test debug command (0x0E). + * + * **The NACK is ambiguous on the wire.** In the firmware's debug dispatch + * (`asm_payload_parse.c`) the MAX86xxx branch is guarded by + * `doesHwSupportPpg()`, and the `else` that catches unrecognised debug + * commands calls the same `sendNackGeneric()`. So after DEV-973 (commit + * `b98c113c3`) three different causes produce a byte-identical + * `NACK_GENERIC` on property `0x09`: + * + * 1. firmware too old to know debug command `0x0E`; + * 2. hardware with no PPG front end (`doesHwSupportPpg()` false); + * 3. the new one — `max86xxx_ledTest()` returned non-success, i.e. the PPG + * bus is wedged or unreachable. + * + * Nothing in the reply separates them, so the only usable discriminator is + * the hardware revision the host already holds from the production config. + * Known-PPG hardware reaching a NACK means the firmware got as far as the + * LED test and it failed; known-no-PPG hardware means it never did. + * + * @param err the error the command path raised + * @param opts.hardwarePpgSupport + * `true`/`false` from {@link getVerisenseHardwareSensorSupport}, or + * `null` when the hardware revision is unknown + */ +export function classifyPpgLedTestFailure( + err: unknown, + opts: { hardwarePpgSupport: boolean | null }, +): VerisensePpgLedTestError { + const { hardwarePpgSupport } = opts; + const message = err instanceof Error ? err.message : String(err); + + if (isDebugNackError(message)) { + if (hardwarePpgSupport === false) { + return new VerisensePpgLedTestError( + 'not-supported', + 'PPG LED test refused: this hardware revision has no PPG front end, so the ' + + 'firmware never ran the test. Not a unit fault.', + hardwarePpgSupport, + err, + ); + } + + const hardwareCaveat = + hardwarePpgSupport === null + ? ' (hardware revision unknown — read the production config to rule out ' + + 'a board with no PPG front end, or firmware too old to support this command)' + : ''; + + return new VerisensePpgLedTestError( + 'ppg-comms', + 'PPG LED test FAILED: the device refused the command — PPG comms failure ' + + '(wedged or unreachable PPG bus). The LEDs themselves are NOT known to be ' + + `dead; do not scrap this board as a dead-LED fault${hardwareCaveat}.`, + hardwarePpgSupport, + err, + ); + } + + if (isTimeoutError(message)) { + return new VerisensePpgLedTestError( + 'no-response', + `PPG LED test inconclusive: no reply from the device (${message}). This is a ` + + 'link problem, not a verdict on the PPG LEDs.', + hardwarePpgSupport, + err, + ); + } + + return new VerisensePpgLedTestError( + 'unknown', + `PPG LED test failed: ${message}`, + hardwarePpgSupport, + err, + ); +} + +/** + * Resolve whether a parsed production config describes hardware with a PPG + * front end. Returns `null` when the revision cannot be established (config + * erased, unreadable, or non-numeric fields), which + * {@link classifyPpgLedTestFailure} treats as "assume a comms fault". + */ +export function resolveHardwarePpgSupport( + parsed: { revHwMajor?: number | null; revHwMinor?: number | null } | null | undefined, +): boolean | null { + const major = Number(parsed?.revHwMajor); + const minor = Number(parsed?.revHwMinor); + if (!Number.isFinite(major) || !Number.isFinite(minor)) return null; + // An erased production config reads back as 0xFF bytes; that is not a + // hardware revision, it is an unprogrammed unit. + if (major === 0xff || major <= 0) return null; + return getVerisenseHardwareSensorSupport(major, minor).ppg; +} diff --git a/src/index.ts b/src/index.ts index 2771e44..cb812ff 100644 --- a/src/index.ts +++ b/src/index.ts @@ -193,6 +193,13 @@ export type { FactoryTestRunOptions, AckVerdict, } from './devices/factoryTest/capture.js'; +export { + VerisensePpgLedTestError, + isVerisensePpgLedTestError, + classifyPpgLedTestFailure, + resolveHardwarePpgSupport, +} from './devices/verisense/ppgLedTest.js'; +export type { VerisensePpgLedTestFailureReason } from './devices/verisense/ppgLedTest.js'; // EEPROM brand (advertising name) record — shared by Shimmer3/Shimmer3R over // BLE/BT (readDaughterCardMem) and the dock UART / USB-C (CARD_MEM) diff --git a/tests/verisense/ppg-led-test.test.ts b/tests/verisense/ppg-led-test.test.ts new file mode 100644 index 0000000..8163dc8 --- /dev/null +++ b/tests/verisense/ppg-led-test.test.ts @@ -0,0 +1,134 @@ +import { describe, it, expect } from 'vitest'; +import { + classifyPpgLedTestFailure, + isVerisensePpgLedTestError, + resolveHardwarePpgSupport, + VerisensePpgLedTestError, +} from '../../src/devices/verisense/ppgLedTest.js'; + +/** + * The exact string `validatePendingResponse` builds for a refusal — the only + * thing the classifier has to work with, because the firmware's debug dispatch + * NACKs an unknown command, a board with no PPG and a wedged PPG bus through + * the same `sendNackGeneric()` call (DEV-973 / DEV-1021). + */ +const NACK_GENERIC_DEBUG = new Error('Device returned NACK command=0x70 property=0x9'); +const TIMEOUT = new Error('Request timeout'); + +describe('classifyPpgLedTestFailure', () => { + it('reads a NACK on known-PPG hardware as a PPG comms failure', () => { + const err = classifyPpgLedTestFailure(NACK_GENERIC_DEBUG, { hardwarePpgSupport: true }); + + expect(err).toBeInstanceOf(VerisensePpgLedTestError); + expect(err.reason).toBe('ppg-comms'); + expect(err.hardwarePpgSupport).toBe(true); + expect(err.cause).toBe(NACK_GENERIC_DEBUG); + }); + + it('warns the operator not to scrap the board as a dead-LED fault', () => { + const err = classifyPpgLedTestFailure(NACK_GENERIC_DEBUG, { hardwarePpgSupport: true }); + + // The whole point of DEV-1021: the reason must separate "comms failure" + // from "operator says the LEDs are not lit". + expect(err.operatorMessage).toMatch(/PPG comms failure/i); + expect(err.operatorMessage).toMatch(/not\s+.*scrap|do not scrap/i); + // No hardware caveat when the revision is known. + expect(err.operatorMessage).not.toMatch(/hardware revision unknown/i); + }); + + it('reads a NACK on hardware with no PPG front end as not-supported', () => { + const err = classifyPpgLedTestFailure(NACK_GENERIC_DEBUG, { hardwarePpgSupport: false }); + + expect(err.reason).toBe('not-supported'); + expect(err.operatorMessage).toMatch(/no PPG front end/i); + expect(err.operatorMessage).toMatch(/not a unit fault/i); + }); + + it('fails loud when the hardware revision is unknown, but flags the ambiguity', () => { + const err = classifyPpgLedTestFailure(NACK_GENERIC_DEBUG, { hardwarePpgSupport: null }); + + expect(err.reason).toBe('ppg-comms'); + expect(err.hardwarePpgSupport).toBeNull(); + expect(err.operatorMessage).toMatch(/hardware revision unknown/i); + }); + + it.each([ + ['0x50', new Error('Device returned NACK command=0x50 property=0x9')], + ['0x60', new Error('Device returned NACK command=0x60 property=0x9')], + ['0x70', NACK_GENERIC_DEBUG], + ])('recognises NACK opcode %s on the debug property', (_opcode, raised) => { + expect(classifyPpgLedTestFailure(raised, { hardwarePpgSupport: true }).reason).toBe( + 'ppg-comms', + ); + }); + + it('does not treat a NACK on another property as a PPG verdict', () => { + const err = classifyPpgLedTestFailure( + new Error('Device returned NACK command=0x70 property=0x4'), + { hardwarePpgSupport: true }, + ); + + expect(err.reason).toBe('unknown'); + }); + + it('separates a link timeout from a PPG verdict', () => { + const err = classifyPpgLedTestFailure(TIMEOUT, { hardwarePpgSupport: true }); + + expect(err.reason).toBe('no-response'); + expect(err.operatorMessage).toMatch(/not a verdict on the PPG LEDs/i); + }); + + it('classifies anything else as unknown, preserving the message', () => { + const err = classifyPpgLedTestFailure(new Error('GATT operation failed'), { + hardwarePpgSupport: true, + }); + + expect(err.reason).toBe('unknown'); + expect(err.operatorMessage).toMatch(/GATT operation failed/); + }); + + it('handles a non-Error rejection', () => { + const err = classifyPpgLedTestFailure('something broke', { hardwarePpgSupport: null }); + + expect(err.reason).toBe('unknown'); + expect(err.operatorMessage).toMatch(/something broke/); + }); + + it('is identifiable through the exported type guard', () => { + expect( + isVerisensePpgLedTestError( + classifyPpgLedTestFailure(NACK_GENERIC_DEBUG, { hardwarePpgSupport: true }), + ), + ).toBe(true); + expect(isVerisensePpgLedTestError(new Error('plain'))).toBe(false); + }); +}); + +describe('resolveHardwarePpgSupport', () => { + it('reports PPG hardware from the production-config revision', () => { + // SR68 Pulse+ and SR62 GSR+ both carry a MAX86xxx. + expect(resolveHardwarePpgSupport({ revHwMajor: 68, revHwMinor: 9 })).toBe(true); + expect(resolveHardwarePpgSupport({ revHwMajor: 62, revHwMinor: 1 })).toBe(true); + }); + + it('reports no PPG on IMU hardware', () => { + expect(resolveHardwarePpgSupport({ revHwMajor: 61, revHwMinor: 5 })).toBe(false); + expect(resolveHardwarePpgSupport({ revHwMajor: 61, revHwMinor: 1 })).toBe(false); + }); + + it('returns null rather than guessing when the revision is unusable', () => { + expect(resolveHardwarePpgSupport(null)).toBeNull(); + expect(resolveHardwarePpgSupport(undefined)).toBeNull(); + expect(resolveHardwarePpgSupport({})).toBeNull(); + // An erased production config reads back as 0xFF. + expect(resolveHardwarePpgSupport({ revHwMajor: 0xff, revHwMinor: 0xff })).toBeNull(); + expect(resolveHardwarePpgSupport({ revHwMajor: 0, revHwMinor: 0 })).toBeNull(); + }); + + it('assumes PPG on unknown development hardware, so a NACK still fails loud', () => { + // getVerisenseHardwareSensorSupport reports every block present for SR64 + // and any unrecognised major, so a wedged bus is not written off as + // "this board has no PPG". + expect(resolveHardwarePpgSupport({ revHwMajor: 64, revHwMinor: 1 })).toBe(true); + }); +});