danomatika / ofxMidi

(maintained) Midi addon for openFrameworks
Other
262 stars 72 forks source link

ofxMidiIn: ofThreadChannel #87

Closed artificiel closed 10 months ago

artificiel commented 1 year ago

this is a possible implementation for a threadChannel for ofxMidiIn:

danomatika commented 1 year ago

I haven't used thread channels yet but changes look minimal so not much to merge. What systems have you tested this on? Would there need to be a separate example to demonstrate / test this feature?

artificiel commented 1 year ago

the process (down to function names) is lifted from ofxOsc. i've been using it for a few months on macOS with pretty dense controller data (machine-generated, multiple controllers per update() cycle (where they are actually consumed), which were causing thread problems -- initially solved by a mutex around the access).

i will make an example.

and yes the changes are unobtrusive they don't affect current behavior unless you do not define a listener in which case the data implicitly flows into the threadchannel (which is the only possible model with ofxOsc).

danomatika commented 1 year ago

My feeling is that enabling a thread channel should be explicit and not implicit. The channel could be nullptr that is enabled upon calling setup().

artificiel commented 1 year ago

About setup(), adding a specific midiIn.enableThreadChannel() is possible but I like the fact that it behaves in the same streamlined manner of ofxOsc (and other hardware/network-driven async interfaces like ofxMQTT and ofxArtnet follows a similar pattern) -- having less specifics things to learn to get up and running without having to wrap things in a mutex is a gain.

What about that the first call to getNextMessage() dynamically instantiates the threadChannel if nullptr? the usual approach is to call it in update() so it would happen more or less as soon as the event loop starts running.

also I wonder what do you thing about a setup method that would wrap the openPort call (keeping the direct methods). so a typical setup function could look like:

oscReceiver.setup(12000);
oscSender.setup("192.168.2.2", 12001);
artnetSender.setup("10.0.0.4");
midiInput.setup("IAC Bus 1");
midiOutput.setup(3);

and considering the number of options available, provide a Settings that make the bools explicit?

ofxMidiInputSettings settings;
settings.portName = "IAC Bus 1";
settings.ignoreSysex = True;
settings.verbose = True;
// etc..
midiInput.setup(settings);

this would follow the pattern for more complex setups such as SoundStream, Window, Shader, etc.

artificiel commented 11 months ago

I went ahead an made the threadChannel dynamically allocated (so "opting in" is implicit if you query the channel, and an app based on event callbacks will not instantiate it) and also provided a basic setup() alias to openPort — the complete Settings idea would be good, but can come at a later stage (I'd like to make it as homogeneous as possible between the different async messaging protocols and still some groundwork to do before that happens).

danomatika commented 10 months ago

I looked at this again today and decided to implement it slightly differently. I have "manually integrated" much of this PR but have not taken the commits directly. You get credit for this all the same. :smile:

simpler to integrate (no need to inherit ofxMidiListener or implement newMidiMessage)

As suggested, this is now available in ofxMidiIn via hasWaitingMessages() and getNextMessage().

identical access pattern as ofxOsc (coherence of general OF API)

That's a matter of taste to some degree. The "general OF API" is a point but we don't have to be slavish to it. For instance, I find no need to add setup() function aliases as the conventions of "open" and "close" fit much better conceptually to MIDI ports as opposed to "setup" and "clear".

Similarly, I don't see the need for settings structs as the open methods do not have an abundance of parameters to set, unlike those such as ofTrueTypeFont.

thread safe

Yep.

still allows thread-unsafe immediate/synchronous listeners (setting a listener bypasses the threadChannel)

Nope. I decided the choice between "direct message passing" and "queued message passing" (thread channel) needs to be explicit. The thread channel is created by default but is deleted if you set a listener. The documentation and example have been updated to explain this. This should not break existing projects.

I also did not see the need for two input examples and included both types of receiving to the existing midiInputExample with commenting to explain the differences.

In any case, thanks for your contribution. It is now in ofxMidi 1.3.0 (in spirit, if not as commits).