Implement a reliable port selection API

[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]

After implementing ALSA, JACK, WinMM and Core i created another pull request against the winks branch. See #27 for details. As I need the mutabor branch for other development I'll close this request and refer to #27 .

[garyscavone]

I have a few questions / comments:

1. Why did you submit the pull request against the winks branch, instead of a new branch (or maybe that’s not possible?)? I was hoping to keep the winks branch specifically for fixes to the KS code to allow it to be remerged into the master branch.
2. I think the “pre2.1.0” branch should be deleted … does anyone care if I remove it?
3. One of the long running “goals” of RtMidi (and RtAudio) was to have everything confined to as few files as possible, since I (and other users I know) tend to pick and choose the classes I need when making a program (rather than linking with external libraries) and having to copy a lot of files / directories is undesirable. The class structure for RtMidi was a bit more complicated than for RtAudio because I felt it was necessary to separate MIDI input and output functionalities into different classes (whereas this was not necessary in RtAudio). I would probably agree that the code structure could be “cleaner” if things were separated into different files (perhaps a different file or directory for each API). However, I’d prefer to keep it all in a few files if possible.
4. I cannot remember if I had any valid reason for using the “virtual" inline functions in RtMidiIn and RtMidiOut … I think it was probably "left over" from the pre-2.0 versions that only supported a single compiled API at a time. If you see things that can be simplified, I’m all for that.
5. There are getCurrentApi() functions in RtMidi(In/Out). The means for setting the API is via the constructor. But perhaps I’m not understanding the question?
6. It seems from a quick reading that the system you have implemented might not fix the problem in WinMM?

Finally, I’d like to look through the code you’ve created though not necessarily via the winks branch. To go back to the motivation for your code changes, my expectation all along has been that the client should always re-poll RtMidi for the available devices any time a user might be allowed to make a new port connection (for exactly the reason you gave as an example … someone might have disconnected something). That is, client programs should not “store” a set of available devices and then continue to use that set without always checking to verify it is still valid. But no matter, the existing port identification scheme is still weak and could use a more robust implementation. Thus, I’m happy you have taken the time to work out a possible solution.

Regards,

—gary

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

After implementing ALSA, JACK, WinMM and Core i created another pull request against the winks branch. See #27 for details. As I need the mutabor branch for other development I'll close this request and refer to #27 .

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

[keinstein]

Hi Gary,

Am 07.05.2014 18:23, schrieb garyscavone:

I have a few questions / comments:

1. Why did you submit the pull request against the winks branch,
   instead of a new branch (or maybe that’s not possible?)? I was hoping
   to keep the winks branch specifically for fixes to the KS code to
   allow it to be remerged into the master branch.
   The main reason was that I started development before you removed WinKS.
   Nevertheless you should base WinKS on master and not vice versa.
   Otherwise git tries to remove WinKS when you merge the two branches.
2. I think the “pre2.1.0” branch should be deleted … does anyone care
   if I remove it?
   You might want to add a development branch. Generally it's a good idea
   to add only to the repository as everything you remove might confuse the
   ohers. But it's your choice.
3. One of the long running “goals” of RtMidi (and RtAudio) was to have
   everything confined to as few files as possible, since I (and other
   users I know) tend to pick and choose the classes I need when making a
   program (rather than linking with external libraries) and having to
   copy a lot of files / directories is undesirable. The class structure
   for RtMidi was a bit more complicated than for RtAudio because I felt
   it was necessary to separate MIDI input and output functionalities
   into different classes (whereas this was not necessary in RtAudio). I
   would probably agree that the code structure could be “cleaner” if
   things were separated into different files (perhaps a different file
   or directory for each API). However, I’d prefer to keep it all in a
   few files if possible.
   Generally, putting everything in one binary library gives a cleaner
   layout. In case of a dynamic library (so/dll) it's useless if you use
   several copies of the library. For static libraries the linker will omit
   everything that's not needed. My feeling for the classes is: combining
   input and output could simplify things for certain interfaces.
4. There are getCurrentApi() functions in RtMidi(In/Out). The means
   for setting the API is via the constructor. But perhaps I’m not
   understanding the question?
   The question is to open the library for user supplied backends. For
   example, besides the the mentioned All-Backend-API I'm missing a debug
   backend that tells me which data my application has sent to the “MIDI
   port”. User supplied API's would need a function that adds a new backend
   api at runtime. If you don't want that, you should remove the
   definitions for the backends from the header file. In that case you can
   hide the implementation details completely from the user.
5. It seems from a quick reading that the system you have implemented
   might not fix the problem in WinMM?
   I did the best I could to solve it. But before opening a port, Windows
   refuses to give you a port handle. So I don't see any chance to solve
   the problem. Nevertheless, I implemented a check after opening the port
   that the port name didn't diverge from the stored one
   during the session. So the behaviour should not differ too much from the
   other APIs, though confusion might still occur if the new port has the
   same name as the old one.
   Finally, I’d like to look through the code you’ve created though not
   necessarily via the winks branch. To go back to the motivation for
   your code changes, my expectation all along has been that the client
   should always re-poll RtMidi for the available devices any time a user
   might be allowed to make a new port connection (for exactly the reason
   you gave as an example … someone might have disconnected something).
   That is, client programs should not “store” a set of available devices
   and then continue to use that set without always checking to verify it
   is still valid. But no matter, the existing port identification scheme
   is still weak and could use a more robust implementation. Thus, I’m
   happy you have taken the time to work out a possible solution.
   Well, implementing a patch bay (as in MUTABOR) may cause you headaches
   if you don't have a passive (i.e. offline) mode. Especially on Windows
   I'd like the user to control whether the ports are connected or not (not
   the focus as suggested in Microsoft's API documentation). For ALSA there
   is also an exclusive mode for MIDI access. I don't think that it is a
   good user experience to have to reconfigure everything just because you
   forgot to upload a SysEx dump or similar.

Regards,

Tobias

Regards,

—gary

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

After implementing ALSA, JACK, WinMM and Core i created another pull
request against the winks branch. See #27 for details. As I need the
mutabor branch for other development I'll close this request and refer
to #27 .

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

—
Reply to this email directly or view it on GitHub
#21 (comment).