fix(j1939)!: report SPN indicator ranges instead of scaling them

[dborgards]

What does this change?

SPN extraction applied physical = raw × resolution + offset to every bit pattern, including the SAE J1939-71 §5.1.1 indicator ranges that sit at the top of every SPN's raw range. An EEC1 frame from an ECU that does not have SPN 190 therefore reported 8191.875 rpm rather than "no reading" — a plausible, wrong measurement, which is the worst failure mode for diagnostic data. Extraction now returns a J1939SpnValue that says which of the two it found, and signed SPNs are supported.

Closes #37.

The shape

J1939SpnValue speed = J1939Spn.Extract(payload, byteOffset: 3, startBit: 0,
    bitLength: 16, resolution: 0.125, offset: 0.0);

if (speed.TryGetValue(out double rpm)) Use(rpm);
else if (speed.IsNotAvailable) { /* the ECU does not have this parameter */ }
else if (speed.IsError)        { /* the ECU flagged its own reading as wrong */ }

• Kind — Valid, NotAvailable, Error, Reserved, ParameterSpecific. NotAvailable is 0 so a default-initialised value does not read as a valid zero.
• Raw — the bit pattern as read off the wire, indicator or not, so the exact code can still be logged or forwarded.
• Value throws InvalidOperationException unless the field carries a measurement; TryGetValue and GetValueOrDefault (NaN by default) are the non-throwing routes.
• J1939Spn.Classify exposes the range check on its own; J1939Spn.FromRaw scales an already-extracted raw.
• Signed SPNs: J1939SpnDefinition.IsSigned, an isSigned parameter on Extract, and ExtractRawSigned. For a signed parameter the indicator codes sit at the top of the signed range (0x7B..0x7F), so 0xFFFF is an ordinary −1, not "not available".

Modelled on the existing TxConfirmation idiom (a readonly record struct plus a reason enum) rather than a new pattern.

Why the signature changed rather than gaining a sibling

ADR 0001 and the owner's decision on #37: until the v1.3.0 tag, breaking API changes are allowed and map to a minor bump, and no [Obsolete] shim may be introduced to dodge a break. A TryGetSpnValue beside a GetSpnValue that still reported 0xFFFF as 8191.875 rpm would have left the defect reachable through the API that is easiest to call. src/ still contains no [Obsolete] member.

What review changed

1. The inferred sub-byte widths are now pinned, and the open question is filed rather than settled here. §5.1.1 tabulates 8/16/32-bit, 4-bit and 2-bit fields. Classify generalises to the rest by keeping the leading group a fixed size per width class, which means the indicator fraction of the range is whatever the tabulated width of that class already spends. Those inferred widths (3, 5, 6, 7) were the only ones with no test — exactly backwards, since a reading the standard does not state is the one that can drift unnoticed. Twenty-six known-answer cases now pin them.

The consequence worth a second opinion is that a 3-bit field spends half its value space on indicators. Codex proposed the alternative reading (terminal codes only, so 5-bit would be 27..31). I have not adopted it here and the reasoning is on the thread: the standard preserves the fraction across widths — a 16-bit SPN spends 256 raw values on "not available", not one — and the narrower reading fails in the dangerous direction, scaling an indicator into a plausible measurement, which is the defect #37 exists to fix. Filed as #99 with all four candidate readings and this comparison, because changing a public classifier belongs in its own PR, not the one that introduced it.

2. Coverage. The five patch lines Codecov flagged were all real gaps. TryGetValue and GetValueOrDefault were asserted only on the indicator side — their measurement side, the path almost every caller takes, was unpinned. FromIndicator's out-of-range guard was never reached. J1939SpnDefinition.Name/Pgn/Unit had no assertion at all, so a catalog entry could have carried the wrong PGN or unit with every decode test passing. Both files are now 100 % line and branch, measured rather than assumed.

3. The signed encoding was documented backwards in places. Every J1939SpnValueKind summary described only the unsigned codes, although Classify returns the same members for signed SLOTs at the sign-bit-clear codes — so a consumer branching on IsNotAvailable and trusting the doc would have treated -1 as "no reading". Worse, the comment on the signed catalog test stated the rule inverted, directly above an assertion saying the opposite. Both fixed, and the corrected rule is now pinned by an assertion (signed 0xFFFF → -0.1 deg) rather than only described.

Interpretation notes

• §5.1.1 defines the ranges on the field's leading byte for parameters one byte or wider, and gives separate tables for 4-bit and 2-bit fields. Widths the standard does not tabulate (3, 5..7, 12, 64 …) follow the same leading-group rule, documented on Classify and tracked in J1939 SPN: a 3-bit field spends half its value space on indicators #99.
• A 1-bit field, and a signed sub-byte field, are always Valid: J1939-71 defines no indicator codes there, and inventing some would report real measurements as missing.

Docs updated

