Apply critical sections to uses of registers where exclusivity is not guaranteed

[HarkonenBade]

No description provided.

[therealprof]

As discussed on IRC yesterday this has quite a bit of impact size-wise and I disagree with the general sentiment that these accesses cause any problems since they can only cause troubles in interrupt/exception handlers but it is (currently) not possible to safely transfer anything peripheral or handle into an interrupt handler without using a cortex_m::Mutex which requires a criticial section and thus is safe. During initialisation it is not a problem because the affected MCUs are single-core so there're no overlapping RMW operations possible.

However to cater for the peace of mind for concerned people I would suggest to make some (breaking) changes which will grant us the same safety guarantees without adding some hidden overhead:

• Add an borrowed RCC parameter for all functions requiring access instead of conjuring the pointer out of thin air
• Add a borrowed CS parameter to all functions doing RMW on registers to ensure that this only happens while a critical section is in place. In interrupt handlers this is already the case so it's basically a no-op safety guard while during initialisation it's probably a good idea anyway to make sure nothing wonky happens or interrupts are being handled right after turning them on but before the initialisation is complete. Maybe we should even consider proposing a #[init] function that this this automatically to guide users towards good programming practices.

[david-sawatzke]

I agree, i find the method with the borrowed rcc parameter preferrable, as it's the clearest and doesn't disable interrupts without your knowledge. Although i'd add separate gpio parameters for modifications that only affect the gpio peripherals.

In interrupt handlers this is already the case

Wait, we can have nested interruprs, right? Or does cortex-m-rt disable interrupts?

[therealprof]

@david-sawatzke A critical section disables interrupts. To access something protected by a Mutex you need a critical section.

[HarkonenBade]

I agree, i find the method with the borrowed rcc parameter preferrable, as it's the clearest and doesn't disable interrupts without your knowledge. Although i'd add separate gpio parameters for modifications that only affect the gpio peripherals.

I'm half tempted to make the current 'Parts' structure into an actually persistent GPIO Port handle, it will take some tweaking but would then represent a valid thing to reference for mode shifting functions, or anything else that requires exclusive access to GPIO regs.

[HarkonenBade]

The biggest issue will be allowing pins to be removed in a structured fashion, my current thinking leans towards something similar to the current parts structure, but with pins being stored in options and using take() to gain handles to them similar to how the peripheral singletons work currently.

[david-sawatzke]

I dislike the unnecessary overhead this implies, if it isn't properly optimized (which i wouldn't count on). I'd prefer adding a gpio_periph<GPIO> (or similar) in Parts, containing a non-pub GPIOx instance.

[therealprof]

I'm half tempted to make the current 'Parts' structure into an actually persistent GPIO Port handle, it will take some tweaking but would then represent a valid thing to reference for mode shifting functions, or anything else that requires exclusive access to GPIO regs.

Why? I don't see how that improves any of the "problem" areas which have been identified. On the contrary it makes sharing GPIOs even trickier because you not only need to have the handle to the GPIO pin itself but also the GPIO port structure to make any changes.

I see the current implementation as pretty close to ideal with the small wrinkle that if you decide to bypass the standard Mutex<RefCell<Option<...>>> dance by writing a CS less custom Mutex, the current implementation will become unsafe.

[HarkonenBade]

I dislike the unnecessary overhead this implies, if it isn't properly optimized (which i wouldn't count on). I'd prefer adding a gpio_periph<GPIO> (or similar) in Parts, containing a non-pub GPIOx instance.

The issue with the current Parts structure is that the second you have moved any pins out of it it becomes no longer valid to borrow, hence why I was requiring the option pins.

[HarkonenBade]

I'm half tempted to make the current 'Parts' structure into an actually persistent GPIO Port handle, it will take some tweaking but would then represent a valid thing to reference for mode shifting functions, or anything else that requires exclusive access to GPIO regs.

Why? I don't see how that improves any of the "problem" areas which have been identified. On the contrary it makes sharing GPIOs even trickier because you not only need to have the handle to the GPIO pin itself but also the GPIO port structure to make any changes.

It would only be required for mode shifting operations (the current into_* functions), not standard pin use (set_*, is_*). It's presence would be to ensure the mutual exclusion on the GPIO registers without requiring a CriticalSection parameter.

[therealprof]

@HarkonenBade

It would only be required for mode shifting operations (the current into_* functions), not standard pin use (set_, is_). It's presence would be to ensure the mutual exclusion on the GPIO registers without requiring a CriticalSection parameter.

I don't see the benefit of not requiring a CriticalSection because you'll need the CriticalSection anyway to share the GPIO port access and also it's yet another handle one has to lug around...

The issue with the current Parts structure is that the second you have moved any pins out of it it becomes no longer valid to borrow, hence why I was requiring the option pins.

What's the use case for this?

[david-sawatzke]

The issue with the current Parts structure is that the second you have moved any pins out of it it becomes no longer valid to borrow, hence why I was requiring the option pins.

Which is why i'd want a separate thing for that, because i dislike the option bits.

We could probably provide a unsafe function, getting the peripheral handler (Part or gpio_periprh<GPIO or whatever) and an additional safe one requiring cs and getting a &mut . This would make it possible to use it with rust typesystem by having it when needed, but has a simple way out.

also it's yet another handle one has to lug around

The typical use case only inits the gpios in the beginning, so we optimize for that case, making it more efficient and (hopefully) smaller than always using a CS.

[HarkonenBade]

@HarkonenBade

It would only be required for mode shifting operations (the current into_* functions), not standard pin use (set__, is__). It's presence would be to ensure the mutual exclusion on the GPIO registers without requiring a CriticalSection parameter.

I don't see the benefit of not requiring a CriticalSection because you'll need the CriticalSection anyway to share the GPIO port access and also it's yet another handle one has to lug around...

I suppose the aim was to avoid needing to wrap regular initialisation in a CS, also it would mean that the into_ functions could entirely dispense with generating GPIO pointers and just reference the stored peripheral in the Port structure.

[therealprof]

This would make it possible to use it with rust typesystem by having it when needed, but has a simple way out.

Brrr. This is exactly what people are complaining about: Too much unsafe use. 🤷‍♂️

The typical use case only inits the gpios in the beginning, so we optimize for that case, making it more efficient and (hopefully) smaller than always using a CS.

A CS is cheap in terms of size (multiple, one per access as implemented by this PR are not). It's probably a good idea anyway to do all initialisation under the control of a CS to prevent unintended execution of an interrupt handler in the middle of initialisation. Taking a borrowed CS itself is completely free and doing so clarifies intent of a function to a user.

[HarkonenBade]

The issue with the current Parts structure is that the second you have moved any pins out of it it becomes no longer valid to borrow, hence why I was requiring the option pins.

Which is why i'd want a separate thing for that, because i dislike the option bits.

We could probably provide a unsafe function, getting the peripheral handler (Part or gpio_periprh<GPIO or whatever) and an additional safe one requiring cs and getting a &mut . This would make it possible to use it with rust typesystem by having it when needed, but has a simple way out.

Hmm, that does seem quite nice tbh, giving a way to trade a CS for access to the gpio regs for the duration of that CS, but also providing one in the Parts struct so you can go wild at initialization.

[HarkonenBade]

@therealprof I suppose partially I prefer the side that takes a GPIO port reference as that makes it clear what the function actually requires exclusive access to. Rather than the CS which just requires total exclusion and no finer grained distinction.

[therealprof]

I suppose the aim was to avoid needing to wrap regular initialisation in a CS, also it would mean that the into_ functions could entirely dispense with generating GPIO pointers and just reference the stored peripheral in the Port structure.

My aim is to avoid CSes around individual calls. I kind of like the idea enforcing a CS during initialisation and even think we should provide a convenient user interface for that: rust-embedded/wg#256 (comment)

[HarkonenBade]

But I'm also very in favour of essentially having a function with the form:

fn get_port(_cs: &CriticalSection) -> &GpioPort {}

Which you can use to safely get a handle anywhere where you are already enforcing exclusive access, and that only lasts for the lifetime of that exclusion.

[HarkonenBade]

I suppose the aim was to avoid needing to wrap regular initialisation in a CS, also it would mean that the into_ functions could entirely dispense with generating GPIO pointers and just reference the stored peripheral in the Port structure.

My aim is to avoid CSes around individual calls. I kind of like the idea enforcing a CS during initialisation and even think we should provide a convenient user interface for that: rust-embedded/wg#256 (comment)

I'm also very in favour of avoiding CSs around individual calls, I think this version of this PR has demonstrated to me very well the downsides to that. Though I'm not totally sold on the 'all initialisation must be done in a CS, as it feels a little overkill.

[therealprof]

Rather than the CS which just requires total exclusion and no finer grained distinction.

Err, we're taking about changing operation mode of a pin here. I find it very reasonable to require some extra safety measures for such a case. After all people are mostly reasoning about safe register accesses but completely forget about the operational and physical effects of invoking such a function; it is very often not safe to change the alternate function on the fly and this can have immediate effects like triggering interrupts, too.

[HarkonenBade]

Rather than the CS which just requires total exclusion and no finer grained distinction.

Err, we're taking about changing operation mode of a pin here. I find it very reasonable to require some extra safety measures for such a case. After all people are mostly reasoning about safe register accesses but completely forget about the operational and physical effects of invoking such a function; it is very often not safe to change the alternate function on the fly and this can have immediate effects like triggering interrupts, too.

hmm, this is not something I have as much of a handle on, and if you feel that the CS solution is notably the best one I would be happy to yield to that.

[therealprof]

I'm also very in favour of avoiding CSs around individual calls, I think this version of this PR has demonstrated to me very well the downsides to that. Though I'm not totally sold on the 'all initialisation must be done in a CS, as it feels a little overkill.

Actually the more I think about that the more I like it and it totally does not feel like overkill to me, rather than a very simple and elegant solution which in most cases will even be zero or negative size overhead.*

* The only case where it won't be zero overhead is if your whole application is consists only of an #[entry] function which I dearly hope is not a representative use case outside of the Arduino world.

[therealprof]

hmm, this is not something I have as much of a handle on, and if you feel that the CS solution is notably the best one I would be happy to yield to that.

I do think it's by far the best. It's a bit sad that we're forced to do a breaking API change due to user complaints but at the very least we're making things more explicit without losing anything. I'd be curious to see the effects of the RCC change, I think this could even improve code size slightly.

[HarkonenBade]

I think at the moment we are still quite in the genesis of how to think about the structure of the HAL, so from my perspective I kinda expect a certain amount of API juggling currently.

[HarkonenBade]

Particularly given my current desire to experiment with making the HAL eat all the peripherals.

[HarkonenBade]

Ok i've scrabbled together a quick test implementation for us to discuss and work forwards from. I've updated one of the examples(adc_values) I'll do more at some point tomorrow. I'd be very interested to hear feedback on any of the changes.
@david-sawatzke @therealprof

[david-sawatzke]

Generally looks good to me. I'd like a way to get a Clocks instance, since it's helpful to be able to determine what the current frequency is (and maybe only require a Clocks for the systick stuff). Maybe returning a tuple, making the Clocks field pub or adding a pub fn get_clocks() method.

As an aside: I dislike disabling the "unused" warnings, so it builds without warning when no features are selected. (I generally don't see the point of compiling without features selected, I'd rather fail quick and clearly, looks like a lot of generally unneeded overhead, but) I don't see much value in having it compile without warnings, but as a side effect it disables helpful pointers when developing and may lead to left-over stuff.

[therealprof]

(I generally don't see the point of compiling without features selected, I'd rather fail quick and clearly, looks like a lot of generally unneeded overhead, but) I don't see much value in having it compile without warnings, but as a side effect it disables helpful pointers when developing and may lead to left-over stuff.

There doesn't seem to be a way to manually select features when doing cargo publish. So in order for that to work it either needs to be compilable without features or default features need to be specified in Cargo.toml which people also frown upon...

The unused stuff is also required for devices which only support a fraction of the peripherals otherwise one will get a boatload of useless warnings and miss the important ones in the noise... I agree it's not ideal but as far as I know it's the best we can do; if you can think of better ways I'd certainly like to hear them...

