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

Add support for dcd_sof_enable() to some additional ports. #2647

Merged
merged 7 commits into from
May 27, 2024

Conversation

andrewleech
Copy link
Contributor

Describe the PR
This PR fills out the stubbed dcd_sof_enable() function for mimxrt, samd and renesas-ra ports.

Additional context
I'm working on a TinyUSB feature in micropython that uses SOF to provide some simple timing / delay shortly after connection of CDC: micropython/micropython#14485

This also relates to #2595 where it was found a delay is needed after CDC connection before sending / flushing TX data from TinyUSB (device) to host so its not lost.

In the micropython stm port (not using tinyusb) the SOF interrupt is enabled on CDC connection / DTR enable and is used to count 8ms before disabling the SOF interrupt again and flushing buffered data to the host. I'm planning on replicating this process using TinyUSB on other micropython ports, eventually replacing the STM USB stack with TinyUSB on that port also.

USB->DEVICE.INTENSET.bit.SOF = 1;
} else {
USB->DEVICE.INTENCLR.bit.SOF = 1;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SAMD support has been tested (both enable and disable) on a Seeed Studio XIAO SAMD21

dcd_reg->USBINTR |= INTR_SOF;
} else {
dcd_reg->USBINTR &= ~INTR_SOF;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implementation has been tested (both enable and disable) on NXP MIMXRT-1010-EVK where the interrupt fires at 125ms as expected for a high speed connection.

} else {
_dcd.sof_enabled = false;
NRF_USBD->INTENCLR = USBD_INTENCLR_SOF_Msk;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested on nrf52840 dongle (PCA10059).

@HiFiPhile
Copy link
Collaborator

HiFiPhile commented May 17, 2024

Thanks for your PR, since your are working on it , could you please also add dcd_event_sof() to pass event to upper layer ?
Like:

dcd_event_sof(rhport, frame, true);

For ci_hs it can be read from FRINDEX, samd21 and nrf5x I'm less familiar...

@andrewleech
Copy link
Contributor Author

could you please also add dcd_event_sof() to pass event to upper layer ?

Is this different? I noticed they already had something like this?

dcd_event_bus_signal(rhport, DCD_EVENT_SOF, true);

My testing from within micropython is using the tud_sof_cb() function which is being called when it's enabled at least.

@HiFiPhile
Copy link
Collaborator

Is this different? I noticed they already had something like this?

They are similar, except dcd_event_sof() pass the frame number which is useful mostly for audio application.

I need to fix tud_sof_cb() always receive frame_count 0

@HiFiPhile
Copy link
Collaborator

I just fixed the CI issue and frame count in tud_sof_cb().

@andrewleech
Copy link
Contributor Author

I've added dcd_event_sof() for the 4 ports now covered in this PR. I think I've got the frame count reading correct but haven't had a chance to test any of these yet. I'll get these tested before calling this PR ready.

I haven't tested renesas changes at all yet as I'm chasing some support to get USB working at all on the renesas evk I've got.

The 4 ports covered at this point are the main ones I'm planning on adding at this time as these (mimx, samd, nrf, renesas) are the only ones I've got access to where the matching micropython ports directly use libusb, other than rp2 where these functions have already been added.

I will also look at esp32s3 and esp32s2 are a separate issue, I do want that added too but in expecting some complications from IDF integration. If that's in fact trivial I'll add support for this platform too.

@andrewleech
Copy link
Contributor Author

mimx and samd frame_count are tested working, eg.
image

@HiFiPhile
Copy link
Collaborator

I will also look at esp32s3 and esp32s2 are a separate issue, I do want that added too but in expecting some complications from IDF integration. If that's in fact trivial I'll add support for this platform too.

esp32 use dcd_dwc2 which already have sof support, it is little more complicate if you want to try:

# 1. Source ESP-IDF
# Windows
%userprofile%\esp\esp-idf\export.bat
# Linux
. $HOME/esp/esp-idf/export.sh
# 2. Enter example directory
cd tinyusb/examples/device/hid_composite_freertos
# 3. Run cmake in project directory specifying the board
cmake -DBOARD=espressif_s3_devkitc -B build -G Ninja .
ninja.exe -C build

@andrewleech
Copy link
Contributor Author

I would have assumed esp32s2/3 uses this driver which doesn't have these sof functions configured?

@HiFiPhile
Copy link
Collaborator

HiFiPhile commented May 19, 2024

dcd_esp32sx is old driver still maintained by espressif (with some specific logging), a new driver has been created for all devices with dwc2:
src/portable/synopsys/dwc2/dcd_dwc2.c

@andrewleech
Copy link
Contributor Author

Thanks @HiFiPhile that's really interesting about the esp32sX driver! With that info I've been able to get a build of micropython working on the S3 that's using our existing tinyusb submodule and this driver directly, ignoring most of the esp32 usb implementation. It just took a little bit of extra support to manage switching from USB serial/jtag mode to USB OTG mode, but my test code for that seems to be working pretty well so far.
Being able to use the same shared tinyunb intergration code across all ports in micropython will be a great advantage, I appreciate your input here!

@andrewleech andrewleech force-pushed the additional_dcd_sof_enable branch 2 times, most recently from 957f924 to eca7837 Compare May 27, 2024 10:20
@andrewleech
Copy link
Contributor Author

andrewleech commented May 27, 2024

Thanks for your SOF frame count fix, I've also now finished adding dcd_event_sof() for each of the ports I'm targeting here.

I've been able to test all of them and have confirmed the frame counts are passed through as expected.

In case it helps, the code in using sof interrupt for plus the test printf to confirm SOF frame count is here: https://github.com/andrewleech/micropython/blob/54e0abf5d15441ad7a2a9d8058e45c6f8e193f68/shared/tinyusb/mp_usbd_cdc.c#L124

I now consider this PR complete and ready for final review thanks (pending me rebasing it / fixing the merge conflict).

// TODO implement later
rusb2_reg_t* rusb = RUSB2_REG(rhport);
_dcd.sof_enabled = en;
rusb->INTENB0_b.SOFE = en ? 1: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renesas updates tested on a EK-RA4W1 which had a usb cable added to the appropriate breakout header pins (the USB peripheral actually isn't connected to a USB port on these eval boards).

@andrewleech andrewleech changed the title DRAFT: Add support for dcd_sof_enable() to some additional ports. Add support for dcd_sof_enable() to some additional ports. May 27, 2024
Copy link
Collaborator

@HiFiPhile HiFiPhile left a comment

Choose a reason for hiding this comment

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

Thanks for your work and tests !

@HiFiPhile HiFiPhile merged commit 1fe86f6 into hathach:master May 27, 2024
76 checks passed
@andrewleech
Copy link
Contributor Author

Thanks for your support and reviews @HiFiPhile !

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.

2 participants