src/CanKit.Pro.J1939/README.md (rendered into docs/packages/j1939.md), the docs/index.md quickstart, the J1939 quickstart sample, and the FR-J1939-002 row of the SRS — which previously required only scale/offset extraction and now requires the indicator ranges to be reported as such, with the verification column naming the known-answer tests.

Type of change

• feat — new behaviour (minor release)
• fix / perf — bug or performance fix (patch release)
• docs / test / refactor / chore / ci — no release
• Breaking change (! in the title, plus a BREAKING CHANGE: footer explaining the migration)

The BREAKING CHANGE: footer is on commit f1f7c61, in the commit message — this repository merges rather than squashes, so the analyser reads commits and never reads this body. Per ADR 0001 §7 breaking maps to minor for the duration of the pre-1.3.0 window, so this does not publish 2.0.0.

Checklist

• dotnet build CanKit.Pro.sln -c Release succeeds — run as -p:CI=true (warnings as errors): 0 warnings, 0 errors
• dotnet test CanKit.Pro.sln -c Release passes — --no-build --framework net10.0: 583 passed, 0 failed (140 of them under J1939Spn*), after merging main at a723b3c
• Public API changes are documented with XML comments
• New behaviour is covered by a test
• The requirement or ADR this relates to is referenced — FR-J1939-002, ADR 0001

Also run: dotnet format CanKit.Pro.sln --verify-no-changes — clean (exit 0). The net48 leg was not run; it is Windows-only in this checkout.

Tests

tests/CanKit.Pro.Tests/TestCases/J1939/J1939SpnIndicatorTests.cs pins both boundaries of every band, so an off-by-one classifier fails:

Width	Valid	Parameter specific	Reserved	Error	Not available
1 byte	0x00, 0xFA	0xFB	0xFC, 0xFD	0xFE	0xFF
2 bytes	0x0000, 0xFAFF	0xFB00, 0xFBFF	0xFC00, 0xFDFF	0xFE00, 0xFEFF	0xFF00, 0xFFFF
4 bytes	0x00000000, 0xFAFFFFFF	0xFB000000, 0xFBFFFFFF	0xFC000000, 0xFDFFFFFF	0xFE000000, 0xFEFFFFFF	0xFF000000, 0xFFFFFFFF
4 bits	0x0, 0xA	0xB	0xC, 0xD	0xE	0xF
2 bits	0b00, 0b01	—	—	0b10	0b11
1 bit	0, 1	—	—	—	—
5 / 6 / 7 bits (inferred)	21 / 43 / 87	22 / 44 / 88	24 / — / —	28 / 56 / 112	30–31 / 60–63 / 120–127
3 bits (inferred)	0..3	—	—	4, 5	6, 7
signed	0x7AFF, 0x8000, 0xFFFF	0x7B00	0x7C00, 0x7DFF	0x7E00, 0x7EFF	0x7F00, 0x7FFF

Plus: the 12- and 64-bit generalisation, the regression itself (0xFFFF at 0.125 rpm/bit is not 8191.875), the top of the valid band still scaling (0xFAFF → 8031.875 rpm) and the very next raw count flipping to an indicator, signed sign-extension and ExtractRawSigned, Classify rejecting out-of-range bit lengths, the value type's default/factories/ToString, both non-throwing accessors on the measurement side, FromIndicator's out-of-range guard, and a catalog definition's descriptive members.

API approval baseline

tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.J1939.approved.txt was replaced with the test's own generated .received.txt, not hand-written. Like TxConfirmation, the new readonly record struct renders as public readonly struct … : IEquatable<> — a PublicApiGenerator limitation, matching the existing precedent in CanKit.Pro.RawCan.approved.txt.

A note on CI

macos-latest went red once on an earlier head with J1939TpTests.Parallel_Bam_And_TwoCm_Sessions_Do_Not_Interfere, a wall-clock T1 timeout in code this diff does not touch. It passed on a re-run of the identical commit. That is the #92 population — six distinct tests in about an hour, none repeating, all on branches whose diffs cannot reach the failing code.

🤖 Generated with Claude Code

https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj

[cursor]

PR Summary

Medium Risk
Breaking public API for all SPN extraction call sites, but behavior is heavily covered by new indicator tests and aligns with diagnostic data correctness (FR-J1939-002).

Overview
Breaking change: SPN decode no longer returns a double. J1939Spn.Extract, J1939SpnCatalog.Extract, and J1939SpnDefinition.Extract now return J1939SpnValue, which distinguishes a real measurement from SAE J1939-71 §5.1.1 indicators (not available, error, reserved, parameter-specific). That fixes cases like 0xFFFF on engine speed being scaled to 8191.875 rpm instead of “no reading”.

Decoding adds Classify, FromRaw, ExtractRawSigned, and an optional isSigned flag (also on J1939SpnDefinition). Callers use TryGetValue, GetValueOrDefault, or Value (throws on indicators); Raw keeps the wire pattern for logging.

