atsams-rs / atsamx7x-rust

Rust HAL, PACs, and examples for the Microship SAM E70/S70/V70/V71
Apache License 2.0
24 stars 11 forks source link

Integration with `mcan` crate #51

Closed glaeqen closed 1 year ago

glaeqen commented 1 year ago

This commit provides mcan crate integration code

Tested with SAMV71 Xplained Ultra

michalfita commented 1 year ago

May I ask why mcan instead of embedded-can?

Wouldn't it be wise to gate this behind feature and make the dependency optional? CAN common in automotive industry, but rarely elsewhere.

Dzięki za wkład 👍🏼

michalfita commented 1 year ago

It would be nice to add some usage example to board examples.

glaeqen commented 1 year ago

May I ask why mcan instead of embedded-can?

They are different things. mcan-core is a small crate that provides two traits that should be implemented within a target HAL in order to make mcan crate operational. mcan is an MCAN HAL that provides actual APIs to receive and send messages.

MCAN is the Bosch IP that is integrated almost as-is by vendors into their MCUs, it has the same memory-mapped HW registers and same internals in every single MCU it has been integrated into.

User can link against mcan crate in their application crate if wants to and use hal::can::Dependencies to resolve platform-specific shenanigans in order to get it to work.

Also, we will be able to do breaking changes in mcan without a necessity of bumping and releasing target HALs that integrate with mcan. That's because mcan-core is supposed to be very stable. So mcan and target HALs can move independently.

embedded-can is just like embedded-hal, it allows driver implementors to have a generalized I2C or whatever so their library does not have to depend on any specific HAL. Drivers for devices that communicate over the I2C or CAN bus. In comparison, MCAN is the CAN peripheral embedded in the MCU.

Wouldn't it be wise to gate this behind feature and make the dependency optional? CAN common in automotive industry, but rarely elsewhere.

This is a crate that does not contain any code. Two traits with big doc-strings. Code in a x7x HAL will be optimised away if not used in a final binary. Introduction of feature-gating seems like an unnecessary over-complication. To be fair, I did this implementation as an exercise, as other @GrepitAB folk uses V71 so I had development boards within a reach and I wanted to verify mcan assumptions.

It would be nice to add some usage example to board examples.

I'll try to do something.

Dzięki za wkład 👍🏼

Żaden problem 👍🏼

glaeqen commented 1 year ago

Fixed feature-gating for chips that have only single MCAN instance / have no CAN at all. Hopefully I didn't do it too restrictively.

glaeqen commented 1 year ago

Tagging @sundbom-grepit

glaeqen commented 1 year ago

@michalfita @martinmortsell @tmplt is there anything missing? Should we merge it in?

tmplt commented 1 year ago

I can't verify the example without hardware access, but otherwise lgtm.

tmplt commented 1 year ago

No objections since last comment. Merging.