Add support for multiple Cyclic Messages in Task #610

karlding · 2019-06-02T04:23:47Z

This adds support for multiple Cyclic Messages in a Cyclic Task. The
default implementation is also changed to provide support for this, by
iterating over the list of Cyclic Messages.

The SocketCAN interface takes advantage of the Linux BCM APIs to do so,
while the IXXAT interface maintains its original behaviour.

This also introduces a new example that illustrates how to use Cyclic
Messages, backed by the SocketCAN interface.

Fixes #606

karlding · 2019-06-02T04:25:46Z

can/broadcastmanager.py

-        self.message = message
-        self.can_id = message.arbitration_id
-        self.arbitration_id = message.arbitration_id
+        if not isinstance(messages, list):


TBH I'm not really a fan of keeping backwards compatibility and supporting both a List and a Message type. It seems kludgy to muddle the API like that. Plus, a single Message can still be supported by a list of length 1...

No one has really requested this feature for years and neither one of IXXAT, Kvaser, NI-CAN, or Systec interfaces which provide cyclic message transmissions have seen the need for this and won't support it. Therefore I believe it is not justified to force everyone to change their implementations for the very few people that need this. At the very least we must have a deprecation period to allow implementations to adjust. A library that changes its API in backwards incompatible ways too often are not very popular among developers relying on it for critical applications.

Maybe it only has to be converted to a list at one place. Maybe in the send_periodic() method?

I appreciate how you've (mostly) kept backwards compatibility at the user facing API level - I'm strongly in favor of that.

I agree that it feels kludgy to support both List[Message] and Message throughout the stack and would prefer to try limit that to bus.send_periodic(). The Task returned will still have to adhere to the current api - e.g. ModifiableCyclicTaskABC.modify_data must continue to take a single message (possibly a List[Message] as well).

codecov · 2019-06-02T04:32:45Z

Codecov Report

Merging #610 into develop will decrease coverage by 0.3%.
The diff coverage is 29.35%.

@@             Coverage Diff             @@
##           develop     #610      +/-   ##
===========================================
- Coverage    63.65%   63.34%   -0.31%     
===========================================
  Files           63       63              
  Lines         5544     5582      +38     
===========================================
+ Hits          3529     3536       +7     
- Misses        2015     2046      +31

codecov · 2019-06-02T04:32:46Z

Codecov Report

Merging #610 into develop will decrease coverage by 2.76%.
The diff coverage is 78.12%.

@@             Coverage Diff             @@
##           develop     #610      +/-   ##
===========================================
- Coverage    63.14%   60.37%   -2.77%     
===========================================
  Files           66       66              
  Lines         5904     5908       +4     
===========================================
- Hits          3728     3567     -161     
- Misses        2176     2341     +165

christiansandberg

Perhaps you could refactor out some of the repeated checks to a utility function.

christiansandberg · 2019-06-02T07:45:16Z

can/broadcastmanager.py

+
+        # Take the Arbitration ID of the first element
+        self.can_id = messages[0].arbitration_id
+        self.arbitration_id = messages[0].arbitration_id


Could we perhaps remove one of these now?

christiansandberg · 2019-06-02T07:46:50Z

can/broadcastmanager.py

        while not self.stopped:
            # Prevent calling bus.send from multiple threads
            with self.lock:
                started = time.time()
                try:
-                    self.bus.send(self.message)
+                    self.bus.send(self.messages[msg_index])
+                    msg_index = (msg_index + 1) % len(self.messages)


You could move the msg_index increment outside the lock.

Maybe I'm missing something, but why is this lock even necessary? What is this lock intended to protect?

From what I understand, there is a 1:1 mapping between cyclic tasks and threads, so _run is essentially running in its own context for each thread. As such, the thread can get preempted, but nothing should be modifying its context state while it is context switched out.

According to git blame, you were the one that added this locking, so I thought I'd take the opportunity to ask you 😂

If this lock is actually protecting against other threads poking into the context, then I believe the increment still needs to be protected by the lock, since Python doesn't provide atomics.

