Skip to content

Conversation

@andresovela
Copy link
Contributor

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
Copy link
Contributor Author

Any feedback on this one? Concerns?

@therealprof
Copy link
Owner

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

@GrantM11235
Copy link
Contributor

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
Copy link
Contributor Author

Yeah, it does require downgrading pins.

@GrantM11235
Copy link
Contributor

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

@therealprof
Copy link
Owner

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Owner

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)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants