Fix J1939 PGN getter/setter to use full 18bits. Fixes #474

src/canmatrix/canmatrix.py

@@ -606,25 +606,25 @@ def j1939_pgn(self):
     def pgn(self):
         if not self.extended:
             raise J1939needsExtendedIdetifier
+        # PGN is bits 8-25 of the 29-Bit Extended CAN-ID
+        # Made up of PDU-S (8-15), PDU-F (16-23), Data Page (24) & Extended Data Page (25)
+        # If PDU-F >= 240 the PDU-S is interpreted as Group Extension
+        # If PDU-F < 240 the PDU-S is interpreted as a Destination Address
+        _pgn = 0
+        if self.j1939_pdu_format == 2:
+            _pgn += self.j1939_ps
+        _pgn += self.j1939_pf << 8
+        _pgn += self.j1939_dp << 16
+        _pgn += self.j1939_edp << 17
 
-        ps = (self.id >> 8) & 0xFF
-        pf = (self.id >> 16) & 0xFF
-        _pgn = pf << 8
-        if pf >= 240:
-            _pgn += ps
         return _pgn
 
     @pgn.setter
     def pgn(self, value):  # type: (int) -> None
         self.extended = True
-        ps = value & 0xff
-        pf = (value >> 8) & 0xFF
-        _pgn = pf << 8
-        if pf >= 240:
-            _pgn += ps
-
-        self.id &= 0xff0000ff
-        self.id |= (_pgn & 0xffff) << 8  # default pgn is None -> mypy reports error
+        _pgn = value & 0x3FFFF
+        self.id &= 0xfc0000ff
+        self.id |= (_pgn << 8 & 0x3FFFF00)  # default pgn is None -> mypy reports error
 
 
 
@@ -640,7 +640,7 @@ def j1939_tuple(self):  # type: () -> typing.Tuple[int, int, int]
     def j1939_destination(self):
         if not self.extended:
             raise J1939needsExtendedIdetifier
-        if self.j1939_pf < 240:
+        if self.j1939_pdu_format == 1:
             destination = self.j1939_ps
         else:
             destination = None
@@ -669,11 +669,21 @@ def j1939_pf(self):
             raise J1939needsExtendedIdetifier
         return (self.id >> 16) & 0xFF
 
+    @property
+    def j1939_pdu_format(self):
+        return 1 if (self.j1939_pf < 240) else 2
+
+    @property
+    def j1939_dp(self):
+        if not self.extended:
+            raise J1939needsExtendedIdetifier
+        return (self.id >> 24) & 0x1
+
     @property
     def j1939_edp(self):
         if not self.extended:
             raise J1939needsExtendedIdetifier
-        return (self.id >> 24) & 0x03
+        return (self.id >> 25) & 0x1
 
     @property
     def j1939_priority(self):
@@ -684,7 +694,7 @@ def j1939_priority(self):
     @j1939_priority.setter
     def j1939_priority(self, value):  # type: (int) -> None
         self.extended = True
-        self.id = (self.id & 0x2ffffff) | ((value & 0x7) << 26)
+        self.id = (self.id & 0x3ffffff) | ((value & 0x7) << 26)
 
     @property
     def j1939_str(self):  # type: () -> str

src/canmatrix/j1939_decoder.py

@@ -78,7 +78,6 @@ def decode(self, arbitration_id, can_data, matrix = None):
 
         elif arbitration_id.pgn == canmatrix.ArbitrationId.from_pgn(0xEBFF).pgn:
             # transfer data
-
             self._data = self._data + can_data[1:min(8, self.bytes_left + 1)]
             self.bytes_left = max(self.bytes_left - 7, 0)
 

src/canmatrix/tests/test_canmatrix.py

@@ -597,8 +597,58 @@ def test_frame_calc_j1939_id():
     frame.source = 0x22
     frame.pgn = 0xAAAA
     frame.priority = 3
-    assert frame.arbitration_id.id == 0xcaa0022
-
+    assert frame.arbitration_id.id == 0xCAAAA22
+
+@pytest.mark.parametrize(
+    'priority, pgn, source, id',
+    (
+        (0, 0, 0, 0),
+        (1, 1, 1, 0x4000101),
+        (2, 2, 2, 0x8000202),
+        (3, 0xAAAA, 0x22, 0xCAAAA22),
+        (0, 0x1F004, 0xEE, 0x1F004EE),
+        (3, 0x1F004, 0xEE, 0xDF004EE),
+        (7, 0x1FFFF, 0xFF, 0x1DFFFFFF),
+        (3, 0, 0xB, 0xC00000B),
+        (3, 0xEF27, 0xFD, 0xCEF27FD),
+        (3, 0xFFCA, 0xFD, 0xCFFCAFD),
+        (3, 0, 3, 0xC000003),
+        (3, 0xF002, 3, 0xCF00203),
+        (6, 0xFE4A, 3, 0x18FE4A03),
+        (3, 0x103, 5, 0xC010305),
+    ), )
+def test_frame_j1939_id_from_components(priority, pgn, source, id):
+    # we have to set all j1939 properties in the __init__ otherwise the setters crash
+    frame = canmatrix.canmatrix.Frame()
+    frame.source = source
+    frame.pgn = pgn
+    frame.priority = priority
+    assert hex(frame.arbitration_id.id) == hex(id)
+
+@pytest.mark.parametrize(
+    'priority, pgn, source, id',
+    (
+        (0, 0, 0, 0),
+        (1, 0, 1, 0x4000101),
+        (2, 0, 2, 0x8000202),
+        (3, 0xAA00, 0x22, 0xCAAAA22),
+        (0, 0x1F004, 0xEE, 0x1F004EE),
+        (3, 0x1F004, 0xEE, 0xDF004EE),
+        (7, 0x1FFFF, 0xFF, 0x1DFFFFFF),
+        (3, 0, 0xB, 0xC00000B),
+        (3, 0xEF00, 0xFD, 0xCEF27FD),
+        (3, 0xFFCA, 0xFD, 0xCFFCAFD),
+        (3, 0, 3, 0xC000003),
+        (3, 0xF002, 3, 0xCF00203),
+        (6, 0xFE4A, 3, 0x18FE4A03),
+        (3, 0x100, 5, 0xC010305),
+    ), )
+def test_frame_decode_j1939_id(source, pgn, priority, id):
+    # we have to set all j1939 properties in the __init__ otherwise the setters crash
+    frame = canmatrix.canmatrix.Frame(arbitration_id=canmatrix.ArbitrationId(id=id, extended=True))
+    assert hex(frame.source) == hex(source)
+    assert hex(frame.pgn) == hex(pgn)
+    assert hex(frame.priority) == hex(priority)
 
 def test_frame_add_transmitter(empty_frame):
     empty_frame.add_transmitter("BCM")
@@ -781,18 +831,15 @@ def test_canid_parse_values():
     can_id = canmatrix.ArbitrationId(id=0x01ABCD02, extended=True)
     assert can_id.j1939_source == 0x02
     assert can_id.j1939_destination == 0xcd
-    assert can_id.j1939_pgn == 0xAB00
+    assert can_id.j1939_pgn == 0x1AB00
     assert can_id.j1939_destination == 0xCD
     assert can_id.j1939_priority == 0
-    assert can_id.j1939_tuple == (0xCD, 0xAB00, 2)
+    assert can_id.j1939_tuple == (0xCD, 0x1AB00, 2)
 
-    test_data = {0xc00000b : 0,  0xcef27fd : 61184,  0xcffcafd : 65482, 0xc000003 : 0, 0xcf00203 : 61442, 0x18fe4a03 : 65098, 0xc010305 : 256}
-    for canId, pgn in test_data.items():
-        assert canmatrix.ArbitrationId(id=canId, extended=True).pgn == pgn
 
 def test_canid_repr():
     can_id = canmatrix.ArbitrationId(id=0x01ABCD02, extended=True)
-    assert can_id.j1939_str == "DA:0xCD PGN:0xAB00 SA:0x02"
+    assert can_id.j1939_str == "DA:0xCD PGN:0x1AB00 SA:0x02"
 
 
 # DecodedSignal tests
@@ -878,7 +925,7 @@ def test_canmatrix_get_frame_by_pgn(empty_matrix, empty_frame):
     empty_frame.arbitration_id.id = 0xA123456
     empty_frame.arbitration_id.extended = True
     empty_matrix.add_frame(empty_frame)
-    assert empty_matrix.frame_by_pgn(0x1234) == empty_frame
+    assert empty_matrix.frame_by_pgn(0x21234) == empty_frame
 
 def test_canmatrix_get_frame_by_wrong_pgn(empty_matrix, empty_frame):
     empty_frame.arbitration_id.id = 0xAB123456

src/canmatrix/tests/test_j1939_decoder.py

@@ -27,19 +27,19 @@ def test_j1939_decoder():
     t = canmatrix.j1939_decoder.j1939_decoder()
 
     #  BAM
-    (type, signals) = t.decode(canmatrix.ArbitrationId(id = 0xec0000, extended= True),
+    (type, signals) = t.decode(canmatrix.ArbitrationId(id = 0xecFF00, extended= True),
         bytearray([0x20,10,0,1,0xff,0x66,0x1,0]), matrix)
     assert "BAM " in type
  #   print (type, signals)
 
     # data 1
-    (type, signals) = t.decode(canmatrix.ArbitrationId(id = 0xeb0000, extended= True),
+    (type, signals) = t.decode(canmatrix.ArbitrationId(id = 0xebFF00, extended= True),
         bytearray([0x0,1,1,1,1,1,1,1]), matrix)
     assert "BAM data" in type
     #print (type, signals)
 
     # data 2
-    (type, signals) = t.decode(canmatrix.ArbitrationId(id = 0xeb0000, extended= True),
+    (type, signals) = t.decode(canmatrix.ArbitrationId(id = 0xebFF00, extended= True),
         bytearray([0x1,1,1,1,1,1,1,1]), matrix)
     assert "BAM last data" in type
 #    print (type, signals)
@@ -54,17 +54,15 @@ def test_j1939_decoder():
              can_data[i], matrix)
 
     print ("-------- test data -------- ")
-    test_frames = collections.OrderedDict ([
-        (0xcef27fd , "fffae1ff00ffff"),
-        (0xcffcafd , "c0fffffffffff800"),
-        (0xcf00203 , "cc00000000b812ff"),
-        (0xfe4a03 , "fffcffffffffffff"),
-        (0xc010305 , "ccfffaffff204e0a"),
-        (0x0CF00400, "F4DEDE3028FFF0FF")])
+    test_frames = collections.OrderedDict([
+        (0xcef27fd, ("fffae1ff00ffff", "")),
+        (0xcffcafd, ("c0fffffffffff800", "")),
+        (0xcf00203, ("cc00000000b812ff", "J1939 known: ETC1")),
+        (0xfe4a03, ("fffcffffffffffff", "J1939 known: ETC7")),
+        (0xc010305, ("ccfffaffff204e0a", "J1939 known: TC1")),
+        (0x0CF00400, ("F4DEDE3028FFF0FF", "J1939 known: EEC1"))])
 
-    expected = ["EEC1","TC1","ETC7","ETC1"]
-    for arb_id, asc_data in test_frames.items():
+    for arb_id, (asc_data, expected) in test_frames.items():
         (type, signals) = t.decode(canmatrix.ArbitrationId(id=arb_id, extended=True),
                                    bytearray.fromhex(asc_data), matrix)
-        if type is not None and "J1939 known" in type:
-            assert expected.pop() in type
+        assert expected in type