send_periodic() only sends messages once and then exits

[Felhamed]

I am using python-can with Python 3.5.2 and 4.13.0-37-generic #42~16.04.1-Ubuntu with the following code:

#!/usr/bin/python3
import can
import time
import logging

logging.basicConfig(level=logging.INFO)

def periodic_send(msg,bus):
        task = bus.send_periodic(msg, 0.20)
        assert isinstance(task, can.CyclicSendTaskABC)

if __name__ == "__main__":
    bus = can.interface.Bus(bustype='socketcan', channel='can0', bitrate=250000)
    can_ids = [0x181080F5, 0x181081F5, 0x181082F5, 0x181083F5, 0x181084F5, 0x181085F5, 0x181086F5, 0x181087F5, 0x181088F5, 0x181089F5]
    for can_id in can_ids:
        msg_volt = can.Message(arbitration_id=can_id, data=[1, 0, 0, 0, 0, 0, 0, 0], extended_id=True)
        periodic_send(msg_volt,bus)
        msg_temp = can.Message(arbitration_id=can_id, data=[2, 0, 0, 0, 0, 0, 0, 0], extended_id=True)
        periodic_send(msg_temp,bus)

I am trying to send each set of data over the 10 can_ids in the array periodically (200ms) forever, when I try to run the script it only sends each message once and exits. Am I missing something? Appreciate your help.

[christiansandberg]

What happens if you add a long sleep after?

[Felhamed]

#!/usr/bin/python3
import can
import time
import logging

logging.basicConfig(level=logging.INFO)

def periodic_send(msg,bus):
        task = bus.send_periodic(msg, 0.20)
        assert isinstance(task, can.CyclicSendTaskABC)

if __name__ == "__main__":
    bus = can.interface.Bus(bustype='socketcan', channel='can0', bitrate=250000)
    can_ids = [0x181080F5, 0x181081F5, 0x181082F5, 0x181083F5, 0x181084F5, 0x181085F5, 0x181086F5, 0x181087F5, 0x181088F5, 0x181089F5]
    for can_id in can_ids:
        msg_volt = can.Message(arbitration_id=can_id, data=[1, 0, 0, 0, 0, 0, 0, 0], extended_id=True)
        periodic_send(msg_volt,bus)
        msg_temp = can.Message(arbitration_id=can_id, data=[2, 0, 0, 0, 0, 0, 0, 0], extended_id=True)
        periodic_send(msg_temp,bus)
    time.sleep(200)

It will send 1 round of messages then nothing for the remainder of the time

[eerimoq]

The modified script below successfully sends the frames periodically for 200 seconds. The original script fails because there is no reference to the task once periodic_send() returns, and it is freed by Python.

#!/usr/bin/python3
import can
import time
import logging

logging.basicConfig(level=logging.INFO)

def periodic_send(msg, bus):
    task = bus.send_periodic(msg, 0.20)
    assert isinstance(task, can.CyclicSendTaskABC)

    return task

if __name__ == "__main__":
    bus = can.interface.Bus(bustype='socketcan', channel='vcan0', bitrate=250000)
    can_ids = [
        0x181080F5, 0x181081F5, 0x181082F5, 0x181083F5,
        0x181084F5, 0x181085F5, 0x181086F5, 0x181087F5,
        0x181088F5, 0x181089F5
    ]
    tasks = []

    for can_id in can_ids:
        msg_volt = can.Message(arbitration_id=can_id,
                               data=[1, 0, 0, 0, 0, 0, 0, 0],
                               extended_id=True)
        tasks.append(periodic_send(msg_volt,bus))
        msg_temp = can.Message(arbitration_id=can_id,
                               data=[2, 0, 0, 0, 0, 0, 0, 0],
                               extended_id=True)
        tasks.append(periodic_send(msg_temp,bus))

    time.sleep(200)

[christiansandberg]

Right, the socket is garbage collected otherwise. Maybe we should keep internal references to the cyclic tasks when created?

[eerimoq]

It sounds like a good idea as most users probably expects the message to be periodically sent even if the returned task of not referenced.

How would the internal reference be removed once no longer needed?

[Felhamed]

Thanks for the code modification @eerimoq it is working now. I am basically trying to gather data from energy systems remotely and require to have the CAN messages sent as long as the script is running, the CAN message starting with 1 requests voltage data and the one starting with a 2 requests temperature data. I had assumed that the periodic function generated a thread and runs it indefinitely unless I specified a stop timer. For my purpose, how do you propose to set the script to run unless stopped. I was thinking of replacing the last time.sleep with

while True:
    time.sleep(1)

[christiansandberg]

The method for sending periodically differs depending on the interface's capabilities. For SocketCAN the kernel's broadcast manager is used to delegate to a lower level with better timing. However, for most other interfaces a simple thread is used.

Something like this is common:

while True:
    try:
        time.sleep(1)
    except KeyboardInterrupt:
        print('Stopping')
        break

I'm not sure how to handle removing of references when the message is stopped. We don't want to leak memory and socket connections...

[felixdivo]

We could attach the references pointing to the CyclicTasks to a set in bus instance, and stop/delete them in the shutdown() method. That would also ensure that they are shut down gracefully instead of crashing eventually when writing to a closed bus. Manually stopping such a task should cause the task to remove itself from the bus instance. This should in my opinion be handled by an interface agnostic API.

[hardbyte]

At a bare minimum we need to document this behavior in https://python-can.readthedocs.io/en/develop/bcm.html#broadcast-manager

[hardbyte]

@christiansandberg do you know what happens when calling stop() to a non-started, or already stopped task? I know with socketcan we would send another TX_DELETE to the kernel's bcm interface.

Assuming that is okay I'd say we go with your idea @felixdivo. The Bus.send_periodic api is changed to keep a reference to the task in the bus instance and in Bus.shutdown we call stop() on all the (running/remaining) tasks.

I know python-can often gets used in tools which run for days/months so we don't really want to keep all periodic tasks around if they have been manually stopped. Storing a Weak Reference in the Bus won't help us because then we don't get the benefit of keeping the task alive while the Bus is there. I guess we can hijak the Task.stop() or Task.__del__ method to remove itself from the Bus (note there are restartable tasks...) but I'm open to any better ideas?

[christiansandberg]

For socketcan, couldn’t we have one BCM socket per bus? Then all periodic transmissions will be active until it’s closed at shutdown.