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

embassy-usb support #1517

Merged
merged 12 commits into from
May 1, 2024
Merged

embassy-usb support #1517

merged 12 commits into from
May 1, 2024

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Apr 27, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • My changes were added to the CHANGELOG.md in the proper section.

Extra:

Pull Request Details 📖

Description

This PR uses the (as of yet unreleased) embassy-usb-synopsys-otg crate to implement embassy-usb support for ESP32-S2 and ESP32-S3. The PR also adds an example, similar in functionality to the existing usb_serial example.

Resolves #1191

Testing

Ran the new example on an ESP32-S2-DevKitC-1 and ESP32-S3-DevKitC-1

@bugadani
Copy link
Contributor Author

This is a bit hacky but it works on at least the S2. There are some code quality issues (e.g. missing documentation), and I couldn't figure out how to make the new dependencies not be included for other targets.

The example is a blend of the esp-hal and the embassy-stm32 usb_serial examples.

@bugadani bugadani marked this pull request as ready for review April 29, 2024 12:27
@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 29, 2024

Awesome! Do we know if/when embassy-usb-synopsys-otg might get released (since merging this now would prevent us from releasing until then)

I think we should remove the logger-init from the examples - I personally like it but not everyone does (and I think I forgot to remove one of them myself before 🤔 )

@bugadani
Copy link
Contributor Author

Awesome! Do we know if/when embassy-usb-synopsys-otg might get released (since merging this now would prevent us from releasing until then)

I don't think it needs to cook for long, but dirbaio probably will want to make sure we don't end up breaking embassy-stm32 with the refactoring.

I think we should remove the logger-init from the examples - I personally like it but not everyone does (and I think I forgot to remove one of them myself before 🤔 )

Apologies, it was a leftover of my testing.

@jessebraham
Copy link
Member

I am sure we will merge this eventually, however since we're blocked on embassy-usb-synopsys-otg being published I'm just going to convert this to a draft until it's ready for review.

@jessebraham jessebraham marked this pull request as draft April 30, 2024 14:07
@Dirbaio
Copy link
Contributor

Dirbaio commented Apr 30, 2024

released https://crates.io/crates/embassy-usb-synopsys-otg

@jessebraham
Copy link
Member

Oh that was quicker than expected, thanks! 😁

@jessebraham jessebraham marked this pull request as ready for review April 30, 2024 15:29
@bugadani bugadani force-pushed the usb branch 3 times, most recently from 45cd5ed to 1f94741 Compare April 30, 2024 15:32
@bugadani bugadani marked this pull request as draft April 30, 2024 15:33
@bugadani bugadani marked this pull request as ready for review April 30, 2024 15:33
@bugadani bugadani force-pushed the usb branch 2 times, most recently from 15efca8 to c9c9e5f Compare May 1, 2024 13:03
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

As part of #1485, all examples now compile on stable. I believe you're not using type_alias_impl_trait so lets go ahead and remove it.

After that, I think think this is good to go! I got this running with my keyboard firmware and its working well, thanks for looking into this!

examples/src/bin/embassy_usb_serial.rs Outdated Show resolved Hide resolved
@bugadani
Copy link
Contributor Author

bugadani commented May 1, 2024

I got this running with my keyboard firmware and its working well, thanks for looking into this!

With this PR I have a better behaved debug probe than my priced-like-gold J-Link 😭

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@jessebraham jessebraham added this pull request to the merge queue May 1, 2024
Merged via the queue into esp-rs:main with commit fd1c7b4 May 1, 2024
21 checks passed
@bugadani bugadani deleted the usb branch May 1, 2024 21:00
MabezDev pushed a commit to MabezDev/esp-hal that referenced this pull request May 2, 2024
* embassy-usb support

* Add changelog entry

* Update embassy-usb-synopsys-otg

* Change VID/PID to match the blocking example

* Add missing initialisation

* Clean up

* fmt

* Remove log init

* Use released crate

* Revert to released embassy-usb

* Update vid/pid

* Remove redundant TAIT feature gate
@morfert
Copy link

morfert commented May 14, 2024

Hey, @MabezDev I have been trying to get usb hid to work. I have used the embassy-usb raspberry pi example as a reference and have managed to make it work.
However, when I add a report id to my hid report descriptor it no longer works even if I prepend each report with the id.

As part of #1485, all examples now compile on stable. I believe you're not using type_alias_impl_trait so lets go ahead and remove it.

After that, I think think this is good to go! I got this running with my keyboard firmware and its working well, thanks for looking into this!

You mentioned you have a keyboard firmware so I was wandering if you knew how I should be adding report ids to the reports.

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.

Support embassy-usb on ESP32-S2 and ESP32-S3
7 participants