amitlissack commented 3 years ago

Description

Using PCAN interface on the mac using version >=0.9 of libPCBUSB results in ID and DLC errors.

The ID has unexpected bits well beyond bit 29. And the DLC is always zero.

Reproduce

install version >=0.9 of libPCBUSB. Read a message from a connected device on a MAC using PCAN interface.

Expected Behaviour

The ID will be 11 or 29 bits. The DLC will be the data byte count in the message.

Further explanation

I am using python-can on a MAC test some my firmware using a PEAK CANFD analyzer. The FW will loopback known messages.

The test python code is this:

import can

bus3 = can.interface.Bus(bustype='pcan')

def send_receive(m):
    print(m)
    bus3.send(m)
    r = bus3.recv(1)
    print(r)

messages = (
    can.Message(extended_id=True, arbitration_id=0x3, is_fd=False, data=[0xf, 0, 0xf, 0]),
    can.Message(extended_id=True, arbitration_id=0x1, is_fd=False, data=[]),
    can.Message(extended_id=True, arbitration_id=0x4, is_fd=False, data=[]),
)

for mess in messages:
    send_receive(mess)

The output is:

Timestamp:        0.000000    ID: 00000003    X                DLC:  4    0f 00 0f 00
Timestamp: 128352589856006.984375    ID: 40200018000    S                DLC:  0
Timestamp:        0.000000    ID: 00000001    X                DLC:  0
Timestamp: 81064793768616.765625    ID: 200008000    S                DLC:  0
Timestamp:        0.000000    ID: 00000004    X                DLC:  0
Timestamp: 269934503141466.968750    ID: 200020000    S                DLC:  0

40200018000 is not a valid ID. And the DLC should be 4.

Theory

The PCAN MACOS implementation specializes the TPCANMsg to use c_ulong instead of c_uint. This change was made about two years ago.

Looking at the PCBUSB.h file include in version 0.9 and 0.10 there is a statement to the effect that the ID has changed from 64 to 32 bits.

typedef struct tagTPCANMsg
{
     DWORD             ID;      //!< 11/29-bit message identifier (ATTENTION: changed from 64-bit to 32-bit)
     TPCANMessageType  MSGTYPE; //!< Type of the message
     BYTE              LEN;     //!< Data Length Code of the message (0..8)
     BYTE              DATA[8]; //!< Data of the message (DATA[0]..DATA[7])
 } TPCANMsg;

To test this theory I changed TPCANMsgMac's ID field to be type c_uint.

Rerunning the above code the result is correct:

Timestamp:        0.000000    ID: 00000003    X                DLC:  4    0f 00 0f 00
Timestamp: 51791396190795.273438    ID: 00018000    X                DLC:  4    0f 00 0f 00
Timestamp:        0.000000    ID: 00000001    X                DLC:  0
Timestamp: 28710448100521.488281    ID: 00008000    X                DLC:  0
Timestamp:        0.000000    ID: 00000004    X                DLC:  0
Timestamp: 94294117674104.343750    ID: 00020000    X                DLC:  0

(the ID in the response is supposed to be shifted up 15 bits).

System

OS-version: 10.15.7 Python version: 3.7.4 python-can version: 3.3.4 python-can interface/s: PCAN

dominikfigueroa commented 2 years ago

Thanks so much for opening this issue. I was facing the exact same thing and changing the ID field to c_uint fixed this for me. I'm guessing downgrading to an older version (<0.9) of libPCBUSB would work too?

/usr/local/lib/python3.9/site-packages/can/interfaces/pcan/basic.py

class TPCANMsgMac (Structure):
    """
    Represents a PCAN message
    """
    _fields_ = [ ("ID",      c_uint),          # CHANGED FROM C_ULONG
                 ("MSGTYPE", TPCANMessageType), # Type of the message
                 ("LEN",     c_ubyte),          # Data Length Code of the message (0..8)
                 ("DATA",    c_ubyte * 8) ]     # Data of the message (DATA[0]..DATA[7])

dominikfigueroa commented 2 years ago

I'm guessing downgrading to an older version (<0.9) of libPCBUSB would work too?

Confirmed that libPCBUSB v0.8.0 works with python-can as-is (("ID", c_ulong)).

amitlissack commented 2 years ago

I went with this terrible monkey patch hack in order to stay version 9:

if platform.system() == "Darwin":
    # TODO (amit, 2021-09-29): remove hacks to support `pcan` when we don't
    #  need it anymore.
    #  Super bad monkey patch to deal with MAC issue:
    #  https://github.com/hardbyte/python-can/issues/1117
    from can.interfaces.pcan import basic

    basic.TPCANMsgMac = basic.TPCANMsg
    basic.TPCANMsgFDMac = basic.TPCANMsgFD
    basic.TPCANTimestampMac = basic.TPCANTimestamp

    from can.interfaces.pcan import pcan

    pcan.TPCANMsgMac = pcan.TPCANMsg
    pcan.TPCANMsgFDMac = pcan.TPCANMsgFD
    pcan.TPCANTimestampMac = pcan.TPCANTimestamp
    # end super bad monkey patch

keylevel commented 2 years ago

I am seeing similar with 0.10.1. I'm using CanAnalyzer to receive the messages and can see they are sent out with the wrong ID and always have a DLC of 0.

Diffing the PCANBasic.py example file that ships with MacCAN against basic.py shows they are basically the same, with the MacCAN version possibly being more up-to-date (except it uses 'print' instead of 'logger') and correctly using c_uint (it does not have the Mac specific classes).

It looks as if this can be used, provided the Darwin specific calls in pcan.py are removed?

hardbyte / python-can