Incorrect frame re-transmission when arbitration ID is duplicate

[sbourkedev]

When arbitration ID is duplicate, only the last message with that arbitration ID is re-transmitted according to period.

Below is a code snippet of how messages are defined.
Note that pm_snapshot_msg, vin1_msg, vin2_msg & vin3_msg have the same arbitration_id 0x400.

self.pm_msg = can.Message(
            arbitration_id=0xFF, 
            data=self.pm_data, 
            extended_id=False, 
            dlc=8)

        self.pm_snapshot_msg = can.Message(
            arbitration_id=0x400, 
            data=self.pm_snapshot_data,
            extended_id=False, 
            dlc=8)
        self.nm_msg = can.Message(
            arbitration_id=0x51E, 
            data=self.nm_data, 
            extended_id=False, 
            dlc=8)

        self.vs_msg = can.Message(
            arbitration_id=0x11, 
            data=self.vs_data, 
            extended_id=False, 
            dlc=8)

        self.vin1_msg = can.Message(
            arbitration_id=0x400, 
            data=self.vin1_data,
            extended_id=False, 
            dlc=8)

        self.vin2_msg = can.Message(
            arbitration_id=0x400, 
            data=self.vin2_data,
            extended_id=False, 
            dlc=8)

        self.vin3_msg = can.Message(
            arbitration_id=0x400, 
            data=self.vin3_data,
            extended_id=False, 
            dlc=8)

Below is code where messages are sent.

def wake(self):
        if self.can_state == "sleep":
            self.nm_data[1] = 0x2
            self.nm_msg.data = self.nm_data
            self.nm_task = self.bus.send_periodic(self.nm_msg, self.nm_period)
            if not isinstance(self.nm_task, can.ModifiableCyclicTaskABC):
                print("This interface doesn't seem to support modification")
                task.stop()
                exit(-1)
            self.pm_task = self.bus.send_periodic(self.pm_msg, self.pm_period)
            self.pm_snapshot_task = self.bus.send_periodic(self.pm_snapshot_msg, self.pm_snapshot_period)
            self.vs_task = self.bus.send_periodic(self.vs_msg, self.vs_period)
            self.vin1_task = self.bus.send_periodic(self.vin1_msg, self.vin_period)
            self.vin2_task = self.bus.send_periodic(self.vin2_msg, self.vin_period)
            self.vin3_task = self.bus.send_periodic(self.vin3_msg, self.vin_period)
            self.can_state = "active"
        else:
            print("Sim already running")

Below is the candump after wake() is called. Note that all messages are transmitted at first, but after that only the last message with arbitration id of 0x400 is sent, namely vin3_msg, which was the last message to be defined with arbitration id of 0x400.

can0  51E   [8]  10 02 00 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  400   [8]  B0 A1 B1 C1 D1 E1 04 00 <-- pm_snapshot_msg 0x400
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  400   [8]  57 00 00 00 4B 4C 31 54  <-- vin1_msg 0x400
  can0  400   [8]  58 44 35 44 45 33 42 42  <-- vin2_msg 0x400
  can0  400   [8]  59 31 39 35 37 35 36 00  <-- vin3_msg 0x400
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  51E   [8]  10 02 00 00 00 00 00 00
  can0  400   [8]  59 31 39 35 37 35 36 00  <-- vin3_msg 0x400
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  51E   [8]  10 02 00 00 00 00 00 00
  can0  400   [8]  59 31 39 35 37 35 36 00  <-- vin3_msg 0x400
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  51E   [8]  10 02 00 00 00 00 00 00
  can0  400   [8]  59 31 39 35 37 35 36 00  <-- vin3_msg 0x400
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  51E   [8]  10 02 00 00 00 00 00 00
  can0  400   [8]  59 31 39 35 37 35 36 00  <-- vin3_msg 0x400
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  51E   [8]  10 02 00 00 00 00 00 00
  can0  400   [8]  59 31 39 35 37 35 36 00  <-- vin3_msg 0x400
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00

[christiansandberg]

It’s a limitation in SocketCAN that you can only have one periodic transmission per ID. Why would you want to have two?

[sbourkedev]

According to the spec we have been given, the arbitration IDs for pm_snapshot and VIN must be the same, but if this is a socketcan limitation then there isn't much we can do. Perhaps an implementation in C rather than Python would better suited, although the socketcan limitations would still apply.

[sbourkedev]

Also, this would explain Issue 605, as that pertains to duplicate IDs also.

[sbourkedev]

Will try a C implementation and come back with more information.

[christiansandberg]

When you receive a message with ID 0x400, how do you know which one it is?

[sbourkedev]

I have tested an implementation using socketcan in C and it seems to be working. By passing each CAN frame to an array, socketcan has no issues with duplicate arbitration ID.

struct can_frame VIN_number_1 = {.can_id  = 0x400,
                                  .can_dlc = 8,
                                  .data[0] = 0x57,
                                  .data[1] = 0x00,
                                  .data[2] = 0x00,
                                  .data[3] = 0x00,
                                  .data[4] = 0x4B,
                                  .data[5] = 0x4C,
                                  .data[6] = 0x31,
                                  .data[7] = 0x54
                                };

