therealprof / display-interface

Rust crates providing a generic interface for display drivers and some default implementations (GPIO, SPI and I2C)
Apache License 2.0
76 stars 27 forks source link

Simplify 8-bit display interface #16

Closed andresovela closed 3 years ago

andresovela commented 3 years ago

I simplified the API for the parallel GPIO interface by grouping the bus into an array of pins rather than 8 individual pins. This allows to further simplify the set_value() function.

andresovela commented 3 years ago

Any feedback on this one? Concerns?

therealprof commented 3 years ago

Sorry, didn't get to testing it yet. No concerns from just reading the code.

GrantM11235 commented 3 years ago

This only works if all of the data pins are the same type, which is not always the case. For example, in my application p0 is stm32f1xx_hal::gpio::gpiob::PB0<Output<PushPull>> and p1 is stm32f1xx_hal::gpio::gpiob::PB1<Output<PushPull>> etc.

With stm32f1xx_hal you can work around this by "downgrading" pins to a single type, but I am not sure if all HALs support that.

andresovela commented 3 years ago

Yeah, it does require downgrading pins.

GrantM11235 commented 3 years ago

20 solves this problem in a different way, feel free to check it out and let me know what you think

therealprof commented 3 years ago

@andresovela I have to agree with @GrantM11235 here that having to downgrade pins is not ideal. The main problem that I see is that using downgraded pins usually cause some additional overhead due to the dynamic calculation of the right addresses and bits and that is not ideal for this usecase.

andresovela commented 3 years ago

Alright. Closing this PR in favor of #20 then. May I ask about the "additional overhead" though? I was not aware of that. What dynamic calculation of addresses and bits?

GrantM11235 commented 3 years ago

Here is a simplified example of how set_high() could be implemented based on this example

impl PA0 {
    fn set_high(&mut self) {
        port_a_registers.set_output_bits(0b00000001);
    }
}

impl Pin {
    fn set_high(&mut self) {
        let registers = match self.port {
            Port::A => port_a_registers,
            Port::B => port_b_registers,
            Port::C => port_c_registers,
            Port::D => port_d_registers,
        };

        let bits = 1 << self.pin;

        registers.set_output_bits(bits);
    }
}

To be honest, I don't actually think that performance is a big concern here. With inlining and LTO, PA0.set_high(); and Pin { port: Port::A, pin: 0 }.set_high(); will probably compile down to the exact same thing

therealprof commented 3 years ago

May I ask about the "additional overhead" though? I was not aware of that. What dynamic calculation of addresses and bits?

Usual pins are zero-sized types, the implementation of which get monomorphized into pretty much ideal code. OTOH downgraded pins are by concept unified structures containing registerblock adresses and pin offsets so a generic implementation can be used.

To be honest, I don't actually think that performance is a big concern here. With inlining and LTO, PA0.set_high(); and Pin { port: Port::A, pin: 0 }.set_high(); will probably compile down to the exact same thing

Well, it is theoretically possible that it compiles into the comparable code but it's somewhat unlikely due to dependence on a number of factors (implementation, optimization flags and what not)...