send_periodic fails on 32bit system using socketcan

[fuchsch1234]

Trying to use send_periodic on a socketcan bus fails with the following error message:

Traceback (most recent call last):  

  File "./test.py", line 8, in <module>
    bus.send_periodic(msg, 0.01, 9)
  File "/usr/local/lib/python3.5/dist-packages/can/bus.py", line 201, in send_periodic
    task = self._send_periodic_internal(msg, period, duration)
  File "/usr/local/lib/python3.5/dist-packages/can/interfaces/socketcan/socketcan.py", line 601, in _send_periodic_internal
    task = CyclicSendTask(bcm_socket, msg, period, duration)
  File "/usr/local/lib/python3.5/dist-packages/can/interfaces/socketcan/socketcan.py", line 254, in __init__
    self._tx_setup(message)
  File "/usr/local/lib/python3.5/dist-packages/can/interfaces/socketcan/socketcan.py", line 274, in _tx_setup
    send_bcm(self.bcm_socket, header + frame)
  File "/usr/local/lib/python3.5/dist-packages/can/interfaces/socketcan/socketcan.py", line 206, in send_bcm
    raise can.CanError(base + "You are probably referring to a non-existing frame.")
can.CanError: Couldn't send CAN BCM frame. OS Error 22: Invalid argument
You are probably referring to a non-existing frame.

I'm using python-can 3.0.0 with python 3.5.3 on 32-bit Debian Stretch.

The same set of flags and values sent to the BCM is working in C.
Debugging shows the problem is a wrong size of the bcm_header. On my machine sizeof(struct bcm_msg_header) is 40 bytes, but the BCM header created in build_bcm_header is 36 bytes long. This causes the invalid argument error from the kernel.
Adding 4 padding bytes at the end of the bcm header remedies the error.

[hardbyte]

Thanks for the bug report @fuchsch1234 I just tried to replicate this on a raspberry pi:

pi@node-1:~ $ getconf WORD_BIT
32
pi@node-1:~ $ arch
armv7l
pi@node-1:~ $ python
Python 2.7.13 (default, Sep 26 2018, 18:42:22) 
[GCC 6.3.0 20170516] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> bcm_cmd_msg_fmt = "@3I4l2I0q"
>>> import struct
>>> s = struct.Struct(bcm_cmd_msg_fmt)
>>> s.size
40

The last change f119490 looks to be included in python-can 3.0.0 - would you mind confirming the issue is there in the latest release?

[fuchsch1234]

Hi,
thanks for your reply. The bug is still there in version 3.1.1:

$ getconf WORD_BIT
32
$ arch
i686
$ python
Python 3.5.3 (default, Sep 27 2018, 17:25:39)
[GCC 6.3.0 20170516] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import struct
>>> bcm_cmd_msg_fmt = "@3I4l2I0q"
>>> s = struct.Struct(bcm_cmd_msg_fmt)
>>> s.size
36
>>> import can
>>> can.__version__
'3.1.1'
>>> bus = can.Bus(interface='socketcan', channel='can0', bitrate=500000)
>>> msg = can.Message(arbitration_id=0x80, data=[])
>>> bus.send_periodic(msg, 0.01)
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/can/interfaces/socketcan/socketcan.py", line 200, in send_bcm
    return bcm_socket.send(data)
OSError: [Errno 22] Invalid argument

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.5/dist-packages/can/bus.py", line 201, in send_periodic
    task = self._send_periodic_internal(msg, period, duration)
  File "/usr/local/lib/python3.5/dist-packages/can/interfaces/socketcan/socketcan.py", line 599, in _send_periodic_internal
    task = CyclicSendTask(bcm_socket, msg, period, duration)
  File "/usr/local/lib/python3.5/dist-packages/can/interfaces/socketcan/socketcan.py", line 253, in __init__
    self._tx_setup(message)
  File "/usr/local/lib/python3.5/dist-packages/can/interfaces/socketcan/socketcan.py", line 273, in _tx_setup
    send_bcm(self.bcm_socket, header + frame)
  File "/usr/local/lib/python3.5/dist-packages/can/interfaces/socketcan/socketcan.py", line 205, in send_bcm
    raise can.CanError(base + "You are probably referring to a non-existing frame.")
