Repository: ebroecker/canmatrix

Issue:
J1939 PGN does not include Extended Data Page (EDP) and Data Page (DP) bits
I think the code for parsing J1939 IDs isn't quite up to date. 

In the code for the PGN property of the ArbitrationId class, the PGN is only constructed from the PS and PF fields. However, there are two more bits in the 29 bit ID which contribute to the PGN. I believe that initially they were reserved for future expansions, so are only set in newer PGNs.

I think it is simple enough to fix, so I will add some tests and try to make a Pull Request.

![J1939 29 Bit Can ID](https://assets.vector.com/cms/_processed_/f/7/csm_SAE-J1939-29-Bit-CAN-Identifier-Parameter-Group-1920x1080_RGB_EN_814ad669a6.png)

See these sites for reference.

https://www.kvaser.com/about-can/higher-layer-protocols/j1939-introduction/

https://www.vector.com/int/en/know-how/technologies/protocols/sae-j1939/#c103492

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/ebroecker/canmatrix/pull/475?src=pr&el=h1) Report
> Merging [#475](https://codecov.io/gh/ebroecker/canmatrix/pull/475?src=pr&el=desc) into [development](https://codecov.io/gh/ebroecker/canmatrix/commit/08ec80e87dc8144f68716bb981a78903428192f6&el=desc) will **increase** coverage by `0.08%`.
> The diff coverage is `92.50%`.

[![Impacted file tree graph](https://codecov.io/gh/ebroecker/canmatrix/pull/475/graphs/tree.svg?width=650&height=150&src=pr&token=I5J8CV2skH)](https://codecov.io/gh/ebroecker/canmatrix/pull/475?src=pr&el=tree)

```diff
@@               Coverage Diff               @@
##           development     #475      +/-   ##
===============================================
+ Coverage        59.86%   59.94%   +0.08%     
===============================================
  Files               45       45              
  Lines             8184     8193       +9     
  Branches          1808     1806       -2     
===============================================
+ Hits              4899     4911      +12     
+ Misses            2905     2901       -4     
- Partials           380      381       +1     
```


| [Impacted Files](https://codecov.io/gh/ebroecker/canmatrix/pull/475?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/canmatrix/j1939\_decoder.py](https://codecov.io/gh/ebroecker/canmatrix/pull/475/diff?src=pr&el=tree#diff-c3JjL2Nhbm1hdHJpeC9qMTkzOV9kZWNvZGVyLnB5) | `54.68% <ø> (ø)` | |
| [src/canmatrix/canmatrix.py](https://codecov.io/gh/ebroecker/canmatrix/pull/475/diff?src=pr&el=tree#diff-c3JjL2Nhbm1hdHJpeC9jYW5tYXRyaXgucHk=) | `85.22% <83.33%> (+0.24%)` | :arrow_up: |
| [src/canmatrix/tests/test\_canmatrix.py](https://codecov.io/gh/ebroecker/canmatrix/pull/475/diff?src=pr&el=tree#diff-c3JjL2Nhbm1hdHJpeC90ZXN0cy90ZXN0X2Nhbm1hdHJpeC5weQ==) | `97.69% <100.00%> (+0.02%)` | :arrow_up: |
| [src/canmatrix/tests/test\_j1939\_decoder.py](https://codecov.io/gh/ebroecker/canmatrix/pull/475/diff?src=pr&el=tree#diff-c3JjL2Nhbm1hdHJpeC90ZXN0cy90ZXN0X2oxOTM5X2RlY29kZXIucHk=) | `100.00% <100.00%> (ø)` | |
| [src/canmatrix/formats/\_\_init\_\_.py](https://codecov.io/gh/ebroecker/canmatrix/pull/475/diff?src=pr&el=tree#diff-c3JjL2Nhbm1hdHJpeC9mb3JtYXRzL19faW5pdF9fLnB5) | `82.47% <0.00%> (+1.03%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/ebroecker/canmatrix/pull/475?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/ebroecker/canmatrix/pull/475?src=pr&el=footer). Last update [08ec80e...f70bacf](https://codecov.io/gh/ebroecker/canmatrix/pull/475?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/src/canmatrix/canmatrix.py b/src/canmatrix/canmatrix.py
index 355b7c3..78dd771 100644
--- a/src/canmatrix/canmatrix.py
+++ b/src/canmatrix/canmatrix.py
@@ -606,25 +606,25 @@ class ArbitrationId(object):
     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 @@ class ArbitrationId(object):
     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 @@ class ArbitrationId(object):
             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 @@ class ArbitrationId(object):
     @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
diff --git a/src/canmatrix/j1939_decoder.py b/src/canmatrix/j1939_decoder.py
index 9653d34..c5e01cc 100644
--- a/src/canmatrix/j1939_decoder.py
+++ b/src/canmatrix/j1939_decoder.py
@@ -78,7 +78,6 @@ class j1939_decoder(object):
 
         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)
 


Test patch:
diff --git a/src/canmatrix/tests/test_canmatrix.py b/src/canmatrix/tests/test_canmatrix.py
index 2bbf935..c08bcd8 100644
--- a/src/canmatrix/tests/test_canmatrix.py
+++ b/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
diff --git a/src/canmatrix/tests/test_j1939_decoder.py b/src/canmatrix/tests/test_j1939_decoder.py
index d318060..bb42f45 100644
--- a/src/canmatrix/tests/test_j1939_decoder.py
+++ b/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
\ No newline at end of file