Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Device builder should consume the bus allocator #105

Open
parasyte opened this issue Sep 27, 2022 · 1 comment
Open

Device builder should consume the bus allocator #105

parasyte opened this issue Sep 27, 2022 · 1 comment

Comments

@parasyte
Copy link

parasyte commented Sep 27, 2022

First a little context. I wasted a lot of time diagnosing why I couldn't create a usbd_serial::SerialPort. The reason is because I was creating the SerialPort after the UsbDevice. The device builder will freeze the UsbBusAllocator, disallowing any future uses. This causes SerialPort::new() to panic. I don't have a debugger setup for my target at the moment, so runtime panics are hard to debug!

What do I expect to happen? The following code should not compile:

let mut usb_dev = UsbDeviceBuilder::new(&usb_bus, UsbVidPid(0x16c0, 0x27dd))
    .product("Serial port")
    .device_class(USB_CLASS_CDC)
    .build();

// ERROR: `usb_bus` is frozen by the call to `build()`.
let mut serial = SerialPort::new(&usb_bus);

An easy way to prevent this UX issue is for UsbDeviceBuilder::new() to take ownership of the UsbBusAllocator:

// Notice `usb_bus` is moved here.
let mut usb_dev = UsbDeviceBuilder::new(usb_bus, UsbVidPid(0x16c0, 0x27dd))
    .product("Serial port")
    .device_class(USB_CLASS_CDC)
    .build();

// Compile error: attempt to access moved value `usb_bus`.
let mut serial = SerialPort::new(&usb_bus);

Perhaps there are valid use cases for the device builder to borrow the bus allocator. "Flexibility" is often cited in similar situations, and it always leads to extra complexity such as interior mutability and the kind of invalid states and runtime panics shown here.

I'm also aware this is a breaking change and could potentially end up being a great deal of work. I believe that it will provide a less error-prone interface and may even afford you a way to clean up some of the library internals. For these reasons, I think it is worth considering.

@parasyte
Copy link
Author

I am thinking this suggestion won't work because the allocator needs to outlive the SerialPort.

The suggestions in #9 are tangentially related. The borrowing means that these types cannot easily be moved to a struct because it would create self-references.

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

No branches or pull requests

1 participant