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

open port by name on windows #21

Closed
ddiakopoulos opened this issue Sep 7, 2013 · 16 comments · Fixed by #22
Closed

open port by name on windows #21

ddiakopoulos opened this issue Sep 7, 2013 · 16 comments · Fixed by #22

Comments

@ddiakopoulos
Copy link

Didn't have a chance to dig into this, but on Windows 7 I couldn't get ports to be opened by their name.

bool ofxRtMidiOut::openPort(string deviceName)

The equality operator was failing on the string comparison in this method. My hacked fix was to convert them to c style strings and use strcmp.

@danomatika
Copy link
Owner

Are you absolutely sure? That's really weird and sounds like a problem with RtMidi which is supposedly returning a C++ string.

Maybe there's a stray endline char in there or something ...

@timmb
Copy link

timmb commented Sep 12, 2013

Hi - just came across this issue as well. It does indeed seem to be a bug with rtMidi. It is somehow inserting a NULL into the middle of the string, which creates an invalid std::string object.

My MIDI device is called "loopMidi Port" and rtMidi should report it as "loopMidi Port 0". However, the actual string it returns (in hex) is this:
6c 6f 6f 70 4d 49 44 49 20 50 6f 72 74 0 20 30 0

As you can see, there is a terminator (zero) after the 74 (the t in "Port") as well as after the " 0" (20 30).

@danomatika
Copy link
Owner

Maybe it's something in the RtMidiOut Windows Unicode conversion. Can you try commenting out some lines starting here: https://github.com/danomatika/ofxMidi/blob/master/libs/rtmidi/RtMidi.cpp#L2210

So this:

#if defined( UNICODE ) || defined( _UNICODE )
  int length = WideCharToMultiByte(CP_UTF8, 0, deviceCaps.szPname, -1, NULL, 0, NULL, NULL);
  stringName.assign( length, 0 );
  length = WideCharToMultiByte(CP_UTF8, 0, deviceCaps.szPname, wcslen(deviceCaps.szPname), &stringName[0], length, NULL, NULL);
#else
  stringName = std::string( deviceCaps.szPname );
#endif

Becomes this:

//#if defined( UNICODE ) || defined( _UNICODE )
//  int length = WideCharToMultiByte(CP_UTF8, 0, deviceCaps.szPname, -1, NULL, 0, NULL, NULL);
//  stringName.assign( length, 0 );
//  length = WideCharToMultiByte(CP_UTF8, 0, deviceCaps.szPname, wcslen(deviceCaps.szPname), &stringName[0], length, NULL, NULL);
//#else
  stringName = std::string( deviceCaps.szPname );
//#endif

I have a feeling the assign(length, 0); is the culprit ...

@danomatika
Copy link
Owner

Also, does "loopMidi Port 1" etc return ok? Maybe the numbers are coming through in their decimal values instead of ascii chars aka 0 as 0 instead of 48.

@timmb
Copy link

timmb commented Sep 13, 2013

Sorry just realised you're talking about Midi out whereas I'm only using Midi in. The issue appears to be the same, however there are some differences in how the function is implemented

std::string MidiInWinMM :: getPortName( unsigned int portNumber )
{
  std::string stringName;
  unsigned int nDevices = midiInGetNumDevs();
  if ( portNumber >= nDevices ) {
    std::ostringstream ost;
    ost << "MidiInWinMM::getPortName: the 'portNumber' argument (" << portNumber << ") is invalid.";
    errorString_ = ost.str();
    //RtMidi::error( RtError::INVALID_PARAMETER, errorString_ );
    RtMidi::error( RtError::WARNING, errorString_ );
    return stringName;
  }

  MIDIINCAPS deviceCaps;
  midiInGetDevCaps( portNumber, &deviceCaps, sizeof(MIDIINCAPS));

#if defined( UNICODE ) || defined( _UNICODE )
  int length = WideCharToMultiByte(CP_UTF8, 0, deviceCaps.szPname, -1, NULL, 0, NULL, NULL);
  stringName.assign( length, 0 );
  length = WideCharToMultiByte(CP_UTF8, 0, deviceCaps.szPname, wcslen(deviceCaps.szPname), &stringName[0], length, NULL, NULL);
#else
  stringName = std::string( deviceCaps.szPname );
#endif

  // Next lines added to add the portNumber to the name so that 
  // the device's names are sure to be listed with individual names
  // even when they have the same brand name
  std::ostringstream os;
  os << " ";
  os << portNumber;
  stringName += os.str();

  return stringName;
}
  • Removing the wide-char conversion causes a compile error:

    ...\openframeworks\addons\ofxmidi\libs\rtmidi\rtmidi.cpp(2140): error C2440: '' : cannot convert from 'WCHAR [32]' to 'std::string'

I think I've found the source of the bug though, and it arises through a misuse of the std::string::assign function.

WideCharToMultiByte returns the number of bytes required to store the string it will produce as a C string. string::assign produces a string with the given number of characters, not including the null terminator at the end. Therefore the line stringName.assign(length, 0) creates a std::string with length null characters onto which std::string then appends its own null character. In other words, both WideCharToMultiByte and string::assign append a null terminator. This causes stringName to have an incorrect length.

