stm32-rs / stm32f7xx-hal

A Rust embedded-hal HAL for all MCUs in the STM32 F7 family
Apache License 2.0
115 stars 67 forks source link

solved issue: cant freeze rcc clock when using spi #104

Closed hardfau1t closed 3 years ago

hardfau1t commented 3 years ago

solves #65 this PR tackles both when using SPI as well as dma, so there are 2 commits separate, when called spi enable it requires mutable reference to rcc

    let mut spi1:Spi1 = Spi::new(p.SPI1, (sck, miso, mosi)).enable(
        &mut rcc,
        ClockDivider::DIV8,
        spi::Mode{
            polarity : Polarity::IdleHigh,
            phase : Phase::CaptureOnFirstTransition,
        }
        );

which fails when when freeze is called,

18 |     let clks = rcc.cfgr.sysclk(32.mhz()).freeze();
   |                -------- value partially moved here
...
25 |         &mut rcc,
   |         ^^^^^^^^ value borrowed here after partial move
   |
   = note: partial move occurs because `rcc.cfgr` has type `stm32f7xx_hal::rcc::CFGR`, which does not implement the `Copy` trait

when enabling spi we can use rcc::Enable which solves above issue, now instead of giving mutable reference of rcc to enable function we are giving mutable reference to rccbus to new function same as in i2c.

i am not a professional in this case, feel free to reject this PR if it is wrong

mvertescher commented 3 years ago

Thanks for looking at this @7h3qu1rkyb1t! At a glance, this fix seems reasonable to me.

Any thoughts @pftbest or @hannobraun? If not, I'll merge this later this week.

hannobraun commented 3 years ago

I had a quick glance, and one thing that jumped out to me is that enabling the SPI peripheral clock moved from Spi::enable to Spi::new. Not sure if this is a problem (I don't really have a big-picture overview here, and no time to get re-acquainted right now), but it's certainly misleading.

hardfau1t commented 3 years ago

I think when we create new spi peripheral first thing we need to do is enable peripheral clock, if not we can't configure anything on that peripheral. Here We are not enabling spi, that would be big mistake.

If let say in future we are making some default configuration when the peripheral is created, if peripheral clock enable is en enable state then we all the default configuration added while creating it wont take effect. Another thing is if anyone creates a peripheral he meant to initiate that peripheral means start the clock Please let me know if this concept is wrong, if so May be i can move that back to spi::enable still solving that issue

hardfau1t commented 3 years ago

One more thing is i think we need to restructure the api such that every peripheral initiation is same, (if yes may be you can assign that to me). It shouldn't be confusing like i2c doesn't have init or enable but spi do have

hannobraun commented 3 years ago

I think I wrote this code originally. I don't recall what I thought back then, but there's a reason why I prefer leaving any initialization out of the constructor and doing that in a separate method. I think having a separate constructor each HAL API is wrong, for several reasons which are besides the point here (but which I should definitely take the time to write down at some point).

I think a better approach is for the HAL to wrap pac::Peripherals in its own Peripherals struct. If you do that, you can't have initialization code in the constructor, otherwise you get a lot of unnecessary runtime overhead just for using the HAL. An example of a HAL that works that way is LPC8xx HAL.

I don't think any of it matters here. I'm just providing some background on why the decisions might have been made that way.

hardfau1t commented 3 years ago

Moved clock initiation to enable method

hardfau1t commented 3 years ago

@hannobraun changed initialization as you suggested

hannobraun commented 3 years ago

Okay then, since nobody objected, I'll merge now. Thank you for the PR, @hardfau18!