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.

[hathach]

Thank you very much everyone for putting time and effort for this PR. Especially @atoktoto and @rppicomidi for getting most of the work done. I am sorry for being super late, I have to constant switching works and haven't worked much with rp2040 until now.

Though since there is lots of changes since this PR is created, I have update and also make lots of changes, notably:

• lost of API rename and signature changes to match other class driver consistency e.g num_rx_cables --> rx_cable_count
• remove the devstring and added tuh_midi_descriptor_cb() along with callback data to leave extracting midi jack info to application if needed
• use the new endpoint stream API for streamging data
• allow to turn off stream API CFG_TUH_MIDI_STREAM_API=0 (which consume lots of logic/footprint). Since more recent high level MIDI library can support multiple cables and can parse 4-byte usb packet natively
• change the API input from dev_addr --> interface index, consistency with cdc, but also allow to support more than 1 midi interface per device (rare case).

It is quite a bit of changes, and may break compatible with existing host driver from application space, @rppicomidi let me know what you think and how you want to chagne/resolve the confict. I am open to all feedback, thank you.

[rppicomidi]

@hathach I am just back from travel. I may need a few days to get you feedback. Sorry for the delay.

[hathach]

@hathach I am just back from travel. I may need a few days to get you feedback. Sorry for the delay.

no problems, please take your time.

[rppicomidi]

The midi_rx example does not build. I tried the build with VS Code using the Raspberry Pi pico-examples project and pico-sdk 2.1.1. The reasons the build failed are likely out of scope for this pull request. I am writing them here because I do not know where else to file them.

It does not build for two reasons:

1. The pico-sdk file src/rp2_common/tinyusb/CMakeLists.txt sets BOARD to pico-sdk, which does not work with the latest TinyUSB code. Even the develop branch of the pico-sdk does not set BOARD and FAMILY based on the PICO_BOARD setting for this CMakeLists.txt file; it also does not test if BOARD or FAMILY are already defined on the cmake command line. I believe this is something that needs to be worked out between the Raspberry Pi pico-sdk project and this one.
2. There are 3 examples in the TinyUSB project whose CMakeLists.txt files call family_configure_device_example(${EXE_NAME} ${RTOS}) when they should call family_configure_device_example(${EXE_NAME} noos). The files are examples in the examples/device directory: board_test, msc_dual_lun and cdc_msc. Because these files do not build, the whole pico-examples project appears broken.

Are these known issues? I did not see bugs filed, but it may be my search. People who follow the Raspberry Pi Pico build workflow from current Raspberry Pi Pico C/C++ documentation will have trouble here and may give up.

I will try to get to a test on hardware and a review of the actual driver when I am able.

[hathach]

@rppicomidi how to do build the example, I normally just go to tinyusb/exmaples/ and cmake there. I surely can try to make it backward compatible.

PS: if you create an empty board.h inside the hw/bsp/rp2040/boards/pico_sdk, will it fix the compile issue ?

[rppicomidi]

@hathach Refer to issue #3004 for reproduce steps. Yes, putting an empty board.h file in the pico-sdk directory seems to allow it to build.

[hathach]

@hathach Refer to issue #3004 for reproduce steps. Yes, putting an empty board.h file in the pico-sdk directory seems to allow it to build.

I don't use vscode, but if this fix the compile issue, let just do it. I guess I miss this board config

[rppicomidi]

Also need to fix 3 CMakeList.txt files. I added command line steps to reproduce to #3004.

[rppicomidi]

In midi_host.c, lines 173-176

    if (xferred_bytes) {
      tu_edpt_stream_read_xfer_complete(&p_midi_host->ep_stream.rx, xferred_bytes);
      tuh_midi_rx_cb(idx, xferred_bytes);
    }

Many MIDI devices, especially from Korg, will always respond to an IN transfer request from the host with 64 bytes of data. If they have no data to send, they will send back all 64-bytes of zero instead of NAK. If they have only a single message to send, they will send back the first 4 bytes encoded as a MIDI message packet and the remaining 60 bytes all zero. And so on.

