search

stm32-rs / synopsys-usb-otg

usb-device implementation for Synopsys USB OTG IP cores
MIT License
41 stars 29 forks source link

Implement force_reset() #16

Closed mattico closed 3 years ago

mattico commented 3 years ago

I needed force_reset() for my USB device to work if USB is connected before initialization completes. (Probably due to my PHY not resetting its registers).

Disasm commented 3 years ago

Sorry for the delay. We definitely need this, but maybe we need a different approach for delays. What do you think about passing a DelayMs to these methods?

mattico commented 3 years ago

Sure. I had thought that DelayMs was from embedded-hal and didn't want to unilaterally require that, but it's actually from cortex-m.

What do we do for riscv?

Disasm commented 3 years ago

DelayMs is a trait which can be implemented on Cortex-M, RISC-V and rich OS environment. I don't see how this cannot work on RISC-V.

mattico commented 3 years ago

Nevermind, DelayMs is from embedded-hal, not sure what I was looking at before. I'll use that.

mattico commented 3 years ago

Where would the DelayMs come from?

Modify the force_reset method of the UsbBus trait to take a DelayMs? That seems like exposing implementation details in the interface. What about hardware which doesn't need a delay implementation? Presumably it exists unless the method was added to the trait and never implemented.

The DelayMs impls I've seen in HALs tend to own a hardware peripheral, we wouldn't want to force all UsbPeripherals to always own that peripheral even if the user never calls force_reset. HALs could create a special impl of DelayMs that doesn't own any hardware just for the UsbPeripheral, which is essentially the delay() method. I'm not sure that's an improved interface, maybe it is.

Am I missing something?

Disasm commented 3 years ago

We shouldn't own the DelayMs object, we can borrow it instead (pass a reference to force_reset as an argument).

mattico commented 3 years ago

Okay, I'll go modify usb-device when I have time.

Disasm commented 3 years ago

I believe force_reset was kind of deprecated, so you can move this method directly into UsbBus. It makes sense to make it a device-specific method because in some cases platform-dependent code should be executed (for example, pulling a specific GPIO pin to deactivate a pullup on D+): https://github.com/stm32-rs/stm32-usbd/blob/ec92dddc453ad63338df18478a643eb231b5c6ae/src/bus.rs#L58-L61

mattico commented 3 years ago

Okay that's much better.