From 948f141adc512a52bfa7de1647698ad0a0a0c474 Mon Sep 17 00:00:00 2001
From: Karl <karlding@users.noreply.github.com>
Date: Thu, 11 Jul 2019 10:00:11 -0700
Subject: [PATCH 1/2] Read the BCM status before creating a Task

Currently bus.send_periodic blindly wraps the BCM socket API, and sends
a BCM operation with the TX_SETUP opcode. However, the semantics that
the kernel driver provides are an "upsert". As such, when one attempts
to create two Tasks with the same CAN Arbitration ID, it ends up
silently modifying the periodic messages, which is contrary to what our
API implies.

This fixes the problem by first sending a TX_READ opcode with the CAN
Arbitration ID that we wish to create. The kernel driver will return
-EINVAL if a periodic transmission with the given Arbitration ID does
not exist. If this is the case, then we can continue creating the Task,
otherwise we error and notify the user.

Fixes #605
---
 can/interfaces/socketcan/constants.py         |  1 +
 can/interfaces/socketcan/socketcan.py         | 25 +++++++++++++++++++
 ...c_multiple.py => test_cyclic_socketcan.py} | 23 ++++++++++++++++-
 3 files changed, 48 insertions(+), 1 deletion(-)
 rename test/{test_socketcan_cyclic_multiple.py => test_cyclic_socketcan.py} (95%)

diff --git a/can/interfaces/socketcan/constants.py b/can/interfaces/socketcan/constants.py
index ba5100403..98bbc39d1 100644
--- a/can/interfaces/socketcan/constants.py
+++ b/can/interfaces/socketcan/constants.py
@@ -11,6 +11,7 @@
 # BCM opcodes
 CAN_BCM_TX_SETUP = 1
 CAN_BCM_TX_DELETE = 2
+CAN_BCM_TX_READ = 3
 
 # BCM flags
 SETTIMER = 0x0001
diff --git a/can/interfaces/socketcan/socketcan.py b/can/interfaces/socketcan/socketcan.py
index f504d1cfc..a4886722f 100644
--- a/can/interfaces/socketcan/socketcan.py
+++ b/can/interfaces/socketcan/socketcan.py
@@ -330,6 +330,31 @@ def _tx_setup(self, messages):
             ival1 = 0
             ival2 = self.period
 
+        # First do a TX_READ before creating a new task, and check if we get
+        # EINVAL. If so, then we are referring to a CAN message with the same
+        # ID
+        check_header = build_bcm_header(
+            opcode=CAN_BCM_TX_READ,
+            flags=0,
+            count=0,
+            ival1_seconds=0,
+            ival1_usec=0,
+            ival2_seconds=0,
+            ival2_usec=0,
+            can_id=self.can_id_with_flags,
+            nframes=0,
+        )
+        try:
+            self.bcm_socket.send(check_header)
+        except OSError as e:
+            assert e.errno == errno.EINVAL
+        else:
+            raise ValueError(
+                "A periodic Task for Arbitration ID {} has already been created".format(
+                    messages[0].arbitration_id
+                )
+            )
+
         header = build_bcm_transmit_header(
             self.can_id_with_flags,
             count,
diff --git a/test/test_socketcan_cyclic_multiple.py b/test/test_cyclic_socketcan.py
similarity index 95%
rename from test/test_socketcan_cyclic_multiple.py
rename to test/test_cyclic_socketcan.py
index ee2f0adaf..bd3042b38 100644
--- a/test/test_socketcan_cyclic_multiple.py
+++ b/test/test_cyclic_socketcan.py
@@ -10,7 +10,7 @@
 
 
 @unittest.skipUnless(TEST_INTERFACE_SOCKETCAN, "skip testing of socketcan")
-class SocketCanCyclicMultiple(unittest.TestCase):
+class CyclicSocketCan(unittest.TestCase):
     BITRATE = 500000
     TIMEOUT = 0.1
 
@@ -244,6 +244,27 @@ def test_cyclic_initializer_different_arbitration_ids(self):
         with self.assertRaises(ValueError):
             task = self._send_bus.send_periodic(messages, self.PERIOD)
 
+    def test_create_same_id_raises_exception(self):
+        messages_a = can.Message(
+            arbitration_id=0x401,
+            data=[0x11, 0x11, 0x11, 0x11, 0x11, 0x11],
+            is_extended_id=False,
+        )
+
+        messages_b = can.Message(
+            arbitration_id=0x401,
+            data=[0x22, 0x22, 0x22, 0x22, 0x22, 0x22],
+            is_extended_id=False,
+        )
+
+        task_a = self._send_bus.send_periodic(messages_a, 1)
+        self.assertIsInstance(task_a, can.broadcastmanager.CyclicSendTaskABC)
+
+        # The second one raises a ValueError when we attempt to create a new
+        # Task, since it has the same arbitration ID.
+        with self.assertRaises(ValueError):
+            task_b = self._send_bus.send_periodic(messages_b, 1)
+
     def test_modify_data_list(self):
         messages_odd = []
         messages_odd.append(

From 6a4da474265c4ca6a81731ee649f8da40aa816e1 Mon Sep 17 00:00:00 2001
From: Karl <karlding@users.noreply.github.com>
Date: Sat, 13 Jul 2019 14:38:59 -0700
Subject: [PATCH 2/2] Raise Exception instead of using assert on EINVAL

---
 can/interfaces/socketcan/socketcan.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/can/interfaces/socketcan/socketcan.py b/can/interfaces/socketcan/socketcan.py
index a4886722f..b99124719 100644
--- a/can/interfaces/socketcan/socketcan.py
+++ b/can/interfaces/socketcan/socketcan.py
@@ -347,7 +347,8 @@ def _tx_setup(self, messages):
         try:
             self.bcm_socket.send(check_header)
         except OSError as e:
-            assert e.errno == errno.EINVAL
+            if e.errno != errno.EINVAL:
+                raise e
         else:
             raise ValueError(
                 "A periodic Task for Arbitration ID {} has already been created".format(