DEV-1021 Classify MAX86xxx PPG LED-test failures instead of raising a raw NACK

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'`.

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<void> {
-    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<void> {

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;
+}

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)

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);
+  });
+});