Missing impls for Pin<_, DynFunction, _>

[jannic]

There are currently not many useful functions implemented for Pin<_, DynFunction, _>. Therefore, pins configured that way can not be used without converting them to a pin with a statically defined function first. That defeats the idea of having a DynFunction in the first place.

One could, for example, implement OutputPin like this:

    #[derive(Debug)]
    pub enum DynFunctionError {
        InvalidFunction,
    }

    impl embedded_hal::digital::Error for DynFunctionError {
        fn kind(&self) -> ErrorKind {
            ErrorKind::Other
        }
    }

    impl<I, P> ErrorType for Pin<I, DynFunction, P>
    where
        I: PinId,
        P: PullType,
    {
        type Error = DynFunctionError;
    }

    impl<I, P> OutputPin for Pin<I, DynFunction, P>
    where
        I: PinId,
        P: PullType,
    {
        fn set_low(&mut self) -> Result<(), Self::Error> {
            if self.function() != DynFunction::Sio(super::DynSioConfig::Output) {
                return Err(DynFunctionError::InvalidFunction);
            }
            self._set_low();
            Ok(())
        }

        fn set_high(&mut self) -> Result<(), Self::Error> {
            if self.function() != DynFunction::Sio(super::DynSioConfig::Output) {
                return Err(DynFunctionError::InvalidFunction);
            }
            self._set_high();
            Ok(())
        }
    }

It may be useful to first describe use cases for DynFunction where it's not feasible to just use a statically defined function.

[jannic]

(I saved some experimental code to https://github.com/jannic/rp-hal/commits/issue-756/)

[vvuk]

Any more progress on this? As it stands I'm not quite sure what the proper way is of implementing something like bitbanged SWD (or bitbanged I2C or anything with a bidirectional pin) without maybe (haven't thought it through) doing a bunch of unsafe things? The issue-756 branch above does work, and I see that's what things like probe-rs use to implement SWD. Happy to provide help in moving this forward as well!

[jannic]

I didn't work on it because nobody came up with an actual use case. If it helps you to implement SWD I can update the branch and make it a merge request.

What do you think about the implementation in the issue-756 branch? Is the API like you want it to be, or would you prefer something different?

[vvuk]

Sounds good -- let me play with it a bit to get a feel for it. At first look though I'm not a fan of having the function-check branch on every get/set.

One thought I had was to have the API work more like borrowing -- where you could "borrow" a specific-functioned pin from a DynFunction pin. The borrow operation would set the pin's function, and the drop would make it available for borrowing with a possibly-different function again. I can try to implement this if that sounds interesting.

[vvuk]

I'm thinking something like this -- untested yet, and needs separate impls for when you do have a non-dynamic pull type (or probably not necessary, you probably want a dynamic pull type if you want a dynamic function). But the idea is to use the borrow checker to allow borrowing the pin as a specific typed pin, relying on mem::transmute (or maybe there's a better way to do this?) to provide the right type annotations after configuration:

impl<I: PinId> Pin<I, DynFunction, DynPullType> {
    pub fn borrow_reconfigured<F2, P2>(&mut self) -> &mut Pin<I, F2, P2>
    where
        F2: func::Function,
        P2: PullType,
        I: func::ValidFunction<F2>,
    {
        unsafe {
            self.unsafe_set_function::<F2>();
            self.unsafe_set_pull_type::<P2>();
            mem::transmute(self)
        }
    }

    unsafe fn unsafe_set_function<F2: func::Function>(&mut self) {
        use func_sealed::Function;

        let prev_function = self.function.as_dyn();
        let function = F2::from(prev_function);
        let new_function = function.as_dyn();

        if prev_function != new_function {
            pin::set_function(&self.id, new_function);
            self.function = new_function;
        }
    }