My usb_midi_host application driver handles this case by parsing the received data in the transfer callback and filtering out all 4-byte packets that are all zero. It only calls the tuh_midi_rx_cb() if the received data contains valid MIDI data. If you choose not to do this filtering in the midi_host driver, then the test midi_rx application's tuh_midi_rx_cb() function should test if bytes_read == 0 before printing. For example:

  uint32_t bytes_read = tuh_midi_stream_read(idx, &cable_num, buffer, sizeof(buffer));
  if (bytes_read == 0) return; // <---rppicomidi recommended change
  printf("Cable %u rx %lu bytes: ", cable_num, bytes_read);
  for (uint32_t i = 0; i < bytes_read; i++) {
    printf("%02X ", buffer[i]);
  }
  printf("\r\n");

This change to driver behavior may change behavior of existing applications that use the usb_midi_host application host driver.

[rppicomidi]

A question about the example program: Why does it not also demonstrate MIDI TX? For example, see https://github.com/rppicomidi/usb_midi_host/blob/main/examples/C-code/usb_midi_host_example/usb_midi_host_example.c

The example will either light up the transport LEDs on MIDI controller devices that support play and record transport buttons, or it will play musical notes on sound generating MIDI devices. If the issue is you have no commercial MIDI hardware to test this, perhaps just hooking it to a Pico or other board with the TinyUSB MIDI Device example will suffice.

[rppicomidi]

There are some MIDI devices, especially guitar pedals, that are designed for high speed USB 2.0 instead of full speed. They also do not provide an alternate descriptor when the host is full speed. As a result, their bulk endpoint descriptor max transfer sizes are always 512 bytes. That is too large for chips like the RP2040 that only support full speed USB. The code in the usb_midi_host application driver limits the endpoint sizes to the maximum that the hardware supports before opening the endpoint. I would recommend that this driver limit the endpoint buffer sizes before calling tuh_edpt_open(). Alternatively, the processor specific hcd code should limit the buffer sizes

For example, midi_host.c around line 268

      case TUSB_DESC_ENDPOINT: {
        tusb_desc_endpoint_t const *p_ep = (tusb_desc_endpoint_t const *) p_desc;
        p_desc = tu_desc_next(p_desc); // next to CS endpoint
        TU_VERIFY(p_desc < p_end && tu_desc_next(p_desc) <= p_end);
        midi_desc_cs_endpoint_t const *p_csep = (midi_desc_cs_endpoint_t const *) p_desc;

        TU_LOG_DRV("  Endpoint and CS_Endpoint descriptor %02x\r\n", p_ep->bEndpointAddress);
        tusb_desc_endpoint_t ep = *p_ep;
        if (p_ep->wMaxPacketSize > TUH_EPSIZE_BULK_MPS)
        {
          ep.wMaxPacketSize = TUH_EPSIZE_BULK_MPS;
        }
        if (tu_edpt_dir(p_ep->bEndpointAddress) == TUSB_DIR_OUT) {
          p_midi->ep_out = p_ep->bEndpointAddress;
          p_midi->tx_cable_count = p_csep->bNumEmbMIDIJack;
          desc_cb.desc_epout = p_ep;
          TU_ASSERT(tuh_edpt_open(dev_addr, &ep));
          tu_edpt_stream_open(&p_midi->ep_stream.tx, &p_ep);
        } else {
          p_midi->ep_in = p_ep->bEndpointAddress;
          p_midi->rx_cable_count = p_csep->bNumEmbMIDIJack;
          desc_cb.desc_epin = p_ep;

          TU_ASSERT(tuh_edpt_open(dev_addr, &ep));
          tu_edpt_stream_open(&p_midi->ep_stream.rx, &ep);
        }
        break;
      }

[rppicomidi]

