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

Unify bit timings with separate class for CAN FD #1468

Merged
merged 32 commits into from Jan 25, 2023

Conversation

zariiii9003
Copy link
Collaborator

This is a continuation of #614 and #615. I would like to hear your opinions before i waste my time on writing tests and docs.

I made the following changes:

  • Create a separate class for CANFD. In my opinion the bit timing requirements for CAN2.0 and CANFD are just too different to use the same class for both.
  • Validate bit timings in __init__ method
  • Add multiple constructors to avoid too many optional parameters. It is now possible to create an instance though BitTiming(**kwargs), BitTiming.from_registers(**kwargs) and BitTiming.from_sample_point(**kwargs)
  • The bit timing classes inherit from typing.Mapping. Now the user can call dict(timing) to get a dictionary of the bit timing parameters. This should make it easier to save the parameters to a config file.
  • Solve the configuration loading problems of Fix Bus.__new__ for PEAK CAN-FD interfaces #1460

Interface implementations should add a single parameter like timing: Union[BitTiming, BitTimingFd]. It would take precedence over other parameters and provide a simple API for the user. The docstring of the parameter should tell the users which values for f_clock are accepted.

@felixdivo
Copy link
Collaborator

Thank you @zariiii9003, the proposal sounds very reasonable! I am honestly not that much of an expert with the bit timing mechanisms, so I'll retreat from this discussion. But at a glance, this looks like a very useful contribution.

@felixdivo felixdivo added this to the Next Release milestone Dec 28, 2022
@felixdivo felixdivo removed their request for review December 28, 2022 12:23
@zariiii9003
Copy link
Collaborator Author

@lumagi @bmeisels Could you take a look, too?

@lumagi
Copy link
Collaborator

lumagi commented Dec 29, 2022

This looks impressive! I will look through the code over the next day or so.

Copy link
Collaborator

@lumagi lumagi left a comment

Choose a reason for hiding this comment

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

I added a few comments with things to came to my mind. I can also support with writing further tests if necessary or with doing manual tests of the Peak interface when this PR is merged.

can/util.py Outdated Show resolved Hide resolved
can/bit_timing.py Show resolved Hide resolved
can/bit_timing.py Show resolved Hide resolved
can/interfaces/canalystii.py Show resolved Hide resolved
can/interfaces/canalystii.py Outdated Show resolved Hide resolved
can/interfaces/cantact.py Outdated Show resolved Hide resolved
can/interfaces/cantact.py Show resolved Hide resolved
test/test_bit_timing.py Show resolved Hide resolved
@zariiii9003
Copy link
Collaborator Author

@hardbyte I'll wait for your approval, since this is a quite a big change.

can/util.py Show resolved Hide resolved
@zariiii9003
Copy link
Collaborator Author

I will merge this in a few days if there are no objections

@zariiii9003 zariiii9003 merged commit 48b18f1 into hardbyte:develop Jan 25, 2023
@zariiii9003 zariiii9003 deleted the bittiming branch January 25, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants