search

imxrt-rs / imxrt-hal

Rust for NXP i.MX RT
Apache License 2.0
122 stars 29 forks source link

FlexCAN peripheral addition #122

Open dstric-aqueduct opened 1 year ago

dstric-aqueduct commented 1 year ago

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

See corresponding update to teensy4-rs crate: https://github.com/mciantyre/teensy4-rs/pull/124

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

dstric-aqueduct commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

dstric-aqueduct commented 1 year ago

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.

collin80 commented 1 year ago

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.