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.