Fix Bus.__new__ for PEAK CAN-FD interfaces #1460
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
zariiii9003
approved these changes
Dec 18, 2022
This was
linked to
issues
Dec 19, 2022
zariiii9003
reviewed
Dec 19, 2022
| ): | ||
| if key in config: | ||
| timing_conf[key] = int(str(config[key]), base=0) | ||
| del config[key] |
Collaborator
There was a problem hiding this comment.
Have you tried just removing this line with del instead of everything?
Collaborator
Author
There was a problem hiding this comment.
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 theBitTiminginstance 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request removes unused code in Bus.new that is responsible for creating BitTiming instances, but is incompatible with PEAK devices. See issue #1458 .
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. For reference, a pull request that would implement BitTiming for PEAK devices has not been merged, see #625 .
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.