-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add basic MCAN driver for MSPM0 #5296
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
base: main
Are you sure you want to change the base?
Conversation
This commit implements a simple driver for the MCAN peripheral on MSPM0, supporting both blocking and non-blocking mode. An example is included for the MSPM0G3107 (the only part I have access to for testing), but this _should_ work on all parts with a CANFD/MCAN peripheral. This driver is intended to be a starting point, and does not yet include a variety of useful features for a well-rounded driver. In particular it does not include: - Async operation (in progress, but not yet implemented here) - CAN-FD - Filtering - TX confirmation - More complex clocking - Bitrate calculation - Automatic bus-off recovery I hope to work towards implementing these features, but I can't commit to any sort of timeline.
| mspm0-metapac = { git = "https://github.com/mspm0-rs/mspm0-data-generated/", tag = "mspm0-data-d9f54366c65cec47957065f42fe04542b852a8cd" } | ||
| rand_core = "0.9" | ||
| mspm0-metapac = { git = "https://github.com/mspm0-rs/mspm0-data-generated/", tag = "mspm0-data-566cf58760f60a9126c957ae9b6b3364e46ed595" } | ||
| bitfield = "0.19.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitfield is also used by embassy-net-adin1110 and I couldn't find other similar bitfield crates in use within embassy. This is used to construct some of the fields for message RAM - I'm open to using a different crate if preferable.
| match sysctl.version { | ||
| Some("g350x_g310x_g150x_g110x") | Some("g351x_g151x") => { | ||
| cfgs.enable("sysctl_syspll"); | ||
| } | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a little strange to me. Open to suggestions on how better to identify if SYSPLL is available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have metadata for a clock tree from sysconfig, just haven't plumbed it up yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this approach reasonable for now, and just plan to improve it down the line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now this is fine.
| // TODO: should these be found via the PAC instead of hard-coded? | ||
| // From looking at the G series TRM, these addresses are constant, | ||
| // but it still feels wrong. | ||
| // Taken from table "Table 1-135. FACTORYREGION_TYPEG Registers" | ||
| // (constants identical to Table 1-116. FACTORYREGION_TYPEA Registers for PLL.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Longer term I do want to expose the factory region in this crate in a factory module. Haven't gotten to it yet.
|
CI fails due to:
|
examples/mspm0g3107/README.md
Outdated
|
|
||
| ## Checklist before running examples | ||
|
|
||
| The MSPM0G3107 does not have a Launchpad or official development board, so you likely need to modify examples to update pin numbers or peripherals to match the specific MCU or board you are using. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth mentioning G3507 will be quite similar to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this line to mention that the examples should also run there with minimal changes.
I decided to create a separate examples folder for this specific MCU since I don't have access to others to test against.
Weird, the pull request clearly adds that to Cargo.toml? |
|
Ugh, yeah. I accidentally deleted |
| pub struct Config { | ||
| /// Input clock divider | ||
| pub clock_div: ClockDiv, | ||
|
|
||
| /// CAN timings to use for standard CAN. (CAN-FD support to come later.) | ||
| pub timing: CanTimings, | ||
|
|
||
| /// If remote frames should be accepted into the RX queue. | ||
| pub accept_remote_frames: bool, | ||
|
|
||
| /// If extended ID frames should be accepted into the RX queue. | ||
| pub accept_extended_ids: bool, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling that the code could calculate some of the timings for you to get a specific bitrate and back calculate from clock config. See the UART driver for an idea of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It certainly can! I have listed it as an excluded feature for now to get something minimal working.
The algorithm is a bit complex, which is why I left it out, but there are other implementations out there (even in embassy-stm32).
I can include it if you want - but my original intention was to follow up and add it in later since it’s another ~250 lines of code.
embassy-mspm0/src/can/mod.rs
Outdated
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] | ||
| #[cfg_attr(feature = "defmt", derive(defmt::Format))] | ||
| /// Config Error | ||
| pub enum InitializationError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ConfigError to match other drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, updated.
embassy-mspm0/src/can/mod.rs
Outdated
| return Err(BusError::BusOff); | ||
| } | ||
|
|
||
| cortex_m::asm::delay(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required due to timing bugs in hardware?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. Didn’t mean to leave this in. Thanks for catching it.
| // I am really not sure that these are good abstractions for real applications. | ||
| // They do not support "confirmable" mesage sending (i.e the docs specifically state that | ||
| // transmit() only enqueues frames, there is no feedback mechanisim to confirm if/when a frame | ||
| // was actually sent) | ||
| // They also do not support monitoring the peripheral's status or recovering from bus-off. | ||
| // I will implement them regardless to play nice with the overall ecosystem, | ||
| // but similar to embassy-stm32, I'm going to offer a HAL-specific API which provides | ||
| // more useful functionality, and add an async variant at some point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is a question that could be an issue on the embedded_can crate. I am not 100% familar with CAN, but would confirmation be something that hardware reports or is that part of a "CAN stack".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on the hardware as far as I know - the MCAN IP does support hardware reports of confirmed successful transmission, but I don’t think all IPs do. Some higher level protocols might include protocol-layer ACKs as well, since the CAN ACK just means some device received it correctly, not that the destination received it.
I don’t neccessarily see it as an issue within embedded_can - confirmable messages and asynchronous support are important to the specific applications that I have built, but general hardware support may not be there to expect every MCU to support them.
It’s more that our peripheral can do more, so I want to expose methods to make more use of what is there.
embassy-mspm0/src/can/mod.rs
Outdated
| use crate::can::msgram::{McanMessageRAM, MessageRAMAccess}; | ||
| use crate::gpio::{AnyPin, PfType}; | ||
| use crate::mode::{Blocking, Mode}; | ||
| use crate::pac::canfd::{Canfd as Regs, vals as CanVals}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we don't rename path elements. Leaving it as vals is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops - leftover from out of tree. Updated.
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] | ||
| #[cfg_attr(feature = "defmt", derive(defmt::Format))] | ||
| #[repr(C)] | ||
| pub(super) struct RxBufferElement { | ||
| // a bit awkward to split this into two structs, but this is hidden from downstream consumers anyways. | ||
| pub hdr: MsgHeader, | ||
| pub rxhdr: RxHeader, | ||
| pub data: [u8; MAX_DATA_LEN], | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another question around packed and C repr.
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] | ||
| #[cfg_attr(feature = "defmt", derive(defmt::Format))] | ||
| #[repr(C)] | ||
| pub(super) struct TxBufferElement { | ||
| // a bit awkward to split this into two structs, but this is hidden from downstream consumers anyways. | ||
| pub hdr: MsgHeader, | ||
| pub txhdr: TxHeader, | ||
| pub data: [u8; MAX_DATA_LEN], | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packed question again
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] | ||
| #[cfg_attr(feature = "defmt", derive(defmt::Format))] | ||
| #[repr(C)] | ||
| pub(super) struct TxEventElement { | ||
| // a bit awkward to split this into two structs, but this is hidden from downstream consumers anyways. | ||
| pub hdr: MsgHeader, | ||
| pub event: EventHeader, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another packed struct question
| #[repr(C)] | ||
| pub(super) struct McanMessageRAM { | ||
| filters: [StandardFilter; 0], | ||
| extended_filters: [ExtendedFilter; 0], | ||
| rxfifo0: [RxBufferElement; NUM_RX_ELEMENTS], | ||
| rxfifo1: [RxBufferElement; 0], // Note: while we're not using most of these features yet, I've included their offsets and structures anyways to save the next person some pain. | ||
| rxbuffers: [RxBufferElement; 0], | ||
| txevents: [TxEventElement; NUM_TX_EVENTS], | ||
| txfifo0: [TxBufferElement; NUM_TX_ELEMENTS], | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another packed struct question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one actually doesn’t need to be packed and alignment doesn’t matter because we tell the peripheral the offsets within msgram for each array.
| }; | ||
| } | ||
|
|
||
| macro_rules! impl_ram_access { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if https://github.com/google/zerocopy could do a lot of this macro magic for you? Not going to demand a move to zerocopy at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be able to implement the same logic, but I don’t see support within zerocopy for volatile reads and writes, which should be used here IMO because the peripheral is also modifying or reading from RAM.
This commit implements a simple driver for the MCAN peripheral on MSPM0, supporting both blocking and non-blocking mode.
An example is included for the MSPM0G3107 (the only part I have access to for testing), but this should work on all parts with a CANFD/MCAN peripheral.
As discussed in Matrix - this also configures SYSPLL in a slightly silly way to allow for providing a valid functional clock without requiring an external oscillator. In testing, this is fine at moderate bitrates.
This driver is intended to be a starting point, and does not yet include a variety of useful features for a well-rounded driver. In particular it does not include:
I hope to work towards implementing these features, but I can't commit to any sort of timeline.