Merge pull request #525 from microsoft/pete-dev

Multiple Canary bug fixes, plus tweak to MIDI 1 port names

src/api/Client/WinMM/MidiSrvPort.h

@@ -66,6 +66,22 @@
                     status >= MIDI_NOTEOFF && \
                     status < MIDI_SYSEX)
 
+#define MIDI_BYTE_IS_STATUS_BYTE(b) (\
+                    (b & MIDI_STATUSBYTEFILTER) == MIDI_STATUSBYTEFILTER)
+
+#define MIDI_BYTE_IS_SYSTEM_REALTIME_STATUS(b) (\
+                    (b & MIDI_SYSTEM_REALTIME_FILTER) == MIDI_SYSTEM_REALTIME_FILTER)
+
+#define MIDI_BYTE_IS_SYSEX_START_STATUS(b) (\
+                    b == MIDI_SYSEX)
+
+#define MIDI_BYTE_IS_SYSEX_END_STATUS(b) (\
+                    b == MIDI_EOX)
+
+#define MIDI_BYTE_IS_DATA_BYTE(b) (\
+                    (b &  MIDI_STATUSBYTEFILTER) == 0)
+
+
 
 class CMidiPort :
     public Microsoft::WRL::RuntimeClass<

src/api/Libs/MidiKs/MidiKs.cpp

@@ -280,12 +280,16 @@ KSMidiOutDevice::SendMidiMessage(
 
     if (m_CrossProcessMidiPump)
     {
-        return m_CrossProcessMidiPump->SendMidiMessage(midiData, length, position);
+        auto hr = m_CrossProcessMidiPump->SendMidiMessage(midiData, length, position);
+        LOG_IF_FAILED(hr);
+        return hr;
     }
     else
     {
         // using standard, write via standard streaming ioctls
-        return WritePacketMidiData(midiData, length, position);
+        auto hr = WritePacketMidiData(midiData, length, position);
+        LOG_IF_FAILED(hr);
+        return hr;
     }
 }
 

src/api/Libs/MidiXProc/MidiXProc.cpp

@@ -25,6 +25,9 @@
 #include "MidiDefs.h"
 #include "MidiXProc.h"
 
+// infinite timeouts when waiting for read events cause the server to just hang
+#define MIDI_XPROC_BUFFER_FULL_WAIT_TIMEOUT 5000
+
 _Use_decl_annotations_
 HRESULT
 GetRequiredBufferSize(ULONG& requestedSize
@@ -397,13 +400,17 @@ CMidiXProc::SendMidiMessage(
             // Buffer is full, wait for the client to read some data out of the buffer to
             // make space
             HANDLE handles[] = { m_ThreadTerminateEvent.get(), m_MidiOut->ReadEvent.get() };
-            DWORD ret = WaitForMultipleObjects(ARRAYSIZE(handles), handles, FALSE, INFINITE);
+            //DWORD ret = WaitForMultipleObjects(ARRAYSIZE(handles), handles, FALSE, INFINITE);
+            DWORD ret = WaitForMultipleObjects(ARRAYSIZE(handles), handles, FALSE, MIDI_XPROC_BUFFER_FULL_WAIT_TIMEOUT);
             if (ret == (WAIT_OBJECT_0 + 1))
             {
                 // space has been made, try again.
-                retry = 1;
+                retry = true;
                 continue;
             }
+
+            // couldn't find room. 
+
         }
     }while (retry);
 

src/api/Midi2.sln

@@ -114,7 +114,7 @@ Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "Midi2.VirtualMidiTransport"
 		{EFB7CF90-7DEF-44CF-868A-191CA30E0FCF} = {EFB7CF90-7DEF-44CF-868A-191CA30E0FCF}
 	EndProjectSection
 EndProject
-Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Endpoint Transport Transports", "Endpoint Transport Transports", "{69CC5CD9-47DC-4118-A3C7-E0D071407185}"
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Endpoint Transports", "Endpoint Transports", "{69CC5CD9-47DC-4118-A3C7-E0D071407185}"
 EndProject
 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Drivers", "Drivers", "{DDB8BEE6-BEDC-4AE8-8E61-67EE655E35EF}"
 EndProject

src/api/Service/Exe/MidiDeviceManager.cpp

@@ -1489,11 +1489,11 @@ CMidiDeviceManager::UpdateEndpointProperties
                 { PKEY_MIDI_CustomPortNumber, false, 0, 0 },
                 { PKEY_MIDI_CustomEndpointName, false, 0, 0 },
                 { PKEY_MIDI_GroupTerminalBlocks, false, 0, 0 },
