Skip to content

Fix Bus.__new__ for PEAK CAN-FD interfaces #1460

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.

Sign up for GitHub

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

Merged
zariiii9003 merged 3 commits into from
Dec 21, 2022
Merged

Fix Bus.__new__ for PEAK CAN-FD interfaces #1460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d53fb3
Add failing unit test to verify that issue #1485 is fixed
lumagi Dec 18, 2022
9e78c3f
Remove unused generic BitTiming creation in Bus.__new__
lumagi Dec 18, 2022
c6d0f87
Format code with black
lumagi Dec 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions can/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,25 +233,6 @@ def _create_bus_config(config: Dict[str, Any]) -> typechecking.BusConfig:
if "data_bitrate" in config:
config["data_bitrate"] = int(config["data_bitrate"])

# Create bit timing configuration if given
timing_conf = {}
for key in (
"f_clock",
"brp",
"tseg1",
"tseg2",
"sjw",
"nof_samples",
"btr0",
"btr1",
):
if key in config:
timing_conf[key] = int(str(config[key]), base=0)
del config[key]
Copy link
Collaborator

@zariiii9003 zariiii9003 Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried just removing this line with del instead of everything?

Copy link
Collaborator Author

@lumagi lumagi Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my specific issue with the PCAN interface, it's actually line 252 that's causing the immediate problem (although the del would also be an issue further down the road). Since it detects that f_clock is present in kwargs, it automatically tries to create the timing_conf and simply assumes that the config must also contain a bitrate key in line 252. This is not always the case for PCAN.

There are multiple reasons for me suggesting the removal of this piece of code:

  • It causes issues with at least two interfaces
  • It doesn't look like the code is actively used by any interface or anyone has picked up on it since it was merged
  • The contract between Bus.__new__ and the interface implementations is very weak. It basically says: If you name your parameters in the correct way, we'll automagically create the BitTiming instance for you. But only for Classic CAN, not CAN-FD.
  • The author itself stated in his commit that it (obviously) doesn't support CAN-FD.

if timing_conf:
timing_conf["bitrate"] = config["bitrate"]
config["timing"] = can.BitTiming(**timing_conf)

return cast(typechecking.BusConfig, config)


Expand Down
20 changes: 20 additions & 0 deletions test/test_pcan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,26 @@ def test_auto_reset_init_fault(self):
with self.assertRaises(CanInitializationError):
self.bus = can.Bus(bustype="pcan", auto_reset=True)

def test_peak_fd_bus_constructor_regression(self):
# Tests that the following issue has been fixed:
# https://github.com/hardbyte/python-can/issues/1458
params = {
"interface": "pcan",
"fd": True,
"f_clock": 80000000,
"nom_brp": 1,
"nom_tseg1": 129,
"nom_tseg2": 30,
"nom_sjw": 1,
"data_brp": 1,
"data_tseg1": 9,
"data_tseg2": 6,
"data_sjw": 1,
"channel": "PCAN_USBBUS1",
}

can.Bus(**params)


if __name__ == "__main__":
unittest.main()