[therealprof]

@HarkonenBade Looks great on a first glance! Will give it a closer look later.

[david-sawatzke]

There doesn't seem to be a way to manually select features when doing cargo publish. So in order for that to work it either needs to be compilable without features or default features need to be specified in Cargo.toml which people also frown upon...

Ah ok, didn't know that.

But letting stuff like SysTick or the gpio macros be unused is solely for build without features, not for "pratical" purposes.

[HarkonenBade]

Generally looks good to me. I'd like a way to get a Clocks instance, since it's helpful to be able to determine what the current frequency is (and maybe only require a Clocks for the systick stuff). Maybe returning a tuple, making the Clocks field pub or adding a pub fn get_clocks() method.

The clocks field of the Rcc struct is pub.

[HarkonenBade]

There doesn't seem to be a way to manually select features when doing cargo publish. So in order for that to work it either needs to be compilable without features or default features need to be specified in Cargo.toml which people also frown upon...

Ah ok, didn't know that.

But letting stuff like SysTick or the gpio macros be unused is solely for build without features, not for "pratical" purposes.

You can run cargo publish --no-verify which will not attempt to build to verify, though I can see the uncertainty that comes with that. I think that the plain builds are difficult as they are increasingly fragile and require a huge amount of cfg-gate/lint-rule cruft. I'd be partially tempted to just make it so that if plain builds do need to remain, that they just build lib.rs with all modules excluded, so there will be a little cfg noise in lib.rs, but none in any of the actual module sources.

[therealprof]

You can run cargo publish --no-verify which will not attempt to build to verify, though I can see the uncertainty that comes with that.

Yeah, no. ;)

I think that the plain builds are difficult as they are increasingly fragile and require a huge amount of cfg-gate/lint-rule cruft.

Yeah. Would be great if we could improve that. There's also the possibility to use build.rs to check for correct configuration and guide the user but I'm really not a fan of automagic.

[HarkonenBade]

@therealprof What would be your feeling on just gating out all modules on plain builds? (i.e. annotate all module definitions in lib.rs with #[cfg(feature="device-selected")])?

[therealprof]

@HarkonenBade No feelings here either way. As long as stuff doesn't get in the way I'm happy...

[HarkonenBade]

Ok build gating is now much less insane and a plain build still produces a clean build. It has thrown up that it seems that the HSI48 that was added by another PR is currently unaccessable.

[therealprof]

@HarkonenBade Love the changes. Except for the basic blinky (which didn't use a CS before and now does) all examples even lose a few bytes. I find the the use of a &mut rcc a bit annoying, and I don't see why we can't use a &rcc in most places.

[HarkonenBade]

@HarkonenBade Love the changes. Except for the basic blinky (which didn't use a CS before and now does) all examples even lose a few bytes. I find the the use of a &mut rcc a bit annoying, and I don't see why we can't use a &rcc in most places.

The answer is that in most places we are modifying registers in the Rcc, and &Rcc doesn't enforce exclusion. In the few places that only read data i've only required &Rcc.

[HarkonenBade]

Also i've now fixed up all the examples.

[therealprof]

@HarkonenBade Are we really? Calling functions on RCC is not a mutation. I just tested and it works fine with a regular borrow.

[HarkonenBade]

@HarkonenBade Are we really? Calling functions on RCC is not a mutation. I just tested and it works fine with a regular borrow.

Yes, but ostensibly something like rcc.regs.apb2enr.modify(|_, w| w.adcen().set_bit()); is an operation that is only safe while you have exclusive access to rcc, otherwise it is an opportunity for a race. To ensure that the peripheral in question has exclusive access to rcc I made it require an &mut rcc as that will always be an exclusive reference.

[therealprof]

Hm... 🤔

[HarkonenBade]

Any further thoughts as to the content of this PR?

[therealprof]

Other than the conflict and the lack of a CHANGELOG.md entry looks goods to me.

[HarkonenBade]

Done and squashed down to a more manageable commit with less back and forth.

[therealprof]

LGTM, thanks.