I think the lock prevents Bus.send(...) from being called concurrently, and not the update of msg_index. This is required since Bus.send(...) is not guaranteed to be thread safe.

Ah, thanks!

christiansandberg · 2019-06-02T07:48:55Z

can/interfaces/ixxat/canlib.py

+    def __init__(self, scheduler, msgs, period, duration, resolution):
+        super().__init__(msgs, period, duration)
+        # Only supports 1 Message in the group?
+        if len(self.messages) == 1:


I would probably just do a check in the beginning instead.

if len(self.messages) != 1: raise ValueError("IXXAT Interface only supports 1 element")

christiansandberg · 2019-06-02T07:49:53Z

can/interfaces/ixxat/canlib.py

-            self._index = ctypes.c_uint32()
-            _canlib.canSchedulerAddMessage(self._scheduler, self._msg, self._index)
-        _canlib.canSchedulerStartMessage(self._scheduler, self._index, self._count)
+        if len(self.messages) == 1:


Don't think we need to check self.messages again since it should not have changed.

christiansandberg · 2019-06-02T07:50:29Z

can/interfaces/socketcan/socketcan.py

+        body = bytearray()
+        for message in messages:
+            self.can_id_with_flags = _add_flags_to_can_id(message)
+            self.flags = CAN_FD_FRAME if message.is_fd else 0


These two can be set outside the loop to the value of the first message.

christiansandberg · 2019-06-02T07:50:43Z

can/interfaces/socketcan/socketcan.py

+                ival1 = 0
+                ival2 = self.period
+            body += build_can_frame(message)
+            log.debug("Sending BCM command")


Move outside loop.

christiansandberg · 2019-06-02T07:51:31Z

can/interfaces/socketcan/socketcan.py

+        )
+        for message in messages:
+            self.can_id_with_flags = _add_flags_to_can_id(message)
+            self.flags = CAN_FD_FRAME if message.is_fd else 0


Move outside loop using first entry.

christiansandberg · 2019-06-02T07:51:41Z

can/interfaces/socketcan/socketcan.py

+            self.can_id_with_flags = _add_flags_to_can_id(message)
+            self.flags = CAN_FD_FRAME if message.is_fd else 0
+            body += build_can_frame(message)
+            log.debug("Sending BCM command")


Move outside loop.

hardbyte

Overall I think this is a good addition.

We should improve the docs and add a couple of tests similar to SimpleCyclicSendTaskTest before merging.

hardbyte · 2019-06-03T07:22:25Z

can/bus.py

@@ -174,7 +174,7 @@ def send_periodic(self, msg, period, duration=None, store_task=True):
        - :meth:`BusABC.stop_all_periodic_tasks()` is called
        - the task's :meth:`CyclicTask.stop()` method is called.

-        :param can.Message msg:
+        :param List[can.Message] msgs:


This should be something like Union[List[can.Message], can.Message], the docstring should be updated too.

hardbyte · 2019-06-03T07:26:12Z

can/interfaces/ixxat/canlib.py

+    def __init__(self, scheduler, msgs, period, duration, resolution):
+        super().__init__(msgs, period, duration)
+        if len(self.messages) != 1:
+            raise ValueError("IXXAT Interface only supports 1 element")


Suggested change

raise ValueError("IXXAT Interface only supports 1 element")

raise ValueError("IXXAT Interface only supports periodic sending of 1 element")

can/interfaces/socketcan/socketcan.py

examples/cyclic_multiple.py

hardbyte · 2019-06-03T09:53:40Z

can/broadcastmanager.py

-        self.message = message
-        self.can_id = message.arbitration_id
-        self.arbitration_id = message.arbitration_id
+        if not isinstance(messages, list):


I appreciate how you've (mostly) kept backwards compatibility at the user facing API level - I'm strongly in favor of that.

I agree that it feels kludgy to support both List[Message] and Message throughout the stack and would prefer to try limit that to bus.send_periodic(). The Task returned will still have to adhere to the current api - e.g. ModifiableCyclicTaskABC.modify_data must continue to take a single message (possibly a List[Message] as well).

