Pin D10 / PB2 unavailable for PWM Output while SPI enabled

[Fundevoge]

I am trying to rewrite my Arduino C project using this crate and found out that it is impossible to use D10/PB2 as a PWM output while SPI is in use:

fn main(){
    let dp = arduino_hal::Peripherals::take().unwrap();
    let pins = arduino_hal::pins!(dp);

    let timer_1_pwm = Timer1Pwm::new(dp.TC1, Prescaler::Direct);
    let d10 = pins.d10.into_output().into_pwm(&timer_1_pwm);

    let (spi, d10_pwm) = Spi::new(
        dp.SPI,
        pins.d13.into_output(),
        pins.d11.into_output(),
        pins.d12.into_pull_up_input(),
        d10,
        Settings::default(),
    );
}

According to The Documentation, PB2 should be in Output Mode while the SPI is in use, otherwise the chip could be pushed into SPI Slave Mode (I only found out about this because of Spi::new requires PB2 as an output, which is very good to know, imagine trying to find that bug).
The Documentation also says:
If SS is configured as an output, the pin is a general output pin which does not affect the SPI system. Typically, the pin will be driving the SS pin of the SPI Slave.
In my case, I need PB2 as a PwmOutput and use a different Pin as ChipSelect, but the current code requires PB2 to be configured as a regular Output Pin.

Maybe a fix could be to make a Spi::new constructor where cs is a PwmOutput, something like this:

pub fn new<TC>(
        p: SPI,
        sclk: port::Pin<port::mode::Output, SCLKPIN>,
        mosi: port::Pin<port::mode::Output, MOSIPIN>,
        miso: port::Pin<port::mode::Input<port::mode::PullUp>, MISOPIN>,
        cs: port::Pin<port::mode::PwmOutput<TC>, CSPIN>,
        settings: Settings,
    ) -> (Self, ChipSelectPinPwm<CSPIN, TC>) {
        let mut spi = Self {
            p,
            sclk,
            mosi,
            miso: miso.forget_imode(),
            write_in_progress: false,
            _cs: PhantomData,
            _h: PhantomData,
        };
        spi.p.raw_setup(&settings);
        (spi, ChipSelectPinPwm(cs))
    }

And then add the matching traits to a new ChipSelectPinPwm struct, similar to ChipSelectPin

[Rahix]

First of all, sorry for the long silence...

Your usecase is one we hadn't considered previously, but it is of course a valid one. I think the easiest solution is the one I just proposed in #529. For your usecase, it would look something like this:

fn main(){
    let dp = arduino_hal::Peripherals::take().unwrap();
    let pins = arduino_hal::pins!(dp);

    let (spi, cs) = Spi::new(
        dp.SPI,
        pins.d13.into_output(),
        pins.d11.into_output(),
        pins.d12.into_pull_up_input(),
        pins.d10.into_output(),
        Settings::default(),
    );

    let timer_1_pwm = Timer1Pwm::new(dp.TC1, Prescaler::Direct);
    // SAFETY: D10 is only used as a PWM output which means it will not get
    // converted into input mode.
    let d10 = unsafe { cs.into_pin_unchecked() }.into_pwm(&timer_1_pwm);
}

What do you think?

[Fundevoge]

As you said, this is probably the simplest and most maintainable way to implement this usecase.
I'm not a fan of having to use unsafe for something that could be done without it, though. Also, I'm not sure if there is any other usecase for which getting the underlying Pin<> would be required. If there isn't one, I would rather spell this one out such that the safety guarantees are enforced by the compiler instead of by the user reading the documentation.

These two changes inside spi.rs would be required (this is the solution I have been using in the meantime).

// Inside impl<> Spi<>

    /// Instantiate a SPI with the registers, SCLK/MOSI/MISO/CS pins, and settings,
    /// with the internal pull-up enabled on the MISO pin.
    ///
    /// The pins are not actually used directly, but they are moved into the struct in
    /// order to enforce that they are in the correct mode, and cannot be used by anyone
    /// else while SPI is active.  CS is placed into a `ChipSelectPinPwm` instance and given
    /// back so that its output state can be changed as needed.
    pub fn new_with_pwm<TC>(
        p: SPI,
        sclk: port::Pin<port::mode::Output, SCLKPIN>,
        mosi: port::Pin<port::mode::Output, MOSIPIN>,
        miso: port::Pin<port::mode::Input<port::mode::PullUp>, MISOPIN>,
        cs: port::Pin<port::mode::PwmOutput<TC>, CSPIN>,
        settings: Settings,
    ) -> (Self, ChipSelectPinPwm<CSPIN, TC>) {
        let mut spi = Self {
            p,
            sclk,
            mosi,
            miso: miso.forget_imode(),
            write_in_progress: false,
            _cs: PhantomData,
            _h: PhantomData,
        };
        spi.p.raw_setup(&settings);
        (spi, ChipSelectPinPwm(cs))
    }

/// Wrapper for the CS pin in PWM Mode
///
/// Used to contain the chip-select pin during operation to prevent its mode from being
/// changed from Output. This is necessary because the SPI state machine would otherwise
/// reset itself to SPI slave mode immediately. This wrapper can be used just like a
/// pwm output pin, because it implements all the same traits.
pub struct ChipSelectPinPwm<CSPIN, TC>(port::Pin<port::mode::PwmOutput<TC>, CSPIN>);

impl<TC, CSPIN: crate::simple_pwm::PwmPinOps<TC>> ChipSelectPinPwm<CSPIN, TC> {
    pub fn enable(&mut self) {
        self.0.pin.enable();
    }

    pub fn disable(&mut self) {
        self.0.pin.disable();
    }

    pub fn get_duty(&self) -> <CSPIN as crate::simple_pwm::PwmPinOps<TC>>::Duty {
        self.0.pin.get_duty()
    }

    pub fn get_max_duty(&self) -> <CSPIN as crate::simple_pwm::PwmPinOps<TC>>::Duty {
        self.0.pin.get_max_duty()
    }

    pub fn set_duty(&mut self, duty: u8) {
        self.0.pin.set_duty(duty);
    }
}