-                { PKEY_MIDI_FunctionBlockDeclaredCount, false, 0, 0 },
+              /*  {PKEY_MIDI_FunctionBlockDeclaredCount, false, 0, 0},
                 { PKEY_MIDI_FunctionBlocksAreStatic, false, 0, 0 },
                 { PKEY_MIDI_FunctionBlocksLastUpdateTime, false, 0, 0 },
                 { PKEY_MIDI_FunctionBlock00, true, PKEY_MIDI_FunctionBlock00.pid, PKEY_MIDI_FunctionBlock31.pid },
-                { PKEY_MIDI_FunctionBlockName00, true, PKEY_MIDI_FunctionBlockName00.pid, PKEY_MIDI_FunctionBlockName31.pid },
+                { PKEY_MIDI_FunctionBlockName00, true, PKEY_MIDI_FunctionBlockName00.pid, PKEY_MIDI_FunctionBlockName31.pid }, */
                 { PKEY_MIDI_EndpointDiscoveryProcessComplete, false, 0, 0 },
                 };
 
@@ -2529,15 +2529,31 @@ CMidiDeviceManager::SyncMidi1Ports(
                     friendlyName = baseFriendlyName;
                 }
 
-                // append the flow(Input or Output) & group number (group index + 1) to the friendly name.
-                if (MidiFlowIn == (MidiFlow) flow)
+
+                // if the device is a new MIDI 2 device, we'll add the group number. Otherwise, we'll leave
+                // off the differentiator. This is in testing with community. If you still see this in 
+                // after the Canary period ends, then this was preferred :)
+                //
+                // The next step if this doesn't work is to see if the name is already used, and then
+                // add the differentiator only in that case.
+
+                if (nativeDataFormat == MidiDataFormats::MidiDataFormats_UMP)
                 {
-                    friendlyName = friendlyName + L" I-" + std::to_wstring(groupIndex+1);
+                    // append the flow(Input or Output) & group number (group index + 1) to the friendly name.
+                    if (MidiFlowIn == (MidiFlow)flow)
+                    {
+                        friendlyName = friendlyName + L" I-" + std::to_wstring(groupIndex + 1);
+                    }
+                    else
+                    {
+                        friendlyName = friendlyName + L" O-" + std::to_wstring(groupIndex + 1);
+                    }
                 }
                 else
                 {
-                    friendlyName = friendlyName + L" O-" + std::to_wstring(groupIndex+1);
+                    // native bytestream port. Leaving off the differentiator for now per Canary note above.
                 }
+
                 interfaceProperties.push_back(DEVPROPERTY{ {DEVPKEY_DeviceInterface_FriendlyName, DEVPROP_STORE_SYSTEM, nullptr},
                     DEVPROP_TYPE_STRING, (ULONG)(sizeof(wchar_t) * (wcslen(friendlyName.c_str()) + 1)), (PVOID)friendlyName.c_str() });
 

src/api/Service/Exe/MidiDevicePipe.cpp

@@ -224,15 +224,6 @@ CMidiDevicePipe::SendMidiMessage(
     LONGLONG timestamp
 )
 {
-    // TODO: 
-    // Figure out how to most efficiently handle real-time messages
-    // - Should we allow them to be scheduled?
-    // - Should they bypass plugins?
-    // Run message through plugins
-    // - The plugins may produce additional messages, or delete the message
-    // - If device is currently in the middle of SysEx7, we will want to park messages that aren't allowed to interrupt that
-    // - After plugin processing, it goes to the scheduler
-
     // TODO: What happens with outgoing JR clock/timestamp messages? They can't take a different path
     // from the message they are attached to. If they follow the same path, aren't excepted, and have
     // the right timestamp order, they should be ok, but that's a "should" not a "will".

src/api/Service/Exe/MidiEndpointProtocolManager.cpp

@@ -45,7 +45,8 @@ CMidiEndpointProtocolManager::Initialize(
     }
     else
     {
-        m_discoveryAndProtocolNegotiationEnabled = false;
+        // default discovery to enabled if the reg key is inaccessible in some way
+        m_discoveryAndProtocolNegotiationEnabled = true;
     }
 
     // we only spin this all up if negotiation is enabled

src/api/Service/Exe/MidiEndpointProtocolWorker.cpp

@@ -245,7 +245,7 @@ CMidiEndpointProtocolWorker::Start(
         LOG_IF_FAILED(RequestAllEndpointDiscoveryInformation());
 
         // hang out until all info comes in or we time out
-        DWORD timeoutMilliseconds{ 15000 };
+        DWORD timeoutMilliseconds{ 20000 };
         m_allInitialDiscoveryAndNegotiationMessagesReceived.wait(timeoutMilliseconds);
 
         RETURN_IF_FAILED(UpdateAllFunctionBlockPropertiesIfComplete());
@@ -286,7 +286,7 @@ HRESULT
 CMidiEndpointProtocolWorker::CheckIfDiscoveryComplete()
 {
     if (m_taskEndpointInfoReceived &&
-        m_taskFinalStreamNegotiationResponseReceived &&
+        /*m_taskFinalStreamNegotiationResponseReceived && */
         m_taskEndpointNameReceived &&
         m_taskEndpointProductInstanceIdReceived &&
         m_taskDeviceIdentityReceived &&
@@ -625,12 +625,12 @@ CMidiEndpointProtocolWorker::ProcessFunctionBlockNameNotificationMessage(interna
         TraceLoggingWideString(m_endpointDeviceInterfaceId.c_str(), MIDI_TRACE_EVENT_DEVICE_SWD_ID_FIELD)
     );
 
-    if (!m_inInitialFunctionBlockDiscovery && m_functionBlocksAreStatic)
-    {
-        // we reject this function block because the initial ones were declared static,
-        // meaning no changes are allowed
-        return S_OK;
-    }
+    //if (!m_inInitialFunctionBlockDiscovery && m_functionBlocksAreStatic)
+    //{
+    //    // we reject this function block because the initial ones were declared static,
+    //    // meaning no changes are allowed
+    //    return S_OK;
+    //}
 
     uint8_t functionBlockNumber = MIDIWORDBYTE3(ump.word0);
 
@@ -1355,10 +1355,10 @@ CMidiEndpointProtocolWorker::UpdateAllFunctionBlockPropertiesIfComplete()
 
     // we only check blocks, not names, because names are optional
     // this may lead to a bit of a race, but it's the best we can do
-    if (m_functionBlocks.size() != m_declaredFunctionBlockCount)
-    {
-        return S_OK;
-    }
+    //if (m_functionBlocks.size() != m_declaredFunctionBlockCount)
+    //{
+    //    return S_OK;
+    //}
 
     // this is for efficiency during initial discovery. This function is called when all 
     // function blocks have been received. It's possible not all names have been received
@@ -1391,7 +1391,7 @@ CMidiEndpointProtocolWorker::UpdateAllFunctionBlockPropertiesIfComplete()
             else
             {
                 props.push_back({{ FunctionBlockNamePropertyKeyFromNumber(fbname.first), DEVPROP_STORE_SYSTEM, nullptr},
-                DEVPROP_TYPE_EMPTY, 0, nullptr });
+                    DEVPROP_TYPE_EMPTY, 0, nullptr });
             }
         }
     }

src/api/Test/Midi2.Transform.unittests/LibMidi2Tests.cpp

@@ -13,6 +13,7 @@
 
 #include "LibMidi2Tests.h"
 #include "libmidi2\bytestreamToUMP.h"
+#include "libmidi2\umpToBytestream.h"
 
 _Use_decl_annotations_
 void LibMidi2Tests::InternalTestSysEx(
@@ -69,6 +70,66 @@ void LibMidi2Tests::InternalTestSysEx(
     VERIFY_ARE_EQUAL(expectedWords.size(), countWordsCreated);
 }
 
+_Use_decl_annotations_
+void LibMidi2Tests::InternalTestSysExFromBytes(
+    std::vector<uint32_t> messageWords,
+    std::vector<uint8_t> const expectedBytes)
+{
+    umpToBytestream ump2bs{};
+
+    std::vector<uint8_t> generatedBytes;
+    generatedBytes.reserve(expectedBytes.size());
+
+    std::cout << "bytes created: " << std::endl;
+
+    uint16_t generatedBytesIndex{ 0 };
+    uint32_t countBytesCreated{ 0 };
+
+    std::cout << std::endl;
+
+    for (uint32_t i = 0; i < messageWords.size(); i++)
+    {
+        std::cout << std::setfill('0') << std::setw(8) << std::hex << messageWords[i] << " ";
+
+        ump2bs.UMPStreamParse(messageWords[i]);
+
+        if (ump2bs.availableBS())
+        {
+            std::cout << std::endl;
+
+            while (ump2bs.availableBS())
+            {
+                auto b = ump2bs.readBS();
+                countBytesCreated++;
+
+                if (countBytesCreated <= expectedBytes.size())
+                {
+                    std::cout
+                        << std::setfill('0') << std::setw(2) << std::hex << (uint16_t)b
+                        << "(" << std::setfill('0') << std::setw(2) << std::hex << (uint16_t)(expectedBytes[generatedBytesIndex]) << ") ";
+
+                    VERIFY_ARE_EQUAL(expectedBytes[generatedBytesIndex], b);
+
+                    generatedBytesIndex++;
+                }
+                else
+                {
+                    std::cout << std::endl;
+                    VERIFY_FAIL();
+                }
+            }
+
+            std::cout << std::endl;
+        }
+    }
+    //std::cout << std::endl << std::endl;
+
+    std::cout << std::endl;
+
+    VERIFY_ARE_EQUAL(expectedBytes.size(), countBytesCreated);
+}
+
+
 void LibMidi2Tests::TestTranslateFromBytesWithSysEx7()
 {
     const uint8_t sysexBytes[] = { 
@@ -109,18 +170,18 @@ void LibMidi2Tests::TestTranslateFromBytesWithEmbeddedRealTimeAndSysEx7()
 {
     const uint8_t sysexBytes[] =
     {
-        0xf0, 
+        0xf0,
         0x0a, 0x0b, 0x0c, 0x0d, 0x0f,           // 5-data-byte sysex message
         0xF8,                                   // real-time clock. because this arrives before previous message created, this ends up first
         0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f,     // 6-data-byte sysex message
-        0xf7,       
+        0xf7,
     };
 
     std::vector<uint32_t> expectedWords
     {
         0x10f80000,                 // RT gets moved to first because full sysex message not yet generated
         0x30160a0b, 0x0c0d0f2a,
-        0x30352b2c, 0x2d2e2f00 
+        0x30352b2c, 0x2d2e2f00
     };
 
     InternalTestSysEx(0, sysexBytes, _countof(sysexBytes), expectedWords);
@@ -141,7 +202,7 @@ void LibMidi2Tests::TestTranslateFromBytesWithEmbeddedStartStopSysEx7()
 
 
     std::vector<uint32_t> expectedWords
-    { 
+    {
         0x30160a0b, 0x0c0d0e1a,   0x30351b1c, 0x1d1e1f00,
         0x30162a2b, 0x2c2d2e3a,   0x30353b3c, 0x3d3e3f00,
         0x30054a4b, 0x4c4d4e00,
@@ -195,3 +256,71 @@ void LibMidi2Tests::TestTranslateFromBytesWithShortSysEx7()
 
     InternalTestSysEx(0, sysexBytes, _countof(sysexBytes), expectedWords);
 }
+
+
+
+void LibMidi2Tests::TestTranslateToBytesWithSysEx7()
+{
+    std::vector<uint32_t> input =
+    {
+        0x30010100, 0x00000000,
+        0x30160801, 0x02030405,
+        0x30320607, 0x00000000
+    };
+
+    std::vector<uint8_t> expectedOutput =
+    {
+        0xF0, 0x01, 0xF7,
+        0xF0, 0x08, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0xF7
+    };
+
+    InternalTestSysExFromBytes(input, expectedOutput);
+
+}
+
+void LibMidi2Tests::TestTranslateToBytesWithInterruptedSysEx7()
+{
+    std::vector<uint32_t> input =
+    {
+        0x30010100, 0x00000000,
+        0x30160801, 0x02030405,
+        0x10FE0000,
+        0x30320607, 0x00000000
+    };
+
+    std::vector<uint8_t> expectedOutput =
+    {
+        0xF0, 0x01, 0xF7,
+        0xF0, 0x08, 0x01, 0x02, 0x03, 0x04, 0x05, 
+        0xFE, 
+        0x06, 0x07, 0xF7
+    };
+
+    InternalTestSysExFromBytes(input, expectedOutput);
+}
+
+void LibMidi2Tests::TestTranslateToBytesWithCanceledSysEx7()
+{
+    std::vector<uint32_t> input =
+    {
+        0x30010100, 0x00000000,
+        0x30160801, 0x02030405,
+        0x20905050,
+        0x20806060,
+        0x30320607, 0x00000000
+    };
+
+    // verifying this behavior stays consistent. LibMidi2 will pick up the rest
+    // of the sysex, and will just embed the note on/off messages inside it
+    std::vector<uint8_t> expectedOutput =
+    {
+        0xF0, 0x01, 0xF7,
+        0xF0, 0x08, 0x01, 0x02, 0x03, 0x04, 0x05,
+        0x90, 0x50, 0x50,   // note off
+        0x80, 0x60, 0x60,   // note on
+        0x06, 0x07, 0xF7    // rest of sysex, including F7
+    };
+
+    InternalTestSysExFromBytes(input, expectedOutput);
+
+}