FlexCAN peripheral addition

[dstric-aqueduct]

This pull request adds functionality for the imxrt's FlexCAN1 and FlexCAN2 peripherals.

See corresponding update to teensy4-rs crate: mciantyre/teensy4-rs#124

Credit to Antonio Brewer (tonton81) for much of the API inspiration from the FlexCAN_T4 library: https://github.com/tonton81/FlexCAN_T4

[mciantyre]

Thanks for putting this together. My first pass is a brief code study. I'm not too familiar with this peripheral, but I'll try to get some time to study the FlexCAN chapter in the reference manual.

I notice that we're re-naming the package and files. I'm guessing that's to support Teensy 4 prototyping. If that's the case, would you mind rebasing this branch onto the maint-v0.4 branch, then re-targeting the PR to maint-v0.4 (edit the PR, and change the base branch)? Once this happens, we won't have to re-name all these files, and it will simplify integration and release into the 0.4 HAL.

My review include considerations for #121. Once we're in a good state, I can port this driver into the next HAL.

[dstric-aqueduct]

Thanks for putting this together. My first pass is a brief code study. I'm not too familiar with this peripheral, but I'll try to get some time to study the FlexCAN chapter in the reference manual.

I notice that we're re-naming the package and files. I'm guessing that's to support Teensy 4 prototyping. If that's the case, would you mind rebasing this branch onto the maint-v0.4 branch, then re-targeting the PR to maint-v0.4 (edit the PR, and change the base branch)? Once this happens, we won't have to re-name all these files, and it will simplify integration and release into the 0.4 HAL.

My review include considerations for #121. Once we're in a good state, I can port this driver into the next HAL.

Hey Ryan - first, a big thanks to you and Tom for all the work on these projects! They're fantastic!

I merged the maint-v0.4 branch into the dev/can branch and then re-targeted to the maint-v0.4 branch for the PR. I think the new file diffs are only the Can feature additions at this point.

[teburd]

Thanks for submitting this! It has been a requested feature by several people and it is great to see it being contributed.

I'd note that the commit set should be rebased, probably squashed a bit, and cleaned up before merging to make looking at the history a bit easier to understand. Commits with one word descriptions aren't going to be all that helpful if we ever need to bisect/blame/etc. Really most of this work I'd think could be squashed down into a couple of commits with a nice commit message describing the flexcan driver being added, how it works, and why.

Happy holidays! Again thank you very much for contributing this.

[dstric-aqueduct]

Well...it appears my attempts to squash the fork commit history did not go well as there are now a bunch of file diffs. Ian, Tom - I'll leave it to you on the best way to go about making the PR history clean.

[mciantyre]

Here's how I would try to clean up the history. I encourage you to take some time to try this out, and see how it works. This isn't the rebase that Tom and I suggested, but it's a quick way to achieve the same outcome and simplify the history.

1. git reset --soft maint-v0.4

   After this, the CAN-specific changes (the contents of this diff, as I write this) are in the index. You can see the changes with git status. Also, the HEAD of dev/can is now maint-0.4.

2. git commit -m "Prototype a FlexCAN peripheral"

   I agree with Tom that having more content in the commit message is helpful. I'll leave that to you.

3. git push [your-remote-name] dev/can --force

   After this, that one new commit from 2 appears in the PR.

[mciantyre]

Sorry, but I'm pausing my review until we simplify the commit history and change how we apply revisions. I notice that corrections are changing back to their pre-review state. See my latest comment on the id.rs file. I'm concerned that this might happen with harder-to-notice, important revisions.

If cleaning up the history is troublesome, I can provide a starting point. I pushed a dev/can branch to the main repository with a simplified history and a base on maint-v0.4. I'll stress that this is just a starting point; I may not have integrated all the changes you expect.

If that branch looks reasonable, I recommend using it for development and review. You can fetch the branch, locally re-name it to dev/can, then force-push it over your remote's dev/can branch to update this PR.

[dstric-aqueduct]

Sorry, but I'm pausing my review until we simplify the commit history and change how we apply revisions. I notice that corrections are changing back to their pre-review state. See my latest comment on the id.rs file. I'm concerned that this might happen with harder-to-notice, important revisions.

If cleaning up the history is troublesome, I can provide a starting point. I pushed a dev/can branch to the main repository with a simplified history and a base on maint-v0.4. I'll stress that this is just a starting point; I may not have integrated all the changes you expect.

