Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests fail on bigendian #1624

Closed
umlaeute opened this issue Jun 29, 2023 · 6 comments · Fixed by #1625
Closed

tests fail on bigendian #1624

umlaeute opened this issue Jun 29, 2023 · 6 comments · Fixed by #1625
Labels

Comments

@umlaeute
Copy link
Contributor

Describe the bug

Running the test-suite on a BingEndian architecture (like s390x), fails in two places:

  • test_bit_timing.py::test_btr_persistence FAILED
  • test_pcan.py::TestPCANBus::test_detect_available_configs FAILED
90s =================================== FAILURES ===================================
 90s _____________________________ test_btr_persistence _____________________________
 90s 
 90s     def test_btr_persistence():
 90s         f_clock = 8_000_000
 90s         for btr0btr1 in PCAN_BITRATES.values():
 90s             btr1, btr0 = struct.unpack("BB", btr0btr1)
 90s     
 90s >           t = can.BitTiming.from_registers(f_clock, btr0, btr1)
 90s 
 90s test_bit_timing.py:180: 
 90s _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 90s /usr/lib/python3/dist-packages/can/bit_timing.py:183: in from_registers
 90s     return cls(
 90s /usr/lib/python3/dist-packages/can/bit_timing.py:70: in __init__
 90s     self._validate()
 90s _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 90s 
 90s self = can.BitTiming(f_clock=8000000, brp=21, tseg1=1, tseg2=1, sjw=1, nof_samples=1)
 90s 
 90s     def _validate(self) -> None:
 90s         if not 8 <= self.nbt <= 25:
 90s >           raise ValueError(f"nominal bit time (={self.nbt}) must be in [8...25].")
 90s E           ValueError: nominal bit time (=3) must be in [8...25].
 90s 
 90s /usr/lib/python3/dist-packages/can/bit_timing.py:74: ValueError
 90s __________________ TestPCANBus.test_detect_available_configs ___________________
 90s 
 90s self = <test.test_pcan.TestPCANBus testMethod=test_detect_available_configs>
 90s 
 90s     def test_detect_available_configs(self) -> None:
 90s         if platform.system() == "Darwin":
 90s             self.mock_pcan.GetValue = Mock(
 90s                 return_value=(PCAN_ERROR_OK, PCAN_CHANNEL_AVAILABLE)
 90s             )
 90s             configs = PcanBus._detect_available_configs()
 90s             self.assertEqual(len(configs), 50)
 90s         else:
 90s             value = (TPCANChannelInformation * 1).from_buffer_copy(
 90s                 b"Q\x00\x05\x00\x01\x00\x00\x00PCAN-USB FD\x00\x00\x00\x00"
 90s                 b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
 90s                 b'\x00\x00\x00\x00\x00\x00\x003"\x11\x00\x01\x00\x00\x00'
 90s             )
 90s             self.mock_pcan.GetValue = Mock(return_value=(PCAN_ERROR_OK, value))
 90s >           configs = PcanBus._detect_available_configs()
 90s 
 90s test_pcan.py:382: 
 90s _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 90s 
 90s     @staticmethod
 90s     def _detect_available_configs():
 90s         channels = []
 90s         try:
 90s             library_handle = PCANBasic()
 90s         except OSError:
 90s             return channels
 90s     
 90s         interfaces = []
 90s     
 90s         if platform.system() != "Darwin":
 90s             res, value = library_handle.GetValue(PCAN_NONEBUS, PCAN_ATTACHED_CHANNELS)
 90s             if res != PCAN_ERROR_OK:
 90s                 return interfaces
 90s             channel_information: List[TPCANChannelInformation] = list(value)
 90s             for channel in channel_information:
 90s                 # find channel name in PCAN_CHANNEL_NAMES by value
 90s >               channel_name = next(
 90s                     _channel_name
 90s                     for _channel_name, channel_id in PCAN_CHANNEL_NAMES.items()
 90s                     if channel_id.value == channel.channel_handle
 90s                 )
 90s E               StopIteration
 90s 
 90s /usr/lib/python3/dist-packages/can/interfaces/pcan/pcan.py:691: StopIteration
 90s =========================== short test summary info ============================
 90s FAILED test_bit_timing.py::test_btr_persistence - ValueError: nominal bit tim...
 90s FAILED test_pcan.py::TestPCANBus::test_detect_available_configs - StopIteration
 90s ================== 2 failed, 427 passed, 97 skipped in 32.84s ==================

To Reproduce

Run the test-suite on a BigEndian machine

Expected behavior

tests should succeed, regardless of endianness

Additional context

component version
OS and version Debian/trixie
Python version 3.11
python-can version 4.2.2
@umlaeute umlaeute added the bug label Jun 29, 2023
@umlaeute
Copy link
Contributor Author

test_btr_persistence

for btr0btr1 in PCAN_BITRATES.values():
btr1, btr0 = struct.unpack("BB", btr0btr1)

on BigEndian, this really returns btr0, btr1.

Is the assumption of the byte-ordering broken in the test (as is indicated by the naming of the variables), or should PCAN_BITRATE hold normalized values in the first place?

@zariiii9003
Copy link
Collaborator

zariiii9003 commented Jun 29, 2023

That part can probably be solved with a <:

 for btr0btr1 in PCAN_BITRATES.values(): 
     btr1, btr0 = struct.unpack("<BB", btr0btr1) 

But the can.BitTiming.from_registers method probably needs to be adapted too. Using ctypes.LittleEndianStructure instead of bit masks and bit shifts might help. Maybe it's not necessary, I'm not sure what the bit shifts do on big endian systems.

@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 29, 2023

now really (in my tests).
it can be solved with

btr0, btr1 = struct.pack(">H", btr0btr1.value)

(which also looks more correct, as the value of btr0btr1 is a BigEndian unsigned short)

@zariiii9003
Copy link
Collaborator

Sounds good to me, would you create a PR?

@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 29, 2023

sure.

any ideas about the other issue?
i guess that

value = (TPCANChannelInformation * 1).from_buffer_copy(
b"Q\x00\x05\x00\x01\x00\x00\x00PCAN-USB FD\x00\x00\x00\x00"
b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
b'\x00\x00\x00\x00\x00\x00\x003"\x11\x00\x01\x00\x00\x00'
)
encodes the data in little endian as well...

see also #1412

@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 29, 2023

generating the data with the native endianness solves the second issue:

data=struct.pack("HBBI33sII", 81, 5, 0, 1, b'PCAN-USB FD', 1122867, 1)
value = (TPCANChannelInformation * 1).from_buffer_copy(data)

@zariiii9003 does this look good to you?

umlaeute pushed a commit to umlaeute/python-can that referenced this issue Jun 29, 2023
PCAN_BITRATES are stored in BIGendian, no use to byteswap on littleendian.
Also the TPCANChannelInformation expects data in the native byte order

Closes: hardbyte#1624
umlaeute pushed a commit to umlaeute/python-can that referenced this issue Jun 29, 2023
PCAN_BITRATES are stored in BIGendian, no use to byteswap on littleendian.
Also the TPCANChannelInformation expects data in the native byte order

Closes: hardbyte#1624
zariiii9003 pushed a commit that referenced this issue Jun 30, 2023
PCAN_BITRATES are stored in BIGendian, no use to byteswap on littleendian.
Also the TPCANChannelInformation expects data in the native byte order

Closes: #1624

Co-authored-by: IOhannes m zmölnig (Debian/GNU) <umlaeute@debian.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants