Open nickray opened 5 years ago
I'd like to note that this was only the first step towards a larger refactoring. The end goal is to create a HAL-level Peripherals
struct that wraps the PAC-level Peripherals
. There are many reasons for this (and a few criticisms :-) ), and I intend to write an article about it that talks about the subject exhaustively.
Since I haven't written that article yet, here's a very short summary of my reasoning:
Peripherals::take
prevents the user from safely modifying the hardware using the PAC API. That means, the HAL API can make assumptions about the initial hardware state after initialization. (With the current approach, I've seen a lot APIs completely ignoring that the user could have changed anything before they were initialized, which makes the type state in these APIs plainly wrong, in my opinion. I don't know whether this specific HAL is affected.)The criticisms of this approach mainly revolve around two areas:
pre_init
.I'd say all those criticisms are valid, but equally (or more so!) apply to whatever we have today, so my approach doesn't make things worse, at least. I also think some of it can be addressed by thinking hard about the subject for some time, which I've only started to do.
But whether you buy into my grand vision or not, I believe that replacing extension traits with plain constructors is worthwhile, for the reasons mentioned.
@hannobraun: Thanks for piping in with background, and looking forward to your article, please do write it! Agree that the interplay between HAL and PAC is not satisfactory yet. Seems hard to get right...
As an example: This L4 chip series has a lower voltage range mode (one use case I have is powering via NFC field), which has implications like: USB not available, maximum clock speed and flash latencies different. If one's app is launched from a (possibly external) bootloader, then multiple assumptions may or may not hold. So this is another way the HAL may initialize with a chip that is not in "reset state". Moreover, the app may have to switch between low and high voltage mode during its lifetime, so clock + peripheral configurations are not one-off and initial, but need to be modified (in a safe way). At least in this case, an "unfreeze" might be able via some tricks to know what is different from reset state, as checking for all assumptions is not a very zero cost approach :)
Another issue with the HAL handing out a sanitized PAC (so it can enforce its assumptions, or hook into changes that modify invariants) is that all these HAL APIs need to be thought out and implemented (and there's already a backlog of unproven/todo APIs, my pet API is USB) to avoid most non-trivial use cases from having to do an unsafe direct access to the original PAC anyway. But agree that it's better, not worse, to have to explicitly be "naughty", instead of casually/inadvertently breaking HAL assumptions.
Certainly interesting and worthwhile to think about what can be moved from PAC to HAL level though! I'd really like there to be substantial consistency between HALs for different vendors (instead of, say, STM32 and nRF forking at some point). Like... a better Java haha.
@nickray
As an example: This L4 chip series has a lower voltage range mode [...] At least in this case, an "unfreeze" might be able via some tricks to know what is different from reset state, as checking for all assumptions is not a very zero cost approach :)
Wanting to avoid having to check all assumptions is definitely one of the motivations that led me to the Peripherals
pattern. In lpc82x-hal
, there's the switch matrix, which controls assignment of functions to various pins. Unlike other peripherals, it can't easily (and cheaply) be reset, so preventing the user from messing with the initial state was the only zero-cost way of upholding the static guarantees that I could find.
I hope my approach will hold up in different scenarios, like the one you describe. I think my article (once I get around to writing it) will only be the start of the discussion though.
Another issue [...] is that all these HAL APIs need to be thought out and implemented [...] to avoid most non-trivial use cases from having to do an unsafe direct access to the original PAC anyway.
I think the initial step when starting a new HAL using the Peripherals
approach would be to just add the unwrapped PAC-level peripherals to the Peripherals
struct, to make them available to the user, then write wrappers over time.
If modifying one peripheral can affect the guarantees of another peripheral's HAL API, add a wrapper that does nothing but provide an unsafe free
method that returns the unwrapped peripheral. This gives you a good place to document what could go wrong, and the unsafe
will hopefully make the user pause and read the documentation :-)
Depending on how much work is put into the HAL, it might take a long time until non-trivial use cases can be handled without dipping into unwrapped APIs, but I agree with you that it's still better (or at least not worse) than the current situation.
Certainly interesting and worthwhile to think about what can be moved from PAC to HAL level though! I'd really like there to be substantial consistency between HALs for different vendors (instead of, say, STM32 and nRF forking at some point). Like... a better Java haha.
I think consistency has its limits in this space, due to the differences between microcontrollers (at least as long as you want to be zero-cost, or as close as possible). I certainly hope that we can come up with some generally accepted solutions though, that work for a broad range of HALs.
Somewhat orthogonal to https://github.com/stm32-rs/stm32l4xx-hal/issues/56, there is a fundamental rethink of the HAL API going on in nRF land. Instead of extension traits with associated constrain/split methods, "constructors" (
new
methods) can be used.Examples:
This is mostly being driven by @hannobraun. Some obvious advantages:
@MabezDev @mathk @FrozenDroid: Do we want this too? :)