Why does configuring a GPIO require a critical section in this hal?

[torkeldanielsson]

I'm using an stm32f042 and come from using the stm32f107. Getting started with this hal there is a different approach here in how gpio:s are acquired. Functions such as into_push_pull_output take a critical section here, whereas in other hal:s they take one or more configuration registers.

Is the current design of this hal by intention?

I was surprised to find that the example for how to blink more than one LED wraps the whole main loop in a critical section. If I am reading things correctly then the main reason for that is in how gpio:s are acquired.

I was looking a bit at restructuring this. It seems f1xx uses a different set of registers for setting up the gpio:s. But f3xx and f4xx seems more reasonably similar and could maybe be used as reference (i.e. place to shamelessly copy code from...). It seems like it would be a bit of work though, and it would cause breaking changes for users of this HAL. And maybe I'm missing an obvious/good reason for it being the way it is? So I thought it best to check here first before putting more effort into it :)

edit: Actually, after writing this and then letting go of trying to get rid of the critical section by changing the hal I came up with the following to not have the main loop inside the critical section while having more than one led:

    let (mut led_yellow, mut led_blue, mut led_red) = cortex_m::interrupt::free(move |cs| {
        let mut led_yellow = gpiob.pb3.into_push_pull_output(cs);
        led_yellow.set_high().ok();
        let mut led_blue = gpiob.pb4.into_push_pull_output(cs);
        led_blue.set_high().ok();
        let mut led_red = gpiob.pb5.into_push_pull_output(cs);
        led_red.set_high().ok();

        (led_yellow, led_blue, led_red)
    });

This does not feel that bad. So maybe I should make a PR changing the multiple-led example to this instead of changing the whole hal?

[therealprof]

Is the current design of this hal by intention?

Yes.

I was surprised to find that the example for how to blink more than one LED wraps the whole main loop in a critical section. If I am reading things correctly then the main reason for that is in how gpio:s are acquired.

That is just laziness. There's no reason to hold the CS for the duration of the program.

And maybe I'm missing an obvious/good reason for it being the way it is? So I thought it best to check here first before putting more effort into it :)

STM32F0 doesn't have atomic operations or bit-banding. The only safe way to set up a pin is to disable interrupts because of the required read-modify-write operations. Typically initialisation (unless you need tri-stating or wild pin setup changes in the middle of the program) is limited to the early setup phase of the program when you usually hold a critical section anyway (or can simply claim you're holding one because you know interrupts are disabled), by passing in that proof to the init functions it is zero-cost.

Other possibilities would to hide the CS in the setup function itself but if you have multiple things to setup you're paying the (small) setup cost for the CS a couple of times plus some people frown upon implicit CSes due to the impact on the worst case execution time of interrupt handlers. 🤷🏻‍♂️ Another possibility sometimes used to simply ignore the possible race in the RMW operation and hope for the best.

This does not feel that bad.

Yes, also if you you want to move out the pin to a Mutex for sharing with an interrupt handler, you can do it right on the spot (because that requires a CS, too).

So maybe I should make a PR changing the multiple-led example to this instead of changing the whole hal?

Yes, I'd appreciate that.

[torkeldanielsson]

Thank you for a good answer! I hadn't thought about the interrupt unsafety of the modify(..) calls :)

A pull request is up, so I close this.