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

USBDevice: add documentation on USB suspend/resume to enter deep sleep #11635

Merged
merged 1 commit into from Oct 17, 2019

Conversation

@hugueskamba
Copy link
Contributor

hugueskamba commented Oct 4, 2019

Description

It is now possible to temporarily suspend a USB Component and safely preserve its state. This functionality is needed to allow a device to enter deep sleep as a USBDevice instance prevents deep sleep.

Pull request type

[] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[X] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@bulislaw @c1728p9 @evedon

Release Notes

@ciarmcom ciarmcom requested review from bulislaw, c1728p9, evedon and ARMmbed/mbed-os-maintainers Oct 4, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 4, 2019

@hugueskamba hugueskamba force-pushed the hugueskamba:hk-fix-deepsleep-usb-components branch from bf2be08 to 1e1b67d Oct 7, 2019
core_util_critical_section_enter();
if (_phy_enabled != enabled) {
if (enabled) {
USBDevice::connect();

This comment has been minimized.

Copy link
@bulislaw

bulislaw Oct 7, 2019

Member

So it seems that deinit calls disconnect and connect calls init so we should be good there. But i'm not 100% sure it will restore the state. Is calling disconnect and phy->deinit legal in every state? Basically if I call enable_phy(false), go to sleep, wake up, call enable_phy(true) and try to continue using USB will it work?

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Oct 8, 2019

Author Contributor

I programmed a K64F with a firmware that allowed me to call enable_phy() by pressing a button to enable/disable USB phy.
After enable_phy(false), nothing can be received on the USB interface and the device goes to deep sleep. After enable_phy(true), bytes can once again be received on the USB interface.
Is that what you were not sure about?

This comment has been minimized.

Copy link
@bulislaw

bulislaw Oct 8, 2019

Member

Yeah, the test you performed is a good one, but I'm not sure it's definitive for all implementation based on the specification. Could you check that it's actually mandated by the spec? Russ would be able to help I'm sure.

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Oct 8, 2019

Author Contributor

Hello @c1728p9 ,
Could you please comment?

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Oct 9, 2019

Contributor

Unlike some of the other peripherals, the USB code to disable deep sleep is target specific and in a subclass of USBPhy. There is no code common to all targets which locks deep sleep. Depending on implementation, targets could unlock deep sleep on a call to disconnect(), or even do it automatically when USB power is lost. Currently all the implementation I'm aware of only unlock deep sleep in deinit() though.

Is calling disconnect and phy->deinit legal in every state?

Yes, this is equivalent to the user unplugging the device. This may have negative side effects though, such as corrupting a mass storage drive if disconnecting in the middle of a write.

But i'm not 100% sure it will restore the state.

The call to enable_phy(true) can make the device re-enumerate, but can't directly restore the state. The USB host controls the enumeration process so it chooses when and how the device will be restored. Even if the device state is returned to what it was before the disconnect, the host PC software may not be where it left off - for example a serial port may need to be re-opended by the user.

The safest thing to do would be only deinit() when USB power is lost and init() and connect() when it is detected. That way there isn't any state to restore.

Finally, one thing to note is that suspend/resume is already terminology used by the USB standard. It referrs to a USB host initiated low power mode. This may add some confusion to users. I'm not sure of a good way to avoid this though.

This comment has been minimized.

Copy link
@bulislaw

bulislaw Oct 9, 2019

Member

If it's also not guaranteed to be clean operation in terms of state being preserved I would say suspend and resume doesn't necessarily work. Is directly calling init/deinit or connect/disconnect a viable solution? Documentation can be clear that it's up to the user to make sure all the transfers have concluded?

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Oct 10, 2019

Author Contributor

Done.

@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Oct 7, 2019

@hugueskamba Could you please update your description to give some context and put it in the right place? Thanks

@hugueskamba hugueskamba force-pushed the hugueskamba:hk-fix-deepsleep-usb-components branch from 1e1b67d to 0ee70e1 Oct 8, 2019
@hugueskamba

This comment has been minimized.

Copy link
Contributor Author

hugueskamba commented Oct 8, 2019

@bulislaw
This force-push splits the previously introduced method (USBDevice::enable_phy) into USBDevice::suspend and USBDevice::resume as they are more user friendly.
The new USBDevice::suspend method powers down the USB device hardware and the USBDevice::resume is able to power it up and resume operation.

@hugueskamba hugueskamba force-pushed the hugueskamba:hk-fix-deepsleep-usb-components branch from 0ee70e1 to 8d29adf Oct 10, 2019
@hugueskamba

This comment has been minimized.

Copy link
Contributor Author

hugueskamba commented Oct 10, 2019

This force-push removes the suspend and resume methods and instead adds documentation to deinit and connect to highlight that they can be used supend and resume USB operation in order to enter deep sleep.

@hugueskamba hugueskamba changed the title USBDevice: Add method to enable/disable phy USBDevice: add documentation on USB suspend/resume to enter deep sleep Oct 10, 2019
@hugueskamba hugueskamba force-pushed the hugueskamba:hk-fix-deepsleep-usb-components branch from 8d29adf to 52e6e30 Oct 10, 2019
@hugueskamba

This comment has been minimized.

Copy link
Contributor Author

hugueskamba commented Oct 10, 2019

This force-push removes the remnants of the removed suspend and resume methods.

It is possible to temporarily suspend USB and safely preserve its
configuration. This is needed to allow a device to enter deep
sleep as a USBDevice instance prevents deep sleep. USB operation can be
suspended with `deinit` and restored with `connect`.
@hugueskamba hugueskamba force-pushed the hugueskamba:hk-fix-deepsleep-usb-components branch from 52e6e30 to a3f20f8 Oct 11, 2019
Copy link
Member

bulislaw left a comment

LGTM

Copy link
Contributor

evedon left a comment

I am fine with this PR which adds documentation on how to achieve power saving.

On a side note, I find confusing the fact that connect() calls init() (although the init() documentation says that "This function must be called before calling any other functions of this class" and deinit() calls disconnect(). It would have been a cleaner API if each function only did one thing (at the expense of the users doing the right thing). But this is not someting I would like to change now.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 16, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 16, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 16, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit e2578db into ARMmbed:master Oct 17, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+72 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8686 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.