search

microsoft / MIDI

Windows MIDI Services
MIT License
301 stars 24 forks source link

MIDI.exe returns different GTB than returned via USB #269

Closed MusicMaker closed 1 month ago

MusicMaker commented 7 months ago

Describe the bug

To Reproduce

Expected behavior

Screenshots image Command: midi.exe endpoint properties
image GTB portion of from command: midi.exe endpoint properties --verbose image Reference; MIDI Workbench (on Linux) shows zero's (right), Linux tool output (left)

Installer Name or Version

Device information, if this is with an external MIDI device:

Additional context Please confirm what Windows uses for bmRequestType and wLength in the call to retrieve the descriptor ? Any possibility to publish any corrections/errata that should be in and updated USB MIDI 2.0 specification, even unofficial ?

Psychlist1972 commented 7 months ago

There's a discord thread on this here: https://discord.com/channels/980245825202552942/1198866304736702555

If Workbench on Linux is showing all zeroes, I would think something is incorrect here.

This needs investigation, as all the other MIDI devices I've tried here show correct GTBs.

Psychlist1972 commented 7 months ago

@AmeNote-Michael @MusicMaker and I have a long thread on this in discord and they've been debugging this quite a bit. We're not 100%, but looking at your driver code, it appears that you're asking for too much data here and ignoring the header size values.

In https://github.com/microsoft/MIDI/blob/main/src/api/Drivers/USBMIDI2/Driver/Device.cpp#L1343 you have a comment that you guess at the size/number of the GTBs. But you should be able to get that information from the header. Later at 1393, instead of using the bytes transferred to calculate the number of group terminal blocks, you should use the size you read from the header and allocate a buffer based on that. (this does change how the code is structured). I suspect the request that goes over requests enough to fill the entire buffer, which is why we get a bunch of garbage data after the GTBs.

Per the USB MIDI 2.0 spec:

wTotalLength is "Total number of bytes returned for the class-specific Group Terminal Block descriptors. Includes the combined length of this header descriptor and all Group Terminal Block descriptors. "

Not sure why this works with Roland, NI, and other devices I have, but it's not working with Muzak's code which does work with Linux which handles the header and GTBs as two requests with two separate buffers.

Psychlist1972 commented 7 months ago

Please confirm what Windows uses for bmRequestType and wLength

I suspect wLength is coming from the total buffer size that gets allocated and that's something our WDF code is doing on our behalf. But I would defer to others with more USB/WDF knowledge than I have. But it would make sense given that the allocated buffer is larger than needed, and we get a bunch of garbage data after your GTB.

bmRequestType I'm not sure of.

Psychlist1972 commented 7 months ago

@AmeNote-Michael just loaded beta Waldorf Iridium firmware and it runs into the same driver bug. So I have a test case here we can use to verify the fix.

image

AmeNote-Michael commented 7 months ago

Bug discovered in setting size of GTB when fetching. Testing needed to verify.

https://github.com/microsoft/MIDI/pull/278

Psychlist1972 commented 7 months ago

Fixed in latest driver source.

Psychlist1972 commented 4 months ago

Reopening until we ship to customers

AmeNote-Michael commented 3 months ago

Updated driver now uses index in Endpoints to determine if a GTB definition should be returned. This should provide the expected function.

AmeNote-Michael commented 2 months ago

@MusicMaker seems you are willing to use test signed drivers and do testing that way based on other posts. You also had raised this issue.

Can you please confirm this issue has been resolved with latest test signed driver.

Psychlist1972 commented 2 months ago

@MusicMaker seems you are willing to use test signed drivers and do testing that way based on other posts. You also had raised this issue.

Can you please confirm this issue has been resolved with latest test signed driver.

Unless you've shared it separately, the test signed driver has only been shared with AMEI, not with the public. I'm fine with sharing it with MusicMaker, but not otherwise publicly, because putting your PC into a mode to enable test signed drivers always causes a percentage of people to brick their PCs or end up in bitlocker recovery mode because they didn't follow all the steps. It's a very unforgiving process.

AmeNote-Michael commented 2 months ago

@Psychlist1972 of course you are correct.

@MusicMaker if you are willing to run your system in test signed mode for which I could send you a driver to test with, resolving this issue will need to wait until after Attestation signed driver which we are hoping will be within the next week.

MusicMaker commented 2 months ago

Sorry, I had been working on other issues and now catching up emails. Yes, I could try the test driver. I am not aware of using bit-locker on Canary builds, unless windows enables that.

MusicMaker commented 2 months ago

How to get hold of this updated test driver ? Is there a link to download it ?

Psychlist1972 commented 1 month ago

Please test with today's driver release, when you get a chance. Thanks!

Psychlist1972 commented 1 month ago

Closing this as fixed in the August 2024 driver release. If you test and find that not to be the case, we will reopen.

MusicMaker commented 1 month ago

Thanks a lot ! will go trough regression with the new driver.

MusicMaker commented 6 days ago

Regressed. the driver correctly respects bNumGrpTrmBlock now. It is also resilient having the wrong total length of all GTB data in the GTB header.