SPI/I2C keeps ref to FtInner, impossible to pass around once initialized

[fbeutel]

As implemented currently, the SPI / I2C / OutputPin structs all keep references to Mutex<RefCell<FtInner<Device>>>.

This makes it difficult / impossible to have FtHal inside a Box or RefCell or otherwise pass it around once SPI has been initialized.
(It is possible, but then each time one would need to access the SPI functions of FtHal, this would require re-initializing the SPI object with Spi::new, which is inefficient as it sends the MPSSE command each time).

Would it be possible to create a shallow Spi::new (and accordingly for the other structs) which basically becomes a no-op if SPI has already been initialized? Or are there other ideas how one could solve this?

My use case is that I need to share FtHal between two devices (e.g. using a shared pointer), each of which live on the Heap and need to have access to the SPI port...

[newAM]

Thanks for opening an issue! Sharing a SPI bus is definitely a use-case that I want to make sure works. I'm a bit busy at work this week, but I should be able to poke around more this weekend.

[newAM]

If it is easy to do so are you able to provide a minimal example of the code that fails to compile?

[geomatsi]

IIRC sharing buses between several devices can be tricky. I used the following two crates that allow to share bus peripherals between multiple devices:

• https://github.com/Rahix/shared-bus
• https://github.com/ryan-summers/shared-bus-rtic

The second crate is not suitable for using with ftdi-embedded-hal since it is tailored for bare-metal environment and RTIC framework. The first crate has certain limitations specific to SPI. But it looks like it can provide a solution in the case of sharing within a single task/thread in the enviroment with std support.

@fbeutel Could you please take a look at shared_bus and let us know if SpiProxy can help with your use-cases.

Regards,
Sergey

[fbeutel]

Thanks for the replies!

My use case is that we have a somewhat complex system consisting of multiple SPI buses (could be multiple FTDI devices or different SPI interfaces). I'd like to be able to configure the assignment of devices and SPI buses dynamically (via config file, etc.). Since we want to support different SPI buses on the same system, we store them in a Box<dyn SPIBus> (inside a Rc<RefCell<...>>), which allows to keep them in a common list (Vec<Box<dyn SPIBus>> using trait objects).

In essence, I'd like to have something like

fn init_spi_device(config_file: &Path) -> Box<dyn SPIBus> {
    // .. load device depending on configuration file, for example:
    let device = ftdi::find_by_vid_pid(0x0403, 0x6010)
        .interface(ftdi::Interface::A)
        .open()
        .unwrap();

    let hal = hal::FtHal::init_freq(device, 3_000_000).unwrap();
    let spi = hal.spi().unwrap();

    Box::new(spi)
}

(complete file: https://gist.github.com/fbeutel/c8d5c974e9ed63ecfc81ef323849009c)

But the last line obviously fails as spi keeps a reference to hal and therefore we cannot move it around on its own. (Even together with hal doesn't work, as self-referential structs are not possible).

In principle this could be overcome by using Arc<Mutex<...>> or something similar instead of a reference inside the SPI struct, but I understand that for most use cases you want to avoid the overhead and keep using plain references.
My question therefore is if there could be a design which is somehow generic over both use cases...

@geomatsi Thanks for the pointer to shared_bus - I've previously looked there, but I believe that the underlying mechanism of SpiProxy works similarly and has the same problem (shared_bus::BusManagerSimple).

[newAM]

I made a PR at #19, will that fix this issue @fbeutel ?

In principle this could be overcome by using Arc<Mutex<...>> or something similar instead of a reference inside the SPI struct, but I understand that for most use cases you want to avoid the overhead and keep using plain references.

We are already using a Mutex, and Atomics are very lightweight, I don't anticipate any problems with this solution.

[fbeutel]

Thanks for the efforts, and sorry for the late reply.

As implemented in #19 it doesn't quite fix the issue yet, because you still store references to Arc<Mutex<...>>. This prevents passing an FtHal struct and an SPI struct around when they don't have static lifetimes.

The fix would simply be to store an Arc directly instead of a reference to an Arc. I implemented it here (fbeutel@43c4a59), and if you like I could create a pull request.
This comes at the cost of one additional clone when creating the SPI struct, but I think that's tolerable, as it is only done once.

(One outstanding issue is that I cannot directly implement this solution for the SpiDevice used with the embedded_hal 1.0 interface, as the lifetime annotations are needed for the SpiDeviceBus struct. I think this should be solvable, however, once the minimal implementation of GATs is stabilized in rust...)

[newAM]

(One outstanding issue is that I cannot directly implement this solution for the SpiDevice used with the embedded_hal 1.0 interface, as the lifetime annotations are needed for the SpiDeviceBus struct. I think this should be solvable, however, once the minimal implementation of rust-lang/rust#44265 is stabilized in rust...)

You're right about the SpiDeviceBus being problematic. I tried it out with nightly + GATs and still couldn't make it work 🤔. Commit 8864256.

As implemented in #19 it doesn't quite fix the issue yet, because you still store references to Arc<Mutex<...>>. This prevents passing an FtHal struct and an SPI struct around when they don't have static lifetimes.

Flipping the problem around: is it possible to pass this around in your code when GATs are stabilized so that you can use a generic lifetime instead of 'static?

[KoviRobi]

I'm running in to the problem where I have something simple like

// src/test1.rs
use ftdi_embedded_hal as hal;
use libftd2xx::Ft4232h;
use std::sync::Arc;

pub fn open<'a>() -> Result<hal::SpiDevice<'a, Ft4232h>, Box<dyn std::error::Error>> {
    let device = libftd2xx::Ft4232h::with_description("Dual RS232-HS A")?;
    let hal = Arc::new(hal::FtHal::init_freq(device, 3_000_000)?);
    let spi_dev = hal.spi_device(3)?;
    Ok(spi_dev)
}

error[E0515]: cannot return value referencing local variable `hal`
  --> src/test1.rs:11:5
   |
10 |     let spi_dev = hal.spi_device(3)?;
   |                   ----------------- `hal` is borrowed here
11 |     Ok(spi_dev)
   |     ^^^^^^^^^^^ returns a value referencing data owned by the current function

The problem is that spi takes a reference to hal, and I can't figure out how to make that reference live long enough so that Rust is happy. If I understand correctly it is related to this issue? Or is there a way for me to do what I want? I feel like the Arc should be a way of keeping the hal alive while the SPI is in use?

If I also return hal, it complains about it being borrowed -- I feel like an indirection via Box should suffice?

// src/test2.rs
use ftdi_embedded_hal as hal;
use libftd2xx::Ft4232h;

pub fn open<'a>(
) -> Result<(Box<hal::FtHal<Ft4232h>>, hal::SpiDevice<'a, Ft4232h>), Box<dyn std::error::Error>> {
    let device = libftd2xx::Ft4232h::with_description("Dual RS232-HS A")?;
    let hal = Box::new(hal::FtHal::init_freq(device, 3_000_000)?);
    let spi_dev = hal.spi_device(3)?;
    Ok((hal, spi_dev))
}

error[E0515]: cannot return value referencing local data `*hal`
 --> src/test2.rs:9:5
  |
8 |     let spi_dev = hal.spi_device(3)?;
  |                   ----------------- `*hal` is borrowed here
9 |     Ok((hal, spi_dev))
  |     ^^^^^^^^^^^^^^^^^^ returns a value referencing data owned by the current function

error[E0505]: cannot move out of `hal` because it is borrowed
 --> src/test2.rs:9:9
  |
4 | pub fn open<'a>(
  |             -- lifetime `'a` defined here
...
8 |     let spi_dev = hal.spi_device(3)?;
  |                   ----------------- borrow of `*hal` occurs here
9 |     Ok((hal, spi_dev))
  |     ----^^^-----------
  |     |   |
  |     |   move out of `hal` occurs here
  |     returning this value requires that `*hal` is borrowed for `'a`

(Sorry if I'm missing something obvious, I'm still fairly new to Rust)

[newAM]

(Sorry if I'm missing something obvious, I'm still fairly new to Rust)

You're not missing anything, there is a serious usability issue here. Thanks for commenting on this issue again, I forgot about this one!

I have a PR that may fix this here: #23

I have not tested that - just quickly hacked it up on my lunch break. It compiles so it must work, right? 😆
I'll poke at it with real hardware this weekend, but feel free to give it a go and let me know if it helps with this issue.