When stringName is then streamed into std::ostringstream, this additional null character is fed into the stream, presumably as the << operator is defined to use std::string's length rather than check whether each character is null.

Down the line this causes problems because comparing two c-strings for equality will check up until the first null character is found, whereas comparing two std::strings for equality will compare all characters up until the length of the string.

So... the bug is easily fixed - simply change line 2136 from

int length = WideCharToMultiByte(CP_UTF8, 0, deviceCaps.szPname, -1, NULL, 0, NULL, NULL);

to

int length = WideCharToMultiByte(CP_UTF8, 0, deviceCaps.szPname, -1, NULL, 0, NULL, NULL) - 1;

I've just submitted a PR to do that.

@danomatika
Copy link
Owner

Thanks! I figured was something simple like that. I'll forward this upstream to the RtMidi Developer.

Also, have you tried the Windows Kernel Streaming api as well? It's supposed to have better performance and I'd like to be sure it doesn't have the same problem, although from looking at the getPortName impelmentations it looks ok (no unicode conversions).

Change the define on this line:

https://github.com/danomatika/ofxMidi/blob/master/src/ofxMidiConstants.h#L21

to

__WINDOWS_KS__

Thanks. If the KS driver works better, I'll change it be the default driver on Win.

@moebiussurfing
Copy link

hey, I am having this same problem. I want to fix the midi in port selection by name, because sometimes it changes his number position.

But: the position number appears listed too in the port name.

I can connect by name:
midiIn.openPort("loopMIDI Port 1 7");

or number:
midiIn_CLOCK.openPort(7);

that's the listing:

[notice ] ofxMidiIn: 8 ports available
[notice ] ofxMidiIn: 0: Automap Propellerhead Mixer 0
[notice ] ofxMidiIn: 1: Automap Propellerhead 1
[notice ] ofxMidiIn: 2: Automap MIDI 2
[notice ] ofxMidiIn: 3: 3- ReMOTE SL Compact 3
[notice ] ofxMidiIn: 4: MIDIIN2 (3- ReMOTE SL Compact) 4
[notice ] ofxMidiIn: 5: MIDIIN3 (3- ReMOTE SL Compact) 5
[notice ] ofxMidiIn: 6: loopMIDI Port 6
[notice ] ofxMidiIn: 7: loopMIDI Port 1 7
[notice ]    connected to MIDI CLOCK IN port 7

Any chance to solve this?

@danomatika
Copy link
Owner

danomatika commented Aug 25, 2018 via email

@moebiussurfing
Copy link

moebiussurfing commented Aug 31, 2018

ok @danomatika, thanks
but the current RtMidi version is the same 3.0.0 (http://www.music.mcgill.ca/~gary/rtmidi/) than the one is inluded in your repo...
Do you mean an unstable branch?

@moebiussurfing
Copy link

i found this:
thestk/rtmidi#30

@moebiussurfing
Copy link

I cloned the repo https://github.com/thestk/rtmidi, and replaced the \openframeworks\addons\ofxMidi\libs\rtmidi
content files.
It compiles. I'll if still working my app; and I'll post some report.
thanks @danomatika !

@moebiussurfing
Copy link

moebiussurfing commented Aug 31, 2018

Hmmm nope. doesn't works like that, Maybe I should compile the sources, to modificate the ofxMidi or something... :(

MidiOutDummy: This class provides no functionality.
MidiInDummy: This class provides no functionality.
MidiOutDummy: This class provides no functionality.
MidiInDummy: This class provides no functionality.

i see the openframeworks\addons\ofxMidi\scripts, seems for the release states,,, but I dunno how to continue for the up to date branch repo.

@danomatika
Copy link
Owner

danomatika commented Sep 1, 2018 via email

@moebiussurfing
Copy link

ok. thanks! it worked adding the line.
Now using the RtMidi up to date repository
i'll check around the port naming and the port hanging issue. thanks again!

@moebiussurfing
Copy link

moebiussurfing commented Sep 1, 2018

It seems there's some changes to new RtMidi thestk/rtmidi#30 :

The API itself is also kept minimal:

getPortCount has its counterpart in getPortList
openPort has been complemented by an implementation that asks the port descriptor to generate a backend object
getPortDescriptor allows to get back the port descriptor from an open virtual port. This is used in the loopback test case to do real MIDI communication (except on Windows that does not support virtual devices).
The new function hasVirtualDevices has been added to query the existence of virtual devices so that the loopback device can shut down gracefully when the backend has no virtual devices.

Now the ports can be handled with something like port description (getPortDescriptor)
So I imagine there's some need to tweak around the ofxMidi...
Btw I don't know the priority of this changes or what other people thinks about the needs.

By using the same methods it works the same:
//midiOut.openPort("Automap MIDI 6"); // by name WORKS
//midiOut.openPort("Automap MIDI"); // by name DO NOT WORKS

@keinstein
Copy link

thestk/rtmidi#30 is not „new“ RtMidi, yet. Currently it is more or less a fork that proposes to solve the problems above. Reviews and discussions about the changes are very welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants