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

da1469x: Fix no VBUS startup #1019

Merged
merged 1 commit into from
Aug 13, 2021

Conversation

kasjer
Copy link
Collaborator

@kasjer kasjer commented Aug 12, 2021

Describe the PR
For self powered device if device started without VBUS present
it would not be correctly attached to USB bus even if tusb_vbus_changed()
was later called.

This modifies dcd_init() so it starts USB state machine without checking
if VBUS is present or not, like all others drivers do.
tusb_vbus_changed() function is also removed its content was moved to dcd_init.

Additional context
VBUS connect/disconnect handling similar to what is done in NRF5x maybe
done in the future.

For self powered device if device started without VBUS present
it would not be correctly attached to USB bus even if tusb_vbus_changed()
was later called.

This modifies dcd_init() so it starts USB state machine without checking
if VBUS is present or not, like all others drivers do.
tusb_vbus_changed() function is also removed its content was moved to dcd_init.
@kasjer kasjer requested a review from hathach August 12, 2021 07:21
@hathach
Copy link
Owner

hathach commented Aug 13, 2021

I am totally OK with changes, though would it increase the power usage since USB PHY is now enabled even without attaching to a host. I am not familiar with DA1469x enough to tell, though on nRF5x (which is also BLE mcu), which has its own off the standard API for managing power event ( vbus detect, ready, removed)
https://github.com/hathach/tinyusb/blob/master/src/portable/nordic/nrf5x/dcd_nrf5x.c#L839

It will only enabled USB peripheral and highspeed clock when vbus is stable (ready) and go black out when vbus is disabled. These event works even when powering from battery (testing with adafruit feather nrf52840) I guess since there is a dedicated pin for vbus on nrf and an diode to prevent current from lipo to vbus https://cdn-learn.adafruit.com/assets/assets/000/068/545/original/circuitpython_nRF52840_Schematic_REV-D.png?1546364754

Admittedly, there isn't a common API for vbus detect just yet, since the implementation is different across platform

  • one with dedicated vbus pin like nrf52, which can be managed entirely within dcd
  • one with configurable vbus, but user hw often just drop it and then force vbus to always present like rp2040 Add Rp2040 suspend & resume support #1020
  • one use normal GPIO to read/check the vbus, then need API like your tusb_vbus_changed()

Note: without vbus detection, it is impossible for device to distinguish between suspend vs disconnection. Since both has the bus on idle (J state) without SOF.

Anyway, IMHO, it is worth to save power on BLE mcu, API can be odd (like nrf), but we will try to find an common one to put it together. Let me know if you want to merge this first, or want to make more improvement. We can always do any follow up PR later on.

@kasjer
Copy link
Collaborator Author

kasjer commented Aug 13, 2021

@hathach I think we could merge it like this for now. I will prepare VBUS handling since users will want to use power saving but for now it is more important to have USB working when it was not present on reset.

I already had something working to detect VBUS. In my first attempt I used dcd_connect/dcd_disconnect when VBUS was detected/lost (I think it worked). But then I was not so sure if this is correct way do handle this.
Maybe tud_connect and tud_disonnect are meant for application level and should be left alone.

Rest assure that I will prepare VBUS handling since there is some client that will push to have USB working efficiently on this MCU with battery powered device, they only just started with USB so I have some time.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know you have plan for the vbus detect. It also buy time for me to also try to gather my thought and come up with an decent API and usage for it as well.

@hathach hathach merged commit 89e4586 into hathach:master Aug 13, 2021
@kasjer kasjer deleted the kasjer/da1469x-fix-no-vbus-startup branch August 13, 2021 11:15
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

Successfully merging this pull request may close these issues.

None yet

2 participants