Implement a reliable port selection API (official pull request against winks branch)

[keinstein]

The current RtMidi implementation has a race condition that may be very annoying in certain cirumstances.

Steps to reproduce the problem:

1. add several virtual and/or usb devices
2. Start a RtMidi application with interactive port selection
3. After the Application presented the port list to the user, remove the removable device in the list from the system (delete or unplug it).
4. Select the a device that is below the unplugged device in the list.
5. RtMidi will fail or open the wrong device.

The current patch set contains a suggestion for a reliable API.
The main problem that shall be addressed is that the current API does not store port descriptors together with the meta data (port names/port numbers). Thus, both can get out of sync.

The patches are based both on master and winks include:

• Winks patches as well as reverting them to avoid problems synchronizing with the master branch.
• some code reorganization (extract common API into a separate class)
• Add a port descriptor class (With a more flexible port naming interface).
• Add port descriptor API: getPortList, openPort(PortDescriptor &, const std::string&), getPortDescriptor()
• Add Example programs.
• Add an automatic test case for all backends that support virtual devices. This has been inspired by the automatic loopback test case of the node-midi project.

I came across the following questions:
• What's the purpose of class RtMidi? Is it used directly?
• RtMidiIn and RtMidiOut serve as API containers. Why do you use virtual inline functions, here? I'd suggest to remove the virtual classifier from the member functions and declare them only in RtMidi if they are used both by RtMidiIn and RtMidiOut. This would render CommonMidiApi useless and I would reintegrate it into MidiApi. Otherwise the inline functions should be marked extern to avoid linker errors.
• There is no function call ot set or get the current API object from RtMidi(In|Out). This and the fact that the implementation classes Midi(In|Out)(Alsa|Jack|WinMM|WinKS|Core|Dummy) are not documented separately suggest that these classes are private to the RtMidi library. Is that right? In this case I'd suggest to remove these classes from RtMidi.h. Otherwise I'd suggest to add two functions to set and get the API object that is used by the RtMidi(In|Out).
• Maybe… probably… I might add a container API that allows to collect several APIs into one common API.

[keinstein]

When #30 is applied this one can be closed, too.

[garyscavone]

Hi Tobias,

I made a clone of the rtmidi repo and then checked out your branch (keinstein-portdescriptor-api). After running configure and typing “make,” I get the following compiler warnings and errors:

g++ -O3 -Wall -Wextra -Iinclude -fPIC -D__MACOSX_CORE__ -c RtMidi.cpp -o RtMidi.o
In file included from RtMidi.cpp:39:
./RtMidi.h:244:2: warning: 'PortDescriptor' defined as a struct here but previously
declared as a class [-Wmismatched-tags]
struct PortDescriptor {
^
./RtMidi.h:168:2: note: did you mean struct here?
class PortDescriptor;
^~~~~
struct
RtMidi.cpp:943:27: warning: implicit conversion of NULL constant to 'MIDIEntityRef'
(aka 'unsigned int') [-Wnull-conversion]
MIDIEntityRef entity = NULL;
~~~~~~ ^~~~
0
RtMidi.cpp:946:15: warning: comparison between NULL and non-pointer ('MIDIEntityRef'
(aka 'unsigned int') and NULL) [-Wnull-arithmetic]
if (entity != NULL) {
~~~~~~ ^ ~~~~
RtMidi.cpp:959:28: warning: implicit conversion of NULL constant to 'MIDIDeviceRef'
(aka 'unsigned int') [-Wnull-conversion]
MIDIDeviceRef device = NULL;
~~~~~~ ^~~~
0
RtMidi.cpp:961:16: warning: comparison between NULL and non-pointer ('MIDIDeviceRef'
(aka 'unsigned int') and NULL) [-Wnull-arithmetic]
if (device != NULL) {
~~~~~~ ^ ~~~~
RtMidi.cpp:1248:11: error: no matching constructor for initialization of 'rtmidi::Error'
throw Error(Error::DRIVER_ERROR,
^ ~~~~~~~~~~~~~~~~~~~~
./RtMidi.h:133:3: note: candidate constructor not viable: no known conversion from
'rtmidi::Error::Type' to 'const std::string' (aka 'const basic_string<char,
char_traits, allocator >') for 1st argument
Error( const std::string& message, Type type = Error::UNSPECIFIED ...
^
./RtMidi.h:114:8: note: candidate constructor (the implicit copy constructor) not viable:
requires 1 argument, but 2 were provided
class Error : public std::exception
^
RtMidi.cpp:1253:11: error: no matching constructor for initialization of 'rtmidi::Error'
throw Error( Error::DRIVER_ERROR,
^ ~~~~~~~~~~~~~~~~~~~~
./RtMidi.h:133:3: note: candidate constructor not viable: no known conversion from
'rtmidi::Error::Type' to 'const std::string' (aka 'const basic_string<char,
char_traits, allocator >') for 1st argument
Error( const std::string& message, Type type = Error::UNSPECIFIED ...
^
./RtMidi.h:114:8: note: candidate constructor (the implicit copy constructor) not viable:
requires 1 argument, but 2 were provided
class Error : public std::exception
^
RtMidi.cpp:1453:8: warning: unused variable 'unlimited' [-Wunused-variable]
bool unlimited = capabilities & PortDescriptor::UNLIMITED;
^
RtMidi.cpp:943:27: warning: implicit conversion of NULL constant to 'MIDIEntityRef'
(aka 'unsigned int') [-Wnull-conversion]
MIDIEntityRef entity = NULL;
~~~~~~ ^~~~
0
RtMidi.cpp:1424:15: note: in instantiation of member function
'rtmidi::CoreSequencer<1>::getPortName' requested here
return seq.getPortName(endpoint,flags);
^
RtMidi.cpp:946:15: warning: comparison between NULL and non-pointer ('MIDIEntityRef'
(aka 'unsigned int') and NULL) [-Wnull-arithmetic]
if (entity != NULL) {
~~~~~~ ^ ~~~~
RtMidi.cpp:959:28: warning: implicit conversion of NULL constant to 'MIDIDeviceRef'
(aka 'unsigned int') [-Wnull-conversion]
MIDIDeviceRef device = NULL;
~~~~~~ ^~~~
0
RtMidi.cpp:961:16: warning: comparison between NULL and non-pointer ('MIDIDeviceRef'
(aka 'unsigned int') and NULL) [-Wnull-arithmetic]
if (device != NULL) {
~~~~~~ ^ ~~~~
10 warnings and 2 errors generated.
make: *** [RtMidi.o] Error 1

Is this code incomplete or did I miss something?

Regards,

—gary

On May 3, 2014, at 5:45 AM, keinstein notifications@github.com wrote:

The current RtMidi implementation has a race condition that may be very annoying in certain cirumstances.

Steps to reproduce the problem:

• add several virtual and/or usb devices
• Start a RtMidi application with interactive port selection
• After the Application presented the port list to the user, remove the removable device in the list from the system (delete or unplug it).
• Select the a device that is below the unplugged device in the list.
• RtMidi will fail or open the wrong device.
The current patch set contains a suggestion for a reliable API.
The main problem that shall be addressed is that the current API does not store port descriptors together with the meta data (port names/port numbers). Thus, both can get out of sync.

The patches are based both on master and winks include:

• Winks patches as well as reverting them to avoid problems synchronizing with the master branch.
• some code reorganization (extract common API into a separate class)
• Add a port descriptor class (With a more flexible port naming interface).
• Add port descriptor API: getPortList, openPort(PortDescriptor &, const std::string&), getPortDescriptor()
• Add Example programs.
• Add an automatic test case for all backends that support virtual devices. This has been inspired by the automatic loopback test case of the node-midi project.

I came across the following questions:
• What's the purpose of class RtMidi? Is it used directly?
• RtMidiIn and RtMidiOut serve as API containers. Why do you use virtual inline functions, here? I'd suggest to remove the virtual classifier from the member functions and declare them only in RtMidi if they are used both by RtMidiIn and RtMidiOut. This would render CommonMidiApi useless and I would reintegrate it into MidiApi. Otherwise the inline functions should be marked extern to avoid linker errors.
• There is no function call ot set or get the current API object from RtMidi(In|Out). This and the fact that the implementation classes Midi(In|Out)(Alsa|Jack|WinMM|WinKS|Core|Dummy) are not documented separately suggest that these classes are private to the RtMidi library. Is that right? In this case I'd suggest to remove these classes from RtMidi.h. Otherwise I'd suggest to add two functions to set and get the API object that is used by the RtMidi(In|Out).
• Maybe… probably… I might add a container API that allows to collect several APIs into one common API.

You can merge this Pull Request by running

git pull https://github.com/keinstein/rtmidi portdescriptor-api
Or view, comment on, or merge it at:

#27

Commit Summary

• Removed Windows kernel streaming code because it was incomplete and uncompilable. The code still exists in the winks branch.
• Deleted include directory with kernel streaming files.
• Removed another reference to kernel streaming support in readme.
• Removed reference to WINKS in midiprobe.cpp
• Corrected documentation regarding API default order in RtMidi.h
• Revert "Removed reference to WINKS in midiprobe.cpp"
• Revert "Removed another reference to kernel streaming support in readme."
• Revert "Deleted include directory with kernel streaming files."
• Revert "Removed Windows kernel streaming code because it was incomplete and uncompilable. The code still exists in the winks branch."
• Partially define an API for reliable port selection.
• Mark RtMidi.h to be C++.
• Merge port Id type into PortDescriptor and go further with cascading the API.
• Complete the API part of the new PortDescriptor based API
• Revert renaming MidiApi into MidiBackendApi and rename the other MidiApi.
• Fix some compiler errros.
• Add missing inline functions and reformat existing ones.
• Namespace based API and get rid of virtual inline functions for RtMidi(In|Out).
• Fix phony make target tests.
• Bump library version.
• Silence some warnings about unused parameters.
• Provide some abstraction layer that will eventually handle all generic ALSA
• Provide the implementation part of the prvious patch.
• Provide the header for the previous patches and pass some strings by reference.
• Some whitespace fixes
• Implement some functions.
• Add first tests for the new port selection API.
• Implement opening ALSA output ports.
• Fix port handling of old port handling.
• fix sendMessage which got an error.
• Adapt qmidiin to the new API.
• Implement the missing functions of the proposed port selection API.
• Provide a test case that uses virtual ports to test communication with virtual ports.
• Some whitespace changes.
• Fix checking for the length of the gool.
• Really show output ports to the user in midiout2.
• Implement new port selection API for JACK
• Flush the output queue when a JACK client connection is going to be closed.
• Update midiout2 provides a virtual output port (not input port) ;-)
• Implement the new API for CoreMidi.
• Fix destructors of some sequencer classes.
• Fix getPortName(PortDescriptor) for Core MIDI.
• Use "RtMidi virtual port" as default for virtual ports.
• Make everything dependent on RtMidi.h
• Improve the identification of virtual ports using Core MIDI.
File Changes

• M Makefile.in (7)
• M RtMidi.cpp (9152)
• M RtMidi.h (1980)
• M configure.ac (14)
• M tests/Makefile.in (18)
• M tests/cmidiin.cpp (2)
• A tests/cmidiin2.cpp (113)
• A tests/loopback.cpp (208)
• M tests/midiout.cpp (2)
• A tests/midiout2.cpp (159)
• A tests/midiprobe2.cpp (134)
• A tests/qmidiin2.cpp (117)
Patch Links:

• https://github.com/thestk/rtmidi/pull/27.patch
• https://github.com/thestk/rtmidi/pull/27.diff
—
Reply to this email directly or view it on GitHub.

[keinstein]

You and your compiler are right there were some errors. But my Mac doesn't complain about them. They should have been fixed, now.

As my Mac is still on 10.5 (and I don't have access to a newer one), I can't check everything against new compilers on Mac OS X. If you find similar errors and warnings it's probably faster if you fix them yourself if the solution is obvious. In any case I'm glad to help if I can.

Another question: You asked me to provide a patch against master. But you replied to the patch against the winks branch. I'm afraid, this could lead to further confusion (and problems on your side), but I'm not shure. Could you be so kind and use #30 for any further conversation?

[drewish]

@keinstein didn't test it extensively but did a quick autoconf && ./configure && make and it compiled with no warnings. compiled the tests and ran midiprobe and it saw ports correctly. So seems like @garyscavone at least won't be blocked by simple stuff this time ;)

[keinstein]

@drewish Did you also check the midiprobe2 and loopback tests?
midiprobe-all is also interesting if you have compiled in several APIs.

[mbeards]

Is there still any interest in getting this PR merged? The improved port selection API would be pretty helpful for me.

[garyscavone]

This pull request involves a lot of changes and I haven’t had time to adequately evaluate it. I was hoping the user community might provide some evaluation and feedback on the possible changes. As always, the goal is to keep the code as simple, yet efficient as possible.

If people want to give the change a try and provide some feedback, that would help me out.

—gary

On Feb 16, 2015, at 3:43 PM, Michael Beardsworth notifications@github.com wrote:

Is there still any interest in getting this PR merged? The improved port selection API would be pretty helpful for me.

—
Reply to this email directly or view it on GitHub.

[keinstein]

Sounds like an chicken and egg problem to me. I plan to use this API in my own projects and maintain the fork at https://github.com/keinstein/rtmidi until it can be merged. @mbeards , I think it would be helpful for all of us if you try this out and share your experiences with us, either as bug reports there or by commenting in this thread.

Regarding the code:

The compatibility functions simply use the old code, so far. This is the efficient way, as it allows to open/close ports in O(1) on Windows or Mac OS (at least the user code part). The simple solution would provide system independent functions similar to those from the JACK backend, which have an open complexity of O(n) where n is the number of available MIDI devices. On windows this should not be a big issue as n cannot grow very large (at most 16, if I'm not mistaken).

Another compitibilty issue is the usage of smart pointers. Even though the patch uses the ones that have been added to the C++ standard, not all compilers support this by default, yet. The compatibilty class can be removed if you drop the compatibilty with old compilers or (probably the better option) rely on Boost for the older C++ standards. The standard smart pointer API is a subset of the Boost API.