    unsafe fn unsafe_set_pull_type<P2: PullType>(&mut self) {
        use pull_sealed::PullType;

        let prev_pull = self.pull_type.as_dyn();
        let pull = P2::from(prev_pull);
        let new_pull = pull.as_dyn();

        if prev_pull != new_pull {
            pin::set_pull_type(&self.id, new_pull);
            self.pull_type = new_pull;
        }
    }
}

[jannic]

Hmm, this looks quite complicated. Is it really worth the effort and the unsafe code?

I made a few experiments, and it looks like rustc is able to optimize the repeated function-check branch on every get/set away. At least in easy cases like calling the same function multiple times in a row.

But then, I agree that your approach can potentially provide a better API to the user. And having some unsafe code inside the HAL might be a sensible tradeoff if it really simplifies application code.

[vvuk]

Yeah, the code can be even simplified, I don't think I need the separate explicit functions.. it just seemed cleaner. But I'm still working on making it work. Will report!

[vvuk]

Ok, here's a take at this: main...vvuk:rp-hal:dyn-pins

There's one serious but I think fixable issue -- I wanted to point it out first in case there are better ideas. Because Pin is defined as

pub struct Pin<I: PinId, F: func::Function, P: PullType> {
    id: I,
    function: F,
    pull_type: P,
}

the different F and P have different sizes -- DynFunction is an enum, whereas FunctionSpi is a 0-sized empty struct. (struct FunctionSpi(pub(super) ())). This means that when try_borrow_as (could use a better name -- try_configure_as?) munges the ref into different types, it's a ref to invalid data -- the struct with fully concrete types is going to be smaller than one with any dynamic types.

One fix would be to define the concrete functions to have a field of the dynamic enum, i.e.:

pub struct FunctionSpi(pub(super) DynFunction);

which would give it the same size as DynFunction. But that means the Pin struct will always have a size, which means there's no way to have pins be completely memory-free. Even a fully statically configured will still take up 3 bytes.

This is annoying, because I really like the ergonomic API (see the gpio example). But I don't think it's acceptable to suddenly require passing around a ref to useless data to use fully statically-defined pins.

So the idea... always have try_borrow_as return a fully non-dynamic ZST Pin, conjured out of a &mut (). This would mean however that you wouldn't be able to borrow something dynamically as output, and then twiddle its pull-ups and pull-dows; you'd have to re-borrow. I think that's an ok-tradeoff?

[vvuk]

Actually, this won't work. This approach or even the original approach wouldn't let you dynamically reconfigure a pin as a Uart pin and use it with a UartPeripheral, for example.

Whether for SWD or bitbanged I2C or whatever, what we're really talking about is using a pin for both input and output SIO. So let's just implement FunctionSioInputOutput?

That struct itself could store some state, and allow you to e.g. configure the state of pullups differently for when it's in input or output mode, and give you an easy way to toggle between the two modes.

I think DynFunction can then stay as just a marker, waiting for you to into_function. In fact I think try_set_function is useless; the only thing you should be able to do with a DynFunction pin is to convert it into a concrete function type -- and back to DynFunction if necessary.

[ithinuel]

Bitbanged I2C already has a type in the hal (InOutPin).

The more I think about it, the more it seems to me that a newtype relying on the overrides would be much simpler and cleaner than trying to shoehorn a solution with unsafe :S

[vvuk]

Bitbanged I2C already has a type in the hal (InOutPin).

Hm do you mean IoPin, in 0.2/unproven? IoPin seems like it could be a cross-platform wrapper around into_function that preserves the IO-ness, so maybe that's the right thing?

But I think in order to implement that, we need a DynInputPin and DynOutputPin function (so that we can implement IoPin for both of those, and InputPin and OutputPin for the individual ones). It can't be DynFunction. At that point, given that it's still in 0.2 and unproven, maybe just not worth it at all, and just use into_function (given that you have to consume the pins into something new anyway)...

[ithinuel]

I mean https://docs.rs/rp2040-hal/latest/rp2040_hal/gpio/struct.InOutPin.html It differs from what you need for SWD in that it switches between driven low and "floating".