therealprof / display-interface

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

Add borrowed SPI bus interfaces #33

Closed Finomnis closed 1 year ago

Finomnis commented 1 year ago

Motivation

In many cases, the SPI bus is not just connected to a single device. So taking full ownership isn't always practical.

It's a lot easier to have the SPI bus managed by a different management struct, and then create a temporary display interface on demand. That way sharing an SPI bus is trivially possible:

let mut spi_manager = SPIManager::new(spi_bus, ...pins...);
let display_bus: SPIInterface = spi_manager.get_display_bus();
do_display_things(display_bus);
// Drop display_bus to re-use SPI for other things

Sadly, this doesn't work with the current API of this crate, as the bus always takes full ownership.

Solution

Finomnis commented 1 year ago

@therealprof Is this still maintained? Just checking, not meaning to stress or insult :) I'd just like to know if it's worth waiting for a response or if I should find a workaround in some other way.

almindor commented 1 year ago

I am a bit confused by the logic here. Isn't it supposed to work such that spi_bus is a singleton and then for each connected device you create a spi_device (on the hal level, before display-interface plays a role)?

Finomnis commented 1 year ago

@almindor Maybe I misunderstand how spi bus on embedded-hal is supposed to work, in that case, let me do some more research into what you are saying here.

Finomnis commented 1 year ago

@almindor I'm not sure what you mean with that; the only experience with embedded I have so far is with the nrf52840, and its PAC exposes a SPIM0 peripheral, which you can only take once. Where here is the differentiation between spi_bus and spi_device?

    let disp_spi = SPIInterface::new(
        hal::Spim::new(
            peripherals.SPIM0,
            hal::spim::Pins {
                sck: Some(disp_scl),
                mosi: Some(disp_si),
                miso: None,
            },
            hal::spim::Frequency::M8,
            hal::spim::MODE_3,
            0,
        ),
        disp_a0,
        disp_cs,
    );

If what you said was true, how would the devices synchronize? They need to make sure they don't pull up multiple CS pins at once. So I don't completely understand what you mean, I think.

I'm further not completely sure what you mean with "on the hal level" - should HAL be aware of which SPI devices are attached to the processor? If you use nrf52840-hal, it doesn't know anything about attached devices, it only known the processor itself.

therealprof commented 1 year ago

@Finomnis Yes, this is still maintained. I'm only checking GH notifications every now and then in a batched fashion since I'm getting literally thousands of notifications per week... Always hoping someone else chimes in, too. ;)

Finomnis commented 1 year ago

@almindor Oh, I just realized that there is a big jump from embedded-hal 0.2.7, which is the current stable, and 1.0.0-alpha.9. I guess I need to do more research about how it will work with embedded-hal 1.0.

Finomnis commented 1 year ago

@therealprof Thanks for the response and apologies for potential annoyance :)

almindor commented 1 year ago

@almindor Oh, I just realized that there is a big jump from embedded-hal 0.2.7, which is the current stable, and 1.0.0-alpha.9. I guess I need to do more research about how it will work with embedded-hal 1.0.

Right, so unless I misunderstand your use case, this is solved by embedded-hal (to be released) where you can get separate spi_device for each CS line on a spi_bus and since display-interface consumed the device and not the bus it should "just work" (tm)

Finomnis commented 1 year ago

@almindor Oh, I just realized that there is a big jump from embedded-hal 0.2.7, which is the current stable, and 1.0.0-alpha.9. I guess I need to do more research about how it will work with embedded-hal 1.0.

Right, so unless I misunderstand your use case, this is solved by embedded-hal (to be released) where you can get separate spi_device for each CS line on a spi_bus and since display-interface consumed the device and not the bus it should "just work" (tm)

Sounds great! Thanks for the info. This makes my change irrelevant then.