Skip to content

feat(addressing): take the frame's kind into Decompose, refuse a PDU1 PGN with a low byte, and give the NAME its wire order - #151

Merged
dborgards merged 2 commits into
mainfrom
fix/addressing-collected-findings
Sep 22, 2026
Merged

dborgards merged 2 commits into
mainfrom
fix/addressing-collected-findings

Conversation

@dborgards

Copy link
Copy Markdown
Owner

What does this change?

Closes #55 — the three Addressing findings from the repository review. J1939Id.Decompose(uint) cannot tell an 11-bit identifier from a 29-bit one by value, so an overload takes the frame's kind and refuses one that is not extended, and the single-argument form documents the limit. ComposePgn refused nothing when given a PDU1 PGN with a non-zero low byte and silently discarded the byte; SAE J1939-21 defines it as 0, so such a value is refused. J1939Name owns its wire order now (ToBytes, WriteTo, FromBytes: least significant byte first, SAE J1939-81), and the J1939 node reads and writes the NAME of an address claim through it instead of BitConverter.ToUInt64, which is the host's byte order.

The Addressing package stays dependency-free, hence byte[] rather than Span<byte> for the NAME conversion.

Type of change

  • feat — new behaviour (minor release): J1939Id.Decompose(uint, bool), J1939Name.ToBytes / WriteTo / FromBytes
  • fix / perf — bug or performance fix (patch release): the node's NAME byte order
  • docs / test / refactor / chore / ci — no release
  • Breaking change (! in the title, plus a BREAKING CHANGE: footer explaining the migration) — ComposePgn now throws for a value that was never a valid PGN; not a change to any documented behaviour

Checklist

  • dotnet build CanKit.Pro.sln -c Release succeeds
  • dotnet test CanKit.Pro.sln -c Release passes (net10.0, all 940)
  • Public API changes are documented with XML comments
  • New behaviour is covered by a test — Decompose_Refuses_An_Identifier_From_A_Frame_That_Is_Not_Extended, ComposePgn_Refuses_A_Pdu1_Pgn_With_A_Nonzero_Low_Byte, J1939Name_Serializes_Least_Significant_Byte_First_And_Round_Trips; each fails with its change reverted (mutation-checked)
  • The requirement or ADR this relates to is referenced — Addressing: collected minor findings (Decompose validation, ComposePgn truncation, NAME byte order) #55; ADR-0001 for the API additions before v1.3.0

🤖 Generated with Claude Code

dborgards and others added 2 commits September 22, 2026 08:12
… PGN with a low byte, and give the NAME its wire order

#55, the three findings. J1939Id.Decompose(uint) cannot tell an 11-bit
identifier from a 29-bit one -- every 11-bit value is a valid 29-bit one
with priority 0 and PDU Format 0 -- so an overload takes the frame's kind
and refuses one that is not extended, and the single-argument form says
so. ComposePgn silently discarded the low byte of a PDU1 PGN; SAE J1939-21
defines it as 0, so a value with it set is refused. And J1939Name owns its
wire order now -- ToBytes, WriteTo, FromBytes, least significant byte
first per SAE J1939-81 -- so no caller reaches for a platform-endian
conversion. Mutation-checked: each of the three tests fails with its
change reverted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#55: the incoming claim's NAME was read with BitConverter.ToUInt64, which
is the host's byte order and wrong on a big-endian one. Both directions
go through J1939Name's own conversion now.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Sep 22, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes J1939 address-claim interoperability (NAME byte order fix) and makes ComposePgn throw on previously accepted invalid PDU1 PGNs; core protocol paths are covered by new tests.

Overview
Closes #55 with tighter J1939 addressing behavior and a fix for address-claim NAME encoding on the node.

J1939Id: Documents that a bare CAN ID cannot distinguish 11-bit from 29-bit frames, and adds Decompose(uint, bool isExtendedFrame) which throws when the frame is not extended. ComposePgn now rejects PDU1 PGN values whose low byte is non-zero (per SAE J1939-21) instead of silently dropping that byte.

J1939Name: Adds ToBytes, WriteTo, and FromBytes so the 64-bit NAME is serialized least-significant-byte first on the wire (J1939-81), independent of host endianness. J1939NodeImpl uses these for address-claim TX/RX instead of BitConverter.ToUInt64 and inline shifting.

README examples, API approval snapshots, and AddressingTests cover the new overload, ComposePgn validation, and NAME round-trip/buffer bounds.

Reviewed by Cursor Bugbot for commit fa832ba. Bugbot is set up for automated code reviews on this repo. Configure here.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-22T06:15:19.014946Z fa832ba PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@codecov

codecov Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/CanKit.Pro.Addressing/J1939Name.cs 71.42% 0 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

@dborgards
dborgards merged commit ea0c91a into main Sep 22, 2026
14 checks passed
@dborgards
dborgards deleted the fix/addressing-collected-findings branch September 22, 2026 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Addressing: collected minor findings (Decompose validation, ComposePgn truncation, NAME byte order)

1 participant