Allow to "freeze" gpio type state

[usbalbin]

1. Changing pin state

Today a gpio pin can be turned into another mode using for example let pin = pin.into_push_pull_output(); or let pin = pin.into_alternate(). This can also be done multiple times for example:

let pin = pin.into_floating_input(); // Configure as digital input
//use as input...

let mut pin = pin.into_push_pull_output(); // Reconfigure the same pin, now as an output
pin.set_low();

let pin = pin.into_analog(); // Reconfigure as analog input
// perform AD-reading...

This is of course very convenient.

2. Same pin, Multiple peripherals

Some analog peripherals like for example the comparators need their input pins to be set in analog mode. Since the pin is set to analog mode it should also be possible to perform AD-readings on it at the same time as the comparator is connected.

This may for example be useful for having near instant over current protection while still allowing regular AD-readings all on the same pin.

3. Peripherals

When setting up a peripheral such as the comparator, the input pins are typically provided by value, thus consuming the pin. This to ensure that no one else will change the mode of the pin.

The problem

Currently 1 and 3 works, but not 2. Changing peripherals to take a pin by reference and store the reference would prove quite cumbersome since the pin would have to live as long as the peripheral and not be moved etc.

Only having having the init function take a reference and then not storing it would not prevent the user from then changing pin mode after peripheral init.

Proposed solution

Add type parameter F to the pin types which may be one of the following types:

- pub struct $PXi<MODE> {
+ pub struct $PXi<MODE, F = IsNotFrozen> {
     _mode: PhantomData<MODE>,
+    _frozen_state: PhantomData<F>,
 }

```rust
/// Type state for a gpio pin frozen to its current mode
/// 
/// The into_alternate/analog/{x}_input/output can not be called on a pin with this state.
/// The pin is thus guaranteed to stay as this type as long as the program is running.
pub struct IsFrozen;

/// See [IsFrozen]
pub struct IsNotFrozen;

The type parameter default to IsNotFrozen so as to keep backwards compatibility and 1 from above works as before. However once the user knows they will want the pin to stay in a particular state for the rest of the program, let pin = pin.freeze(); may be called. Which returns the same type but with IsFrozen this disabling the into_alternate/analog/{x}_input/output methods.

This then allows peripherals init to take the pin by reference (assuming F=IsFrozen) and not having to hold on to it. (only done for some of the peripherals so far)

[usbalbin]

@AdinAck do you have any input on this?

[AdinAck]

I have also been thinking about this!

It's really quite unfortunate that references to ZSTs are not ZSTs themselves, otherwise I believe holding references would have been the correct solution, the lifetime troubles could certainly be worked out.

However, since we cannot do that without introducing significant overhead, this is a clever solution. I can't tell if it's complete though...

I believe that the notion that a peripheral shall be owned by one thing and one thing only is an invariance that should not be broken. Seeing as the goal of the HAL is to maintain a certain parity with the hardware, what if instead we change the HAL pin structs a bit to reflect the hardware? Clearly comparators have an additional special access to select pins.

So the pins could have channels:

struct PXi<Mode> {
    ch1: PXi1<Mode>,
    ch2: PXi2<Mode>,

That way, it can be used like so:

let pin = gpiox.pi.into_input_pull_up().split(); // `split()` will only be implemented for inputs

// i don't know either of these peripheral interfaces
let adc = p.ADC.constrain(pin.ch1);
let comp = p.COMP.constrain(pin.ch2);

Conversion methods (into_x) will not be available on the channels, so the pin is effectively frozen. The inverse of split() can be provided as recombine() (this is an idea I've been playing around with as a standard for all split peripherals):

let pin = pin.common.recombine((adc.reset_and_release().1, comp.reset_and_release().1)).into_push_pull_output();

pin.common is inspired by the PWM interface.

---

I'm still thinking about all this, so it's all in flux. I think this better adheres to the design patterns already present in the HAL, but maybe it's too complicated, let me know what you think. I am not against your proposal, it's certainly a step in the right direction nonetheless.

Edit: Thinking about it more, it may not be immediately clear how many "channels" each pin should have, we could just arbitrarily give all pins more than enough channels (say 8), but that may be a bit weird. Also, unlike the IsFrozen technique, this approach does not lock the pin mode for the duration of the program.

Edit 2: I have done a lot more thinking and I think this is not the right idea. Pins are not the only peripherals that may be "read" by multiple other peripherals, and there is no fixed maximum number of "observers". While it is unfortunate that the IsFrozen approach forbids any kind of collection after usage, it will help improve a lot of interfaces, not limited to GPIO at all.

[AdinAck]

Example of how IsFrozen applies to other peripherals:

let gpiox = p.GPIOX.split();
let px1 = gpiox.px1.into_analog().freeze();
let px2 = gpiox.px2.into_floating_input().freeze();

let (dacx_ch1, ..) = p.DACX.constrain(); // iirc dac channels are intrinsically frozen

let exti = p.EXTI.split();
let (comp1, ..) = p.COMP.split();
let comp1 = comp1.constrain(&dacx_ch1, &px1); // requires both sources are frozen
let comp1 = comp1.freeze(); // now even the comp itself is frozen for use with EXTI

exti.ch2.constrain(&px2).enable(); // requires `px2` is frozen
exti.ch23.constrain(&comp1).enable(); // requires `comp1` is frozen

This example also demonstrates an improved and more consistent EXTI interface.

[AdinAck]

There should also be a Frozen trait which can be added as a requirement to source traits.

Like for comparators:

pub trait PositiveInput<C>: Frozen {
    fn setup(&self, comp: &C);
}

The Frozen trait itself should be unsafe since the implementation determines whether it behaves frozen or not:

unsafe trait Frozen {}

There could also be a Freeze trait:

trait Freeze {
    type Frozen: Frozen;

    fn freeze(self) -> Self::Frozen;
}

Edit: Oh whoops you already had a lot of this 😅

[usbalbin]

So the pins could have channels: [...]

What about something like:

fn split<const N: usize>(self) -> [Channel<N>; N] {
    todo!()
}
fn recombine(..., channels: [Channel<N>; N]) -> ... {
    todo!()
}

The user is free to split into however many channels they want. The channels then remember how many siblings they are so that we can require them all when recombining

[AdinAck]

What about something like...

A fantastic idea! I will need time to think about this :)

[AdinAck]

Ok here's what I've come up with:

What we're dealing with here is a new way that peripherals interact with each other. In this case, EXTI, COMP, and ADC are observing GPIO. To facilitate this, consider:

struct Observed<P, const OBSERVERS: usize> {
    p: P,
}

A struct to hold peripherals which are to be observed.

struct ObservationToken<P> {
    _p: PhantomData<P>,
}

A struct to represent a registered observation of a peripheral.

trait Observable {
    fn observe<const N: usize>(self) -> (Observed<Self, N>, [ObservationToken<Self>; N]) {
        (
            Observed { p: self },
            core::array::from_fn(|| ObservationToken { _p: PhantomData }), // this may be introduce overhead, will have to investigate
        )
    }
}

A trait providing an interface to make peripherals observed.

Usage:

let (pin, [ob_exti, ob_comp, ob_adc]) = gpiox.pxi.into_analog().observe();
// pin type is `Observed<PXi<Analog>, 3>`
// ob_* type is `ObservationToken<PXi<Analog>>`

exti.source(ob_exti).enable();
comp.source(dac_ch1, ob_comp);
adc.constrain(Sequence { .., third: ob_adc }); // very pseudo

let (_, ob_exti) = exti.reset_and_release();
let (_, ob_comp) = comp.reset_and_release();
let (_, ob_adc) = adc.reset_and_release();

let pin = pin.release([ob_exti, ob_comp, ob_adc]);

pin.get_level(); // we can use the pin in software again

The key to my reasoning for this design is these peripheral interfaces only need to "lock" the observed peripheral configuration, and do not invoke any of its methods because the interaction is purely via hardware. Only software interfaces should borrow peripherals by reference.

So HAL interfaces will look like:

fn constrain<INP, INM>(rb: COMP1, inp: ObservationToken<INP>, inm: ObservationToken<INM>)
where
    INP: PositiveInput,
    INM: NegativeInput,
{ .. }

Since the HAL never uses the software methods (really it shouldn't anyways) the usage of ObservationToken explicitly shows the hardware relationship. (The name may be poor but of course, that can be worked out).

This whole design philosophy relies on the invariant fact that up to one instance of any peripheral type exists.

---

Still more thinking to be done, some holes to patch, please let me know your thoughts.

[AdinAck]

I made a quick test in godbolt and rust playground and I think the usage of core::array::from_fn does not add overhead.

[usbalbin]

let (pin, [ob_exti, ob_comp, ob_adc]) = gpiox.pxi.into_analog().observe();
// pin type is `Observed<PXi<Analog>, 3>`
// ob_* type is `ObservationToken<PXi<Analog>>`

exti.source(ob_exti).enable();
comp.source(dac_ch1, ob_comp);
adc.constrain(Sequence { .., third: ob_adc }); // very pseudo

Just trying to wrap my head around this :) Is pin here more or less the same as a frozen pin from my first proposal? So all pin.into_{x} disabled but the pin is otherwise the same as normal with pin.is_high etc available and adc readings possible, assuming the pin is in analog mode?

Or is it more locked down than that? If so, then what use do we even have for pin besides keeping track of the number of observers? Is this better than the observers themselves keeping track of how many siblings they are? Thus removing the need for the Observed it self.

let (_, ob_exti) = exti.reset_and_release();
let (_, ob_comp) = comp.reset_and_release();
let (_, ob_adc) = adc.reset_and_release();

let pin = pin.release([ob_exti, ob_comp, ob_adc]);

pin.get_level(); // we can use the pin in software again

What does "use the pin in software" mean? Was pin.is_high() disabled until now? How about software triggered adc readings, is that "software"? Or am I supposed to use ObservationToken even for software triggered adc readings?

Lots of questions... Just trying to wrap my head around what consequences something like this would result in. :)

[AdinAck]

Is pin here more or less the same as a frozen pin from my first proposal? So all pin.into_{x} disabled but the pin is otherwise the same as normal with pin.is_high etc available and adc readings possible, assuming the pin is in analog mode?

Good point! It should be accessible. Since it's of type Observed<..>, we should probably use AsRef for convenience:

impl<P, const N: usize> AsRef<P> for Observed<P, N> {
    fn as_ref(&self) -> &P {
        self.p
    }
}

This is great because methods like is_high are available and all mutating functions set_high are not available (why should they be?).

How about software triggered adc readings, is that "software"? Or am I supposed to use ObservationToken even for software triggered adc readings?

A software trigger of the ADC would not be the same as calling set_high for example on a pin. An ADC conversion only mutates the ADC peripheral state, the ADC is just observing that pin. The HAL interface for the ADC being used on certain pins should only have an ObservationToken to the pin since the measurement does not require calling any pin methods, is_high, set_high, etc.

In conclusion, if I understand your questions correctly:

• should peripherals be usable by software while observed? yes, immutably
• should HAL interfaces for peripherals which use other peripherals but don't invoke their methods have access to those methods? no, they should have an ObservationToken

It could be argued that one should be able to mutate a peripheral while it is observed, this would be useful for testing, not production. I feel that additional interface is misleading and extraneous, but I could understand the desire for its presence.

Also, I share your concern for introducing this interface, it's a rather sizable change, I agree it is important to make sure this idea is absolutely the correct decision, so I appreciate the questions! :)

[AdinAck]

You know, adding an impl for AsMut is dead easy, might as well add it as well!

[usbalbin]

[...]I agree it is important to make sure this idea is absolutely the correct decision, so I appreciate the questions! :)

Thanks a lot for your input on this :)

Also, I share your concern for introducing this interface, it's a rather sizable change,[...]

Correct me if I am wrong, but a lot (perhaps not all but still, a lot) of this should be possible to do in a backwards compatible way by still allowing the owned(not splitted) peripheral/pin in places where a Observer or Observed are expected, right?

[AdinAck]

Hm, I'm not sure! Only one way to find out, I'll test it out and see what happens :)

[AdinAck]

Ah, yes. We can implement Into<ObservationToken<Self>> for all peripherals, so old code will still work. The new HAL interfaces would accept impl Into<ObservationToken<P>> for backwards compatibility.

[usbalbin]

See #157 for an attempt at implementing that idea

[usbalbin]

Some random thoughts:

Side note, it would be quite interesting to see what could be done with a system like this for the rcc and its consumers. At the very least i suppose that would allow a safe way to change clock frequency once tokens from all consumer are collected without risk of forgetting any peripheral that might not like the clock settings to change.

However doing something like that well is likely not trivial, perhaps requiring something more fine grained? Would if the RTC is clocked by the LSE then changing other parts of the clock tree should not affect the RTC etc.

Side note to the Side note. Thera are probably a lot of interesting things that can be done to the rcc system more generally. Lots of weird hardware limitations with the different steps in the pll system having different max frequencies etc. Unless am completely misremembering I believe the max frequency for the ADC is 60MHz but only if you just use one of them, if you use more then I think just north of 50. The hrtim has a minimum input frequency of 100MHz. There are probably lots of more weird things that would be interesting to see if it would be possible to check for at compile time. Then there are different limitations depending on input voltage etc.

What if the user could use a const fn to generate a clock config that was checked at compile time, that config was then applied at runtime. Perhaps one could even have multiple configs in different constants and it would be completely safe to dynamicly change between them(assuming it would be possible to solve both things above).

[AdinAck]

Side note, it would be quite interesting to see what could be done with a system like this for the rcc and its consumers. At the very least i suppose that would allow a safe way to change clock frequency once tokens from all consumer are collected without risk of forgetting any peripheral that might not like the clock settings to change.

I have been thinking about this for many months and believe that it is the perfect opportunity to flex why Rust is so good for embedded.

However doing something like that well is likely not trivial, perhaps requiring something more fine grained?

I am working on a system that procedurally generates type-enforced peripheral access interfaces and have some examples where this works and stops the developer from forgetting to enable peripheral clocks before usage. gpio example.

I am very excited to begin reasoning about clock speeds in the type system.

What if the user could use a const fn to generate a clock config that was checked at compile time, that config was then applied at runtime.

This is something I have done in the past in stm32g0xx-hal:

const PLL_CFG: rcc::PllConfig = rcc::RawPllConfig {
    mux: rcc::PLLSrc::HSI, // 16MHz
    m: 1,                  // /1
    n: 16,                 // x16
    r: 4,                  // /4 = 64MHz
    q: Some(2),            // /2 = 128MHz
    p: None,
}
.validate();

// in RTIC::init...

let mut rcc = ctx.device.RCC.freeze(rcc::Config::pll().pll_cfg(PLL_CFG));

This is working code using my fork of stm32g0xx-hal.

[usbalbin]

g0xx-hal stuff

Nice! I will take a look at that :)

gpio example

Very nice! Great to have that level of protection all the way down

How/does this interact with svd-files or does this require every peripheral to be manually specified?

[AdinAck]

How/does this interact with svd-files or does this require every peripheral to be manually specified?

No SVD, they are always riddled with errors from the vendor, which is why we patch them so much. They are hard to read, don't encode any hardware relationships at all, just suck to use.

It is so much easier to just scroll through a datasheet and type in the register/field offsets.

Some brief examples:

#[block(
    base_addr = 0x4800_0000,
    entitlements = [super::super::rcc::ahb2enr::gpioaen::Enabled],
    auto_increment,
)]
pub mod gpioa {
    #[register(auto_increment)]
    mod moder {
        #[schema(width = 2, auto_increment)]
        mod mode {
            #[state]
            struct Input;
            #[state]
            struct Output;
            #[state]
            struct Alternate;
            #[state]
            struct Analog;
        }

        #[field_array(range = ..13, schema = mode, read, write, reset = Analog)]
        mod modeX {}

        #[field(schema = mode, read, write, reset = Alternate)]
        mod mode13 {}

        #[field(schema = mode, read, write, reset = Alternate)]
        mod mode14 {}

        #[field(schema = mode, read, write, reset = Alternate)]
        mod mode15 {}
    }

The beginning of the description for GPIOA.

#[block(
    base_addr = 0x4002_0c00,
    entitlements = [super::rcc::ahb1enr::cordicen::Enabled],
    auto_increment,
    erase_mod,
)]
mod cordic {
    #[register(auto_increment)]
    mod csr {
        #[field(width = 4, read, write, reset = Cos, auto_increment)]
        mod func {
            #[state(entitlements = [scale::N0])]
            struct Cos;

            #[state(entitlements = [scale::N0])]
            struct Sin;

            #[state(entitlements = [scale::N0])]
            struct ATan2;

            #[state(entitlements = [scale::N0])]
            struct Magnitude;

            #[state]
            struct ATan;

            #[state(entitlements = [scale::N1])]
            struct CosH;

            #[state(entitlements = [scale::N1])]
            struct SinH;

            #[state(entitlements = [scale::N1])]
            struct ATanH;

            #[state(entitlements = [scale::N1, scale::N2, scale::N3, scale::N4])]
            struct Ln;

            #[state(entitlements = [scale::N0, scale::N1, scale::N2])]
            struct Sqrt;
        }

The func field of CORDIC.

Remember all those crazy types and traits and structures that occupied 1200 lines of code to enforce the CORDIC's hardware invariances? Now they are described in 30 lines of code.

Reading the datasheet is trivial, and this macro statically validates a lot in case mistakes are made.

I'm worried my design of this system doesn't scale but I just keep implementing peripherals and adding things as needed. Hopefully eventually it will encapsulate everything.

[usbalbin]

How/does this interact with svd-files or does this require every peripheral to be manually specified?

No SVD, they are always riddled with errors from the vendor, which is why we patch them so much. They are hard to read, don't encode any hardware relationships at all, just suck to use.

Yeah, cant say I disagree

It is so much easier to just scroll through a datasheet and type in the register/field offsets.

Some brief examples:
[...]
Remember all those crazy types and traits and structures that occupied 1200 lines of code to enforce the CORDIC's hardware invariances? Now they are described in 30 lines of code.

Yes :) Wow!

Reading the datasheet is trivial, and this macro statically validates a lot in case mistakes are made.

Very nice

I'm worried my design of this system doesn't scale but I just keep implementing peripherals and adding things as needed. Hopefully eventually it will encapsulate everything.

I think this might be related to stm32-rs/meta#4 .

[AdinAck]

I think this might be related to stm32-rs/meta#4 .

Very interesting! I'll read this and see if there's anything I can contribute to the conversation, thanks!

[usbalbin]

I'm worried my design of this system doesn't scale but I just keep implementing peripherals and adding things as needed. Hopefully eventually it will encapsulate everything.

Thinking about scaling as in getting something like this to work on many families in the future. As is discussed in the linked issue, there are a lot of devices with a lot of peripherals... Do you think it would be possible and make sense to somehow get this to work with the pacs of today(lots of it which after all the patches of stm32-rs works quite good, yet without all the nice magic type safety things)? Would it even be possible to add the entitlement info and whatnot (sorry have not quite wrapped my head around it) to svd2rust's yaml format so this could be added in peripheral by peripheral while still making use of all the things we already have?

[AdinAck]

Would it even be possible to add the entitlement info and whatnot (sorry have not quite wrapped my head around it) to svd2rust's yaml format

This was my first approach before bailing and making proto-hal. It is certainly possible with a great deal of effort.

My motivation for creating proto-hal is selfish in nature. I want to use it and want it to work exactly how I say. Slowly grossly bandaiding onto the monster that is svd2rust sounds no fun! But that may very well be the best way to improve HALs in general. Just not for me.

So far implementing peripherals with the macro has been very easy and quick, so I'm not really worried about redoing work already done in other HALs/PACs, but maybe I'll run into issues soon.

[usbalbin]

One perhaps not great idea but at least low effort way would perhaps be if your pac types implemented From<CorrespondingStm32RsPacType>? Then the two pacs could be used side by side in case yours was missing a peripheral

[AdinAck]

Hm, yeah. Some adaptor between the two would be nice. I'll think on that, thanks!

[usbalbin]

This observation-thing feels like it could be useful for other hals and perhaps not only parts from stm. Would it make sense to put src/observation.rs into its own crate to reduce duplication and make it easier to make crates that work with different hals?

[AdinAck]

Would it make sense to put src/observation.rs into its own crate...

Yep! I already have it in proto-hal along with other structures I am messing around with for designing HALs in a standardized way.

It's in the stasis file. I moved on from the "observation" naming scheme because since the beginning we've known that's not the best name. For example if we consider the output level of a pin to not contribute to the state of that pin, then an ObservationToken would allow you to mutate the pin. It's a bad name. Which is why I moved on to Entitlements.

Peripherals are entitled to states. Obviously I am using this for far more complicated peripheral interface generation stuff in macros, but the rest of proto-hal is just for HALs in general and the "observation"/"entitlements" stuff is useful all the same.

I would be open to moving these structures to their own crate all by themselves if you really want. But I feel like it makes sense to keep them in proto-hal amongst the other HAL utilities.

[AdinAck]

The reason I created proto-hal in the first place was because I didn't think the maintainers of embedded-hal would be very open to changes, but if you feel otherwise, really that's the ideal place for this.

[AdinAck]

I just migrated over most of the important design features to stasis.

[usbalbin]

Would it make sense to put src/observation.rs into its own crate...

Yep! I already have it in proto-hal along with other structures I am messing around with for designing HALs in a standardized way.

It's in the stasis file. I moved on from the "observation" naming scheme because since the beginning we've known that's not the best name. For example if we consider the output level of a pin to not contribute to the state of that pin, then an ObservationToken would allow you to mutate the pin. It's a bad name. Which is why I moved on to Entitlements.

Oh my bad, sorry I did not get that Entitlements was the same thing as ObservationToken :). Makes sense

Peripherals are entitled to states. Obviously I am using this for far more complicated peripheral interface generation stuff in macros, but the rest of proto-hal is just for HALs in general and the "observation"/"entitlements" stuff is useful all the same.
Ah, ok
I would be open to moving these structures to their own crate all by themselves if you really want. But I feel like it makes sense to keep them in proto-hal amongst the other HAL utilities.

Already seems to be quite broken out, no need to break it out further

The rough gist from what I have seen so far(which is not a lot) is:

• proto-hal/proto-hal - Has some types and stuff like the entitlement system and other useful things for gpios etc.
• proto-hal/macros - Has macros to generate a safe pac that uses the entitlement things
• proto-hal/stm32/g4 - Uses the above to specify a safe pac for the G4 family, or something slightly below a hal?

Is that somewhat accurate?

Very interesting stuff :)

[AdinAck]

Is that somewhat accurate?

100% accurate!

Very interesting stuff :)

I agree! Thanks for all your help!