-
Notifications
You must be signed in to change notification settings - Fork 179
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
Update USB driver #320
Update USB driver #320
Conversation
There was a problem hiding this 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. I left a few notes, but in general, this looks good
@@ -15,7 +15,7 @@ use usbd_serial::{SerialPort, USB_CLASS_CDC}; | |||
|
|||
#[entry] | |||
fn main() -> ! { | |||
let dp = pac::Peripherals::take().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have deprecated the stm32
name in favor of pac
src/lib.rs
Outdated
feature = "stm32-usbd", | ||
any(feature = "stm32f102", feature = "stm32f103") | ||
))] | ||
#[cfg(any(feature = "stm32f102", feature = "stm32f103"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coud feature=stm32-usbd
be used instead here?
src/usb.rs
Outdated
|
||
fn enable() { | ||
let rcc = unsafe { (&*RCC::ptr()) }; | ||
cortex_m::interrupt::free(|_| unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this not make the unsafe
region larger than nessecary? It's only required for the ptr, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand unsafety, the unsafe operation here is accessing the memory via a pointer constructed in unsafe way, not just constructing the pointer. So larger unsafe region looks more correct compared to what we had before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the unsafe code create a reference, not a pointer. So creating this reference is hightly unsafe, but using the reference is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating this reference is better inside the interrupt::free
. So I propose to just move the let rcc
line inside the interrupt::free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With #344 be merged we don't need critical section as it uses bit-banding.
src/usb.rs
Outdated
|
||
fn enable() { | ||
let rcc = unsafe { (&*RCC::ptr()) }; | ||
cortex_m::interrupt::free(|_| unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating this reference is better inside the interrupt::free
. So I propose to just move the let rcc
line inside the interrupt::free.
bors r+ |
No description provided.