If that branch looks reasonable, I recommend using it for development and review. You can fetch the branch, locally re-name it to dev/can, then force-push it over your remote's dev/can branch to update this PR.

Hey Ian - thanks for creating the dev/can branch in the main repo. I was able to force-push to the remote's branch so I think we're finally in sync now.

[mciantyre]

Updated branch looks great. Thanks for working through the clean-up.

CI is detecting build errors. I can also reproduce build errors locally. We'll need a passing build for the CAN contributions.

When clippy kicks in, it will detect warnings for existing, non-CAN code. Please ignore those warnings; I've addressed those warnings on the maint-v0.4 branch in this repo. Unless you'd like to rebase this branch onto those commits, I'll make sure that this branch's CI is green for all other code. I appreciate your help addressing clippy warnings for CAN code.

[mciantyre]

Could we make this method the only way to configure FIFO filters? I'm not sure I see a reason to support two different methods that only vary in the number of their arguments. We could also take filter by reference.

[mciantyre]

don't read interrupt enabled mailboxes

Is this documented somewhere for the user? Although handle_interrupt also returns a MailboxData, it might not be obvious that handle_interrupt is the only way that users should retrieve mailbox data when interrupts are enabled.

[mciantyre]

I rebased the branch onto maint-v0.4 to isolate clippy warnings, formatted the code to pass the next CI step, and collapsed into one commit.

I appreciate help addressing the 21 clippy warnings. You can reproduce them locally by checking out the updated branch, then running clippy like

cargo clippy --features=imxrt1062

If I'm left to fix these, I'll need clarity on the intent of some code; see inline comments.

[mciantyre]

Is this 6_u32 the same as NUMBER_FIFO_RX_MAILBOXES?

[mciantyre]

Unsigned subtraction of baud and clock_freq / [...] -- before casting to i16 -- can overflow. This can cause a panic when building with default debug configurations.

[ERROR teensy4_panic]: panicked at 'attempt to subtract with overflow', /path/to/imxrt-hal/src/can/mod.rs:370:29

I can reproduce the panic in the teensy4-rs can.rs example (as of this commit). The snippet below shows how I'm building the example. I haven't stepped through the code to understand what specific iteration produces this panic.

cargo build --target=thumbv7em-none-eabihf --example=can --all-features

My first thought is to use something like abs_diff. However, if the absolute difference is larger than what fits in an i16, we'll truncate that difference once we cast from u32 to i16.

• Could we revise this routine so that it's less likely to panic in debug builds?
• Are the truncating casts to i16 necessary?

[dstric-aqueduct]

This has been updated to use the abs_diff method.

[mciantyre]

Question about this API and behavior. I'm effectively testing this documentation example through the can.rs example proposed in the teensy4-rs repo (as of this commit). Here's what I'm trying:

1. I'm changing this driver configuration to false, and keeping all other can1 calls in the block the same.

   can1.set_fifo_interrupt(false);

2. I'm then unmasking the CAN1 interrupt before the LED-toggling-CAN-I/O loop.

   // NEW:
   unsafe { cortex_m::peripheral::NVIC::unmask(bsp::interrupt::CAN1) };
   loop { /* Delay, toggle LED, read CAN mailboxes, send CAN frame */ }

3. I implementing the CAN1 ISR elsewhere in the example. It looks like

   use bsp::interrupt;
   #[cortex_m_rt::interrupt]
   fn CAN1() {
       log::warn!("CAN1 interrupt fired");
   }

4. I don't have the required SN65HVD230, so my CAN pins are not connected to anything.

I'm expecting step 1 to disable the CAN interrupts; that seems to be the only public method with something implying interrupt activation in the name. So even though I'm unmasking the CAN1 interrupt, I'm never expecting the ISR to activate.

But when I actually build and run this, I see that the CAN1 ISR is firing repeatedly. I'm connecting to the USB serial console to see all the log warnings.

Is there something else in the driver that's enabling interrupts, and can it be changed by the user? Or, am I using this driver incorrectly?

[dstric-aqueduct]

From the manual (pg 2553):

The module can generate up to 70 interrupt sources (64 interrupts due to message buffers
and 6 interrupts due to Ored interrupts from MBs, Bus Off, Error, Tx Warning, Rx
Warning and Wake Up)).

If the Rx FIFO is enabled (bit RFEN on MCR set), the interrupts corresponding to MBs 0
to 7 have a different behavior. Bit 7 of the IFLAG1 becomes the "FIFO Overflow" flag;
bit 6 becomes the FIFO Warning flag, bit 5 becomes the "Frames Available in FIFO flag"
and bits 4-0 are unused. See Interrupt Flags 1 Register (FLEXCAN_IFLAG1) for more
information.

A combined interrupt for all MBs is also generated by an Or of all the interrupt sources
from MBs. This interrupt gets generated when any of the Mailboxes or FIFO generates an
interrupt. The Arm must read the IFLAG Registers to determine which MB or FIFO
caused the interrupt.

The other 5 interrupt sources (Bus Off, Error, Tx Warning, Rx Warning and Wake Up
generate interrupts like the MB ones, and can be read from both the Error and Status
Register 1 and 2. The Bus Off, Error, Tx Warning and Rx Warning interrupt mask bits
are located in the Control 1 Register and the Wake-Up interrupt mask bit is located in the
MCR.

This method is currently only setting the BUF5I bit in IMASK1 register to interrupt when a frame is available (Frames Available in FIFO flag...):

If the Rx FIFO is not enabled, this bit flags the interrupt for MB5. If the Rx FIFO is enabled, this flag
indicates that at least one frame is available to be read from the Rx FIFO.
This flag is cleared by the FlexCAN whenever the bit MCR[RFEN] is changed by Arm writes

I agree this could be more transparent. More importantly, masking the interrupt vector for the interrupt reason would be beneficial.

This would let us debug why the interrupt is always firing for you (likely Bus Off with no transceiver attached.)

[mciantyre]

When I run the can.rs example in the teensy4-bsp project, it logs a few control registers before entering its I/O loop. Here's three register values I see, followed by a discussion of interrupt-specific register fields.

[INFO imxrt_hal::can]: MCR: 64EB300F

In MCR,

• WRNEN is set. However, we're never setting CTRL1[TWRNMSK] or CTRL1[RWRNMSK], so (from what I can tell) we shouldn't observe an interrupt when error counters cross their threshold. If that's correct, I'm not sure we need to set WRNEN.
• WAKMSK and SLFWAK are set. This means that we could trigger an interrupt when exiting doze or stop modes. (This definitely isn't happening in my limited testing, but it may be a non-obvious behavior of this driver.)

[INFO imxrt_hal::can]: CTRL1: 1624002

In CTRL1,

• BOFFMSK is clear, so there's no "bus off" interrupt.
• ERRMSK is set, so we generate an interrupt when ESR1[ERRINT] is high. This is a hasty observation, but a project-wide CTRL+F for "ESR1" shows no hits. This could indicate we're not clearing error interrupts.
• TWRNMSK and RWRNMSK are both clear; see MCR[WRNEN] discussion, above.

[INFO imxrt_hal::can]: CTRL2: 870000

In CTRL2,

• BOFFDONEMSK is clear, so there's no "bus off done" interrupt.

I'm not evaluating any message buffer interrupts right now. Nevertheless, I'm wary of ERRMSK being set, especially if we're not handling any error and status flags.

[myCatPlaysFetch]

There are FlexCAN errata, at least on the RT1064, that require some extra precautions and/or workarounds. This function and elsewhere in the module would be affected. Those are 5641 (not on RT1064), 5829, 6032, and 9595. Should this driver be expected to know of and handle these errata? Or would there be more board-specific drivers for that? Overall excellent work, just curious how something like that is handled.

[mciantyre]

Thanks for helping with the review. If an erratum's workaround can act as the driver's default behavior, that would be ideal, and we should handle it here. This cuts down on configurations / code paths we maintain. But, if we need separate code paths to handle a driver's errata, we could consider crate configurations / separate driver crates / common- and chip-specific HAL combinations.

[collin80]

The underlying hardware for the Teensy 4/4.1/Micromod supports 3 CAN buses, not two. Also, the third hardware bus is CAN-FD capable. It seems that currently your HAL doesn't have any concept of this third CAN bus. However, would this be possible to add? I realize that the addition of CAN-FD may complicate things. But, it is physically present on the chip so for teensy4-rs it would seem proper for it to eventually be supported.

I'm not entirely asking that "someone else do it for me" but kind of. I am toward the beginning of my rust journey so currently it may be difficult for me to take on such a thing by myself. However, I'm not entirely green at making CAN libraries. I use Tony's FlexCAN-T4 library on Teensy Micromod but on other chips I used my own CAN libraries. So, if this may be something I could potentially help to do I'm open to pointers as to how to get started.

[mciantyre]

#171 rebases this driver onto a later commit.