Skip to content
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

usb: device_next: add new MIDI 2.0 device class #81197

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

titouanc
Copy link
Contributor

@titouanc titouanc commented Nov 10, 2024

Add implementation for the 2nd revision of the USB MIDI device class (the MIDIStreaming subclass of the Audio device class), based on the new USB device stack. Moreover, add a sample application to demonstrate usage of this new device class.

This implementation is based on the following documents:

In this initial implementation, the device class only supports USB-MIDI2.0 (which can convey MIDI1 and MIDI2 data). However, the USB-MIDI2.0 specification requires to define a valid USB-MIDI1.0 interface before the 2.0 one for backward compatibility. Therefore a single interface defined in the device tree will yield 2 USB interfaces (with altsetting 0 and 1). The first one is always a "dummy" one (the minimal interface without any input/output), and the inputs/outputs of the second one are the ones defined in the device tree. As such, data exchange is only possible if the MIDI2.0 (altsetting 1) has been enabled by the host.

Zephyr application code can exchange MIDI data with the host in the form of Universal MIDI Packets through "Group Terminals", as illustrated in midi20: 4. Operational model:
image

@titouanc titouanc changed the title subsys: usb: device_next: add new MIDI 2.0 device class usb: device_next: add new MIDI 2.0 device class Nov 10, 2024
@titouanc titouanc force-pushed the add-usb-midi2 branch 2 times, most recently from e9d967c to 3845704 Compare November 10, 2024 21:13
@titouanc titouanc marked this pull request as ready for review November 10, 2024 21:15
Copy link
Contributor

@tmon-nordic tmon-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning on supporting MIDI 1.0 later? The specification strongly recomments, but does not mandate the MIDI 1.0 backwards compatible interface.

Do you have idea about how to support System Exclusive commands? Should "slicing" the SysEx message be application duty or class?

subsys/usb/device_next/class/usbd_uac2_macros.h Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_uac2_macros.h Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
include/zephyr/usb/class/usb_midi.h Outdated Show resolved Hide resolved
@titouanc titouanc force-pushed the add-usb-midi2 branch 2 times, most recently from 7daaed1 to 45becfb Compare November 12, 2024 18:17
@titouanc
Copy link
Contributor Author

Thank you @tmon-nordic for your first comments !

Are you planning on supporting MIDI 1.0 later? The specification strongly recomments, but does not mandate the MIDI 1.0 backwards compatible interface.

Do you have idea about how to support System Exclusive commands? Should "slicing" the SysEx message be application duty or class?

I decided to go for USB-MIDI2.0, because it transports Universal MIDI Packets (UMPs). The intended benefit is that this is an externally defined spec, independent of the transport layer, supporting both MIDI1 and MIDI2, and carrying "routing" (MIDI group) metadata. I believe these packets could be readily used on other transports such as Bluetooth or IP (but this goes beyond my knowledge). On the contrary, USB-MIDI1.0 has a packet format that is specific to this transport, and therefore somehow "leaks" some implementation details that are specific to USB-MIDI.

Moreover, I decided to NOT use a stream-based API (midi_write/read(some_data_bytes) like a serial port), because it avoids the burden of chunking MIDI (application) data into USB-MIDI packets. Such an implementation would require a MIDI parser and some kind of internal state; which I believe is out of scope of a USB class driver. As you noted, this implies that application code has to handle the extra complexity of splitting up long SysEx commands into valid UMPs. If there's a real need here, we could add the following function to the API, that would be in charge of chunking & sending a larger buffer into individual UMPs:

int usbd_midi_send_sysex(const struct device *dev,
                         const uint8_t *data, size_t size);

Overall, sending/receiving simple MIDI events (note on/off, control changes, system messages, short sysex, etc...) is straightforward with this proposal. Long SysEx are already some kind of "advanced" use cases, so I wouldn't try to do more about it here.

Even though USB-MIDI1.0 transport is not supported, I noticed that I needed to add that "dummy" backward compatible interface before the USB-MIDI2.0 one, otherwise the USBD MIDI Sample wouldn't enumerate properly. I tested both with Linux 6.11/alsa 1.2.12 and Mac OS X 15.1, but I'd be glad to hear about any experience with other hosts (like Android, iOS or Windows). For the record, here is how it looks on Mac OS:

image

And on Linux:

$ amidi -l
Dir Device    Name
IO  hw:2,1,0  Group 1 (USBD MIDI Sample)

I haven't planned on implementing a USB-MIDI1.0 stack, because 2.0 works for me, and its features are a superset of 1.0. Moreover, I haven't a clear idea on how it would look like for application code (are the 1.0/2.0 altsetting distinguably usable from dt/code ? How does the application code know which is selected, and how to format packets ? Otherwise do we have a translation layer 1.0<->2.0 depending on the selected altsetting ?).

Finally, I will dig into the full-speed/high-speed descriptors and amend the PR accordingly.

Thank you again for looking at this !

@tmon-nordic
Copy link
Contributor

Even though USB-MIDI1.0 transport is not supported, I noticed that I needed to add that "dummy" backward compatible interface before the USB-MIDI2.0 one, otherwise the USBD MIDI Sample wouldn't enumerate properly.

It wouldn't enumerate properly because it is mandatory for the MIDI 2.0 interface to be on alternate setting 1. Take a look at USB MIDI 2.0 Specification and note where "should" (recommended but optional) is used and where "shall" (mandatory) is used.

If there's a real need here, we could add the following function to the API, that would be in charge of chunking & sending a larger buffer into individual UMPs

I don't quite know about MIDI 2.0 but in MIDI 1.0 the most significant bit is reserved for Real Time messages. All SysEx commands (and responses) do have the most significant bit cleared to enable having the Real Time message while the SysEx is being transmitted. To what degree this is a problem is unknown to me, but USB-MIDI 1.0 chunks everything into 4 byte USB-MIDI Event Packets that can be freely interleaved.

While this API would be useful for applications that use SysEx messages exclusively (e.g. Digitech effect pedals, see https://github.com/desowin/gdigi for open-source host-side implementation), I have no idea if would introduce problems when chunk interleaving is necessary.

Moreover, I haven't a clear idea on how it would look like for application code (are the 1.0/2.0 altsetting distinguably usable from dt/code ? How does the application code know which is selected, and how to format packets ? Otherwise do we have a translation layer 1.0<->2.0 depending on the selected altsetting ?).

This is something that would have to be solved if we wanted proper backwards compatibility. The alt setting is only known at runtime and is entirely host-dependent. The class would have to somehow provide the information about selected protocol version to the application. About the format conversion I have no idea.

include/zephyr/usb/class/usb_midi.h Outdated Show resolved Hide resolved
include/zephyr/usb/class/usb_midi.h Outdated Show resolved Hide resolved
@kartben
Copy link
Collaborator

kartben commented Nov 19, 2024

Pxl.20241119.152833728.mp4

@titouanc this is really cool :)

samples/subsys/usb/midi/src/main.c Outdated Show resolved Hide resolved
samples/subsys/usb/midi/README.rst Outdated Show resolved Hide resolved
samples/subsys/usb/midi/README.rst Outdated Show resolved Hide resolved
samples/subsys/usb/midi/README.rst Outdated Show resolved Hide resolved
@titouanc titouanc force-pushed the add-usb-midi2 branch 2 times, most recently from fbe792a to 373525c Compare November 25, 2024 22:01
Copy link
Contributor

@tmon-nordic tmon-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap macro values in parentheses to avoid potential issues on macro use.

include/zephyr/usb/class/usb_midi.h Outdated Show resolved Hide resolved
include/zephyr/usb/class/usb_midi.h Outdated Show resolved Hide resolved
include/zephyr/usb/class/usb_midi.h Outdated Show resolved Hide resolved
include/zephyr/usb/class/usb_midi.h Outdated Show resolved Hide resolved
include/zephyr/usb/class/usb_midi.h Outdated Show resolved Hide resolved
include/zephyr/usb/class/usb_midi.h Outdated Show resolved Hide resolved
tmon-nordic
tmon-nordic previously approved these changes Nov 26, 2024
@kartben
Copy link
Collaborator

kartben commented Nov 26, 2024

@titouanc wondering if you missed some of my comments regarding the code sample? Thanks!

@titouanc
Copy link
Contributor Author

Thank you very much @jfischer-no for your review and suggestions !

I just applied all the trivial and "cosmetic" code changes (renames, moving things around, code layout etc...).

I also cleaned up the descriptors definitions which should be much clearer now (that part of the code was inspired by earlier experiments with the old usb device stack; where everything had to be packed together). While reworking them, I found an issue in the legacy usb-midi1.0 class-specific interface descriptor, where .wTotalLength was mistakenly accounting for the size of both the FS and HS endpoints. This is now fixed, this may fix the sample in your case ?

I will shortly apply fixes for the remaining open issues (invalid endpoint id, missing interface association descriptor), otherwise see comments elsewhere.

@stemschmidt
Copy link

@titouanc: Thanks for this extension! I build the sample application for a adafruit_feather_nrf52840 board and it worked!

Do you know how to build the midi extension in a C++ project?

I am having issues with the "static 4" e.g. in
int usbd_midi_send(const struct device *dev, const uint32_t ump[static 4]);

I get this error:
usbd_midi.h:35:65: error: expected primary-expression before 'static'
35 | int usbd_midi_send(const struct device *dev, const uint32_t ump[static 4]);

@titouanc
Copy link
Contributor Author

Thank you very much for testing this @stemschmidt !

( 😜 un)fortunately I don't do C++; so I don't know the best way to get away with this. Here are some elements though:

  • According to Wikipedia

    Array parameter qualifiers in functions are supported in C but not C++

  • If it is expected for such Zephyr APIs to be usable from both C and C++; we may simply remove the static qualifier from the array size
  • If we want to preserve the static part for enhanced compile-time checks in C; we may provide alternate definitions, wrapped in #ifdef __cplusplus

@jfischer-no do you have an opinion ?

@titouanc titouanc force-pushed the add-usb-midi2 branch 3 times, most recently from f59d7f8 to c435973 Compare December 31, 2024 16:29
dts/bindings/usb/zephyr,usb-midi.yaml Outdated Show resolved Hide resolved
dts/bindings/usb/zephyr,usb-midi.yaml Outdated Show resolved Hide resolved
dts/bindings/usb/zephyr,usb-midi.yaml Show resolved Hide resolved
kartben
kartben previously approved these changes Jan 7, 2025
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refreshing +1 for docs/sample - thanks!

subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
ret = usbd_ep_enqueue(data->class_data, buf);
if (ret) {
LOG_ERR("Failed to enqueue Tx net_buf -> %d", ret);
net_buf_unref(buf);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may not be bad idea adding an explicit return here, avoid a bug if code gets added below

subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
Comment on lines +119 to +123
/* Empty MidiStreaming 1.0 on altsetting 0 */
struct usb_if_descriptor if1_0_std;
struct usb_midi_header_descriptor if1_0_ms_header;
struct usb_ep_descriptor if1_0_out_ep_fs;
struct usb_ep_descriptor if1_0_out_ep_hs;
struct usb_midi_cs_endpoint_descriptor if1_0_cs_out_ep;
struct usb_ep_descriptor if1_0_in_ep_fs;
struct usb_ep_descriptor if1_0_in_ep_hs;
struct usb_midi_cs_endpoint_descriptor if1_0_cs_in_ep;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work. I spent a couple of hours looking at your implementation, the Linux kernel implementation, and the specification. I did not find anything in the specification that says you can skip the MIDI1 interface implementation. Rather, you are required to provide a valid interface implementation for backward compatibility. Here should be the MIDI1 Streaming Interface Descriptor, with all the endpoint and jack descriptors. There is an example at the end of the specification. You might also want to take a look at linux/drivers/usb/gadget/function/f_midi2.c.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this wouldn't work ?

From what I understand, Linux attempts to probe a USB-MIDI2 interface and falls back to USB-MIDI1 if such a valid interface does not exit (https://github.com/torvalds/linux/blob/v6.12/sound/usb/midi2.c#L1075-L1105). Moreover, I can tell that the sample from this PR is working with both Linux & Mac OS hosts for me. Other reviewers reported that the USB-MIDI stack works for them too.

From the USB-MIDI2 spec, 3.1.1 MIDI Streaming Interface with Two Alternate Settings: Backward Compatibility:

The first MIDI Streaming Interface exposed by the Device should contain an Alternate
Setting 0 which is compliant with USB Device Class Specification for MIDI Devices
Version 1.0. In other words, if Host software selects the first indexed Alternate Setting on
the MIDI Streaming Interface, the Device should expose a MIDI Function that is compliant
with USB MIDI 1.0. The bcdMSC field in the Class-Specific MIDI Streaming Interface
Header shall be set to 0x0100. This requirement allows the MIDI Function to interoperate
with existing Hosts exactly the same as current legacy MIDI 1.0 Functions.
The first MIDI Streaming Interface exposed by the Device should contain an Alternate
Setting 1 which contains the preferred device implementation which conforms to the new
design defined by this USB MIDI 2.0 specification. The bcdMSC field in the Class-Specific
MIDI Streaming Interface Header shall be set to 0x0200. Further Alternate Settings which
conform to the new design defined by this USB MIDI 2.0 specification may be included in
Alternate Setting 2 or higher.

Nothing says that these 2 altsettings should describe the same topology (and actually they couldn't because elements do not exist in USB-MIDI2, or MIDI2 only groups cannot be represented in USB-MIDI1). Therefore in this implementation, the altsetting 0 contains a perfectly valid empty Midistreaming interface, that doesn't contain any jack bound to its endpoints. Therefore, there is no MIDI port for the host to communicate through, hence no need to implement anything further.

I do see value in a fully fledged USB-MIDI1 implementation, bound to the altsetting 0. However i see this as an extra feature, because it requires an additional layer to translate UMPs into the "old" usb-midi1 format and the other way around. I will gladly do this in a following PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not find anything in the specification that says you can skip the MIDI1 interface implementation. Rather, you are required to provide a valid interface implementation for backward compatibility.

It is not required. It seems that you skipped the "1.5 Reserved Words and Specification Conformance" the word "should" is reserved for "Statements of recommendation" which are "Recommended but not mandatory. An implementation that does not conform to some or all ‘should’ statements is still conformant, providing all ’shall’ statements are conformed to."

All the MIDI1 references are with "should" and therefore the compliant implementation can skip it.

subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
Comment on lines +445 to +448
if (data->altsetting != ALT_USB_MIDI_2) {
LOG_WRN("MIDI2.0 is not enabled");
return -EIO;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I do not want to abuse it that much, there is a MIDI driver API, if something like that is needed it should be implemented there.

subsys/usb/device_next/class/usbd_midi.c Outdated Show resolved Hide resolved
@titouanc titouanc force-pushed the add-usb-midi2 branch 2 times, most recently from 1baf8f1 to 0c942d5 Compare January 10, 2025 08:52
This adds a new USB device class (based on usb/device_next) that implements
revision 2.0 of the MIDIStreaming interface, a sub-class of the USB audio
device class. In practice, the MIDI interface is much more simple and has
little in common with Audio, so it makes sense to have it as a separate
class driver.

MIDI inputs and outputs are configured through the device tree, under a
node `compatible = "zephyr,usb-midi"`. As per the USB-MIDI2.0 spec,
a single usb-midi interface can convey up to 16 Universal MIDI groups,
comprising 16 channels each. Data is carried from/to the host via
so-called Group Terminals, that are organized in Group Terminal Blocks.
They are represented as children of the usb-midi interface in the device
tree.

From the Zephyr application programmer perspective, MIDI data is exchanged
with the host through the device associated with the `zephyr,usb-midi`
interface, using the following API:

* Send a Universal MIDI Packet to the host: `usb_midi_send(device, pkt)`
* Universal MIDI Packets from the host are delivered to the function passed
  in `usb_midi_set_callback(device, void (device, pkt){...})`

Compliant USB-MIDI 2.0 devices are required to expose a USB-MIDI1.0
interface as alt setting 0, and the 2.0 interface on alt setting 1.
To avoid the extra complexity of generating backward compatible USB
descriptors and translating Universal MIDI Packets from/to the old
USB-MIDI1.0 format, this driver generates an empty MIDI1.0 interface
(without any input/output); and therefore will only be able to exchange
MIDI data when the host has explicitely enabled MIDI2.0 (alt setting 1).

This implementation is based on the following documents, which are referred
to in the inline comments:

* `midi20`: [Universal Serial Bus Device Class Definition for MIDI Devices Release 2.0](https://www.usb.org/sites/default/files/USB%20MIDI%20v2_0.pdf)
* `ump112`: [Universal MIDI Packet (UMP) Format and MIDI 2.0 Protocol With MIDI 1.0 Protocol in UMP Format _Document Version 1.1.2_](https://midi.org/universal-midi-packet-ump-and-midi-2-0-protocol-specification)

Signed-off-by: Titouan Christophe <[email protected]>
Add a sample application that demonstrates how to use the new USB-MIDI 2.0
device class. This shows how to set up the device tree, how to exchange
MIDI data with the host, and how to use the data accessors provided by
the new API.

Signed-off-by: Titouan Christophe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants