andrewcsmith / bela-rs

Safe Rust wrapper for the Bela.io API for realtime audio and sensor feedback
30 stars 4 forks source link

Add support for MIDI and other Bela support libraries #12

Open l0calh05t opened 3 years ago

l0calh05t commented 3 years ago

While core libbela support is sufficient for pure audio processing, MIDI and other features require using Bela support libraries. Probably the most important ones are the communication-centric libraries, as the signal processing libraries mostly have crates that serve the same purpose.

Many of these libraries have C++ APIs. In the case of MIDI, there is both a C and a C++ API, resulting in two possible approaches to making bindings:

  1. By adding automatically generated C bindings to bela-sys (some early experiments can be found here: https://github.com/l0calh05t/bela-sys/tree/midi) and wrapping them safely in bela-rs as done for libbela (albeit with some complications and lacking some features)
  2. Directly creating a safe wrapper for the (more full-featured) C++ API using cxx, but this probably requires some additional C++ code as well and I personally have no experience with cxx yet.

No matter which approach is taken, this could feasibly be a separate crate, although I'd prefer to keep the two linked, as otherwise the effort of ensuring that the Xenomai and Bela libs are linked has to be duplicated.

giuliomoro commented 3 years ago

Note that I will at some point try to get rid of the C++ API, or at least make it compiler-independent. Currently it is a hazard to use it with any compiler different from clang-3.9 because compiler-specific implementation details are allowed to change the ABI that is currently exposed. You are better off using the C API where available.

l0calh05t commented 3 years ago

Yes, the only truly safe way to do it right now would be to build Midi.cpp along with the crate, which may become quite complicated to get right, considering all the Xenomai defines etc., but haven't clang++ and g++ been ABI-compatible for a while now (assuming libstdc++ is used on both)?

In any case, sounds like a vote for option 1. @giuliomoro Do you plan to make .a or .so libraries available? It seems right now everything is only present as one or more .o-files.

l0calh05t commented 3 years ago

Current WIP here https://github.com/l0calh05t/bela-rs/tree/midi, should be usable with

[patch.'https://github.com/andrewcsmith/bela-sys.git']
bela-sys = { git = 'https://github.com/l0calh05t/bela-sys.git', branch = 'midi' }

until https://github.com/andrewcsmith/bela-sys/pull/2 is merged

l0calh05t commented 3 years ago

@giuliomoro While the code in my branch works, I can't really provide a safe API as-is, because Midi_new segfaults when called outside of setup, instead of returning a null pointer.

Error: You should call Bela_initAudio() before calling Bela_createAuxiliaryTask()
Segmentation fault

Looking at Midi_c.h, it seems new is assumed to return null instead of throwing an exception (which you shouldn't in an extern "C" function)

giuliomoro commented 3 years ago

hmm ... I don't think there is anything there that would throw in there ...

this should get rid of the segmentation fault:

diff --git a/libraries/Midi/Midi.cpp b/libraries/Midi/Midi.cpp
index 01360bee..751f7d82 100644
--- a/libraries/Midi/Midi.cpp
+++ b/libraries/Midi/Midi.cpp
@@ -313,6 +313,9 @@ int Midi::readFrom(const char* port){
                return err;
        }
        midiInputTask = Bela_createAuxiliaryTask(Midi::readInputLoop, 50, inId, (void*)this);
+
+       if(!midiInputTask)
+               return -1;
        Bela_scheduleAuxiliaryTask(midiInputTask);
        inputEnabled = true;
        return 1;

This should get rid of the risk of throwing

diff --git a/libraries/Midi/Midi_c.cpp b/libraries/Midi/Midi_c.cpp
index d3f7fdc0..5fe421a6 100644
--- a/libraries/Midi/Midi_c.cpp
+++ b/libraries/Midi/Midi_c.cpp
@@ -1,4 +1,5 @@
 #include "Midi_c.h"
+#include <exception>

 int Midi_availableMessages(Midi* midi){
        return midi->getParser()->numAvailableMessages();
@@ -17,11 +18,20 @@ unsigned int Midi_getMessage(Midi* midi, unsigned char* buf){
 }

 Midi* Midi_new(const char* port){
-       Midi* midi = new Midi();
-       if(midi != NULL){
-               midi->readFrom(port);
-               midi->enableParser(true);
+       Midi* midi;
+       try {
+               midi = new Midi();
+       } catch (std::exception e) {
+               return nullptr;
        }
+       if(!midi) {
+               return nullptr;
+       }
+       if(midi->readFrom(port) < 0) {
+               delete midi;
+               midi = nullptr;
+       }
+       else
+           midi->enableParser(true);
        return midi;
 }

(both untested).

Even if we get rid of the segmentation fault, the problem persists that you should construct the Midi object only after Bela_initAudio() has been called. This is an artificial limitation imposed by the fact that we are using Bela_...AuxiliaryTask for the threading in the Midi class, which is not actually needed. A std::thread would suffice. That requires a bit more refactoring though.

l0calh05t commented 3 years ago

Tbh I didn't look into it in detail, an uncaught exception seemed like a likely candidate, as it is the only way to communicate a constructor failure in C++ (and even if that wasn't the cause, new could throw a bad_alloc).

In any case, if Midi_new correctly reports the error and doesn't cause undefined behavior, the Rust function would be safe to call even outside setup (it just wouldn't succeed). I'm working on a fairly significant rewrite/refactor of bela-rs in parallel, where Midi (and auxiliary tasks!) can literally only be constructed inside setup (by introducing a parameter that requires an—unused internally—context reference, and the context is tagged depending on if it is a setup or render context).

l0calh05t commented 3 years ago

Addendum to my previous comment: You can check out https://github.com/l0calh05t/bela-rs/tree/next for the more drastic rewrite/refactor of the API. Still a work in progress, though

chaosprint commented 1 year ago

Any updates on this? I am trying to use the UDP API.

l0calh05t commented 1 year ago

@chaosprint MIDI is in my "next" branch. Haven't looked into UDP