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

CAN driver proposal #75

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

CAN driver proposal #75

wants to merge 8 commits into from

Conversation

paval-shlyk
Copy link

Added support for the CAN driver (tested only on g6u6 model)
Added a flag to start the runtime (for some reason qingke does not initialize what it should)

@paval-shlyk
Copy link
Author

Forgot to mention that CAN implementation primary was taken from this repo: https://github.com/marti157/ch32-can-rs

@andelf andelf self-requested a review December 7, 2024 03:33
@andelf
Copy link
Contributor

andelf commented Dec 10, 2024

Maybe we should cite marti157/ch32-can-rs somewhere.
@marti157

@marti157
Copy link
Contributor

I'm happy to see an initiative to get my CAN implementation into the HAL, since the CAN implementation already depended on the HAL. Some work needs to be done, especially with the pin remapping. I managed to fully test the implementation on a 208wbu6 device with real CAN hardware, so let me know if you need any help!

Copy link
Contributor

@andelf andelf left a comment

Choose a reason for hiding this comment

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

Thanks. I'll handle the rest.

impl<'d, T: Instance> Can<'d, T> {
/// Assumes AFIO & PORTB clocks have been enabled by HAL.
///
/// CAN_RX is mapped to PB8, and CAN_TX is mapped to PB9.
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments can be removed.

Copy link
Author

Choose a reason for hiding this comment

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

I will review the relevance of the comments)


rx.set_mode_cnf(
pac::gpio::vals::Mode::INPUT,
pac::gpio::vals::Cnf::PULL_IN__AF_PUSH_PULL_OUT,
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave these pin configs to me. I'll get it implemented.

Copy link
Author

Choose a reason for hiding this comment

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

I don't mind. What should I do?)

@alexxy alexxy mentioned this pull request Dec 20, 2024
@paval-shlyk
Copy link
Author

I'm testing CAN on 203g6u6 and I found that the filter don't work. Let's say I have:

    let filter = CanFilter {
        bank: 0,
        mode: CanFilterMode::IdMask,
        id_value: (0x580 | 0x42) as u32,
        id_mask: 0x0FF,
    };

As I believe only frames with id equals to 0x5C2 (0x580 | 0x42) should be accepted.
But hearbeats' frames of another node are received too.
@marti157 Everything is working for you?

@paval-shlyk
Copy link
Author

I have introduced a new api for filters' configuration (with a bit unsafe code). But I still can't set up the filter correctly (it's probably a hardware bug, but even without the filter receive method, a frame with the ID 0x1101 is created). Only the "accept_all" strategy works for me (I can intercept real message frames).

@andelf
Copy link
Contributor

andelf commented Dec 29, 2024

@paval-shlyk Thanks for your contribution. I've made some refactoring to your work:

Changes made:

  1. Using generated peripheral definitions
  2. Using generated pin traits and pin remapping
  3. Extended support for CH32L1 series, as the peripheral layout is compatible
  4. removed separate can feature flag, to match embassy-stm32

Please review these changes and let me know if everything looks good to proceed with the merge.

@paval-shlyk
Copy link
Author

@andelf Using the feature flag is, of course, a matter of taste...) I prefer to give the user the ability to customize the library's functionality (as a way to speed up the compilation process) But for ease of use, I agree)

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