From 9d9e45e692460e5ec79a77838d90b9ff6720ec2a Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Tue, 22 Sep 2026 08:12:44 +0200 Subject: [PATCH 1/2] feat(addressing): take the frame's kind into Decompose, refuse a PDU1 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 --- src/CanKit.Pro.Addressing/J1939Id.cs | 32 +++++++++++++ src/CanKit.Pro.Addressing/J1939Name.cs | 45 +++++++++++++++++++ src/CanKit.Pro.Addressing/README.md | 11 ++++- .../CanKit.Pro.Addressing.approved.txt | 4 ++ .../TestCases/AddressingTests.cs | 44 ++++++++++++++++++ 5 files changed, 134 insertions(+), 2 deletions(-) diff --git a/src/CanKit.Pro.Addressing/J1939Id.cs b/src/CanKit.Pro.Addressing/J1939Id.cs index fcd9187..7645ce1 100644 --- a/src/CanKit.Pro.Addressing/J1939Id.cs +++ b/src/CanKit.Pro.Addressing/J1939Id.cs @@ -18,6 +18,13 @@ public static class J1939Id /// The 29-bit extended CAN ID (flag bits, if any, must already be stripped -- pass /// CanFrame.ID/CanFrameView.ID as-is, they are already flag-stripped). /// + /// + /// The value alone cannot tell an 11-bit identifier from a 29-bit one -- every 11-bit + /// value is also a valid 29-bit one, with priority 0 and PDU Format 0 -- so an 11-bit + /// identifier passed here decomposes into fields it never had. The caller knows the + /// frame's kind: pass it to , or skip frames that are + /// not extended before calling this (#55). + /// public static J1939Fields Decompose(uint canId) { CanIdRange.ValidateExtended(canId); @@ -30,6 +37,23 @@ public static J1939Fields Decompose(uint canId) return new J1939Fields(priority, reserved, dataPage, pduFormat, pduSpecific, sourceAddress); } + /// + /// As , for a caller that knows the frame's kind: an + /// identifier from a frame that is not extended is refused, since J1939 uses 29-bit + /// identifiers only and an 11-bit one would decompose into fields it never had (#55). + /// + /// The CAN ID, flag bits stripped. + /// + /// Whether the frame carrying it is an extended (29-bit) frame -- CanFrame.IsExtendedFrame. + /// + /// is false. + public static J1939Fields Decompose(uint canId, bool isExtendedFrame) + { + if (!isExtendedFrame) + throw new ArgumentException("J1939 identifiers are 29-bit: an 11-bit frame's identifier has no J1939 fields.", nameof(isExtendedFrame)); + return Decompose(canId); + } + /// /// Composes a 29-bit CAN ID from its raw J1939 fields. /// @@ -69,6 +93,11 @@ public static uint Compose(byte priority, bool reserved, byte dataPage, byte pdu /// destination address (defaults to the conventional global/broadcast address 0xFF, which /// is simply unused in that case). /// + /// + /// does not fit in 18 bits, or is a PDU1 PGN whose low byte is + /// not zero -- SAE J1939-21 defines a PDU1 PGN with its PDU Specific byte as 0, so such + /// a value is not a PGN, and the byte is not silently discarded (#55). + /// public static uint ComposePgn(byte priority, uint pgn, byte sourceAddress, byte destinationAddress = 0xFF) { if (pgn > 0x3FFFF) throw new ArgumentOutOfRangeException(nameof(pgn), pgn, "PGN must fit in 18 bits (Reserved|DataPage|PF|GE)."); @@ -76,6 +105,9 @@ public static uint ComposePgn(byte priority, uint pgn, byte sourceAddress, byte var reserved = ((pgn >> 17) & 0x1) != 0; var dataPage = (byte)((pgn >> 16) & 0x1); var pduFormat = (byte)((pgn >> 8) & 0xFF); + if (pduFormat < 240 && (pgn & 0xFF) != 0) + throw new ArgumentOutOfRangeException(nameof(pgn), pgn, + "A PDU1 PGN (PDU Format < 240) has a PDU Specific byte of 0; the destination address is a separate argument."); var pduSpecific = pduFormat < 240 ? destinationAddress : (byte)(pgn & 0xFF); return Compose(priority, reserved, dataPage, pduFormat, pduSpecific, sourceAddress); } diff --git a/src/CanKit.Pro.Addressing/J1939Name.cs b/src/CanKit.Pro.Addressing/J1939Name.cs index 89ab2e8..2166882 100644 --- a/src/CanKit.Pro.Addressing/J1939Name.cs +++ b/src/CanKit.Pro.Addressing/J1939Name.cs @@ -147,6 +147,51 @@ public static ulong Compose( /// public static J1939Name Decompose(ulong value) => new J1939Name(value); + /// + /// The NAME as it travels in the data field of an Address Claimed message: eight bytes, + /// least significant first (SAE J1939-81 ยง4.1 -- byte 1 carries bits 1-8 of the + /// Identity Number, byte 8 the Industry Group and the Arbitrary Address Capable bit). + /// Fixed by the standard, not by the host: a platform-endian conversion of + /// is wrong on a big-endian host (#55). + /// + public byte[] ToBytes() + { + var bytes = new byte[8]; + WriteTo(bytes, 0); + return bytes; + } + + /// + /// Writes the NAME's eight bytes, least significant first, into + /// at . See . + /// + /// is null. + /// Fewer than eight bytes from . + public void WriteTo(byte[] destination, int offset = 0) + { + if (destination is null) throw new ArgumentNullException(nameof(destination)); + if (offset < 0 || destination.Length - offset < 8) + throw new ArgumentOutOfRangeException(nameof(offset), offset, "A NAME takes eight bytes."); + for (int i = 0; i < 8; i++) destination[offset + i] = (byte)(Value >> (8 * i)); + } + + /// + /// Reads a NAME from eight bytes, least significant first, at + /// in -- the data field of an Address Claimed message. See + /// . + /// + /// is null. + /// Fewer than eight bytes from . + public static J1939Name FromBytes(byte[] source, int offset = 0) + { + if (source is null) throw new ArgumentNullException(nameof(source)); + if (offset < 0 || source.Length - offset < 8) + throw new ArgumentOutOfRangeException(nameof(offset), offset, "A NAME takes eight bytes."); + ulong value = 0; + for (int i = 7; i >= 0; i--) value = (value << 8) | source[offset + i]; + return new J1939Name(value); + } + /// /// Compares two NAMEs for SAE J1939-81 address claiming priority. /// diff --git a/src/CanKit.Pro.Addressing/README.md b/src/CanKit.Pro.Addressing/README.md index e0a68f0..b781b79 100644 --- a/src/CanKit.Pro.Addressing/README.md +++ b/src/CanKit.Pro.Addressing/README.md @@ -26,8 +26,10 @@ CanIdRange.ValidateStandard(0x800); // throws ArgumentOutOfRangeException // J1939: build a 29-bit ID from priority/PGN/source/destination var id = J1939Id.ComposePgn(priority: 3, pgn: 0xFED9, sourceAddress: 0x17); -// J1939: decompose a received 29-bit ID -var fields = J1939Id.Decompose(id); +// J1939: decompose a received 29-bit ID. The value alone cannot tell an 11-bit identifier +// from a 29-bit one, so pass the frame's kind where you have it; a PDU1 PGN with a non-zero +// low byte is refused by ComposePgn rather than truncated (#55). +var fields = J1939Id.Decompose(id, isExtendedFrame: true); fields.Priority; // 3 fields.Pgn; // 0xFED9 fields.SourceAddress; // 0x17 @@ -53,6 +55,11 @@ var name = new J1939Name( arbitraryAddressCapable: true); var sameName = J1939Name.Decompose(name.Value); J1939Name.CompareClaimPriority(name, sameName); // 0; lower unsigned NAME wins address claiming + +// J1939: the NAME on the wire -- the eight data bytes of an Address Claimed message, least +// significant first (SAE J1939-81), whatever the host's byte order +byte[] claimData = name.ToBytes(); +var claimed = J1939Name.FromBytes(claimData); ``` `CanKit.Pro.RawCan`'s `CanIdFilter` also gained an `Overlaps(CanIdFilter other)` method and diff --git a/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.Addressing.approved.txt b/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.Addressing.approved.txt index 5794e4d..802152e 100644 --- a/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.Addressing.approved.txt +++ b/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.Addressing.approved.txt @@ -26,6 +26,7 @@ namespace CanKit.Pro.Addressing public static uint Compose(byte priority, bool reserved, byte dataPage, byte pduFormat, byte pduSpecific, byte sourceAddress) { } public static uint ComposePgn(byte priority, uint pgn, byte sourceAddress, byte destinationAddress = 255) { } public static CanKit.Pro.Addressing.J1939Fields Decompose(uint canId) { } + public static CanKit.Pro.Addressing.J1939Fields Decompose(uint canId, bool isExtendedFrame) { } } public readonly struct J1939Name : System.IComparable, System.IEquatable { @@ -54,10 +55,13 @@ namespace CanKit.Pro.Addressing public override int GetHashCode() { } public bool HasHigherClaimPriorityThan(CanKit.Pro.Addressing.J1939Name other) { } public bool HasLowerClaimPriorityThan(CanKit.Pro.Addressing.J1939Name other) { } + public byte[] ToBytes() { } public override string ToString() { } + public void WriteTo(byte[] destination, int offset = 0) { } public static int CompareClaimPriority(CanKit.Pro.Addressing.J1939Name left, CanKit.Pro.Addressing.J1939Name right) { } public static ulong Compose(uint identityNumber, ushort manufacturerCode, byte ecuInstance, byte functionInstance, byte function, bool reserved, byte vehicleSystem, byte vehicleSystemInstance, byte industryGroup, bool arbitraryAddressCapable) { } public static CanKit.Pro.Addressing.J1939Name Decompose(ulong value) { } + public static CanKit.Pro.Addressing.J1939Name FromBytes(byte[] source, int offset = 0) { } public static bool operator !=(CanKit.Pro.Addressing.J1939Name left, CanKit.Pro.Addressing.J1939Name right) { } public static bool operator <(CanKit.Pro.Addressing.J1939Name left, CanKit.Pro.Addressing.J1939Name right) { } public static bool operator <=(CanKit.Pro.Addressing.J1939Name left, CanKit.Pro.Addressing.J1939Name right) { } diff --git a/tests/CanKit.Pro.Tests/TestCases/AddressingTests.cs b/tests/CanKit.Pro.Tests/TestCases/AddressingTests.cs index c6eef44..2736da8 100644 --- a/tests/CanKit.Pro.Tests/TestCases/AddressingTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/AddressingTests.cs @@ -233,4 +233,48 @@ public void J1939_Compose_Rejects_Priority_Above_Seven() Action act = () => J1939Id.Compose(priority: 8, reserved: false, dataPage: 0, pduFormat: 0, pduSpecific: 0, sourceAddress: 0); act.Should().Throw(); } + + // #55: the value alone cannot tell an 11-bit identifier from a 29-bit one; the overload + // that takes the frame's kind refuses one that is not extended. + [Fact] + public void Decompose_Refuses_An_Identifier_From_A_Frame_That_Is_Not_Extended() + { + Action act = () => J1939Id.Decompose(0x7DF, isExtendedFrame: false); + act.Should().Throw().WithParameterName("isExtendedFrame"); + J1939Id.Decompose(0x18FECA2A, isExtendedFrame: true).SourceAddress.Should().Be(0x2A); + } + + // #55: a PDU1 PGN carries its PDU Specific byte as 0 (SAE J1939-21); a value with the byte + // set is not a PGN, and is refused rather than having the byte silently discarded. + [Fact] + public void ComposePgn_Refuses_A_Pdu1_Pgn_With_A_Nonzero_Low_Byte() + { + Action act = () => J1939Id.ComposePgn(priority: 3, pgn: 0xC80B, sourceAddress: 0x17, destinationAddress: 0x0B); + act.Should().Throw().WithParameterName("pgn"); + // The PDU2 low byte is the Group Extension and stays. + J1939Id.Decompose(J1939Id.ComposePgn(priority: 6, pgn: 0xFECA, sourceAddress: 0x2A)).Pgn.Should().Be(0xFECAu); + } + + // #55: the NAME's wire order is fixed by SAE J1939-81 -- least significant byte first -- + // whatever the host's; the type owns the conversion. + [Fact] + public void J1939Name_Serializes_Least_Significant_Byte_First_And_Round_Trips() + { + var name = J1939Name.Decompose(0xBA6BAB95B5555555UL); + + var bytes = name.ToBytes(); + bytes.Should().Equal(0x55, 0x55, 0x55, 0xB5, 0x95, 0xAB, 0x6B, 0xBA); + J1939Name.FromBytes(bytes).Should().Be(name); + + var buffer = new byte[10]; + name.WriteTo(buffer, 2); + buffer[2].Should().Be(0x55); + buffer[9].Should().Be(0xBA); + J1939Name.FromBytes(buffer, 2).Should().Be(name); + + Action shortSource = () => J1939Name.FromBytes(new byte[7]); + shortSource.Should().Throw(); + Action shortDestination = () => name.WriteTo(new byte[8], 1); + shortDestination.Should().Throw(); + } } From fa832bae0da64220147ad8f9ec329d36875a74a9 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Tue, 22 Sep 2026 08:12:44 +0200 Subject: [PATCH 2/2] fix(j1939): read and write the NAME of an address claim in wire order #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 --- src/CanKit.Pro.J1939/J1939NodeImpl.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/CanKit.Pro.J1939/J1939NodeImpl.cs b/src/CanKit.Pro.J1939/J1939NodeImpl.cs index 6e78372..c4c363e 100644 --- a/src/CanKit.Pro.J1939/J1939NodeImpl.cs +++ b/src/CanKit.Pro.J1939/J1939NodeImpl.cs @@ -489,7 +489,7 @@ private void OnClaimAnnounceTxFailed(byte preferredAddress, Exception error) private void HandleIncomingAddressClaim(byte peerSa, byte[] payload) { if (payload.Length < 8) return; // malformed - var peerName = J1939Name.Decompose(BitConverter.ToUInt64(payload, 0)); + var peerName = J1939Name.FromBytes(payload); // the wire order, not the host's (#55) // A frame carrying our own NAME cannot be a claim we have to arbitrate against: equal // NAME fails HasHigherClaimPriorityThan in both directions, so both parties would take @@ -619,13 +619,7 @@ private void SendAddressClaimFrame(byte sourceAddress) TransmitFrame(canId, payload); } - private byte[] BuildAddressClaimPayload() - { - var payload = new byte[8]; - ulong v = _name.Value; - for (int i = 0; i < 8; i++) payload[i] = (byte)((v >> (8 * i)) & 0xFF); - return payload; - } + private byte[] BuildAddressClaimPayload() => _name.ToBytes(); /// /// Sends the initial Address Claim with and