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

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
         /// <c>CanFrame.ID</c>/<c>CanFrameView.ID</c> as-is, they are already flag-stripped).
         /// </param>
+        /// <remarks>
+        /// 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 <see cref="Decompose(uint, bool)"/>, or skip frames that are
+        /// not extended before calling this (#55).
+        /// </remarks>
         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);
         }
 
+        /// <summary>
+        /// As <see cref="Decompose(uint)"/>, 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).
+        /// </summary>
+        /// <param name="canId">The CAN ID, flag bits stripped.</param>
+        /// <param name="isExtendedFrame">
+        /// Whether the frame carrying it is an extended (29-bit) frame -- <c>CanFrame.IsExtendedFrame</c>.
+        /// </param>
+        /// <exception cref="ArgumentException"><paramref name="isExtendedFrame"/> is <c>false</c>.</exception>
+        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);
+        }
+
         /// <summary>
         /// Composes a 29-bit CAN ID from its raw J1939 fields.
         /// </summary>
@@ -69,13 +93,21 @@ 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).
         /// </param>
+        /// <exception cref="ArgumentOutOfRangeException">
+        /// <paramref name="pgn"/> 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).
+        /// </exception>
         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).");
 
             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);
         }

src/CanKit.Pro.Addressing/J1939Name.cs

@@ -147,6 +147,51 @@ public static ulong Compose(
         /// </summary>
         public static J1939Name Decompose(ulong value) => new J1939Name(value);
 
+        /// <summary>
+        /// 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
+        /// <see cref="Value"/> is wrong on a big-endian host (#55).
+        /// </summary>
+        public byte[] ToBytes()
+        {
+            var bytes = new byte[8];
+            WriteTo(bytes, 0);
+            return bytes;
+        }
+
+        /// <summary>
+        /// Writes the NAME's eight bytes, least significant first, into
+        /// <paramref name="destination"/> at <paramref name="offset"/>. See <see cref="ToBytes"/>.
+        /// </summary>
+        /// <exception cref="ArgumentNullException"><paramref name="destination"/> is <c>null</c>.</exception>
+        /// <exception cref="ArgumentOutOfRangeException">Fewer than eight bytes from <paramref name="offset"/>.</exception>
+        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));
+        }
+
+        /// <summary>
+        /// Reads a NAME from eight bytes, least significant first, at <paramref name="offset"/>
+        /// in <paramref name="source"/> -- the data field of an Address Claimed message. See
+        /// <see cref="ToBytes"/>.
+        /// </summary>
+        /// <exception cref="ArgumentNullException"><paramref name="source"/> is <c>null</c>.</exception>
+        /// <exception cref="ArgumentOutOfRangeException">Fewer than eight bytes from <paramref name="offset"/>.</exception>
+        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);
+        }
+
         /// <summary>
         /// Compares two NAMEs for SAE J1939-81 address claiming priority.
         /// </summary>

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

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();
 
     /// <summary>
     /// Sends the initial Address Claim with <see cref="ICanBusService.SendConfirmed"/> and

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<CanKit.Pro.Addressing.J1939Name>, System.IEquatable<CanKit.Pro.Addressing.J1939Name>
     {
@@ -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) { }

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<ArgumentOutOfRangeException>();
     }
+
+    // #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<ArgumentException>().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<ArgumentOutOfRangeException>().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<ArgumentOutOfRangeException>();
+        Action shortDestination = () => name.WriteTo(new byte[8], 1);
+        shortDestination.Should().Throw<ArgumentOutOfRangeException>();
+    }
 }