Draft: alternate ManagedCS trait

[ryankurte]

Based on work by @GrantM11235 as an alternative to #245.

There appears to be a choice between a magic / automatic impl that will deal with CS assertion itself, but conflicts with &mut T impls, and a more manual option where users will call .with_cs(|p| { ... }) to perform operations which seems rather less complex.

(Note that for ManagedCsAlt SpiWithCs must implement Transactional etc. which it does not yet in this PR, and &mut T impls are disabled to ManagedCs can compile)

Co-authored-by: Grant Miller GrantM11235@gmail.com

[rust-highfive]

r? @therealprof

(rust-highfive has picked a reviewer for you, use r? to override)

[rust-highfive]

⚠️ Warning ⚠️

• Pull requests are usually filed against the master branch for this repo, but this one is against feature/spi-cs-v2. Please double check that you specified the right target!

[eldruin]

Nice work! Looks pretty interesting and simple to use.
I think it is not too bad to omit the default implementations. Also in light of the downstream problems caused by these elsewhere.
I see the "unnecessary" boilerplate increase, though.
Is the inner() method in ManagedCs necessary? It seems like a method that should rather be part of SpiWithCs.
One thing I did not understand is why ManagedCsAlt requires Transactional (but not ManagedCs?). Is this just not part of this PR yet? I do not see any related trait bounds.

[Dirbaio]

• In ManagedCsAlt, since the "inner" and "outer" types are the same, it's possible to do "nested" CS assertions, which is a bit strange:

spi.with_cs(|d| {
    spi.with_cs(|d| {
        let _ = d.write(&[0xaa, 0xbb, 0xcc]);
        Ok(())
    });
    Ok(())
});

• Users will want to return their own values/errors from the closure, so the closure should be FnMut() -> R and with_cs should return Result<R, Self::Error>. This can cause nested Results though, which is a bit annoying.

• The closures only work for blocking code, we'd need an equivalent async version. The previous ManagedCs marker trait worked seamlessly for both async+blocking, which is something I thought was neat.
  There are no "async closures" in Rust but they can be emulated with a closure returning a future. The resulting generics are incredibly horribly ugly. They seem to work for SpiWithCs but I'm not even sure they'd allow for all possible things impls would want to do...

/// blah
pub trait ManagedCs: ErrorType {
    /// Inner SPI type
    type Inner: ErrorType;

    /// Future returned by the `with_cs` method.
    type WithCsFuture<'a, R, F, Fut>: Future<Output = Result<R, Self::Error>> + 'a
    where
        Self: 'a,
        R: 'a,
        F: FnMut(&'a mut Self::Inner) -> Fut + 'a,
        Fut: Future<Output = R> + 'a;

    /// Execute the provided closure within a CS assertion
    fn with_cs<'a, R, F, Fut>(&'a mut self, f: F) -> Self::WithCsFuture<'a, R, F, Fut>
    where
        F: FnMut(&'a mut Self::Inner) -> Fut + 'a,
        Fut: Future<Output = R> + 'a;

    /// Reference the inner SPI object without manipulating CS
    fn inner(&mut self) -> &mut Self::Inner;
}

/// [`ManagedCs`] implementation for [`SpiWithCs`] wrapper.
/// Provides `with_cs` function that asserts and deasserts CS
impl<Spi, Pin> ManagedCs for SpiWithCs<Spi, Pin>
where
    Spi: ErrorType,
    Pin: OutputPin + DigitalErrorType,
{
    type Inner = Spi;

    type WithCsFuture<'a, R, F, Fut>
    where
        Self: 'a,
        R: 'a,
        F: FnMut(&'a mut Self::Inner) -> Fut + 'a,
        Fut: Future<Output = R> + 'a,
    = impl Future<Output = Result<R, Self::Error>> + 'a;

    /// Execute the provided closure within a CS assertion
    fn with_cs<'a, R, F, Fut>(&'a mut self, mut f: F) -> Self::WithCsFuture<'a, R, F, Fut>
    where
        F: FnMut(&'a mut Self::Inner) -> Fut + 'a,
        Fut: Future<Output = R> + 'a,
    {
        async move {
            self.cs.set_low().map_err(SpiWithCsError::Pin)?;

            let r = f(&mut self.spi).await;

            self.cs.set_high().map_err(SpiWithCsError::Pin)?;

            Ok(r)
        }
    }

    /// Reference the inner SPI object without manipulating CS
    fn inner(&mut self) -> &mut Self::Inner {
        &mut self.spi
    }
}

[ryankurte]

I think it is not too bad to omit the default implementations. Also in light of the downstream problems caused by these elsewhere.

are we talking eliding the default ManagedCs defaults (so cs is automatic for existing methods) or the &mut T defaults? i feel like it should be possible to allow both to exist somehow, but, i can't work out the trait magic to allow it.

In ManagedCsAlt, since the "inner" and "outer" types are the same, it's possible to do "nested" CS assertions, which is a bit strange

yep, definitely strange. i think the second has to be a noop, because if you call with_cs then a wrapped method you could call it > once. on balance it does result in simpler traits / bounds though, which i think is a point in favour.

Users will want to return their own values/errors from the closure, so the closure should be FnMut() -> R and with_cs should return Result<R, Self::Error>. This can cause nested Results though, which is a bit annoying.

yep agreed, i just hadn't decided how to bound the error yet. updated now.

The closures only work for blocking code, we'd need an equivalent async version. The previous ManagedCs marker trait worked seamlessly for both async+blocking, which is something I thought was neat.

yep, the advantage here is being able to have user code within CS though eh. i think there's no harm in having separate ManagedCs traits for blocking and async impls though, and one would expect this to become less terrible when we do have async traits.

[Dirbaio]

These should be FnOnce, so it can consume captured stuff.

[ryankurte]

Closing in favour of #351