RP2040 MIDI Host

[atoktoto]

This pull request replaces the previous draft: #1605

This PR adds MIDI host support for RP2040 boards based on work done by @rppicomidi here: https://github.com/rppicomidi/tinyusb/tree/pio-midihost and @Skyler84 in #1434.

It seems to work pretty OK and has no issue with hot-plugging (re-plugging) the device that #1605 had.

[todbot]

About my above comments, since there wasn't an example included in this PR, I took the previous PR's examples/host/midi and tried compiling it.

[todbot]

Still having problems compiling this.
Since there was no Makefile in the PR example, I added the default minimal one:

include ../../../tools/top.mk
include ../../make.mk
INC += \
	src \
	$(TOP)/hw \
# Example source
EXAMPLE_SOURCE += $(wildcard src/*.c)
SRC_C += $(addprefix $(CURRENT_PATH)/, $(EXAMPLE_SOURCE))
include ../../rules.mk

And then I could follow the standard tinyusb build steps but with this PR. Specifically I did:

git clone https://github.com/hathach/tinyusb tinyusb-testmidihost
cd tinyusb-testmidihost
gh pr checkout 1627
git submodule update --init lib
cd examples/host/midi_rx
make BOARD=raspberry_pi_pico get-deps
make BOARD=raspberry_pi_pico all

This fails because all of the unused variables in host/midi_rx/src/main.c if no logging. So, using LOG=1 workaround, I recompile:

make BOARD=raspberry_pi_pico clean && make BOARD=raspberry_pi_pico LOG=1 all

and get the error:

[...]
Scanning dependencies of target midi_rx
make[3]: Leaving directory '/Users/tod/projects/tinyusb/tinyusb-testmidihost/examples/host/midi_rx/_build/raspberry_pi_pico'
make[3]: Entering directory '/Users/tod/projects/tinyusb/tinyusb-testmidihost/examples/host/midi_rx/_build/raspberry_pi_pico'
[ 16%] Building C object CMakeFiles/midi_rx.dir/src/main.c.obj
<command-line>: error: no macro name given in #define directive
compilation terminated due to -Wfatal-errors.
make[3]: *** [CMakeFiles/midi_rx.dir/build.make:76: CMakeFiles/midi_rx.dir/src/main.c.obj] Error 1

Any clues as to what I'm doing wrong?

[atoktoto]

Thanks for being persistent on this one. I'm new at this and was not even aware of the standard way to build TinyUSB.
I'm building this starting from pico-examples CMakeLists.txt as the project includes both pico-sdk and TinyUSB examples.

[todbot]

Do you have a specific recommended build process for this PR?

I have tried both using standard Pico build process inside of midi_rx and moving midi_rx to pico-examples and building there. Specifically I have tried:

cd tinyusb-testmidihost/examples/host/midi_rx
mkdir build
cd build
cmake ..

And I have tried:

cd pico_examples
cp -a ../tinyusb-testmidihost/examples/host/midi_rx .
echo "add_subdirectory(midi_rx)" >> CMakeLists.txt
mkdir build
cd build
cmake ..

[atoktoto]

Generally I'm following this guide: https://datasheets.raspberrypi.com/pico/getting-started-with-pico.pdf and building from CLion IDE, but the commands it's running are:

cmake.exe -G Ninja -S E:\pico\pico-examples -B E:\pico\pico-examples\build
cmake.exe --build E:\pico\pico-examples\build --target tinyusb_host_midi_rx -j 9

(I ommited the env variables specifying the paths to compilers for the second command)

For any other example it would be as simple as:
cd pico-examples
mkdir build
cd build
cmake ..
cd blink (in the build directory)
make -j4

But the TinyUSB examples are pulled in from outside of project root and I don't know where to look for CMake outputs of those.

[todbot]

Seems odd to me you're not following standard tinyusb build process for this.

[atoktoto]

In the meantime (while waiting for #1434 to be merged) I started on adding a nice-looking MIDI API. Turns out the simplest way is to reuse https://github.com/FortySevenEffects/arduino_midi_library. Fortunately it does not depend on Arduino itself and has a transport plug-in architecture.

I wrote a TinyUSB based tramsport plugin for it: https://github.com/atoktoto/pico-midi-usb-transport

void onNote(Channel channel, byte note, byte velocity) {
    printf("Note ON ch=%d, note=%d, vel=%d\n", channel, note, velocity);
}

MIDI.setHandleNoteOn(onNote);

[atoktoto]

@todbot On my machine (Windows, under WSL2) the example now builds with make BOARD=raspberry_pi_pico all in the example directory.

I also verified that the example works as intended when the resulting uf2 is dropped onto RP2040.

I did not get the no macro name given in #define directive error at any point so not sure where to look for the issue.

[todbot]

@atoktoto, I also can get it to compile now (with similar Makefile and changes as your commits, though I had to also fix casts on int -> uint conversion errors cable_num in midi_host.c). Not sure what that no macro name error was but I nuked the directory, recloned and re-pulled and that error is gone.

The pico-midi-usb-transport library is great! The 47effects MIDI library is wonderful.

The CMake files of pico-midi-usb-transport reference to a pico-midi-usb project, like:

add_library(pico-midi-usb INTERFACE)

Where does the pico-midi-usb CMake file come from? (Apologies I don't understand CMake very well)

[atoktoto]

That's a naming inconsistency on my side.

The key fact here is the name of the project is not important. AFAIK add_library(pico-midi-usb INTERFACE) creates a target that can be linked to with target_link_libraries(example-print-notes pico-midi-usb).

[paulhamsh]

I am keen to get this in the master branch so I can use it properly - is this stuck somehow, and can I help?

Seems a failure on this:

/home/runner/work/tinyusb/tinyusb/src/class/midi/midi_host.c:595:29: error: conversion from 'int' to 'uint8_t' {aka 'unsigned char'} may change value [-Werror=conversion]
  595 |         stream->buffer[0] = (cable_num << 4) | msg;
      |                             ^
compilation terminated due to -Wfatal-errors.

I can't really tell why it converts two uint8_t into an int, but wouldn't this fix it?

  595 |         stream->buffer[0] = (uint8_t) ((cable_num << 4) | msg);

[rppicomidi]

@atoktoto I found a fix for hubs not working with this. I made the error in my original pull request. Would you consider adding this change to your pull request?

diff --git a/src/class/midi/midi_host.c b/src/class/midi/midi_host.c
index d242d33c5..bc87898cc 100644
--- a/src/class/midi/midi_host.c
+++ b/src/class/midi/midi_host.c
@@ -490,7 +490,7 @@ bool midih_set_config(uint8_t dev_addr, uint8_t itf_num)
   p_midi_host->configured = true;
 
   // TODO I don't think there are any special config things to do for MIDI
-
+  usbh_driver_set_config_complete(dev_addr, p_midi_host->itf_num);
   return true;
 }

[atoktoto]

Sure, I will to that tonight.

[nevvkid]

+1 for this feature.

[ctag-fh-kiel]

+1 for this feature.

[jfedor2]

You can just use the external application driver linked above (usb_midi_host), it works great. There's no need for this to be part of tinyusb.

[nevvkid]

@jfedor2 thats what we are currently doing. But there are huge performance problems with that if you do more advanced stuff via USB Host. So i would it say it depends. Generally there is room for improvement and better integration of the MIDI Host feature with TinyUSB.

[rppicomidi]

@nevvkid Can you please describe where the performance problems from application drivers come from, and what advanced stuff you are doing that suffers when you use application drivers?

As far as I understand it, the USB host code accesses external driver functions using function pointers; same as built-in drivers. External application driver function pointers are stored in an array; built-in driver function pointers are stored in a different array. To get a pointer to the function pointers of the driver, the host code calls the get_driver() function. Is there something I am missing?

[hathach]

Thank you very much of your PR. It has been a quite and I really appreciate your effort and patient. I have been wanting to review/merge this but keep getting pulled by other works and kind of forgot sometimes. Adding an new host driver is significant works, and reviewing it may take a bit longer. Though I have sync it up with master, also fix some out of sync naming/signature/make changes. I run the example and it works well (great work).

We will need to migrate the endpoint to use the new endpoint stream, maybe rename/move thing around if neccessary. Before that since I am not familliar with MIDI, can you explain what CFG_MIDI_HOST_DEVSTRINGS means ? Is it a terminology since it enable mananging series of midi jack ?

[0:] USBH Device Attach                                                                                                                                                                         
MIDI opening Interface 1 (addr = 1)                                                                                                                                                             
MIDI descriptor parsed successfully                                                                                                                                                             
MIDI device address = 1, IN endpoint 1 has 1 cables, OUT endpoint 1 has 1 cables                                                                                                                
Read bytes 3 cable 0  0000:  90 51 7F                                         |.Q.|                                                                                                             
Read bytes 3 cable 0  0000:  80 4E 00                                         |.N.|              

[hathach]

I made more changes to update the driver to use the new endpoint stream API (similar to cdc host). I have only tested with midi_rx example, didn't test with tx at all. I will try to look up what the string doing with midi and continue to make more update, I think we should also need to make some driver API changes as well. Hopefully we could wrap this up and merge this soon enough.

[Ownasaurus]

Thank you so much for allocating some of your limited time to this. Us MIDI people really appreciate it!!

[ctag-fh-kiel]

Is #2814 relevant (and maybe #2776) could they be accounted for in this merge?

[hathach]

Is #2814 relevant (and maybe #2776) could they be accounted for in this merge?

That PR solve another issue, and isn't part of this.

Thank you so much for allocating some of your limited time to this. Us MIDI people really appreciate it!!

It is my leisure, I wish I could mange the time to do this sooner.

[rppicomidi]

@hathach Set CFG_MIDI_HOST_DEVSTRINGS to 1 to process the virtual cable names of the attached device for MIDI IN and MIDI OUT from the MIDI configuration descriptors and the string descriptors during enumeration and to enable the API for applications to read the strings. For example, on my Arturia KeyLab Essential 88 keyboard, virtual cable 0 is labeled "MIDI" and virtual cable 1 is labeled "DAW" for both MIDI IN and MIDI OUT.

[rppicomidi]

@hathach I recommend that you reference the usb_midi_host project when merging this pull request. I have been maintaining it for quite a while and have fixed a number of issues since this pull request was submitted.

[hathach]

@hathach I recommend that you reference the usb_midi_host project when merging this pull request. I have been maintaining it for quite a while and have fixed a number of issues since this pull request was submitted.

Ah thank you for the head up, I will try to merge this first, then do compare and update later with follow-up PR afterwards. I don't own/use any midi comercial at all, therefore it is all up to you and others to fix its issues. I mostly make sure it is up-to-date with stack API, compiling with other ports and working with simple examples (like midi_rx).

@hathach Set CFG_MIDI_HOST_DEVSTRINGS to 1 to process the virtual cable names of the attached device for MIDI IN and MIDI OUT from the MIDI configuration descriptors and the string descriptors during enumeration and to enable the API for applications to read the strings. For example, on my Arturia KeyLab Essential 88 keyboard, virtual cable 0 is labeled "MIDI" and virtual cable 1 is labeled "DAW" for both MIDI IN and MIDI OUT.

Thank you for explanation

[hathach]

PS: I made more changes to the API and its signature and feeling good for now. I will check the string thing later, need to switch to other works for now. So the string is mostly for displaying the label ? Besides that does it has any behavior or setting impact ?

[rppicomidi]

@hathach Yes, CFG_MIDI_HOST_DEVSTRINGS is for displaying labels for virtual cables. Enabling the setting requires slightly more RAM and code space. Enabling it slows enumeration slightly because the driver needs to parse the Jack Descriptors. The setting does not change run-time behavior after enumeration. Applications with no UI do not need it.

Many MIDI devices do not implement Jack Descriptor strings. Some implement them, but do it poorly. An example RP2040 device that uses Jack Descriptor strings is midi-multistream2usbdev. You may find that project useful to test the Jack Strings when you get to that.

[hathach]

@rppicomidi thank you for your explanation, if string is only used for displaying label. I think we can

• add an optional callback that have all these information when parsing descriptor instead of storing them in the interface. If application need these, they should store it.
• drop it entirely and let the parsing to application, we can expose midi interface descriptor via callback

Making that change now