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 USB MIDI support to libraries/USB #8166

Merged
merged 14 commits into from
Dec 5, 2023

Conversation

EleotleCram
Copy link
Contributor

Description of Change

This PR adds support for USB MIDI to libraries/USB. It wraps the underlying tiny-usb API in an Arduino-like/User-friendly API, similar to the already existing USBHID* classes.

Tests scenarios

All the examples have been tested using an ESP32-S2 Mini. Please see the related links section for video-proof that it works.

Related links

@VojtechBartoska VojtechBartoska added the Status: Review needed Issue or PR is awaiting review label May 11, 2023
@me-no-dev me-no-dev added this to the 3.0.0 milestone May 11, 2023
@me-no-dev
Copy link
Member

Very nice implementation!

  • You need to add USBMIDI.cpp here
  • There are also some other build errors reported by CI here

@VojtechBartoska VojtechBartoska removed the request for review from PilnyTomas May 11, 2023 14:38
@EleotleCram
Copy link
Contributor Author

Thanks!

And I was already suspicious on how silent the arduino-cli output was, I just discovered the --warnings all option ;-)
Will fix this shortly.

@me-no-dev me-no-dev requested a review from SuGlider May 12, 2023 14:01
Copy link
Collaborator

@lucasssvaz lucasssvaz left a comment

Choose a reason for hiding this comment

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

Working as expected

libraries/USB/src/USBMIDI.cpp Outdated Show resolved Hide resolved
libraries/USB/src/USBMIDI.h Outdated Show resolved Hide resolved
libraries/USB/src/USBMIDI.h Outdated Show resolved Hide resolved
libraries/USB/src/USBMIDI.h Outdated Show resolved Hide resolved
libraries/USB/src/USBMIDI.h Outdated Show resolved Hide resolved
@VojtechBartoska VojtechBartoska removed the request for review from SuGlider November 29, 2023 09:03
Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

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

@EleotleCram PTAL on my comments.
Also please add skip files to examples for esp32c6 and esp32h2.
Thanks! Good job :)

libraries/USB/src/USBMIDI.cpp Show resolved Hide resolved
libraries/USB/src/USBMIDI.cpp Outdated Show resolved Hide resolved
libraries/USB/src/USBMIDI.h Outdated Show resolved Hide resolved
libraries/USB/src/USBMIDI.h Outdated Show resolved Hide resolved
@P-R-O-C-H-Y P-R-O-C-H-Y added Resolution: Awaiting response Waiting for response of author Area: Libraries Issue is related to Library support. and removed Status: Review needed Issue or PR is awaiting review labels Nov 29, 2023
@VojtechBartoska
Copy link
Contributor

Hello @EleotleCram,

thanks for you PR.

Unfortunately it seems that CLA form did not show up for your first contribution. Can you please check https://cla-assistant.io/ if you have the CLA signed for this repository.

Thanks a lot

@P-R-O-C-H-Y P-R-O-C-H-Y added Status: Pending Merge Pull Request is ready to be merged and removed Resolution: Awaiting response Waiting for response of author labels Dec 1, 2023
@EleotleCram
Copy link
Contributor Author

@VojtechBartoska AFAIK I already signed the CLA for this repository; when I log in to cla-assistant and I view my signed CLAs it lists this repo.

@EleotleCram
Copy link
Contributor Author

@EleotleCram PTAL on my comments. Also please add skip files to examples for esp32c6 and esp32h2. Thanks! Good job :)

@P-R-O-C-H-Y , I just noticed things were moving again for this PR. I've checked your comments, but it seems the changes have already been made by now, do I need to take further action?

@P-R-O-C-H-Y
Copy link
Member

Hi @EleotleCram, I took over the PR and fixed all the stuff we noticed in the reviews.
If the CLA is signed as you said we are ready to merge it :)

@me-no-dev me-no-dev merged commit 1a7a893 into espressif:master Dec 5, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Libraries Issue is related to Library support. Status: Pending Merge Pull Request is ready to be merged
Projects
Development

Successfully merging this pull request may close these issues.

5 participants