-
Notifications
You must be signed in to change notification settings - Fork 37
feat(uac_host): Add global suspend/resume #211
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
base: master
Are you sure you want to change the base?
Conversation
bc154af to
947a07a
Compare
d6a83d2 to
1c8eb77
Compare
| case USB_HOST_CLIENT_EVENT_DEV_SUSPENDED: | ||
| ESP_LOGD(TAG, "Device suspended"); | ||
| _uac_host_device_suspended(event->dev_suspend_resume.dev_hdl); | ||
| break; | ||
| case USB_HOST_CLIENT_EVENT_DEV_RESUMED: | ||
| ESP_LOGD(TAG, "Device resumed"); | ||
| _uac_host_device_resumed(event->dev_suspend_resume.dev_hdl); | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @leeebo in this MR I am adding support for global suspend/resume of the root port.
In the current uac driver, there are public API functions like:
/**
* @brief Resume a UAC stream with same stream configuration
*/
esp_err_t uac_host_device_resume(uac_host_device_handle_t uac_dev_handle);
and
/**
* @brief Suspend a UAC stream
*
* @param[in] uac_dev_handle UAC device handle
*/
esp_err_t uac_host_device_suspend(uac_host_device_handle_t uac_dev_handle);
And other static functions like uac_host_interface_suspend(), uac_host_interface_resume().
Which are suspending and resuming the UAC stream, not the device itself in terms of power consumption lowering.
My concern here is, that users might get confused with naming of those functions. And might mistakenly call uac_host_device_suspend() with intention to put a UAC device into a suspended state, to lower it's power consumption. But the function only suspends a UAC stream.
Do you thinks we should consider renaming the functions to something like uac_host_device_suspend_stream() and uac_host_device_resume_stream() ? Can you PTAL and advise how to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think rename to _suspend_stream() and _resume_stream() is a good choice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leeebo renaming the public api would cause a breaking change for the UAC component.
Should should we consider bumping a Major version when releasing this feature ?
leeebo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peter-marcisovsky The implements LGTM! Thanks
Description
This MR adds Suspend/Resume events for UAC class driver as a follow-up for the Global/Suspend resume MR in esp-idf
Changes
Added Suspend and Resume UAC Host driver events
As the global Suspend issued by the USB Host Halts and flushes all EPs of all connected devices. The UAC Host driver must submit the in transfer(s) upon Resuming the root port
Related
Testing
TODO add tests
Checklist
Before submitting a Pull Request, please ensure the following: