microsoft / MIDI

Windows MIDI Services
MIT License
301 stars 24 forks source link

[BUG]MT4 messages are not converted to Byte Stream MIDI #298

Closed masahirokakishita closed 2 months ago

masahirokakishita commented 6 months ago

Describe the bug Sending MT4 to USB-MIDI 1.0 Device, MT4 messages are not converted to Byte Stream MIDI.

To Reproduce The enclosed SevenColor4.uf2 is a USB-MIDI 1.0 Device. This is a Byte Stream MIDI device, not a UMP. SevenColor4.uf2.zip

  1. Install SevenColor4 to UUT and connect to PC.
  2. Update driver to USBMidi2
  3. Start two consoles
  4. On a consoles, this is a monitor for messages from SevenColor4.

    midi endpoint monitor --verbose

  5. There other console, this is a sender for MT4 message to SevenColor4.

    midi endpoint send-message 0x40904000 0xFFFF0000

No messages are displayed on first console.

Expected behavior

  1. Send MT2 message from second console.

    midi endpoint send-message 0x2090407F

First console displays MT2 message.

Installer Name or Version Developer Preview 5

Desktop (please complete the following information):

Psychlist1972 commented 6 months ago

I'm of two minds on translating MT4 to byte stream. That is not a clean conversion. It is lossy, and in the case of exact pitch and other MT4 variants, it simply can't be translated. The only clean translation is MT2 to byte stream.

Bremmers commented 6 months ago

I expect this to work. If it doesn't there'd be no output if an app sends the MIDI 2.0 protocol. One could argue an app shouldn't do that, but it would be asking for trouble.

FTR: Apple converts between the 1.0 and 2.0 protocols in both directions 'everywhere'.

Psychlist1972 commented 6 months ago

FTR: Apple converts between the 1.0 and 2.0 protocols in both directions 'everywhere'.

And there are problems with it, from my understanding from others. Chances are, the problems there will be different than the problems we will have, although there will be overlap.

There are edge cases here with things that will not translate, and more are being added in new profiles.

I will reconsider here given some of the cases brought up by AMEI (MIDI 2.0 controller, MIDI 1.0 device, for example) and comments from you and others, but this really goes against my gut. DAWs are the components with all the right context and able to make the right decisions for the user (or prompt them for what they want to do). That's why we provide all the metadata from the function blocks and group terminal blocks as well as information about the native data format of the endpoint.

Psychlist1972 commented 6 months ago

Looking at Andrew's AM_Lib, which we use for conversion internally in the service, it does appear to convert the most basic MT4 to byte stream, so it will work when not using the MIDI 2.0 driver. I know @AmeNote-Michael had reasons for not using AM_Lib in the driver, but I don't know them off the top of my head. (The lib may use types/functions not available in the subset of stdlib available to kernel drivers)

image

Note that it ignores attributes and things like pitch 7.9 and is not going to send valid data in those cases. The note index, which is specifically mentioned as not indicating pitch, will be treated like a note number and the result will not be accurate at all.

Honestly, MT2->MT4 just seems like it's going to cause no end of future problems and bug reports.

Bremmers commented 6 months ago

The use of attributes will typically be negotiated between sender and receiver I think? That would mean a sender won't send 7.9 pitch to a 1.0 destination. If the use of attributes isn't negotiated you can have the same problem with a 2.0 destination: the destination may ignore the 7.9 pitch attribute and the sender won't know.

Psychlist1972 commented 6 months ago

The use of attributes will typically be negotiated between sender and receiver I think? That would mean a sender won't send 7.9 pitch to a 1.0 destination. If the use of attributes isn't negotiated you can have the same problem with a 2.0 destination: the destination may ignore the 7.9 pitch attribute and the sender won't know.

Protocol (which dictates MT4 or MT2) is also negotiated, though.

Psychlist1972 commented 6 months ago

Update:

I've asked Andrew to update the AM lib to ignore any MT4 messages that have a non-zero attribute value.

Will also discuss with @AmeNote-Michael to see what can be done in the driver code here. I will then document in our developer docs that this is a condition for translating Note On/Off messages, and will also mention the lossy nature of the conversion.

Right now, this is for bytestream conversions only, not MT4->MT2. I'm still considering what, if anything, we will do there.

AmeNote-Michael commented 6 months ago

Looking at Andrew's AM_Lib, which we use for conversion internally in the service, it does appear to convert the most basic MT4 to byte stream, so it will work when not using the MIDI 2.0 driver. I know @AmeNote-Michael had reasons for not using AM_Lib in the driver, but I don't know them off the top of my head. (The lib may use types/functions not available in the subset of stdlib available to kernel drivers)

It does not make sense for the driver to use the AM_Lib, it is a redundant and wasteful step, especially considering decision that the driver is not to do any sort of translation (ie MT4 <-> MT2), but more on that in another thread.

So with AM_Lib, the data conversion would be:

USB MIDI 1.0 -> Byte Stream -> UMP with the Byte Stream to UMP being AM_Lib based. UMP -> Byte Stream -> USB MIDI 1.0 with the UMP to Byte Stream being AM_Lib based.

However, if you examine the UMP versus USB MIDI 1.0 payload formats closely, you will notice that they are very, very close in format except for SYSEX (MT3). Really all that is involved is the transposing of Virtual Cable Number <-> Group IDs and assigning CIN base on data type.

Using AM_Lib and thus adding the translation through Byte Stream adds complexity where issues can occur and also un-necessary memory and CPU cycle use.

masahirokakishita commented 6 months ago

I'm of two minds on translating MT4 to byte stream. That is not a clean conversion. It is lossy, and in the case of exact pitch and other MT4 variants, it simply can't be translated. The only clean translation is MT2 to byte stream.

Yes, I am aware that the information on MT4 will be reduced when MT4 is converted to byte stream MIDI. However, our current products need to sound with every application. . On the other hand, I must also consider the case where the MT4 device sends MT4 messages to a MIDI 1.0 current application. However, currently the UMP device is not visible from the MIDI 1.0 current application, so experiments cannot be performed.

AmeNote-Michael commented 6 months ago

In architecture discussions, it was decided that the driver would not be responsible for translating to and from non-MIDI1.0 data. Essentially this means that driver would only translate between MT1, MT2 and MT3 to MIDI 1.0 protocol. The MT4 to MT2 or any other translations from future Message types was to be handled by upper layers.

The main reasons are:

  1. Changes in translation requirements or errors discovered in translations (the more complex code) would require re-certification of driver - a lengthy process. By only handling MIDI 1.0 data in driver, it reduces integration complexity and establishes driver as minimum requirements - fixing the driver to a very manageable simplest form to be certified

  2. In event of future message types being defined that translate in some way to meaningful data for MIDI 1.0, no driver changes would be required which would also require re-certification

  3. The drivers (if created) for BLE MIDI, RTP MIDI, or other MIDI 1.0 transports could follow same rule, only transporting MT1, MT2 and MT3 - thus upper layer solutions could be reused for these drivers as well. Meaning that there are not multiple implementations of the same translations.

I feel this issue of MT4 <-> MT2 translations is NOT a USB MIDI2 Driver issue and should be managed by other methods.

@AmeNote-MikeKent can you also chime in on this thread please.

masahirokakishita commented 6 months ago

I attached one more uf2 file. This firmware is byte stream MIDI through program SevenColor1.uf2.zip

This program use only one cable number. In this case, midi.exe recognizes this device with the existing USB MIDI 1.0 driver.

open two console, one is a monitor.

midi endpoint monitor --verbose

The other is a sender.

midi endpoint send-message 0x40c00001 0x10203040

At the monitor, 0x20B00030 0x20B02040 0x20C01000 are displayed. In this case MT4 message can converted byte stream MIDI to the device and return. I think this behavior is good.

Psychlist1972 commented 6 months ago

I attached one more uf2 file. This firmware is byte stream MIDI through program SevenColor1.uf2.zip

This program use only one cable number. In this case, midi.exe recognizes this device with the existing USB MIDI 1.0 driver.

open two console, one is a monitor.

midi endpoint monitor --verbose

The other is a sender.

midi endpoint send-message 0x40c00001 0x10203040

At the monitor, 0x20B00030 0x20B02040 0x20C01000 are displayed. In this case MT4 message can converted byte stream MIDI to the device and return. I think this behavior is good.

I'm considering what we'll do here. Michael is correct in that having the driver do only the most correct change (MT1,2,3 to MIDI 1.0 data format (byte stream), is the best approach for the driver.

In cases where the new driver is not used (our old driver is used, or a third-party driver is used), the data format conversion inside the service kicks in. That one, using the AM MIDI 2 library, does the most basic MT4 to MT2 translation only for MIDI 1.0 devices. That conversion is not always clean because MT4 supports different ways of specifying note pitch and other data. In some cases, like exact pitch, the note number is supposed to be an index, not a note number. The library treats it as a note number, which means you will get a wrong note.

For MIDI 1.0 format:

Where we run into a problem is:

I will look into both of these.

In addition, we do no translation based on negotiated protocol or function block/group terminal block information.

Long-term, I'm concerned that this will grow into more and more translations. SysEx8 to SysEx7, for example. Or different registered controllers, etc. Each of these end up with message inspection or manipulation and potential performance impacts. Not all users will want these translations, and then we'll be asked to provide knobs in the OS to control them, further complicating the pipeline.

Psychlist1972 commented 5 months ago

This is fixed for the next developer preview. The implementation is in the Midi2.UmpProtocolDownscalerTransform.

Currently, the code is all bespoke in Midi2.UmpProtocolDownscalerMidiTransform.cpp. When AM_MIDI2 supports UMP->UMP downscaling, we'll change to using that.

Psychlist1972 commented 5 months ago

The cases when this downscaler come into play is really just one: outbound messages going to the new USB MIDI 2.0 UMP driver when the target is a MIDI 1.0 device.

Because of how it plugs in, this will also come into play if there are any UMP transports in the future which present a UMP endpoint to Windows MIDI Services, but actually have a MIDI 1.0 endpoint behind it.

Note that this is not needed for MIDI 1.0 devices using other transports (older MIDI 1.0 driver, vendor MIDI 1.0 drivers, BLE MIDI 1.0, etc.) because those go through a full translation in the service which already handles the downscaling.

Psychlist1972 commented 5 months ago

Fixed/completed in the upcoming Developer Preview 6.

Psychlist1972 commented 4 months ago

Opened until we ship to customers

sat-okada commented 2 months ago

@Psychlist1972 This issue was not resolved even with the latest driver and dp6.
When I send MT4 message, no messages are displayed on monitor console.

midi endpoint send-message 0x40904000 0xFFFF0000

sat-okada commented 2 months ago

In addition, I discovered another problem. When I tried monitor console without "--verbose", received messages were displayed. But the conversion result of the velocity value is incorrect. The conversion result of "0x40904000 0xFFFF0000" should be "0x2090407F", but the result is "0x20904001".