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.

[AndrewCapon]

Hi @atoktoto,

The last update from @rppicomidi fixed the enumeration problems but there are still data transfer problems, this fixes it:

diff --git a/src/class/midi/midi_host.c b/src/class/midi/midi_host.c
index bc87898c..8df1b200 100644
--- a/src/class/midi/midi_host.c
+++ b/src/class/midi/midi_host.c
@@ -529,7 +529,7 @@ bool tuh_midi_read_poll( uint8_t dev_addr )
   if (in_edpt_not_busy)
   {
     TU_LOG2("Requesting poll IN endpoint %d\r\n", p_midi_host->ep_in);
-    TU_ASSERT(usbh_edpt_xfer(p_midi_host->dev_addr, p_midi_host->ep_in, _midi_host->epin_buf, _midi_host->ep_in_max), 0);
+    TU_ASSERT(usbh_edpt_xfer(p_midi_host->dev_addr, p_midi_host->ep_in, p_midi_host->epin_buf, p_midi_host->ep_in_max), 0);
     result = true;
   }
   else

[rppicomidi]

Great work fixing issues! I am working on an application you might find useful. It's not done yet, but I made the repo public now
to help with hub testing. See https://github.com/rppicomidi/midi2usbhub

[raveslave]

checking in on this one

[rppicomidi]

@raveslave I wrote a USB MIDI host application driver for TinyUSB in the usb_midi_host project. You might find that helpful. I have tested it on the RP2040 on a Raspberry Pi Pico board and a Pico W board.

[raveslave]

@raveslave I wrote a USB MIDI host application driver for TinyUSB in the usb_midi_host project. You might find that helpful. I have tested it on the RP2040 on a Raspberry Pi Pico board and a Pico W board.

thanks!!

[brendena]

What are the steps holding this back from being merged?