Takes care of power for nRF USB devices #810

huntc · 2022-06-15T02:18:30Z

Modifies the usb-serial example to illustrate how to setup USB for situations where the USB power can be detected and removed.

Gaps:

* No support for the nrf-softdevices as yet, although this should be possible via another constructor.

No support for the nrf5340, although this should be possible via USBREG.

The change is tested and appears to work. Some notes:

There's an existing field named self_powered as a UsbDevice field. It doesn't ever appear to get set. I'm wondering if this field is intended to signal that a device has the nRF VBUS power situation or not. I'm not presently using it.
The new PowerDetected event is generated on the bus initially in situations where just new is used i.e. without power management, including on STM. We can therefore rely on this event always being generated.

Old description:

EnabledUsbDevice is a wrapper around the UsbDevice where its enablement is also subject to external events, such as POWER events for nRF. It is introduced generically to support other platforms should they also require external signaling for enablement.

Dirbaio · 2022-06-15T16:35:35Z

As discussed on Matrix, I think this should be handled by the nrf-specific USB driver, instead of a wrapper on top of the UsbDevice. Power management is a hardware-specific thing.

alexmoon · 2022-06-15T17:51:16Z

As discussed on Matrix, I think this should be handled by the nrf-specific USB driver, instead of a wrapper on top of the UsbDevice. Power management is a hardware-specific thing.

I don't think there's any way to do that that works with both bare metal and nrf-softdevice. nrf-softdevice hijacks the POWER peripheral and gives you SoC events with the usb status changes. I think it may be problematic in other scenarios for the USB driver to require the POWER peripheral as well.

Dirbaio · 2022-06-15T17:55:38Z

It's still a driver concern though. Maybe we can make the driver "customizable" so it works on all scenarios, perhaps making it generic over a "PowerManager" trait? It'd have

"assume always on" noop impl if your device is usb powered
"use POWER" impl for when you don't use the softdevice
User supplies custom impl when using the softdevice (or the nrf-softdevice crate could have it)

huntc · 2022-06-15T19:25:28Z

Right, so different constructors for each trait then… it would be nice to have the interrupt taken care of.

My only outstanding concern though is overloading the suspend/resume events. It feels like we should have power enabled/disabled also, even if they are handled the same way as suspend/resume, accepting also that they aren’t supported by every device. Wdyt?

alexmoon · 2022-06-15T19:34:46Z

I'm not sure I agree it's a driver concern.

I think that most devices don't drive their USB transceiver by Vbus the way nrf does, so this is an nrf-specific concern.
I don't think that even in the bare metal case you're going to want the USB driver to take over the POWER peripheral exclusively.
The app is going to need to respond to connected/disconnected events. If the driver is handling those, they'll need a way to bubble up to the app, which either means another back channel from the nrf USB driver to the app or building connect/disconnect events all the way through the usb stack which could be confusing for devices that don't have that concept.

Honestly, to me it makes the most sense to just leave it up to the app to handle. It's only a couple of lines of code and it gives the app the opportunity to do whatever else it needs to when the events occur.

huntc · 2022-06-15T19:38:07Z

Oh, just remembered. Won’t we have to create a PowerPeripheral also… and consuming the POWER_CLOCK interrupt is a concern too in terms of sharing that around…

Dirbaio · 2022-06-15T20:29:58Z

I think that most devices don't drive their USB transceiver by Vbus the way nrf does, so this is an nrf-specific concern.

Hence it makes sense to handle it in embassy-nrf, not add a EnabledUsbDevice in embassy-usb that will only be useful for nrf.

The way I see it, the Driver's job is "take care of the hardware and all its quirks, then make it into something that impls the Device trait which Just Works". The fact that you have to deinit USBD if VBUS goes away is one of such hardware quirks. Forcing higher layers (or worse, user code) to be aware of that is not great.

The app is going to need to respond to connected/disconnected events. If the driver is handling those, they'll need a way to bubble up to the app, which either means another back channel from the nrf USB driver to the app or building connect/disconnect events all the way through the usb stack which could be confusing for devices that don't have that concept.

On the contrary, having "usb cable connected/disconnected" events in the UsbDevice public API would be a great feature IMO! This way you can do everything top of UsbDevice, which will make your code portable across nrf, stm32, etc with zero changes required.

nRF detects it through POWER. STM32 doesn't have an equivalent, but most boards (like nucleos) wire VBUS to a GPIO so you can still detect it. STM32 Driver impls could optionally take a GPIO to do that.

Dirbaio · 2022-06-15T20:31:40Z

Sharing POWER is a non-issue.