can.CanError: Couldn't send CAN BCM frame. OS Error 22: Invalid argument
You are probably referring to a non-existing frame.

Which distro did you use for your tests? Maybe this is an issue with the python package from Debian, since you see a different struct size for the same format string.

[karlding]

I ran into the same issue as well (but on a 32-bit Ubuntu 16.04 LTS release). Is it okay if I take over triaging this?

[hardbyte]

That would be great @karlding

[karlding]

TL;DR

So the problem seems to be due the fact that the struct requires 8-byte alignment. This is (presumably) what the 0q tries to force, under the assumption that alignof(long long) == 8. Unfortunately, this assumption is not true for all platforms.

Debugging Notes

This seemed like an alignment issue, as the previous comments indicated that the size of the struct was 40 bytes, meaning that padding is inserted at the end of the struct in order to bring the size of the struct to a multiple of its largest scalar member (8 bytes).

So I wrote this test code to verify this theory:

#include <linux/can/bcm.h>

#include <stdalign.h>
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>

int main() {
  struct bcm_msg_head test = { 0 };
  
  printf("sizeof(long): %zu\n", sizeof(long));
  printf("alignof(long): %zu\n", alignof(long));
  printf("sizeof(long long): %zu\n", sizeof(long long));
  printf("alignof(long long): %zu\n", alignof(long long));
  printf("sizeof(struct bcm_msg_head): %zu\n", sizeof(struct bcm_msg_head));
  printf("alignof(struct bcm_msg_head): %zu\n", alignof(struct bcm_msg_head));
  return 0;
}

I then compiled this on Debian Stretch (32-bit) and Ubuntu 16.04 (32-bit):

$ gcc -g test.c && ./a.out
sizeof(long): 4
alignof(long): 4
sizeof(long long): 8
alignof(long long): 4
sizeof(struct bcm_msg_head): 40
alignof(struct bcm_msg_head): 8

Notice that alignof(long long) == 4, and not 8.

Since I had a Pi 3 lying around, I compiled the same program on Raspbian Stretch.

$ gcc -g test.c && ./a.out
sizeof(long): 4
alignof(long): 4
sizeof(long long): 8
alignof(long long): 8
sizeof(struct bcm_msg_head): 40
alignof(struct bcm_msg_head): 8

Notice how alignof(long long) == 8, unlike the failing case. This is presumably why @hardbyte was unable to reproduce the issue.

As such, the format string is behaving exactly as expected, and it seems like the 0q trick to force alignment isn't robust enough.

Potential Solutions

I don't think there's a good way in the struct library to set an 8-byte alignment, which is what is needed (since the kernel headers specify __attribute__((aligned(8))) to force 8-byte alignment). As far as I can tell, the alignment/packing rules in here are too simplistic, and if we were to keep using struct, we would need to implement that logic to insert padding bytes in the format string as necessary.

From an initial look through ctypes, it looks promising, since it seems like there's a _pack_ option that hopefully works. I'll play around with it and see what I come up with later.

Let me know if you have any other ideas, and I'd be happy to try them out!

[karlding]

Grim, so it seems like I misunderstood what _pack_ does. As it turns out, _pack_ only controls the maximum alignment, and not the minimum value, which is what we need to set.

So I think the solution would require us to insert padding bytes ourselves. I wasn't able to find a library that handled this, but perhaps there is and I just missed it.

Anyways, if we were to roll our own function, we would also need to know the alignment (as well as the sizes of each of the struct members) in order to determine whether padding bytes are needed, and struct doesn't provide us with a way of determining the alignment. As such, it probably makes sense to move the packing logic into ctypes, which allows us to derive alignment and whether padding bytes are necessary.

If that general approach sounds okay with you, I'll put up a preliminary PR for review in a bit.