ryankurte / rust-driver-pal

Rust embedded SPI helper / testing package
1 stars 6 forks source link

Cannot build Wrapper object using stm32l0xx_hal #1

Open berkowski opened 5 years ago

berkowski commented 5 years ago

I'm having some trouble using Wrapper::new with stm32l0xx_hal. I suspect the underlying issue is with the hal crate but I'm not sure where to start. I'm trying to follow your example in the radio-sx127x crate in Sx127x::spi method. My crate is at https://gitlab.com/berkowski/b-l072z-lrwan1, but the problem code is below.

#![no_std]
#![no_main]

#[allow(unused_imports)]
use panic_semihosting;

use stm32l0xx_hal as hal;
use hal::prelude::*;
use hal::delay::Delay;
use hal::rcc::{Config as RccConfig, PLLSource, PLLDiv, PLLMul};
use hal::spi::Spi;

use radio_sx127x::prelude::*;
use embedded_spi::wrapper::Wrapper as SpiWrapper;

#[macro_use]
use rtfm::app;

#[app(device=stm32l0xx_hal::pac)]
const APP: () = {

    #[init]
    fn init() {

        let mut rcc = device.RCC.freeze(
            RccConfig::pll(PLLSource::HSI16, PLLMul::Mul6, PLLDiv::Div3)
        );

        let gpioa = device.GPIOA.split(&mut rcc);
        let gpiob = device.GPIOB.split(&mut rcc);
        let gpioc = device.GPIOC.split(&mut rcc);

        let sck = gpiob.pb3.into_floating_input();
        let miso = gpioa.pa6.into_floating_input();
        let mosi = gpioa.pa7.into_floating_input();

        // Build the SPI1 instance
        let spi1 = Spi::spi1(
            device.SPI1,
            (sck, miso, mosi),
            radio_sx127x::SPI_MODE,
            1u32.mhz(),
            &mut rcc
        );

        // Our CS and RESET pins
        let cs = gpioa.pa15.into_push_pull_output();
        let reset = gpioc.pc0.into_push_pull_output();

        // Build our SpiWrapper, this is where we run into problems
        let mut spi_hal = SpiWrapper::new(spi1, cs, Delay::new(core.SYST, rcc.clocks));
        spi_hal.with_reset(reset);

        // Want to eventually use it in an Sx127x instance...
        let radio_config = Config::default();
        let radio = Sx127x::new(spi_hal, &radio_config);

    }

};

Fails to compile with

   Compiling b-l072z-lrwan1 v0.1.0 
error[E0308]: mismatched types
  --> examples/ping.rs:48:28
   |
48 |         spi_hal.with_reset(reset);
   |                            ^^^^^ expected struct `stm32l0xx_hal::gpio::gpioa::PA15`, found struct `stm32l0xx_hal::gpio::gpioc::PC0`
   |
   = note: expected type `stm32l0xx_hal::gpio::gpioa::PA15<stm32l0xx_hal::gpio::Output<stm32l0xx_hal::gpio::PushPull>>`
              found type `stm32l0xx_hal::gpio::gpioc::PC0<stm32l0xx_hal::gpio::Output<stm32l0xx_hal::gpio::PushPull>>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
error: Could not compile `b-l072z-lrwan1`.

To learn more, run the command again with --verbose.

Whatever pin I change cs to be I get an error that with_reset expects that same pin, and this level of rust is a bit beyond mine to decipher. Not a great excuse, but I could use some help tracking down the culprit.

ryankurte commented 5 years ago

hey thanks for opening an issue! i'm super excited to play with this on those modules.

this appears to be a poor design decision on my part with the embedded-spi abstraction, that i definitely hadn't intended to propagate through to driver consumers. the spi wrapper defines input and output pin types and expects them to match (see here, here, and a bunch more places), so the cs pin and the reset pin not only have to fulfill the OutputPin bound, but also be the same type (and the same for InputPin bounds).

the quickest fix for you would probably be to wrap the different GPIO pins in a common base enum that implements InputPin or OutputPin for both types, so that the driver only sees one type.

the correct fix here i think is to split the type of each pin so that: pub struct Wrapper<Spi, SpiError, OutputPin, InputPin, PinError, Delay> would become: pub struct Wrapper<Spi, SpiError, CsPin, ResetPin, BusyPin, IrqPin, PinError, Delay> so each of the pins can thus be a different underlying type, or () if unavailable. (i am unsure if there is need to also split pin errors? but it is simpler if not.)

this is actually something i intended to do because it means that each of the implementations (ie. the get_busy method) could then be implemented with a bound of BusyPin: OutputPin and thus only available if the busy pin was available, rather than the panic this would currently cause if get_busy is called and Wrapper.busy is None.

i'd be happy to accept (and coach you through if it's useful) a PR making this change, otherwise i'll put it on my list and the quick fix should work for now ^_^

berkowski commented 5 years ago

I'll have a go at working on a PR; It'd be the best way for me to learn about the guts of this stuff anyway. I've got a decent amount of experience with embedded C so it's mostly just battling the rust approach to it, which is the whole point I'm doing this!

I ran into the same underlying issue modeling my BSP by following the f3 example. The LED's on that board are all off the same port, where mine are on two. Good to see the bundling in an enum approach I ended up with for now isn't a complete hack :)