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

Canfd support #33

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Canfd support #33

wants to merge 10 commits into from

Conversation

tkgdsg
Copy link

@tkgdsg tkgdsg commented Oct 31, 2024

CANFD support

I've added new fd dedicated frame for state_raw.
Could I get your opinion on whether this implementation method is correct?

@yegorich
Copy link
Contributor

yegorich commented Dec 10, 2024

@tkgdsg @nanitsuku could you rebase your PR against the master branch?

@tkgdsg
Copy link
Author

tkgdsg commented Dec 27, 2024

@yegorich
Hi. I've rebased on latest master.

@yegorich
Copy link
Contributor

@hartkopp @mocle could you take a look at this PR? I am still on the pre-CAN FD hardware :-(

@hartkopp
Copy link
Member

Will do the next days. I did a short review and have some remarks.
Thanks for the heads up!

@hartkopp
Copy link
Member

hartkopp commented Jan 2, 2025

Hi @nanitsuku @tkgdsg ,
this patchset can not be merged in this way.
You started with an initial patch which introduces lots of changes (e.g. also indention and wrong whitespace changes or the changes in state_bcm.c) which are then fixed in the follow up commits. This is not how pull requests are intended.
E.g. the patch "support canfd for can raw" reverts the changes to state_bcm.c
And the "< fdframe" command still handles only 8 bytes.
Do you plan to support the BCM functionality?

In general please try to provide several step-by-step commits that can be easily reviewed and understood.

E.g.

  • convert to use struct canfd_frame
  • figure out the interface capabilities
  • convert the protocol flavors like state_raw and state_bcm
  • other changes
  • documentation

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants