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

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

Remove the criteria that PF must be >= 240 for PS field to be set as
this seems to be incorrect. (It may be that for some specific case the
PS need to not be used, but it isn't documented here and in general the
PGN should be all 18 bits)
Modifed Tests:

test_canmatrix_get_frame_by_pgn()
seemed to be using made up data and didn't accound for the DP and EDP
fields in expected PGN

test_canid_repr()
Modified the expected repr value to include the PS field in the PGN.

test_canid_parse_values()
Modified expected PGN to include the PS field, Moved the list of
test_data into parametrized cases on a new test.

New Tests:

test_frame_j1939_id_from_components()
Tests constructing a full 29-bit CAN ID from the Priority, Source, and
PGN

test_frame_decode_j1939_id()
Tests extracting Priority Source and PGN from an Arbiration ID

* Add the global desistantion Address 0xFF to the BAM arbitration IDs

* Improve the fix, to more explicitly handle the difference between
PDU-Format 1 and PDU-Format 2, (where the PF >= 240 comes in).
The PGN setter now will now set the arbitration ID to contain the
destination address, but when the PGN getter is used it will strip the
destination field off. I think that it is unlikely for the PGN setter
to be used with PDU-F1 PGN containing a destination. But in the case
that it is I think it makes more sense to not throw the destination
address field away.

Author: Seneda

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