Skip to content

add can support #246

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

Merged
merged 1 commit into from
Jan 22, 2021
Merged

add can support #246

merged 1 commit into from
Jan 22, 2021

Conversation

xoviat
Copy link
Contributor

@xoviat xoviat commented Jan 2, 2021

This adds support for can with the bxcan crate. This differs slightly from the stm32f1 pull request and the stm32f7 pull request: the Can::new method takes ownership of the pins, which requires the user to configure the pins correctly, and doesn't use the afio functionality, which doesn't appear to be present here.

@xoviat xoviat mentioned this pull request Jan 2, 2021
6 tasks
@xoviat xoviat changed the title wip: add can support add can support Jan 12, 2021
@xoviat xoviat marked this pull request as ready for review January 12, 2021 16:36
@mgottschlag
Copy link
Contributor

mgottschlag commented Jan 12, 2021

While the RCC interface looks completely unlike everything else, I'd argue that it is less racy than the existing implementations which just do a read-modify-write cycle in a potentially multitasked (i.e., interrupts of different priority) application.

Edit: nevermind, everything else uses bitbanding for atomic access, so not racy at all. I guess this interface is probably not that great as it makes usage more complex.

src/can.rs Outdated
//! | Function | Pins |
//! |----------|-------|
//! | TX | PB13 |
//! | RX | PB12 |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those tables do not fully reflect the pins defined on L59.

}
}

pins! {
Copy link

@timokroeger timokroeger Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick look at the stm32f407 datasheet shows that the CAN signals can be mapped to even more pins (for larger packages).

src/can.rs Outdated
Instance: Enable<Bus = APB1>,
{
/// Creates a CAN interaface.
pub fn new<P>(can: Instance, _pins: P, apb: &mut APB1) -> Can<Instance>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a common pattern to take ownership of the pins.
For CAN it might be a bit too restrictive though: stm32-rs/stm32f1xx-hal#215 (section Separate assign_pins())

Copy link
Member

@thalesfragoso thalesfragoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, nice work. I just left a few questions and suggestions, it would be nice if you could also drop the RTC commits.

src/can.rs Outdated
fn enable() {
unsafe {
let rcc = &(*RCC::ptr());
bb::set(&rcc.apb2enr, $peren)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops!

@xoviat xoviat force-pushed the canbus branch 2 times, most recently from 76bc6c8 to ac87594 Compare January 19, 2021 02:01
Copy link
Member

@thalesfragoso thalesfragoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, sorry for the delay.

bors r+

@bors bors bot merged commit 840819f into stm32-rs:master Jan 22, 2021
@xoviat xoviat deleted the canbus branch January 22, 2021 02:32
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.

4 participants