Docs, the J1939 quickstart sample, FR-J1939-002 in the SRS, and the public API approval baseline are updated. J1939SpnIndicatorTests pins indicator boundaries across field widths (including signed SLOT rules).

^{Reviewed by Cursor Bugbot for commit 2ac4e6d. Bugbot is set up for automated code reviews on this repo. Configure here.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1f7c61a6a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[dborgards]

CI: macos-latest red on 4f7f817, and it is not this PR's

What failed. One test, J1939TpTests.Parallel_Bam_And_TwoCm_Sessions_Do_Not_Interfere, with J1939TpAbortException: J1939-TP Bam RX session timed out (T1) waiting for next TP.DT from 0x10. 567 of 568 passed; the build itself was clean (0 warnings, 0 errors). ubuntu-latest and windows-latest passed.

Why it is not this PR's. The diff against main is thirteen files and touches no J1939-TP, actor, scheduler or RawCan code:

docs/index.md                          src/CanKit.Pro.J1939/J1939Spn.cs
docs/requirements/SRS-CanKit.Pro.md    src/CanKit.Pro.J1939/J1939SpnCatalog.cs
samples/…/J1939Quickstart/Program.cs   src/CanKit.Pro.J1939/J1939SpnDefinition.cs
src/CanKit.Pro.J1939/README.md         src/CanKit.Pro.J1939/J1939SpnValue.cs
tests/…/J1939NodeTests.cs              tests/…/J1939SpnCatalogTests.cs
tests/…/J1939SpnIndicatorTests.cs      tests/…/PublicApiSurfaceTests.cs
tests/…/ApiApprovals/CanKit.Pro.J1939.approved.txt

The failing test exercises BAM reassembly in CanKit.Pro.J1939Tp. Nothing in this change reaches that path — J1939SpnValue is a new value type and the SPN extraction functions are pure. The J1939-TP code on this branch is whatever main carries, since the branch merged main at 4f7f817 and adds nothing there.

It is the #92 population. A T1 expiry waiting on the next TP.DT is a wall-clock timeout, which is the failure mode #92 documents — originally over four CI runs of #91, a branch containing no .cs changes at all. I also ran this exact test 40 times locally on this tree earlier today: 0 failures. The full suite passes locally on the merged tree, 568/568, and passed on the other two legs of this same run.

What I did. Re-ran the failed job once — the single re-run the situation allows. I have not touched the test: loosening a timing assertion to get a green tick would be hiding the very signal #92 is trying to characterise, and skipping it is not on the table.

No fix exists to port. #92 is open and its durable fix is to drive the actor's DeadlineScheduler from a virtual clock the test advances, rather than measuring wall-clock time on a shared runner. That is a test-infrastructure change spanning several suites and does not belong in a PR about SPN decoding.

This is a new data point for #92 — a fifth distinct test, on a leg and commit where the code under test is untouched — and I have recorded it there.

---

Generated by Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

🔵 Needs a closer look

The breaking API and signed-value behavior warrant human approval, with signed-mode documentation nits still unresolved.

Review details

Suppressed comments (4)

src/CanKit.Pro.J1939/J1939SpnValue.cs:84

• The IsNotAvailable XML summary repeats the same unsigned-only rule (all bits set), but J1939Spn.Classify(..., isSigned: true) intentionally returns this property for the signed leading-0x7F indicator range. Update the property documentation to describe the signed encoding as well.

    /// <summary>True when the sending ECU does not have this parameter (all bits set).</summary>

src/CanKit.Pro.J1939/J1939SpnValue.cs:29

• This summary documents only the unsigned parameter-specific code (0xFB), although signed SPNs use the corresponding 0x7B leading-byte range and return the same enum member. Without that distinction the public API documentation is inconsistent with the signed behavior added here.

    /// <summary>
    /// The "parameter specific indicator" code (<c>0xFB</c> leading byte): its meaning is defined
    /// by the individual SPN, not by J1939-71, so it cannot be scaled into a physical value here.
    /// </summary>
    ParameterSpecific = 2,

src/CanKit.Pro.J1939/J1939SpnValue.cs:35

• The reserved-range summary is also unsigned-only (0xFC..0xFD), while signed classification reports 0x7C..0x7D in the same Reserved state. Please include the signed encoding in this public API documentation.

    /// <summary>
    /// Reserved for future indicator bits (<c>0xFC</c>..<c>0xFD</c> leading byte). Not a
    /// measurement.
    /// </summary>
    Reserved = 3,

src/CanKit.Pro.J1939/J1939SpnValue.cs:41

• This error summary names only the unsigned 0xFE leading byte, but signed SPNs report 0x7E as Error. The documentation should cover both forms just as Classify does.

    /// <summary>
    /// The transmitting ECU has the parameter but detected an error in it (<c>0xFE</c> leading
    /// byte).
    /// </summary>
    Error = 4,

• Files reviewed: 13/13 changed files
• Comments generated: 2
• Review effort level: Lite