Add Support for FlexCAN(1/2) #171
Conversation
Implementation of the FlexCAN1 and FlexCAN2 peripherals for the NXP i.MX RT 1060 processor. Includes APIs for initializing the peripherals, setting the clock source, setting mailbox receive filters, setting mailbox FIFO functionality, and receiving and transmitting frames. The FlexCAN3 peripheral is not implemented.
- Remove StandardId and ExtendedId structs in favor of embedded-hal - convert TryFrom to From for FlexCanMailboxCSCode
- added bounds checks on StandardID and ExtendedID for creation of IdReg - improved Error descriptiveness - added docs to Frame timestamp field - Update Cargo.toml
- fixed clock configuration in builder method and removed set_ccm_ccg method - removed unused `instance` method - removed duplicated `set_fifo_filter` method in favor of single method that accepts a `FlexCanFilter` as arg - removed dead code
Implementation of the FlexCAN1 and FlexCAN2 peripherals for the NXP i.MX RT 1060 processor. Includes APIs for initializing the peripherals, setting the clock source, setting mailbox receive filters, setting mailbox FIFO functionality, and receiving and transmitting frames. The FlexCAN3 peripheral is not implemented.
We need properly-formmated code to pass CI.
It can't automatically fix all the issues, but the remaining fixes are straightforward.
Use the fact that a bool is either a zero or a one to simplify the filter value. Warned by clippy, and implemented by hand.
NOTE: There was a bug with read_mailbox() where it would return all 8 bytes of the frame, even if only a few of them were valid. If these bytes crossed a word boundary (4 byte) - garbage data would be loaded from the mailbox into the data frame. For example, the can frame: 01#01 would cause the following output: `[0x1, 0x0, 0x0, 0x0, ?, ?, ?, ?]` Where `?` indicates an unknown value.
There was a problem hiding this comment.
More than happy to take any code comments / improvement suggestions!
Our CI workflows will provide you with the fastest feedback. I recommend enabling our GitHub actions on your fork. That way, you can run our automated checks yourself whenever you push to your fork.
One notable issue: builds for the 1011 and 1015 MCUs are failing. These MCUs do not have a CAN peripheral, so the RAL doesn't export any CAN registers. Since CAN isn't common to all MCUs, we won't be able to place the CAN driver under the common module tree. If we export the CAN driver through a chip configuration module (like imxrt1020, imxrt1060, and imxrt1170), then those build errors should go away.
If you have any questions as you work through CI issues, leave them here and I'll try to help. I'll provide a detailed review once CI is passing. Until then, here's some high-level questions based on the prior PR:
- Does this driver solve the activating interrupt problem noted here?
- How does this driver account for MCU errata?
- Observers seem to want CAN FD support. Is there a path forward for adding that capability?
|
Thanks for your comments - I'll start by addressing the failing pipelines, and then I'll have a look at answering your questions about the interrupts, MCU errata, and CANFD support. |
NOTE: This change moves clock control file locations so that they are conditionally included only for chips that support the CAN clocks
NOTE: There has GOT to be a better way to do this, but I'm not sure how currently.
|
I've gone through and moved the CAN implementation out of I'm not sure this is the right approach, but it's working. Definitely open to feedback here! I've also updated the docs + docs example code - so all of the pipeline tests should pass. Currently I only expose the CAN interface on the 1060 since the only device I have for testing is a Teensy 4.1 |
| /// Disable flexcan clock | ||
| Disable = 3, |
There was a problem hiding this comment.
Note for other / future reviewers: This variant is not documented in the 1020 reference manual. It's also not a constant known to the 1020 SVD. Nevertheless, I say we keep this as-is; we cover the full bit width of the field.
|
|
||
| /// Decodes value of the ID Extended (IDE) and Remote Transmission Request (RTR) bits | ||
| /// | ||
| /// TODO: Break this up into seperate RTR and IDE structs |
There was a problem hiding this comment.
What's the plan to address these TODOs? I see the options as
- Now, during this review.
- After we merge, but before we release.
- After we release, in a breaking change.
Question and options applies to other TODOs listed in documentation. I'd prefer option 1, or option 2 if it can happen soon after we merge.
(In this instance, I haven't thought if separation is appropriate. I'll change my consideration if the answer is option 1.)
| const RTR_MASK: u32 = 0b1_u32 << Self::RTR_SHIFT; | ||
|
|
||
| const DLC_SHIFT: u32 = 16; | ||
| const DLC_MASK: u32 = 0b111_u32 << Self::DLC_SHIFT; |
There was a problem hiding this comment.
The DLC mask should be four bits wide. I'm checking against both the 1020 and 1060 RMs. That high bit will be important when considering an FD driver implementation.
When fixing, please include a frame/tests.rs unit test that evaluates four bit DLCs.
| const DLC_MASK: u32 = 0b111_u32 << Self::DLC_SHIFT; | |
| const DLC_MASK: u32 = 0b1111_u32 << Self::DLC_SHIFT; |
| reg: ral::can::Instance<M>, | ||
| _pins: PhantomData<P>, | ||
| _module: PhantomData<ral::can::RegisterBlock>, | ||
| clock_frequency: u32, |
There was a problem hiding this comment.
We're not using clock_frequency to do anything meaningful. We only make it available to the user with the implicit caveat "it might have changed." How about we remove it? Or, is there a reason to keep it I'm not considering?
| clock_frequency: u32, |
| /// Wrapper around [`Self::set_fifo(true)`](Self::set_fifo()) | ||
| pub fn enable_fifo(&mut self) { | ||
| self.set_fifo(true); | ||
| } | ||
|
|
||
| /// Wrapper around [`Self::set_fifo(false)`](Self::set_fifo()) | ||
| pub fn disable_fifo(&mut self) { | ||
| self.set_fifo(false); | ||
| } |
There was a problem hiding this comment.
set_fifo seems to be a great way to control the FIFO. If you prefer to keep these helpers, these methods could be good candidates for inline.
| /// Wrapper around [`Self::set_fifo(true)`](Self::set_fifo()) | |
| pub fn enable_fifo(&mut self) { | |
| self.set_fifo(true); | |
| } | |
| /// Wrapper around [`Self::set_fifo(false)`](Self::set_fifo()) | |
| pub fn disable_fifo(&mut self) { | |
| self.set_fifo(false); | |
| } |
| #[allow(rustdoc::private_intra_doc_links)] | ||
| /// Wrapper around [`Self::set_fifo_filter_mask()`] which sets the filter mask to [`filter::FlexCanFlten::RejectAll`] | ||
| pub fn set_fifo_reject_all(&mut self) { | ||
| self.set_fifo_filter_mask(filter::FlexCanFlten::RejectAll) | ||
| } | ||
|
|
||
| #[allow(rustdoc::private_intra_doc_links)] | ||
| /// Wrapper around [`Self::set_fifo_filter_mask()`] which sets the filter mask to [`filter::FlexCanFlten::AcceptAll`] | ||
| pub fn set_fifo_accept_all(&mut self) { | ||
| self.set_fifo_filter_mask(filter::FlexCanFlten::AcceptAll) | ||
| } |
There was a problem hiding this comment.
The #[allow(rustdoc::private_intra_doc_links)] means that we render a documentation link to a private method. When a user clicks that link in their browser, it goes nowhere. I don't think that's a great experience for the reader.
Let's document why the user might want to call these methods, instead of documenting how the method does what it does.
| #[allow(rustdoc::private_intra_doc_links)] | |
| /// Wrapper around [`Self::set_fifo_filter_mask()`] which sets the filter mask to [`filter::FlexCanFlten::RejectAll`] | |
| pub fn set_fifo_reject_all(&mut self) { | |
| self.set_fifo_filter_mask(filter::FlexCanFlten::RejectAll) | |
| } | |
| #[allow(rustdoc::private_intra_doc_links)] | |
| /// Wrapper around [`Self::set_fifo_filter_mask()`] which sets the filter mask to [`filter::FlexCanFlten::AcceptAll`] | |
| pub fn set_fifo_accept_all(&mut self) { | |
| self.set_fifo_filter_mask(filter::FlexCanFlten::AcceptAll) | |
| } | |
| /// The FIFO will reject CAN frames that do not exactly match its filter. | |
| /// | |
| /// This call is ignored if the FIFO is disabled. The call only takes effect when the FIFO is enabled. | |
| pub fn set_fifo_reject_all(&mut self) { | |
| self.set_fifo_filter_mask(filter::FlexCanFlten::RejectAll) | |
| } | |
| /// The FIFO will accept any CAN frame, no matter the filter. | |
| /// | |
| /// This call is ignored if the FIFO is disabled. The call only takes effect when the FIFO is enabled. | |
| pub fn set_fifo_accept_all(&mut self) { | |
| self.set_fifo_filter_mask(filter::FlexCanFlten::AcceptAll) | |
| } |
| can::can::<1>(), | ||
| can::can::<2>(), | ||
| can::can_pe::<1>(), | ||
| can::can_pe::<2>(), |
There was a problem hiding this comment.
It looks like only the CAN PE clock gates are downstream of the FlexCAN clock root. I'm gleaning this from table 14-4 of the 1060 RM (rev 3). The CAN clock gates are downstream of the IPG clock.
| can::can::<1>(), | |
| can::can::<2>(), | |
| can::can_pe::<1>(), | |
| can::can_pe::<2>(), | |
| can::can_pe::<1>(), | |
| can::can_pe::<2>(), |
Fix a couple CAN frame decoding issues: 1. Extended vs. Standard ID (these were reversed) 2. Proper payload length by implementing `From<&[u8]> for Data`
Fix CAN Frame Decoding
Fix issue where DLC was decoded as 3 bits instead of 4 Add new functionality to parse a frame from a &[u8] slice
Add DLC fix and frame parsing from slice
|
🥳 |
This PR adds support for the FlexCAN1 and FlexCAN2 interfaces to
imxrt-hal0.5.FlexCAN1 interface has been tested in HW using a Teensy4.1 - I'll have a corresponding PR in
teensy4-bspwith anRTICexample which was used to test.More than happy to take any code comments / improvement suggestions!
Acknowledgements:
The vast majority of this work is from @dstric-aqueduct - who wrote the initial FlexCAN peripheral code in #122.
@SebKuzminsky rebased @dstric-aqueduct's work onto
imxrt-hal0.5 which gave me a great starting point to work from.