Reference to task is lost when starting a second Sync periodic

[yves-chevallier]

Consider this snippet:

import canopen
network = canopen.Network()
network.connect(channel='vcan0', bustype="socketcan")
network.sync.start(1.0)
network.sync.start(0.5)
# Using `candump -L vcan0` I have the two tasks running in parallel
network.sync.stop() # Only the latter task is stopped
network.sync.stop() # CanOperationError, the latter task is already stopped

In sync.py, we notice that attribute self._task is erased with the new task, which became inaccessible. I don't know if this hack will work, but we could only allow one sync event at the time:

diff --git a/canopen/sync.py b/canopen/sync.py
index d373451..6965b94 100644
--- a/canopen/sync.py
+++ b/canopen/sync.py
@@ -33,9 +33,14 @@ class SyncProducer:
         if not self.period:
             raise ValueError("A valid transmission period has not been given")
 
+        if self._task is not None:
+            self._task.stop()
+            self._task = None
+
         self._task = self.network.send_periodic(self.cob_id, [], self.period)
 
     def stop(self):
         """Stop periodic transmission of SYNC message."""
         if self._task is not None:
             self._task.stop()
+            self._task = None

Does it make sens?

[sveinse]

The code in sync.py seems to suggest that there isn't support for running more than one task running at any given time. Simply overwriting the old value is perhaps not the best thing to do. However, I'm not convinced stopping the former and starting a new is better, as it might be unexpected and could mask an error. I think it's better to fail with an exception if one attempts to start another when the task is running.

[acolomb]

I agree that it should raise an exception when attempting to start the SYNC transmission twice. At least while the code only supports keeping one task reference. If the need ever arises to have multiple SYNC cycles (which is certainly possible), we can relax the check and instead keep a list of task references.

[acolomb]

Thinking about it some more, I guess creating another SyncProducer instance manually is the correct way to work with several distinct SYNC transmission cycles. So the fix is correct in assuming that only one task and one period exists for a specific SyncProducer instance.