Organizing usbd_device code without tripping borrow checker

Embedded
1

I am developing firmware for a stm32h7 device with usb, and I want to make a struct for the usb-related stuff (with a read/write function exposed typically). My first attempt looked like this (there is a UsbDeviceBuilder involved as well, but it does the same as SerialPort so removed for simplicity):

use synopsys_usb_otg::UsbPeripheral;
use usb_device::prelude::*;
use usbd_serial::SerialPort;

static mut USB_MEMORY: [u32; 1024] = [0u32; 1024];

pub struct Usb<'a, USB: UsbPeripheral> {
    usb_bus: UsbBusAllocator<UsbBus<USB>>,
    usb_serial: SerialPort<'a, UsbBus<USB>>,
}

impl <USB: UsbPeripheral> Usb<'_, USB>  {
    pub fn new(usb: USB) -> Self {
        let usb_bus = UsbBus::new(usb, unsafe { &mut USB_MEMORY });
        let usb_serial = usbd_serial::SerialPort::new(&usb_bus);

        Usb {
            usb_bus,
            usb_serial,
        }
    }
}

The problem is that SerialPort::new stores the reference to usb_bus, and since this is moved when returning from the function, the compiler chokes:

     pub fn new(usb: USB) -> Self {
   |                               ---- return type is Usb<'1, USB>
17 |           let usb_bus = UsbBus::new(usb, unsafe { &mut USB_MEMORY });
18 |           let usb_serial = usbd_serial::SerialPort::new(&usb_bus);
   |                                                         -------- borrow of `usb_bus` occurs here
19 |
20 | /         Usb {
21 | |             usb_bus,
   | |             ^^^^^^^ move out of `usb_bus` occurs here
22 | |             usb_serial,
23 | |         }
   | |_________- returning this value requires that `usb_bus` is borrowed for `'1`

Usb is instantiated from my Board struct (where I do Peripherals::take and setup everything), so I was thinking about sending in the usb_bus from there, but even that won't help since it would again be moved when board is instantiated from main.

fn main() {
    let board = Board::new();
}
impl Board {
    pub fn new() {
        let p = Peripherals::take();
        let usb_bus = UsbBus::new(p.USB1, ...);
        let usb = Usb::new(&usb_bus);
        Board {
            usb // <- Moved here as well
        }
    }
}

And since it takes the usb-peripheral as input, I also don't want to instantiate usbbus on main, since I would have to take Peripherals there instead of in board.

How can I organize this in a proper way?

2

Safe Rust doesn't allow self-referencing types. The proper way to do this is to instantiate the owning and the referring values separately.

3

Also I am pretty sure your Board::new() function can not be done that way. You cannot call Peripheral::take() in a function that returns, because it will be dropped at the end of the function.

4

Right. Being somewhat new to Rust, could this be considered a poor design choice in the library since it prevents encapsulation? The alternative would be to take usb_bus as a parameter to the methods of the type (e.g. embedded-hal spi follows this pattern if I remember correctly).

5

Interesting. I instantiate all the gpio, spi, sdram etc that I use, and store references to it in Board type if I need access to it later. Yes, Peripheral is dropped at the end, but since I've moved out everything I need, I don't think it matters?

This seemed to be the natural way to do it, as the board can take care of all the complexity in setting everything up, and provide a simplified interface. I can also then swap out the Board with a different one by just changing one line in main.

It seems to work perfectly fine for me, but if there are problems with this approach that I am not aware of, it would be great if you could shed some light on that?

6

I'm not sure what "this" refers to in the above sentence. Storing a reference to another value is certainly not, in itself, a design error.

7

I misunderstood the problem with you Board type. If I understand you correctly, your approach normally works, but fails in this case.

If that's the case then the reason is that you can't return something that is referenced, because this would invalidate the reference. In your case usb_serial references usb_bus so you can't return usb_bus, but can't not return usb_bus either, because then it would be dropped and usb_serial's reference would be invalidated, too.

I'm no export, so there might be a better solution, but the only solution I can come up with is to move usb_bus somewhere where it's lifetime is long enough. The easiest way would be to put it in a Box if you are working with alloc. It's probably also possible with other container types like RefCell in a global static variable.

8

Yes, it can be, and I've seen it several times. People design their libraries around simple obvious programs that do

fn main() {
    let a = A::new();
    let b = B::new(&a);
    loop { // or not even a loop
        b.do_something();
    }
}

and don't notice that this makes it hard to create abstractions or to participate in callback systems.

(Though note that it is possible to solve this problem; an async block Future can contain self-references, so if you can frame the environment around it as either exchanging messages with an async task, or resembling an async executor itself, then you can take advantage of that. A thread can also be used, but is probably undesirable here.)

The alternative would be to take usb_bus as a parameter to the methods of the type

The important thing is not using a long-lived borrow and there are two ways to do that I'd highlight:

  1. Take the necessary borrow as a parameter in the individual operations, as you mention. However, this offers weaker guarantees (to the SerialPort, or B in my example above) because the thing borrowed could be different on each call (either a different object, or simply mutated).
  2. Make the B type generic where it is currently fixed to &A, and allow it to either contain a reference or to own the referent. This is the strategy used in practice by many standard library traits and adapters, like the Read, Write, and Iterator traits, which are implemented for both references and owned values; thus any adapter accepting <W: Write> can either borrow or own its target.
9

Thanks for all your responses! I marked kpreids answer as the solution - it was very elaborate and useful (even if it didn't strictly speaking "solve" my problem)!

Alloc is not desirable. A global static variable would be ok I think(?), but I'm not sure how to instantiate that? I looked at lazy_static crate, but don't think that supports input parameters(?).

Unless there is an easy and good way to make it global static, I think I will:

  • Put UsbBusAllocator in Board (which makes sense since it is tied to a peripheral which depends on board hardware)
  • Make a separate usb abstraction that holds serialport and usbdevicebuilder that is instantiated after board from main and references board.usb_bus.
10

What kind of parameters are you thinking of?

11

once_cell::sync::OnceCell is suitable for something you initialize once and then want to borrow (immutably). But it may be better to avoid static variables if you can find an alternative, as you wrote below.

12

once_cell require std I think?

I just noticed that stm32h7xx-hal/examples/usb_phy_serial_interrupt.rs uses a static Option like so:

pub static mut USB_BUS_ALLOCATOR: Option<UsbBusAllocator<UsbBus<USB1_ULPI>>> 
    = None;

unsafe fn main() -> ! {
    <..>
    USB_BUS_ALLOCATOR = Some(UsbBus::new(usb, &mut USB_MEMORY));
    let usb_serial =
        usbd_serial::SerialPort::new(USB_BUS_ALLOCATOR.as_ref().unwrap());
  <..>
}

But then, they have wrapped all of main in unsafe... :sweat_smile: So yeah. I guess that might be the most straightforward solution to all of this, but not using static is probably better.

13 
impl<USB: UsbPeripheral> UsbBus<USB> {
    /// Constructs a new USB peripheral driver.
    pub fn new(peripheral: USB, ep_memory: &'static mut [u32]) 
            -> UsbBusAllocator<Self> {

It takes a static mut [u32] and a peripheral which would typically be something like this:

   let usb = USB1::new(
        dp.OTG1_HS_GLOBAL,
        dp.OTG1_HS_DEVICE,
        dp.OTG1_HS_PWRCLK,
        gpioa.pa11.into_alternate(),
        gpioa.pa12.into_alternate(),
        ccdr.peripheral.USB1OTG,
        &ccdr.clocks,
    );
    let usb_bus = UsbBus::new(usb, unsafe { &mut USB_MEMORY });
14

I think once_cell would be a pretty good fit in this case. That way it can be done without any unsafe code and only very minor changes to your current code: Playground

How do you use your Board struct in the rest of the application? You obviously don't put it in a global static, but passing it around can also be quite difficult in a Rust application, especially if mutable references are required.

15

Ah, missed your response. Thanks for the code example! Unfortunately, once_cell requires std and is not compatible with no_std. lazy_static is no_std, and it might be possible to achieve the same with that (haven't tried).

I instantiate board from main and pass it into anything that requires it. Board owns everything and doesn't contain mutable references. The usb library with its references might complicate matters though, I will find out once I start putting more pieces together.

16

once_cell works with no_std like this:

cortex-m = { version = "0.7.7", features = ["critical-section-single-core"] }
once_cell = { version = "1.17.1", default-features = false, features = ["critical-section"] }
17

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.