Currently nothing else takes POWER so it's not a problem now.
In the future, if something does, we can easily "split" the POWER peripehral like we do with GPIO, GPIOTE, PPI...

huntc · 2022-06-15T21:13:17Z

Ok, I’ll give the device approach another look.

huntc · 2022-06-15T21:24:57Z

Wdyt about a PowerPeripheral being introduced along with an Instance type? Something to convey the pac type (which doesn’t exist right now)…

Dirbaio · 2022-06-15T21:30:44Z

I don't think an Instance trait is warranted, the chip only has one POWER. You could add a singleton to chips/*.rs, but I don't think it's required either, we can assume owning USBD means you own the USB part of POWER. You'd have to take just the POWER Irq in this case.

About the PAC owned singleton, just steal it.

alexmoon · 2022-06-16T15:53:13Z

nRF detects it through POWER. STM32 doesn't have an equivalent, but most boards (like nucleos) wire VBUS to a GPIO so you can still detect it. STM32 Driver impls could optionally take a GPIO to do that.

Thanks, I understand your vision much better now. That makes sense to me: we have the STM32 driver take an InputPin and the nrf driver take a PowerManager trait.

huntc · 2022-06-19T08:19:22Z

I've now re-implemented the machinery to enable/disable the USBD given the respective power events. However, upon removing power, I'm not seeing the USB become disabled, or at least conveying events in this regard. I'm waking up BUS_WAKER and EP0_WAKER within the power interrupt, and I'm seeing that the power event is handled. For example, with the USB initially connected all appears well as I'm guarding against a double-enablement in the USB library. However, when I disconnect the USB:

7.649566 TRACE usb: reset
└─ embassy_usb::{impl#2}::handle_bus_event::{async_fn#0} @ /Users/huntc/Projects/hacking/embassy/embassy-usb/src/fmt.rs:112
7.649932 TRACE Power event: removed
└─ embassy_nrf::usb::{impl#2}::poll::{closure#0} @ /Users/huntc/Projects/hacking/embassy/embassy-nrf/src/fmt.rs:112
7.650238 TRACE usb: power removed
└─ embassy_usb::{impl#2}::handle_bus_event::{async_fn#0} @ /Users/huntc/Projects/hacking/embassy/embassy-usb/src/fmt.rs:112
7.650634 INFO  Disconnected
└─ usb_serial::____embassy_main_task::{async_fn#0}::{async_block#1} @ src/bin/usb_serial.rs:80

... I'd expect to see trace messages reporting that USB has been suspended. But I don't... Clearly, there's something about the way things are supposed to work that I don't understand.

huntc · 2022-06-28T05:53:31Z

Tested and appears to work. Some notes:

There's an existing field named self_powered as a UsbDevice field. It doesn't ever appear to get set. I'm wondering if this field is intended to signal that a device has the nRF VBUS power situation or not. I'm not presently using it.
The new PowerDetected event is generated on the bus initially in situations where just new is used i.e. without power management, including on STM. We can therefore rely on this event always being generated.

EnabledUsbDevice is a wrapper around the UsbDevice where their enablement is also subject to external events, such as POWER events for nRF. It is introduced generically to support other platforms should they also require external signalling for enablement.

huntc · 2022-07-07T07:52:56Z

Tested from my production-focused code and also appears to behave well. Applying the API from here felt good.

alexmoon

I only looked at the embassy-nrf and embassy-usb version since I haven't worked with the stm32 implementation at all.

alexmoon · 2022-06-15T11:39:31Z

embassy-usb/src/util.rs

+                Either::Second(enable) => {
+                    if !enable {
+                        self.underlying.disable().await;
+                        while !self.enable_usb_signal.wait().await {}


There's an inconsistency here depending on whether or not the device is suspended when a disable event occurs. I'd recommend introducing a let mut enabled = false variable outside the loop, then at the start of the loop do:

if !enabled { while !self.enable_usb_signal.wait().await {} enabled = true; }

Then your select cases just need to update the enabled variable and continue. That will ensure the usb device always starts resumed.

Sorry if I've caused some confusion here by leaving in that first commit. The first commit was per the original approach that packaged a (your?) example and created a util module. That module has gone now and the commit was left there for posterity only.

alexmoon · 2022-07-07T18:14:22Z

embassy-nrf/src/usb.rs

+    /// Establish a new device that then uses the POWER peripheral to
+    /// detect USB power detected/removed events are handled.
+    #[cfg(not(feature = "_nrf5340-app"))]
+    pub fn with_power_management(


If we're incorporating power management into embassy-usb, then this functionality probably needs to be part of the general new method. To handle things like the nrf-softdevice situation, instead of taking the power_irq directly, it should take a generic type which implements a UsbPowerDetect trait. That trait can be implemented on a struct which takes an impl Unborrow<Target = POWER> for users that can make use of the POWER peripheral directly. Other users can implement their own version of the trait.

I think overloading new adds complexity to situations where power detection isn't required e.g. STM, or where it is to be manually managed on nRF as per the existing scenario. Having with_power_management present and the device driver handling the IRQ setup is highly convenient and distinct from a user's perspective.

For the nrf-softdevice situation, I'd really like to see how we should handle that. My suspicion is that we would end up with a with_power_signal or something. As I'm personally not using nrf-softdevice though, I'm reluctant to cater for it right now as I don't have anything set up to test it. Meanwhile, the user can also rely on the existing new method when programming the softdevice, as per today.

This is the nrf driver's new we're talking about so stm32 doesn't apply. What I'm saying is that with_power_management should replace the nrf driver's new method. However, instead of taking the irq directly, it should take a trait to cover the following cases:

The POWER peripheral and/or irq need to be shared with other parts of the application

nrf_softdevice users who don't have access to the POWER peripheral

My understanding of what @Dirbaio was looking for is to incorporate the Powered state of the USB device state spec into embassy-usb itself. Once we do that it becomes a requirement of the drivers to be able to provide those state transitions to the library. There may be cases where the driver never generates those transitions (e.g. an stm32 device with no IO pin wired to detect Vbus) but the assumption should be that the capability is there.

I'm still not convinced about the naming/organization of the driver constructor beyond what I've got. It feels ok to me. Perhaps we can have another PR improving on that to what you and/or Dario are thinking about? Thanks.

Actually, I saw that I had a bug re. the initial vbus state, so I've fixed that and also provided a means to set power state per softdevice usage .Please see commit 81796d2

embassy-usb/src/lib.rs

alexmoon · 2022-07-07T18:23:16Z

embassy-usb/src/lib.rs

@@ -114,6 +114,7 @@ struct Inner<'d, D: Driver<'d>> {

    device_state: UsbDeviceState,
    suspended: bool,
+    power_available: bool,


This is a bit bike shedding, but the USB spec refers to this state as powered which might be a better name than power_available. In fact, rather than a separate bool, I'd add a Unpowered state to the UsbDeviceState enum. We will also need a fn powered(&self, powered: bool) method on the DeviceStateHandler.

I have a separate bool because it represents the fact that power is signalled independently of UsbDeviceState.

It's not possible for a USB device to be unpowered and suspended simultaneously. Once Vbus is interrupted the device needs to be reset at the very least, and usually you'll want to disable the device until Vbus is restored. Chapter 9 of the USB 2.0 spec has a nice diagram of the device state machine that might be helpful.

Again, the signal comes from a separate source, hence my modeling it this way. Apologies if I'm misunderstanding something though. I'm not very familiar with the internals of USB.

No worries, it's a complicated spec. I guess my main point is that we want the embassy-usb library to model an "ideal" USB device as closely as possible. The signals do come from separate sources, but that's an implementation detail. What we want to model is a device state that goes Unpowered -> Default -> Addressed -> Configured (<-> Suspended). In practice, that means the power removed event trumps everything else. This stuff is tricky to get right.

Here's a summary of how I think it should work:

Unpowered should be another UsbDeviceState variant

run_until_suspend() should only return when the UsbDeviceState is Suspended. Other state changes should be handled automatically without returning.

wait_for_resume() should return as soon as the UsbDeviceState is anything other than Suspended

A power removed event should immediately set the device state to Unpowered and it should remain there until a power detected event is received

The reason for the run_until_suspend() and wait_for_resume() behaviors is because the only reason those methods are split out from run() is because the run() future is not safe to drop yet we need to be able to call remote_wakeup() (which requires an exclusive reference to the UsbDevice) when the device is suspended. Therefore the behavior of those methods should be strictly tied to the Suspended device state.

I've now implemented your recommendations regarding the new Unpowered state. Thanks so much - it looks and behaves great! Please see commit 8d71a35.

huntc · 2022-07-07T22:49:57Z

I only looked at the embassy-nrf and embassy-usb version since I haven't worked with the stm32 implementation at all.

Thanks for the feedback!

Replaces the sub-state of representing being being available. Power states also now set� enable/disable directly too, which simplifies code.

Also, correctly sets the initial power management state when using power management

alexmoon

This looks much better to me. One area that I think still needs some work is the difference between Disabled and Unpowered. I think we need both states to allow applications to disable the USB device without dropping the whole thing even if USB is still connected.

I view the Disabled state as something the app manually enters when it wants to disable the USB device regardless of the state of the bus. The Disabled state is exited by calling one of the run*() methods. The Unpowered state is simply part of the normal lifecycle of the device, handled automatically by the driver with no input from the application.

Therefore, I think the device should still start in the Disabled state rather than Unpowered. run_until_suspend() should still check for the Disabled state and call bus.enable() if it is set, though it should now transition to the Unpowered state rather than Default.

We probably also need some documentation in driver.rs that poll should return PowerDetected on the first call after enable is called if power is present. And maybe that after a PowerRemoved event there should be no other events until a PowerDetected event has been returned.

Finally, since disable() is something called manually by the app, I don't think it needs to call the DeviceStateHandler::enabled() callback. So we could rename that callback to powered() to allow the app to track the powered state of the bus.

Let me know what you think of these changes. I think the embassy-usb side of things starting to look pretty solid. I do think some more work is still needed on the embassy-nrf side to handle the POWER peripheral issues I've mentioned before. Perhaps you could look into another constructor that would take a Signal to provide the power state and how that could integrate with the poll function?

alexmoon · 2022-07-08T14:57:52Z

embassy-usb/src/lib.rs

-            if let Some(h) = &self.inner.handler {
-                h.enabled(true);
-            }
-        }


Keep this, but transition to Unpowered instead of Default

alexmoon · 2022-07-08T15:00:01Z

embassy-usb/src/lib.rs

@@ -376,6 +370,24 @@ impl<'d, D: Driver<'d>> Inner<'d, D> {
                    h.suspended(true);
                }
            }
+            Event::PowerDetected => {
+                trace!("usb: power detected");
+                self.bus.enable().await;


The driver should do this itself inside the poll function

My understanding is that this is no longer an issue given the agreement on Matrix that we'll use my approach.

alexmoon · 2022-07-08T15:00:03Z

embassy-usb/src/lib.rs

+            }
+            Event::PowerRemoved => {
+                trace!("usb: power removed");
+                self.bus.disable().await;


Same as above

My understanding is that this is no longer an issue given the agreement on Matrix that we'll use my approach.

huntc · 2022-07-09T02:25:59Z

I view the Disabled state as something the app manually enters when it wants to disable the USB device regardless of the state of the bus. The Disabled state is exited by calling one of the run*() methods. The Unpowered state is simply part of the normal lifecycle of the device, handled automatically by the driver with no input from the application.

A driver can also be enabled/disabled from the outside, no?

Therefore, I think the device should still start in the Disabled state rather than Unpowered. run_until_suspend() should still check for the Disabled state and call bus.enable() if it is set, though it should now transition to the Unpowered state rather than Default.

The nRF documentation does state that we should not enable the USB until the USBDETECTED event is received. I think then, starting in the Unpowered state, receiving a PowerDetected event, and then enabling the USB appears to be the right sequence of events to enforce the requirement.

Reference: "Enable USBD only after VBUS has been detected"

Finally, since disable() is something called manually by the app, I don't think it needs to call the DeviceStateHandler::enabled() callback. So we could rename that callback to powered() to allow the app to track the powered state of the bus.

Right now, the enabled callback takes a bool indicating whether or not it is enabled. Can we deal with any changes there in another PR?

Let me know what you think of these changes. I think the embassy-usb side of things starting to look pretty solid. I do think some more work is still needed on the embassy-nrf side to handle the POWER peripheral issues I've mentioned before. Perhaps you could look into another constructor that would take a Signal to provide the power state and how that could integrate with the poll function?

Thanks again for the feedback.

Regarding the signal, just wondering if you noticed the new with_power_state constructor and power method. I was thinking about the nRF softdevice scenario with these. They leverage an internal signal. Please let me know what you think.

huntc · 2022-07-09T06:44:00Z

Hey @alexmoon - I've now added commit 8785fbc which introduces a trait in the spirit of what you had - albeit with slightly different trait methods. I also used new as the constructor method in place of take to be consistent with how I see we normally pass in IRQs. Easily changed again though.

The change also removes a signal I had, resulting in a little more simplicity.

I also introduced a SignalledSupply for use with the softdevice.

Your fix for where we weren't waiting for the power to become available is also incorporated.

Eliminated a signal by using a simpler trait method that returns whether VBus power is available. Also includes a UsbSupply that can be signalled for use with the nRF softdevice. Includes the requirement for waiting for power to become available.

Dirbaio

Thank you @huntc and @alexmoon , this is great work! :)

it's exciting to see embassy-usb maturing so fast :O

bors r+

huntc · 2022-07-11T00:02:06Z

@alexmoon I believe I've addressed everything. I've tested thoroughly on the nRF52840 with the examples and with my production code. All appears well. Will be merging as at the very least, the API looks good now.

huntc · 2022-07-11T00:03:53Z

bors r+

bors · 2022-07-11T00:03:55Z

Already running a review

bors · 2022-07-11T00:17:08Z

Build succeeded:

all

huntc requested a review from Dirbaio June 15, 2022 02:18

huntc force-pushed the enabled-usb-device branch 2 times, most recently from 822b4b3 to ba5f129 Compare June 16, 2022 06:38

huntc changed the title ~~Introduces EnabledUsbDevice~~ Takes care of power for nRF USB devices Jun 16, 2022

huntc force-pushed the enabled-usb-device branch 2 times, most recently from 42efc50 to 7cec893 Compare June 19, 2022 07:27

huntc marked this pull request as draft June 19, 2022 07:29

huntc force-pushed the enabled-usb-device branch from 7cec893 to ee0e398 Compare June 19, 2022 08:04

huntc force-pushed the enabled-usb-device branch 2 times, most recently from 0dee050 to cae640c Compare June 19, 2022 08:44

huntc force-pushed the enabled-usb-device branch 2 times, most recently from 6fa9a82 to 7f9c751 Compare June 28, 2022 05:52

huntc marked this pull request as ready for review June 28, 2022 05:53

huntc added 2 commits July 7, 2022 10:08

Introduces EnabledUsbDevice

c46e9b6

EnabledUsbDevice is a wrapper around the UsbDevice where their enablement is also subject to external events, such as POWER events for nRF. It is introduced generically to support other platforms should they also require external signalling for enablement.

Puts in the machinery to handle power detected/removed

4a8f117

huntc force-pushed the enabled-usb-device branch from 7f9c751 to 4a8f117 Compare July 7, 2022 00:09

alexmoon reviewed Jul 7, 2022

View reviewed changes

huntc added 2 commits July 8, 2022 15:30

Build in a new Unpowered state

8d71a35

Replaces the sub-state of representing being being available. Power states also now set� enable/disable directly too, which simplifies code.

New constructor to cater for the softdevice

81796d2

Also, correctly sets the initial power management state when using power management

alexmoon reviewed Jul 8, 2022

View reviewed changes

huntc force-pushed the enabled-usb-device branch from 2c87692 to 23a9a65 Compare July 9, 2022 07:49

Trait for UsbSupply

8785fbc

Eliminated a signal by using a simpler trait method that returns whether VBus power is available. Also includes a UsbSupply that can be signalled for use with the nRF softdevice. Includes the requirement for waiting for power to become available.

huntc force-pushed the enabled-usb-device branch from 23a9a65 to 8785fbc Compare July 9, 2022 07:57

Dirbaio approved these changes Jul 11, 2022

View reviewed changes

bors bot merged commit 9753f76 into embassy-rs:master Jul 11, 2022

huntc deleted the enabled-usb-device branch July 11, 2022 01:25

pperanich mentioned this pull request Nov 13, 2024

SoftwareVBusDetect for USB peripheral alexmoon/nrf-sdc#36

Open

Takes care of power for nRF USB devices #810

Takes care of power for nRF USB devices #810

Conversation

huntc commented Jun 15, 2022 • edited Loading

Dirbaio commented Jun 15, 2022

alexmoon commented Jun 15, 2022

Dirbaio commented Jun 15, 2022

huntc commented Jun 15, 2022 • edited Loading

alexmoon commented Jun 15, 2022

huntc commented Jun 15, 2022

Dirbaio commented Jun 15, 2022

Dirbaio commented Jun 15, 2022

huntc commented Jun 15, 2022

huntc commented Jun 15, 2022

Dirbaio commented Jun 15, 2022

alexmoon commented Jun 16, 2022

huntc commented Jun 19, 2022 • edited Loading

huntc commented Jun 28, 2022

huntc commented Jul 7, 2022

alexmoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntc Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntc Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntc commented Jul 7, 2022

alexmoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntc commented Jul 9, 2022

huntc commented Jul 9, 2022 • edited Loading

Dirbaio left a comment

Choose a reason for hiding this comment

huntc commented Jul 11, 2022

huntc commented Jul 11, 2022

bors bot commented Jul 11, 2022

bors bot commented Jul 11, 2022

huntc commented Jun 15, 2022 •

edited

Loading

huntc commented Jun 15, 2022 •

edited

Loading

huntc commented Jun 19, 2022 •

edited

Loading

huntc Jul 7, 2022 •

edited

Loading

huntc Jul 7, 2022 •

edited

Loading

huntc commented Jul 9, 2022 •

edited

Loading