Add public functions to toggle PWREN bits

[skammer]

Discussion started in #62 so I'll just copy description from there:

When enabling standby mode, there is a need to set PWREN bit in APB1 ENR register.

Right now it can be achieved by calling constrain on rcc.bkp. https://github.com/stm32-rs/stm32f1xx-hal/blob/master/src/rcc.rs#L367

And while it does set the right bits in the right places, it enables other registers as well, which may to may not be desirable.

Ideally, I'd like to be able to do it manually:

rcc.apb1.enr().modify(|_r, w| {
    w.pwren().set_bit()
});

Or maybe just call a function that does it for me, something like what set_sleepdeep() does: https://github.com/rust-embedded/cortex-m/blob/master/src/peripheral/scb.rs#L585

rcc.apb1.set_pwren();

---

As suggested by @TheZoq2 in the same issue #62 (comment), this is the PR for further discussion.

[TheZoq2]

Thanks for the PR!

I believe that set_pwren is fine, but clear_pwren will break the bkp abstraction as having ownership over the Bkp struct indicates that everything done in constrain still holds.

[skammer]

I find clear_pwren to be explicit enough at what it does to deter people from calling it unintentionally, while constrain on Bkp does stuff implicitly.

Since there is no way to clear PWREN bit and reset changes made by constrain in case you no longer need access to backup registers, I find it quite useful to have both set_pwren and clear_pwren.

Another concern of mine is that one particular feature — access to backup registers — commands implementation of completely independent feature — deep sleep mode. I fully understand why ownership abstraction over Bkp is needed, I'm just not sure prohibiting access to a shared register is the best way to achieve that.

[TheZoq2]

I find clear_pwren to be explicit enough at what it does to deter people from calling it unintentionally, while constrain on Bkp does stuff implicitly.

I disagree, in the current implementation, there is no indication that it would break the backup domain abstraction.

Another concern of mine is that one particular feature — access to backup registers — commands implementation of completely independent feature — deep sleep mode

Agreed, tying the pwr registers and backup domain together isn't a perfect solution and I wouldn't mind having an abstraction where you need pass a PWR token to BKP in order to initialise it. That way, you could hand BKP back in order to disable the pwr regs. I think we may have discussed that in the PR I mentioned, but I can't remember.

[skammer]

One option would be to require passing PWR to clear_pwren so we can check is it interferes with BKP:

    /// Clear power interface clock (PWREN) bit in RCC_APB1ENR
    pub fn clear_pwren(&mut self, pwr: PWR) {
        let backup_register_access = pwr.cr.read().dbp().bit();

        assert!(backup_register_access, "Clearing PWREN will interfere with BKP");

        self.enr().modify(|_r, w| {
            w.pwren().clear_bit()
        })
    }

But then again, it would require some way to unconstrain bkp, which is outside of the scope of this PR.

I think we can keep set_pwren and just remove clear_pwren until someone comes up with a better option. Would that be an OK solution for the time being? Clearing PWREN is not really needed in my use case, as it's cleared automatically after MCU wakes up.

[TheZoq2]

I think we can keep set_pwren and just remove clear_pwren until someone comes up with a better option

That sounds like a good option to me. @TeXitoi You had lots of comments on the RTC, do you see any problems with this approach?

Sidenote:

assert!(backup_register_access, "Clearing PWREN will interfere with BKP");

I'm not a fan of runtime assertions in embedded systems, so I'd prefer something static

[TeXitoi]

I'll try to find the time to review this PR before the end of the week.

[TeXitoi]

clear_pwren would break the backup abstraction, thus I'm against it as is.

I can't find any harm on set_pwren, but what's the point? I can't find any reference to this register except on backup domain and RTC. The goal of the HAL is to abstract the hardware in a way that you cannot do something that would not work. Why do you need to set this register?

[TheZoq2]

but what's the point? I can't find any reference to this register except on backup domain and RTC.

It is used for low power modes. For example, in table 15 in section 5.3.5, it says you need to set PDDS in PWR_CR and WUF in PWR_CSR. If I recall correctly, those can't be written to unless PWRen has been set.

[skammer]

Yes, @TheZoq2 is absolutely correct. I've written a post about the specifics https://vasiliev.me/blog/sending-stm32f1-to-deep-sleep-with-rust/. The lack of access to apb1 from outside hal crate makes setting pwren very unintuitive — by calling constrain on BKP.
Having a way to set pwren in a more obvious way would be very useful, at least in my particular use case.

[TheZoq2]

Nicely written blog post, I have been trying to make deep sleep work as well, but I can't quite get it to go below one milliamp in my project.

I'd be fine with merging this, but since the only use case is to go to sleep which itself is a bit of a strange process, do you think it would make more sense to add a function to do that directly?

[skammer]

I am not entirely sure that would be possible. There is a lot of configuration involved: which low power mode you want to enable, whether or not you want to wake up on a watchdog timer or WKUP pin, which peripherals to disable, etc, etc, etc. Also, the place to finally call wfi may also be very important, as you may want to integrate with different processes. Just too many fiddly bits, in my opinion.

[TheZoq2]

Yea, perhaps it's not as easy as I think it would be, I might give it a try at some point though. For now, i'd be happy to merge this if you remove the clear_pwren function

[skammer]

Done

[TheZoq2]

Very nice, thanks!