Skip to content
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

Integration with the mcan #654

Merged
merged 4 commits into from
Mar 10, 2023
Merged

Integration with the mcan #654

merged 4 commits into from
Mar 10, 2023

Conversation

glaeqen
Copy link
Contributor

@glaeqen glaeqen commented Dec 26, 2022

This PR provides trait implementations from mcan-core crate. It enables the safe usage of the mcan crate.

mcan crate provides a HAL for the CAN peripheral embedded into ATSAME5{1, 4}.

@glaeqen
Copy link
Contributor Author

glaeqen commented Dec 26, 2022

I'll try to provide a similar example to the one in atsams-rs/atsamx7x-rust#51 soon.

Done, unblocked by #666 as well.

@sajattack
Copy link
Member

My only comment would be that the example is a little long and complicated, but I suppose that's just the nature of the beast. Overall, LGTM

@sajattack
Copy link
Member

oh, one more thing, please add the can feature to crates.json for appropriate PACs, so that it is tested.

@glaeqen
Copy link
Contributor Author

glaeqen commented Mar 6, 2023

My only comment would be that the example is a little long and complicated, but I suppose that's just the nature of the beast. Overall, LGTM

I could probably cut it down to something simpler, I wanted it to be a little more fun, thorough and interactive. Like, listen to messages on interrupt and print them out AND send a message on a button press. It also contains the filter setup which brings the value of an example up. Anyone who wants to use it can drop whatever they deem unnecessary - this is how I thought about it

@glaeqen
Copy link
Contributor Author

glaeqen commented Mar 6, 2023

oh, one more thing, please add the can feature to crates.json for appropriate PACs, so that it is tested.

Done, not sure about these hal_doc_variants though. Are they supposed not to include samE chips? Only samD?

@sajattack
Copy link
Member

oh, one more thing, please add the can feature to crates.json for appropriate PACs, so that it is tested.

Done, not sure about these hal_doc_variants though. Are they supposed not to include sam_E_ chips? Only sam_D_?

Sounds like a mistake on my part. Feel free to fix here or in a followup, at your discretion.

@sajattack
Copy link
Member

sajattack commented Mar 7, 2023

Something weird going on here. Maybe a previous PR mixed up the features in hal/Cargo.toml?
https://github.com/atsamd-rs/atsamd/actions/runs/4348898859/jobs/7597968312#step:5:86
@bradleyharden PTAL

same51g = ["same51", "atsamd51p", "periph-e51g", "pins-e51g"]

@glaeqen
Copy link
Contributor Author

glaeqen commented Mar 7, 2023

oh, one more thing, please add the can feature to crates.json for appropriate PACs, so that it is tested.

Done, not sure about these hal_doc_variants though. Are they supposed not to include sam_E_ chips? Only sam_D_?

Sounds like a mistake on my part. Feel free to fix here or in a followup, at your discretion.

#677, feel free to comment what do you think. Maybe I completely misunderstood the idea behind how it worked before.

@glaeqen
Copy link
Contributor Author

glaeqen commented Mar 7, 2023

Something weird going on here. Maybe a previous PR mixed up the features in hal/Cargo.toml? https://github.com/atsamd-rs/atsamd/actions/runs/4348898859/jobs/7597968312#step:5:86 @bradleyharden PTAL

same51g = ["same51", "atsamd51p", "periph-e51g", "pins-e51g"]

FTR: Covered by #676, already rebased.

@sajattack
Copy link
Member

What do you think about calling out the example in the atsame54_xpro BSP changelog? I could go either way.

@glaeqen
Copy link
Contributor Author

glaeqen commented Mar 8, 2023

What do you think about calling out the example in the atsame54_xpro BSP changelog? I could go either way.

I wasn't sure about that. I'm not changing the BSP there if one ignores extra feature (forwarded to the HAL)? I guess that's the only thing that is worth being mentioned.

@sajattack sajattack merged commit e8fbf5d into atsamd-rs:master Mar 10, 2023
@glaeqen glaeqen deleted the mcan-core branch March 13, 2023 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants