Skip to content

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
Member

@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

@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 :)

@P-R-O-C-H-Y P-R-O-C-H-Y added Resolution: Awaiting response 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 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
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