Parallel OutputBus trait

[GrantM11235]

OutputBus

This PR changes PGPIO8BitInterface to use a simple OutputBus trait instead of a bunch of individual pins:

pub trait OutputBus {
    type Word: Copy;

    fn set_value(&mut self, value: Self::Word) -> Result<(), DisplayError>;
}

Generic8BitBus

This PR also provides an OutputBus implementation that can be constructed from 8 OutputPins:

let bus = Generic8BitBus::new((p0, p1, p2, p3, p4, p5, p6, p7)).unwrap()

or

let bus = (p0, p1, p2, p3, p4, p5, p6, p7).try_into().unwrap()

(Note that the construction is fallible because it attempts to set all the pins low, which fixes #18)

Custom OutputBus

For reference, here is an example of an optimized OutputBus implementation for an stm32f1 with a bus on pins PB0 through PB7

pub struct OutputBusPins(
    PB0<Output<PushPull>>,
    PB1<Output<PushPull>>,
    PB2<Output<PushPull>>,
    PB3<Output<PushPull>>,
    PB4<Output<PushPull>>,
    PB5<Output<PushPull>>,
    PB6<Output<PushPull>>,
    PB7<Output<PushPull>>,
);

impl OutputBus for OutputBusPins {
    type Word = u8;

    fn set_value(&mut self, value: Self::Word) -> Result<(), DisplayError> {
        let bits = 0x00ff_0000 | value as u32;

        unsafe { (*hal::pac::GPIOB::ptr()).bsrr.write(|w| w.bits(bits)) }

        Ok(())
    }
}

Pros

• PGPIO8BitInterface doesn't take a million arguments anymore
• Users can create their own non-HAL implementations of OutputBus with much higher performance
• When combined with New api #19, this allows us to trivially add support for 16 bit buses
• Still works with heterogeneous pin types
• Fixes Parallel GPIO 8-bit interface does not work if the data bus pins are not zero on init #18

Cons

• Users must construct an OutputBus before they can construct a PGPIO8BitInterface. This is slightly more verbose, but it is more readable in my opinion

Todo

• Add/update documentation
• Update changelog

[therealprof]

That's a good improvement, I like it.

Nit: Your example impl of set_value doesn't match the trait definition.

NB: I have been meaning to add support for parallel busses to embedded-hal but never got to it. Maybe it's time to revive this idea?

[andresovela]

Looks like a great alternative to me, but I'll have to say that this still doesn't fully fix #18, because the user can still implement a custom OutputBus that doesn't drive all pins low when instantiated

[andresovela]

I think with these changes this would fix #18

[GrantM11235]

@therealprof

That's a good improvement, I like it.

Thanks!

Nit: Your example impl of set_value doesn't match the trait definition.

Oops, fixed

NB: I have been meaning to add support for parallel busses to embedded-hal but never got to it. Maybe it's time to revive this idea?

An embedded-hal trait would be great! I saw that there is a digital::OutputPort RFC, I plan to add some of my thoughts there.

---

@andresovela

Looks like a great alternative to me

Thanks!

but I'll have to say that this still doesn't fully fix #18, because the user can still implement a custom OutputBus that doesn't drive all pins low when instantiated

The OutputBus trait doesn't require the bus to have any specific value until the first time that set_value is called. #18 is fixed for Generic8BitBus by synchronizing the state of the pins with the value of self.last, it does this by setting self.last to all ones before calling self.set_value(0) which forces all pins to zero regardless of their previous state.

An implementation of OutputBus that unconditionally sets all pins, like my example, will never exhibit the symptoms of #18

I will be sure to make a note of all of this when I write the docs.

[andresovela]

The OutputBus trait doesn't require the bus to have any specific value until the first time that set_value is called. #18 is fixed for Generic8BitBus by synchronizing the state of the pins with the value of self.last, it does this by setting self.last to all ones before calling self.set_value(0) which forces all pins to zero regardless of their previous state.

Yeah I know, I'm not arguing that Generic8BitBus has the problem described in #18.

An implementation of OutputBus that unconditionally sets all pins, like my example, will never exhibit the symptoms of #18

This is already the case. A user implementing a PGPIO8BitInterface that unconditionally sets all pins to 0 in the beginning will never see #18.

Anyway, now that I think about it, with your implementation this is a non-issue, because if such a problem comes up it will be the user's fault and not this crate's fault, since the user has the responsibility of implementing this correctly.

[GrantM11235]

Anyway, now that I think about it, with your implementation this is a non-issue, because if such a problem comes up it will be the user's fault and not this crate's fault, since the user has the responsibility of implementing this correctly.

Exactly 😉

[therealprof]

I think this is a great PR. Is this ready to be merged?

[GrantM11235]

It is now! 👍

Let me know if I should squash anything.

[therealprof]

A squash would be nice, yes.

[therealprof]

LGTM, thanks for sticking with me.