stm32-rs / stm32f0xx-hal

A Rust `embedded-hal` implementation for all MCUs in the STM32 F0 family
BSD Zero Clause License
134 stars 59 forks source link

Move usb-remap internals for certain ICs to USB enable() #119

Closed brainstorm closed 4 years ago

brainstorm commented 4 years ago

Hopefully fixes issue #118 ?

mvirkkunen commented 4 years ago

I think it would be preferable to avoid extra feature flags. Would it be possible to implement this using a separate Peripheral type and/or constructor?

therealprof commented 4 years ago

I concur with @mvirkkunen and would prefer this to be not dependent on a feature flag.

However the elephant in the room is certainly the enabling of the SYSCFG peripheral. It is a quite important peripheral but then again maybe not that important that we would want to unconditionally enable it? Hm...

brainstorm commented 4 years ago

Totally broken code passes? hm...

therealprof commented 4 years ago

Totally broken code passes? hm...

How do you mean?

brainstorm commented 4 years ago

Totally broken code passes? hm...

How do you mean?

Uh, sorry, it turns out I had problems with my local .cargo and rustup... anyway, I don't think that fn enable(usb_remap: bool) is even reachable or a good idea, can you please advise/review?

Thanks in advance!

therealprof commented 4 years ago

Yeah, something is off about that PR but in principle it seems like a good approach to me. Will check it out in detail, hopefully in a few hours.

therealprof commented 4 years ago

Okay, first of all thanks for noticing that CI didn't test USB features at all. That's addressed in https://github.com/stm32-rs/stm32f0xx-hal/commit/86ecf612f5cc70161a23d5ebd490c9e05001d3bc.

I think that's almost there, you could simply check for self.usb_remap in the existing enable function and pack your remapping block in there.

brainstorm commented 4 years ago

I think that's almost there, you could simply check for self.usb_remap in the existing enable function and pack your remapping block in there.

Hmm... Shall I add fn usb_remap() member on UsbPeripheral (which is part of stm32-usbd crate)? Otherwise I'm not 100% how to do this :-S

therealprof commented 4 years ago

Hm, need to think about this. It's a bit unfortunate that the enable function is a freestanding function instead of a method (which I kind of had expected...).

therealprof commented 4 years ago

Okay, I played with this a bit and here's what I came up with: Instead of putting the remap into the Peripheral (and fail to get it back out of there), we can put the code into a free remap_pins function in usb.rs which can additionally take mutable references to RCC and SYSCFG so we don't need to conjure them out of thin air. This could look like:

pub fn remap_pins(rcc: &mut RCC, syscfg: &mut SYSCFG) {
    cortex_m::interrupt::free(|_| {
        // Remap PA11/PA12 pins to PA09/PA10 for USB on
        // TSSOP20 (STM32F042F) or UFQFPN28 (STM32F042G) packages
        rcc.apb2enr.modify(|_, w| w.syscfgen().set_bit());
        syscfg.cfgr1.modify(|_, w| w.pa11_pa12_rmp().remapped());
    });
}

(plus the appropriate doc comments)

therealprof commented 4 years ago

Oh, and my apologies for sending you through this journey by making assumptions about the current implementation instead of properly checking what's there.

brainstorm commented 4 years ago

No worries @therealprof, this got me acquainted with Rust embedded HALs for the first time, so I shall thank you ;)

Tusen tack!

therealprof commented 4 years ago

Thanks for sticking around!

brainstorm commented 4 years ago

My pleasure! Would you mind pushing a 0.17.1 release for this? :)

therealprof commented 4 years ago

Can do.

mvirkkunen commented 4 years ago

N.b. the commented out code in the example is missing the arguments