Rcc bit-band #344
Rcc bit-band #344
Conversation
|
This seems like a super ergonomic change! I haven't gone through the code in detail, but from an overview, it looks OK from a safety perspective (assuming the bit banding thing is atomic, which it is as far as I can tell) |
burrbull
force-pushed
the
rcc-enable
branch
4 times, most recently
from
96394ff to
13e242e
Compare
|
Rebased. |
burrbull
force-pushed
the
rcc-enable
branch
4 times, most recently
from
f7c6e7f to
249c292
Compare
|
Removed dependency from #337 |
|
cc @TheZoq2 I think it's ready to merge. |
src/rcc.rs
Outdated
| @@ -576,191 +543,26 @@ impl GetBusFreq for APB2 { | |||
| } | |||
| } | |||
|
|
|||
| use crate::pac::rcc::RegisterBlock as RccRB; | |||
There was a problem hiding this comment.
I don't like this alias, it doesn't seem like just typing out RegisterBlock directly in a few places is that annoying, and saves some jumping around when reading the code (unless there are multiple RegisterBlocks in scope here, though in that case I'd prefer to just write out rcc::RegisterBlock
src/rcc/enable.rs
Outdated
| macro_rules! bus { | ||
| ($($PER:ident => ($apbX:ty, $bit:literal),)+) => { | ||
| $( | ||
| impl Sealed for crate::pac::$PER {} |
There was a problem hiding this comment.
Weird spacing (is this a rustfmt oddity?)
| GPIOB => (APB2, 3), | ||
| GPIOC => (APB2, 4), | ||
| GPIOD => (APB2, 5), | ||
| GPIOE => (APB2, 6), | ||
| I2C1 => (APB1, 21), | ||
| I2C2 => (APB1, 22), | ||
| PWR => (APB1, 28), | ||
| SPI1 => (APB2, 12), | ||
| SPI2 => (APB1, 14), | ||
| USART1 => (APB2, 14), | ||
| USART2 => (APB1, 17), | ||
| USART3 => (APB1, 18), | ||
| WWDG => (APB1, 11), | ||
| } | ||
|
|
||
| #[cfg(any(feature = "xl", feature = "high"))] | ||
| bus! { | ||
| GPIOF => (APB2, 7), | ||
| GPIOG => (APB2, 8), | ||
| } | ||
|
|
||
| #[cfg(any(feature = "high", feature = "connectivity"))] | ||
| bus! { | ||
| SPI3 => (APB1, 15), | ||
| } | ||
|
|
||
| ahb_bus! { | ||
| CRC => (6), | ||
| DMA1 => (0), | ||
| DMA2 => (1), | ||
| } | ||
|
|
||
| #[cfg(feature = "high")] | ||
| ahb_bus! { | ||
| FSMC => (8), | ||
| } | ||
|
|
||
| bus! { | ||
| TIM2 => (APB1, 0), | ||
| TIM3 => (APB1, 1), | ||
| } | ||
|
|
||
| #[cfg(any(feature = "stm32f100", feature = "stm32f103", feature = "connectivity"))] | ||
| bus! { | ||
| TIM1 => (APB2, 11), | ||
| } | ||
|
|
||
| #[cfg(any(feature = "stm32f100", feature = "high", feature = "connectivity"))] | ||
| bus! { | ||
| TIM6 => (APB1, 4), | ||
| } | ||
|
|
||
| #[cfg(any( | ||
| all(feature = "high", any(feature = "stm32f101", feature = "stm32f103")), | ||
| any(feature = "stm32f100", feature = "connectivity") | ||
| ))] | ||
| bus! { | ||
| TIM7 => (APB1, 5), | ||
| } | ||
|
|
||
| #[cfg(feature = "stm32f100")] | ||
| bus! { | ||
| TIM15 => (APB2, 16), | ||
| TIM16 => (APB2, 17), | ||
| TIM17 => (APB2, 18), | ||
| } | ||
|
|
||
| #[cfg(feature = "medium")] | ||
| bus! { | ||
| TIM4 => (APB1, 2), | ||
| } | ||
|
|
||
| #[cfg(any(feature = "high", feature = "connectivity"))] | ||
| bus! { | ||
| TIM5 => (APB1, 3), | ||
| } | ||
|
|
||
| #[cfg(any(feature = "xl", all(feature = "stm32f100", feature = "high",)))] | ||
| bus! { | ||
| TIM12 => (APB1, 6), | ||
| TIM13 => (APB1, 7), | ||
| TIM14 => (APB1, 8), | ||
| } | ||
|
|
||
| #[cfg(all(feature = "stm32f103", feature = "high",))] | ||
| bus! { | ||
| TIM8 => (APB2, 13), | ||
| } | ||
|
|
||
| #[cfg(feature = "xl")] | ||
| bus! { | ||
| TIM9 => (APB2, 19), | ||
| TIM10 => (APB2, 20), | ||
| TIM11 => (APB2, 21), | ||
| } | ||
|
|
||
| #[cfg(any(feature = "stm32f102", feature = "stm32f103"))] | ||
| bus! { | ||
| USB => (APB1, 23), | ||
| } |
There was a problem hiding this comment.
Note to self, check these mappings
src/timer.rs
Outdated
| $TIMX::enable(rcc); | ||
| $TIMX::reset(rcc); |
There was a problem hiding this comment.
This note applies to all copies of enable and reset function calls: Should we not move them out of the unsafe block? They are safe to call so they don't need to be there as far as I can tell?
On the other hand, does it make sense to not have them be unsafe. We could make them public API and mark them as unsafe in case people want to circumvent part of the HAL in a project but still want to init the peripherals via RCC
There was a problem hiding this comment.
This note applies to all copies of
enableandresetfunction calls: Should we not move them out of the unsafe block? They are safe to call so they don't need to be there as far as I can tell?
Moved.
There was a problem hiding this comment.
On the other hand, does it make sense to not have them be unsafe. We could make them public API and mark them as unsafe in case people want to circumvent part of the HAL in a project but still want to init the peripherals via RCC
We can, but I don't see sense to make them unsafe as rcc can be gotten in safe way only during initialization. In all other places we need to use unsafe RCC::ptr().
|
One small final note, these changes should go into Breaking Changes in the changelog, other than that this looks ready for squash and merge |
|
Done. |
Depends on #337