python-can
python-can copied to clipboard
hardbyte
big endian support broken(?)
Describe the bug
it seems that python-can fails the test-suite on big-endian machines.
See https://ci.debian.net/data/autopkgtest/testing/s390x/p/python-can/24833388/log.gz (see below for an excerpt)
To Reproduce
run the test-suite on a big-endian architecture.
Expected behavior
big-endian is supported.
Additional context
| OS and version | Debian/GNU Linux/unstable (as of 2022-08-15) |
| Python version | 3.10 |
| python-can version | 4.0.0 |
| python-can interface/s (if applicable) | n.n. |
| Architecture | s390x |
Traceback and logs
_______ SocketCANTest.test_build_bcm_header_sizeof_long_8_alignof_long_8 _______
self = <test.test_socketcan.SocketCANTest testMethod=test_build_bcm_header_sizeof_long_8_alignof_long_8>
@unittest.skipIf(
not (
ctypes.sizeof(ctypes.c_long) == 8 and ctypes.alignment(ctypes.c_long) == 8
),
"Should only run on platforms where sizeof(long) == 8 and alignof(long) == 8",
)
def test_build_bcm_header_sizeof_long_8_alignof_long_8(self):
expected_result = (
b"\x02\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x01\x04\x00\x00\x01\x00\x00\x00"
)
> self.assertEqual(
expected_result,
build_bcm_header(
opcode=CAN_BCM_TX_DELETE,
flags=0,
count=0,
ival1_seconds=0,
ival1_usec=0,
ival2_seconds=0,
ival2_usec=0,
can_id=0x401,
nframes=1,
),
)
E AssertionError: b'\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00[176 chars]\x00' != b'\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00[176 chars]\x01'
test_socketcan.py:290: AssertionError
@hartkopp can you take a look?
Is the functionality broken or is it just a brittle test design due to the expected result here? https://github.com/hardbyte/python-can/blob/2da28c1a1c87776618a60218b0b97800cf2deb34/test/test_socketcan.py#L279
Sure!
Just take the two examples
opcode=CAN_BCM_TX_DELETEwhereopcodeis an uint32 value and CAN_BCM_TX_DELETE = 2can_id=0x401wherecan_idis an uint32 value
self.assertEqual(
expected_result,
build_bcm_header(
opcode=CAN_BCM_TX_DELETE,
flags=0,
count=0,
ival1_seconds=0,
ival1_usec=0,
ival2_seconds=0,
ival2_usec=0,
can_id=0x401,
nframes=1,
),
)
@unittest.skipIf(
not (
ctypes.sizeof(ctypes.c_long) == 8 and ctypes.alignment(ctypes.c_long) == 8
),
"Should only run on platforms where sizeof(long) == 8 and alignof(long) == 8",
)
def test_build_bcm_header_sizeof_long_8_alignof_long_8(self):
expected_result = (
b"\x02\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x01\x04\x00\x00\x01\x00\x00\x00"
)
The results are checked with a byte field ("expected_result"):
opcode=CAN_BCM_TX_DELETEresults in b"\x02\x00\x00\x00\x00\x00\x00\x00" (first line)can_id=0x401results in b"\x01\x04\x00\x00\x01\x00\x00\x00" (last line)
... which is a correct representation of unit32 values in little endian.
Using these little endian byte fields for testing on a big endian system is supposed to fail.
When testing on a big endian system is needed we would need to provide separate big endian bytefields or provide another implementation for these tests. A quick fix could be to skip those tests on big endian systems as long as there is no solution.
something like this?
diff --git a/test/test_socketcan.py b/test/test_socketcan.py
index a2c4fae..5d2de5d 100644
--- a/test/test_socketcan.py
+++ b/test/test_socketcan.py
@@ -276,6 +276,10 @@ class SocketCANTest(unittest.TestCase):
),
"Should only run on platforms where sizeof(long) == 8 and alignof(long) == 8",
)
+ @unittest.skipIf(
+ ctypes.c_uint32.__ctype_le__.__name__.endswith("le"),
+ "Should only run on Little Endian platforms",
+ )
def test_build_bcm_header_sizeof_long_8_alignof_long_8(self):
expected_result = (
b"\x02\x00\x00\x00\x00\x00\x00\x00"
I'd prefer to make the expected result dependent on the system byte order:
if sys.byteorder == "little":
expected_result = ...
else:
expected_result = ...
well, that was the "quick fix".
a somewhat nicer fix would be to convert the buffer to native byte ordering first:
From a01d03ad075911496b5a9f4d899dbd9e5f539ce5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?IOhannes=20m=20zm=C3=B6lnig?= <[email protected]>
Date: Wed, 17 Aug 2022 12:21:28 +0200
Subject: [PATCH] Convert test-data to native endianness
---
test/test_socketcan.py | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/test/test_socketcan.py b/test/test_socketcan.py
index a2c4fae..192b988 100644
--- a/test/test_socketcan.py
+++ b/test/test_socketcan.py
@@ -277,6 +277,8 @@ class SocketCANTest(unittest.TestCase):
"Should only run on platforms where sizeof(long) == 8 and alignof(long) == 8",
)
def test_build_bcm_header_sizeof_long_8_alignof_long_8(self):
+ import struct
+
expected_result = (
b"\x02\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
@@ -286,6 +288,10 @@ class SocketCANTest(unittest.TestCase):
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x01\x04\x00\x00\x01\x00\x00\x00"
)
+ fmt = "%di" % (len(expected_result) / 4,)
+ expected_result = struct.pack(
+ "@" + fmt, *struct.unpack("<" + fmt, expected_result)
+ )
self.assertEqual(
expected_result,
--
2.36.1
(i haven't actually tested this in real life)
and the same should probably be applied to test_build_bcm_header_sizeof_long_4_alignof_long_4.
so it's probably worth using a helper-function:
def bytes2native(data, item_type="i", item_size=4):
import struct
fmt = "%d%c" % (len(data) / item_size, item_type)
return struct.pack(
"@" + fmt, *struct.unpack("<" + fmt, data)
)
and then:
expected_result = bytes2native(
b"\x02\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00"
b"\x01\x04\x00\x00\x01\x00\x00\x00"
)