struct can_frame VIN_number_2 = {.can_id  = 0x400,
                                  .can_dlc = 8,
                                  .data[0] = 0x58,
                                  .data[1] = 0x44,
                                  .data[2] = 0x35,
                                  .data[3] = 0x44,
                                  .data[4] = 0x45,
                                  .data[5] = 0x33,
                                  .data[6] = 0x42,
                                  .data[7] = 0x42
                                };

struct can_frame VIN_number_3 = {.can_id  = 0x400,
                                  .can_dlc = 8,
                                  .data[0] = 0x59,
                                  .data[1] = 0x31,
                                  .data[2] = 0x39,
                                  .data[3] = 0x35,
                                  .data[4] = 0x37,
                                  .data[5] = 0x35,
                                  .data[6] = 0x36,
                                  .data[7] = 0x00
                                };

struct can_frame *ptr_array[] = {&VIN_number_1, &VIN_number_2, &VIN_number_3};

Resulting candump

  can0  400   [8]  57 00 00 00 4B 4C 31 54 
  can0  400   [8]  58 44 35 44 45 33 42 42  
  can0  400   [8]  59 31 39 35 37 35 36 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  51E   [8]  10 02 00 00 00 00 00 00
  can0  400   [8]  57 00 00 00 4B 4C 31 54 
  can0  400   [8]  58 44 35 44 45 33 42 42  
  can0  400   [8]  59 31 39 35 37 35 36 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  0FF   [8]  00 00 04 60 00 00 00 00
  can0  011   [8]  00 C0 0C 00 00 00 00 00
  can0  51E   [8]  10 02 00 00 00 00 00 00
  can0  400   [8]  57 00 00 00 4B 4C 31 54 
  can0  400   [8]  58 44 35 44 45 33 42 42  
  can0  400   [8]  59 31 39 35 37 35 36 00

Would much prefer a Python solution though.

[karlding]

Yeah, this is because (at least for the SocketCAN interface) python-can always allocates an array of size 1 when sending the TX_SETUP opcode. If you look at build_bcm_transmit_header:

    nframes = 1

And build_bcm_update_header:

def build_bcm_update_header(can_id, msg_flags):
    return build_bcm_header(socket.CAN_BCM_TX_SETUP, msg_flags, 0, 0, 0, 0, 0, can_id, 1)

If we consider the SocketCAN BCM C interface, it will cycle through nframes, with the time set in your ival1/ival2 between each, as you pointed out. For > 1 messages, it's basically providing an API to implement Signal Multiplexing on a cyclic message.

However, the key difference is that currently, python-can only provides cyclic transmission tasks on a Message level (hence why it takes the last specified Message), instead of providing it on an Arbitration ID level (and allowing you to specify multiple Messages that have the same ID which should be cyclic). Perhaps another API should be introduced?

@sbourkedev what are the semantics that you expect python-can to provide when you pass a second Message with the same Arbitration ID?

[sbourkedev]

Ideally to allow for messages with the same arbitration IDs to be transmitted as you pointed out above, which would mimic how socketcan handles cyclic transmissions. As mentioned above this may link up with Issue 605, which seems to have the same root cause, attempting to stop transmissions when the messages have the same arbitration ID.

[christiansandberg]

One option could be to allow a list of messages in addition to a single message. Some interfaces like IXXAT would not support this however.

[felixdivo]

We could still fall back to iterating over the messages in such a case.

[karlding]

Yeah. Currently the *CyclicSendTasks provide semantics that only allow for 1 message per arbitration ID. Is the plan therefore to introduce a breaking change in the behaviour so that bus.send_periodic will take an Iterable instead of a Message? Would the introduction of any additional APIs make sense?

The next release is also 4.0, so an API breakage like this isn't the worst possible thing, and technically still follows SemVer if this proposed change is taken.

I think there's a few changes to each of the interfaces to support the cases that would need to be handled:

1. The interface does not natively support cyclic Messages. Changes in the generic implementation will support this.
2. The interface supports some bounded number of cyclic Messages per ID. This will be supported up to the maximum number of cyclic Messages the interface supports. Any additional Messages should probably result in falling back to the generic implementation.

Note: I don't think any interface would support an unbounded number of cyclic messages, which is why it's not explicitly called out.

I wrote up a quick Proof-of-Concept with just the bare minimum changes in the SocketCAN interface (with no regard for support with other interfaces or error-checking) to mirror the behaviour for the Linux BCM interface for TX, but some additional cases probably need some consideration. If we're going forward with this change, I can clean it up and get the ball rolling on adding support for this.

[sbourkedev]

Tested using the the wip_socketcan_bcm_multiple_cyclic_support branch and appears to be working. Will wait for release version to implement locally.

[christiansandberg]

I still believe this is an edge case, so we shouldn’t break backwards compatibility. Most people will just need to send one message. Both a single message and an iterable of messages should be accepted.