Serial Pins design discussion

[dbrgn]

Right now the Pins trait looks like this:

pub trait Pins<USART> {
    fn setup(&self);
}

It is then implemented for pin pairs using a macro:

macro_rules! impl_pins {
    ($($instance:ty, $tx:ident, $rx:ident, $alt:ident;)*) => {
        $(
            impl<Tx, Rx> Pins<$instance> for ($tx<Tx>, $rx<Rx>) {
                fn setup(&self) {
                    self.0.set_alt_mode(AltMode::$alt);
                    self.1.set_alt_mode(AltMode::$alt);
                }
            }
        )*
    }
}

impl_pins!(
    USART1,  PA9,  PA10, AF4;
    USART1,  PB6,  PB7,  AF0;
    USART2,  PA2,  PA3,  AF4;
    USART2,  PA14, PA15, AF4;
    USART2,  PD5,  PD6,  AF0;
    // ..
);

The problem with this is, that it assumes that a single pin pair can be used for an USART peripheral. However, pins can be mixed, so I could use USART1 with PA9 and PB7.

One approach would be to generate Pins implementations for all permutations (which can go into dozens or even hundreds of impls), but that's a workaround for a design problem in my opinion: Different pins can act as Tx and as Rx for an USART independently. (You could even use multiple Tx pins for the same USART if you wanted...)

My suggestion would be to drop the Pins<USART> trait, and to add TxPin<USART> and RxPin<USART> traits instead.

Here's a quick way to play around with this idea without having to adjust the serial trait:

pub trait TxPin<USART> {
    fn setup(&self);
}

pub trait RxPin<USART> {
    fn setup(&self);
}

impl<USART, TX, RX> Pins<USART> for (TX, RX) where TX: TxPin<USART>, RX: RxPin<USART> {
    fn setup(&self) {
        self.0.setup();
        self.1.setup();
    }
}

impl<Mode> TxPin<USART1> for PA9<Mode> { fn setup(&self) { self.set_alt_mode(AltMode::AF4); } }
impl<Mode> RxPin<USART1> for PA10<Mode> { fn setup(&self) { self.set_alt_mode(AltMode::AF4); } }
impl<Mode> TxPin<USART1> for PB6<Mode> { fn setup(&self) { self.set_alt_mode(AltMode::AF0); } }
impl<Mode> RxPin<USART1> for PB7<Mode> { fn setup(&self) { self.set_alt_mode(AltMode::AF0); } }

What do you think about this? I can create a PR with a concrete implementation if you want.

If dropping the Pins directly is not an option, we could also deprecate it and implement the Serial trait for both Pins and RxPin / TxPin.

cc @rnestler

[hannobraun]

I think your suggestion sounds good. I'd personally prefer to replace the Pins trait instead of incorporating it into the new design. As far as breaking changes go, this one shouldn't have too big of an impact, and I'm more concerned about making things confusing by keeping around an unnecessary trait.

[dbrgn]

As far as breaking changes go, this one shouldn't have too big of an impact, and I'm more concerned about making things confusing by keeping around an unnecessary trait.

I agree! I'll try to come up with a good design.

Another thing that was brought up in the Matrix chat: It's currently possible that multiple Tx pins can be used for the same UART (maybe also Rx, but that might cause problems). That could break exclusive access to an UART even though the pins have been moved into the driver that uses the UART. I don't think we can do anything about that, right?

[hannobraun]

Another thing that was brought up in the Matrix chat: It's currently possible that multiple Tx pins can be used for the same UART (maybe also Rx, but that might cause problems). That could break exclusive access to an UART even though the pins have been moved into the driver that uses the UART. I don't think we can do anything about that, right?

I don't quite understand. Are you saying that using multiple TX pins would be fine on the hardware level but cause problems in the HAL? Or are you saying using multiple pins for the same functions is possible in the HAL API, but shouldn't be possible at all?

In any case, we can model all kinds of behavior, by making alternate functions their own struct. Then you would need to involve the function struct when changing the function of a pin. That way, we can move the function struct to make it unusable for another pin, or change its type state to model whatever behavior we desire.

Check out LPC8xx HAL's SWM API for reference. This code hasn't been cleaned in a while, but it demonstrates how to enforce the LPC800 series' quite complicated pin assignment rules at compile time.

[dbrgn]

Ah, my assumptions were mistaken. This is what I originally meant to write:

---

A serial driver (e.g. for a modem using AT commands) could be initialized with two USART1 pins (e.g. PA9 / PA10). Those pins are then captured by the driver using ownership, which creates a sense of safety and isolation.

But there's nothing stopping you from using PB6 (also connected to USART1) and to write to the serial device in parallel from a separate task, right?

---

...but I missed that the USART1 peripheral itself would already be captured by the driver, so it cannot be shared. That answers my question, there's no problem to be fixed here 🙂

[arkorobotics]

Do we want to close this and PR #68?

[rnestler]

Do we want to close this and PR #68?

Why would you close it? Or did you mean merge it for the PR?

[arkorobotics]

Sorry for the confusion, I meant to merge PR #68 and close this issue.

[dbrgn]

In that case the question was probably directed at @hannobraun?