-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
WiP : MIDI Host #1122
base: master
Are you sure you want to change the base?
WiP : MIDI Host #1122
Conversation
…ts the packet content (zero runtime cost)
…an integrate this in to TinyUSB
…UG = 2 or 3, not tried with 1)
… - commented out broken debug lines for a quick fix
uint8_t bDescriptorType ; ///< Descriptor Type. Value: TUSB_DESC_CS_INTERFACE. | ||
uint8_t bDescriptorSubType ; ///< Descriptor SubType. Value: AUDIO_CS_AC_INTERFACE_HEADER. | ||
uint16_t bcdADC ; ///< Audio Device Class Specification Release Number in Binary-Coded Decimal. Value: U16_TO_U8S_LE(0x0200). | ||
uint8_t bCategory ; ///< Constant, indicating the primary use of this audio function, as intended by the manufacturer. See: audio_function_t. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes definition from Audio 2 specification section 4.7.2 to version 1.0 section 4.3.2 with limitation to 1 interface.
Maybe better way to handle this is to have 1.0 version separated and not change just one structure to different specification version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Yes. I didn't realise that was the root of the problem!
I would suggest the v1 be called audio_desc_cs_ac_interface_t
and v2 audio_desc_cs_ac_interface_v2_t
...but I guess that might be seen as an undesirable breaking-change :/
Do you have any ideas for the best/right way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert the change, audio v1 is only used by MIDI, all other class moves to audio v2. MIDI driver could handle this locally within its driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR and sorry for late response, I have been busy with other work. The PR is still WIP therefore I converted it to draft. Could you update:
- remove unnecessary files from the PR such as one in
_MIDIH.tmp/
, and other local used only such as dbg-tusbh.h - revert the changes to endpoint (mentioned in review) and audio cs interface (by kasjer)
Note: rp2040 host stack does not work with bulk endpoint just yet, due to the chip limitation that can only carry out 1 bulk transfer at a time and lower level HCD need more update to work with . Therefore using rp2040 for developing midi host isn't a good idea, I would suggest to change the MCU, and swtich back to rp2040 when that is adddressed.
MIDI_CIN_NOTE_OFF = 8, | ||
MIDI_CIN_NOTE_ON = 9, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch, it is my typo
uint8_t bRefresh ; ///< currently unused https://www.usb.org/sites/default/files/midi10.pdf | ||
uint8_t bSynchAddress ; ///< currently unused B.5.1 Standard Bulk OUT Endpoint Descriptor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove these since it will break entire stack. MIDI (audio v1) is the only class that has these extra 2 field. MIDI driver should handle it locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Thanks.
Thanks for your code review :)
Absolutely! When it is working & tested, you WILL get a CLEAN pull request . The files in .TMP will ultimately be the MIDI host The final version will also NOT contain abominations like : 62b1529#diff-e7025ce0e682cf4e30670612dd559328a733bb6a3ec53e449e51fb7088ef9155R40 ...I'm a n00b to "the git(hub) way", and mostly just wanted to make sure I exposed everything here - in case someone else wants to help, or 'steal' some code. [I also need to add MIT headers to all my files] |
No problem at all, I am not git expert either, just use some GUI, that make adding file, commit and push easier. I uses git-cola on Linux, but any should do the work. |
Is the work on this still active? MIDI host support would be quite useful for a project I'm working on |
@timonsku I wrote a USB MIDI host application driver for TinyUSB in the usb_midi_host project. You might find that helpful. I have only tested it on the RP2040 on a Raspberry Pi Pico board and a Pico W board. |
MIDI Host - work in progress
Includes descriptor decoding routines (I'm learning USB as I go)
Some commits can be included immediately as they are generic "bug fixes" ...these commits are flagged "**" because I have no idea how to use git properly (yet)
The MIDI Engine code (~3kloc) [see MIDI folder at root level] cannot be properly integrated, tested, and debugged until I get the basic driver going - which is not going well :( ...but it at least compiles in a test harness :)