examples/cyclic_multiple.py

can/broadcastmanager.py

felixdivo

Sorry, I don't have time for a full review but am okay with changing the API to use lists.

can/broadcastmanager.py

can/bus.py

can/broadcastmanager.py

hardbyte · 2019-06-06T12:44:10Z

can/interfaces/socketcan/socketcan.py

+        Note: The number of new cyclic messages to be sent must be equal to the
+        original number of messages originally specified for this task.
+
+        :param Union[List[can.Message], tuple(can.Message), can.Message] messages:


nit: I'd do this with typing.Sequence: Union[Sequence[can.Message], can.Message]

Or typing.Iterable instead of Sequence?

@felixdivo correct me if I'm wrong, but I don't think you can get the length of an Iterable without actually performing the iteration and storing the result.

If want to support an Iterable, do we want to deal with buffering potentially unbounded input? Or implement specific bounded iterators for each interface that only iterates over the maximum number of elements the interface task can support, and errors otherwise?

Ah I see. We could wrap it in a list if it's not already a sequence. Implementing something like that ourselves seems overkill. If you want, stay with what you have or a sequence.

pierreluctg · 2019-06-07T11:57:50Z

can/broadcastmanager.py

+
+        return messages
+
+    def _check_modified_messages(self, messages):


Not sure this is the write location for this method since it's only ever use by ModifiableCyclicTaskABC.modify_data. Maybe it should be in the ModifiableCyclicTaskABC class or nested inside ModifiableCyclicTaskABC.modify_data.

pierreluctg · 2019-06-19T18:26:49Z

can/broadcastmanager.py

        """
-        self.message = message
+        messages = self._check_and_convert_messages(messages)
+        messages = self._check_modified_messages(messages)


Minor, but self._check_modified_messages(messages) do not make any modification to messages, so you do not really need to return messages

christiansandberg

Haven’t checked in detail but seems good.

can/interfaces/socketcan/socketcan.py

This adds support for multiple Cyclic Messages in a Cyclic Task. The default implementation is also changed to provide support for this, by iterating over the list of Cyclic Messages. In order to maintain backwards compatibility, the periodic APIs now take a CAN Message as before, in addition to a Sequence of CAN Messages. The SocketCAN interface takes advantage of the Linux BCM APIs to do so, while the IXXAT interface maintains its original behaviour. This also introduces a new example that illustrates how to use Cyclic Messages, backed by the SocketCAN interface. We previously tracked the can_id and arbitration_id class members due to the ongoing deprecation of the can_id Message attribute. Now that can_id is replaced by arbitration_id, we no longer need this in CyclicSendTaskABC either. As such, this removes the deprecated can_id member from the Cyclic Task. Fixes hardbyte#606

karlding requested review from hardbyte, christiansandberg and felixdivo June 2, 2019 04:23

karlding commented Jun 2, 2019

View reviewed changes

christiansandberg requested changes Jun 2, 2019

View reviewed changes

hardbyte requested changes Jun 3, 2019

View reviewed changes

pierreluctg reviewed Jun 3, 2019

View reviewed changes

can/broadcastmanager.py Outdated Show resolved Hide resolved

pierreluctg reviewed Jun 3, 2019

View reviewed changes

can/broadcastmanager.py Outdated Show resolved Hide resolved

pierreluctg reviewed Jun 3, 2019

View reviewed changes

can/broadcastmanager.py Show resolved Hide resolved

felixdivo reviewed Jun 4, 2019

View reviewed changes

karlding force-pushed the add_support_for_multiple_cyclic_messages branch from 46fd898 to 21eec39 Compare June 4, 2019 08:00

pierreluctg reviewed Jun 4, 2019

View reviewed changes

can/broadcastmanager.py Show resolved Hide resolved

pierreluctg reviewed Jun 4, 2019

View reviewed changes

can/bus.py Show resolved Hide resolved

pierreluctg reviewed Jun 6, 2019

View reviewed changes

can/broadcastmanager.py Show resolved Hide resolved

hardbyte approved these changes Jun 6, 2019

View reviewed changes

pierreluctg reviewed Jun 7, 2019

View reviewed changes

karlding force-pushed the add_support_for_multiple_cyclic_messages branch 4 times, most recently from 900ccca to b12ea74 Compare June 18, 2019 23:11

pierreluctg reviewed Jun 19, 2019

View reviewed changes

karlding force-pushed the add_support_for_multiple_cyclic_messages branch from b12ea74 to f4f2bc7 Compare June 22, 2019 22:54

felixdivo assigned karlding Jun 23, 2019

felixdivo added api enhancement bug labels Jun 23, 2019

felixdivo added this to the 4.0 Release milestone Jun 23, 2019

karlding force-pushed the add_support_for_multiple_cyclic_messages branch from f4f2bc7 to 3ff4476 Compare June 23, 2019 14:57

karlding force-pushed the add_support_for_multiple_cyclic_messages branch from 3ff4476 to 4f45fd5 Compare June 24, 2019 07:00

pierreluctg approved these changes Jun 25, 2019

View reviewed changes

hardbyte requested a review from christiansandberg June 26, 2019 23:36

christiansandberg approved these changes Jun 27, 2019

View reviewed changes

can/interfaces/socketcan/socketcan.py Outdated Show resolved Hide resolved

karlding force-pushed the add_support_for_multiple_cyclic_messages branch from 4f45fd5 to 7671687 Compare June 28, 2019 04:25

karlding force-pushed the add_support_for_multiple_cyclic_messages branch from 7671687 to c5cff12 Compare June 28, 2019 06:28

Merge branch 'develop' into add_support_for_multiple_cyclic_messages

c3cf602

karlding merged commit 56f153f into hardbyte:develop Jul 10, 2019

karlding deleted the add_support_for_multiple_cyclic_messages branch July 11, 2019 03:15

karlding mentioned this pull request May 2, 2020

Example: crc.py, AttributeError: 'list' object has no attribute 'channel' #823

Closed

aquatsr mentioned this pull request May 25, 2020

Possible to Port Multiple Cyclic Messages in Task to 3.x Release #842

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple Cyclic Messages in Task #610

Add support for multiple Cyclic Messages in Task #610

karlding commented Jun 2, 2019

karlding Jun 2, 2019

christiansandberg Jun 2, 2019

hardbyte Jun 3, 2019

codecov bot commented Jun 2, 2019

codecov bot commented Jun 2, 2019 •

edited

christiansandberg left a comment

christiansandberg Jun 2, 2019

christiansandberg Jun 2, 2019

karlding Jun 3, 2019

felixdivo Jun 3, 2019

karlding Jun 3, 2019

christiansandberg Jun 2, 2019

christiansandberg Jun 2, 2019

christiansandberg Jun 2, 2019

christiansandberg Jun 2, 2019

christiansandberg Jun 2, 2019

christiansandberg Jun 2, 2019

hardbyte left a comment

hardbyte Jun 3, 2019

hardbyte Jun 3, 2019

hardbyte Jun 3, 2019

felixdivo left a comment

hardbyte Jun 6, 2019

felixdivo Jun 6, 2019 •

edited

karlding Jun 6, 2019

felixdivo Jun 7, 2019

pierreluctg Jun 7, 2019

pierreluctg Jun 19, 2019

christiansandberg left a comment

	raise ValueError("IXXAT Interface only supports 1 element")
	raise ValueError("IXXAT Interface only supports periodic sending of 1 element")


		return messages

		def _check_modified_messages(self, messages):

Add support for multiple Cyclic Messages in Task #610

Add support for multiple Cyclic Messages in Task #610

Conversation

karlding commented Jun 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 2, 2019

Codecov Report

codecov bot commented Jun 2, 2019 • edited

Codecov Report

christiansandberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixdivo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixdivo Jun 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christiansandberg left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 2, 2019 •

edited

felixdivo Jun 6, 2019 •

edited