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

Opening a PCanBus in FD mode with Bus.__new__ breaks with python-can 4.0.0 / 4.1.0 #1458

Closed
lumagi opened this issue Dec 12, 2022 · 1 comment · Fixed by #1460
Closed

Opening a PCanBus in FD mode with Bus.__new__ breaks with python-can 4.0.0 / 4.1.0 #1458

lumagi opened this issue Dec 12, 2022 · 1 comment · Fixed by #1460
Labels

Comments

@lumagi
Copy link
Collaborator

lumagi commented Dec 12, 2022

Describe the bug

I am using the PCAN interface in an FD configuration with custom bit timings. I provide the parameters, i.e. f_clock, nom_brp, data_brp, ..., directly as kwargs to Bus.__new__ (see example below). With the transition from version v3.3.4 to v4.1.0, I can no longer instantiate the bus and always receive the error message below.

I took a look at the code in question, and it appears like this might be a corner case that was forgotten about when adding the BitTimingclass. When instantiating the PcanBus for a CAN-FD bus, it is mandatory to supply the f_clock parameter as well as the bit timings directly to the constructor. The bitrate parameter is ignored because the PCAN driver configures the interface directly based on the timing parameters (see pcan.py).
The exception is thrown in _create_bus_config in util.py. The function in question assumes that whenever certain timing-related arguments (such as f_clock, tseg1, tseg2 or brp) are present in kwargs, the bitrate argument must be present as well. For the case of the PcanBus, this is not the case since the interface implementation does not use the BitTiming class nor requires bitrate to be set when running in CAN-FD mode.

I did some searching in the repository and found issue #614 by @christiansandberg . I also added a comment there, but didn't receive a response.

To Reproduce

See code example below

Expected behavior

The constructor should not require the bitrate parameter to be set and instantiate the bus correctly.

Additional context

OS and version: Windows
Python version: 3.9
python-can version: 4.1.0
python-can interface/s (if applicable): PEAK PCAN-USB FD

Traceback and logs
Traceback (most recent call last):
  File "<...>", line 23, in <module>
    main()
  File "<...>", line 12, in main
    bus = Bus(**params)
  File "<...>", line 108, in __new__
    kwargs = load_config(config=kwargs, context=context)
  File "<...>", line 192, in load_config
    bus_config = _create_bus_config(config)
  File "<...>", line 252, in _create_bus_config
    timing_conf["bitrate"] = config["bitrate"]
KeyError: 'bitrate'
from can import Bus

from can.interfaces.pcan import PcanBus

def main():
    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'}

    # Breaks
    bus = Bus(**params)

    # doesn't break
    bus = PcanBus(**params)

    while True:
        r = bus.recv()
        print(r)

if __name__ == "__main__":
    main()
@lumagi lumagi added the bug label Dec 12, 2022
lumagi added a commit to lumagi/python-can that referenced this issue Dec 18, 2022
lumagi added a commit to lumagi/python-can that referenced this issue Dec 18, 2022
The BitTiming class is an attempt at unifying the various timing
parameters of the individual interfaces. The idea is that instead of
manually supplying multiple parameters that make up the timing
definition of the interface, one can instead supply a single instance of
the BitTiming class, which will also automatically calculate derivative
values from its input.

At the moment, this class is only used by two interfaces: CANtact and
CANanalystii. Both either accept a single bitrate or a BitTiming
instance. The latter will overrule the former.

The code that is removed with this commit is part of the generic
Bus.__new__ constructor. The removed code searches the set of kwargs
parameters for timing-related values. If it finds at least one such
value, it creates a BitTiming class instance and adds it to the list of
parameters. However, it breaks compatibility with the PEAK interface,
see issue hardbyte#1458. Additionally, the code in question is generic and
applies to all interfaces. Instantiating a class here is prone to issues
since it must be generic enough to fit all use cases. A better approach
would be to simply forward the parameters as was done previously and
leave it up to the individual interfaces to handle things properly.
@lumagi
Copy link
Collaborator Author

lumagi commented Dec 18, 2022

For reference, there are multiple issues/PR that discuss / implement the BitTiming handling:

@zariiii9003 zariiii9003 linked a pull request Dec 19, 2022 that will close this issue
zariiii9003 pushed a commit that referenced this issue Dec 21, 2022
* Add failing unit test to verify that issue #1485 is fixed

#1458

* Remove unused generic BitTiming creation in Bus.__new__

The BitTiming class is an attempt at unifying the various timing
parameters of the individual interfaces. The idea is that instead of
manually supplying multiple parameters that make up the timing
definition of the interface, one can instead supply a single instance of
the BitTiming class, which will also automatically calculate derivative
values from its input.

At the moment, this class is only used by two interfaces: CANtact and
CANanalystii. Both either accept a single bitrate or a BitTiming
instance. The latter will overrule the former.

The code that is removed with this commit is part of the generic
Bus.__new__ constructor. The removed code searches the set of kwargs
parameters for timing-related values. If it finds at least one such
value, it creates a BitTiming class instance and adds it to the list of
parameters. However, it breaks compatibility with the PEAK interface,
see issue #1458. Additionally, the code in question is generic and
applies to all interfaces. Instantiating a class here is prone to issues
since it must be generic enough to fit all use cases. A better approach
would be to simply forward the parameters as was done previously and
leave it up to the individual interfaces to handle things properly.

* Format code with black

Co-authored-by: lumagi <lumagi@users.noreply.github.com>
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.

1 participant