tuh_midi_stream_write() should handle running status to make converting serial streams with running status encoded easier. This feature was added to the usb_midi_host application driver by pull request. See https://github.com/rppicomidi/usb_midi_host/blob/82a5c8e04bb16d638beafbb594c8f241279c6764/usb_midi_host.c#L616

[hathach]

Many MIDI devices, especially from Korg, will always respond to an IN transfer request from the host with 64 bytes of data. If they have no data to send, they will send back all 64-bytes of zero instead of NAK. If they have only a single message to send, they will send back the first 4 bytes encoded as a MIDI message packet and the remaining 60 bytes all zero. And so on.

Ah sorry, I didn't pay attention to this, I just did a quick check to see if all zeroes is a valid packet to make sure we don't drop a meaningful payload. The CIN = 0 is lisetd as Miscellaneous function codes. Reserved for future extensions. seem like we can just drop it, I will make the changes

A question about the example program: Why does it not also demonstrate MIDI TX? For example, see https://github.com/rppicomidi/usb_midi_host/blob/main/examples/C-code/usb_midi_host_example/usb_midi_host_example.c

The example will either light up the transport LEDs on MIDI controller devices that support play and record transport buttons, or it will play musical notes on sound generating MIDI devices. If the issue is you have no commercial MIDI hardware to test this, perhaps just hooking it to a Pico or other board with the TinyUSB MIDI Device example will suffice.

Example is part of the PR, I don't know the MIDI enough or have any consumer device to make a meaningful tx example. I mostly test with tinyusb midi device to print out console. If possible, please make an follow-up PR to update the example to both rx & tx.

There are some MIDI devices, especially guitar pedals, that are designed for high speed USB 2.0 instead of full speed. They also do not provide an alternate descriptor when the host is full speed. As a result, their bulk endpoint descriptor max transfer sizes are always 512 bytes. That is too large for chips like the RP2040 that only support full speed USB. The code in the usb_midi_host application driver limits the endpoint sizes to the maximum that the hardware supports before opening the endpoint. I would recommend that this driver limit the endpoint buffer sizes before calling tuh_edpt_open(). Alternatively, the processor specific hcd code should limit the buffer sizes

Hmm, this is out of USB specs, what if the device send 512bytes or simply more than 64 bytes on the bus ? that would corrupt buffer/buffer overflow on the rp2040 side. Maybe giving a LOG warnin, I will check, some of the sanity check may freakout.

tuh_midi_stream_write() should handle running status to make converting serial streams with running status encoded easier. This feature was added to the usb_midi_host application driver by pull request. See https://github.com/rppicomidi/usb_midi_host/blob/82a5c8e04bb16d638beafbb594c8f241279c6764/usb_midi_host.c#L616

thanks for the info, maybe we can do that in follow-up PR.

[hathach]

@rppicomidi I made more changes to addres 2 of your reviewed issues

• all zero response: it is special case, I think we can just simply check if all rx bytes are zeroes instead of doing it in 4 bytes which is quicker. I guess there is no device that has zero packet between meanningful packets in the same response ?
• force bulk 512 -> 64 bytes is done in tu_edpt_validate() which will help other drivers using bulk/stream ep API as well. I don't have such as MIDI device to test with, if you can give it a try for confirmation, that would be great.

Other issues can be resolved in follow-up PR, please make one if you have time, I will try to review it faster :). Also let me know that the callback with descriptors are good enough for your application to display, since I found bundling it into driver make thing a bit more complicated and may not extract/inteprete all infomration that user want. So it is better to leave that to application.

[rppicomidi]

I will attempt to integrate this driver with some of my applications after you merge it. Any issues I encounter will result in smaller pull requests that should be simpler to review and manage.

[hathach]

I will attempt to integrate this driver with some of my applications after you merge it. Any issues I encounter will result in smaller pull requests that should be simpler to review and manage.

Thank you very much for the effort and patient so far. Feel free to submit more follow-up PRs, I will try to review them as soon as I could.