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