search

BelaPlatform / supercollider

an environment and programming language for real time audio synthesis and algorithmic composition
GNU General Public License v3.0
14 stars 8 forks source link

scsynth: adding BelaScopeUGen #75

Closed elgiano closed 3 years ago

elgiano commented 3 years ago

Purpose and Motivation

Addressing issue #6, I'm proposing a bus-based implementation for accessing Bela's Scope from SuperCollider.

  1. BelaScopeUGen reads a bus and calls Scope.log() frame-by-frame
  2. sclang interfaces are provided for:
    • setup: creating a single synth which contains a BelaScopeUGen, and a bus for it
    • logging: UGen.belaScope (and Array.belaScope) are just helpers to write on that bus

Not sure if it's the best implementation, even less sure if the way I link Scope through CMake is optimal. It seems to work fine on my system though.

Known issue: there is a frame drop when BelaScopeUGen is created. Could it be because a new Scope() is created on the server, and that allocation is not real-time safe?

Types of changes

Checklist

Remaining Work

elgiano commented 3 years ago

I see now that my diff for BELAUGens.cpp is big.. it's actually full of trailing space correction that I haven't done, I guess they happened automatically. The only changes I made are about BelaScopeUGen, near the end of the file.

giuliomoro commented 3 years ago

thanks!

I know mostly nothing about Sc's ugens, I have only contributed to another one before. Is BelaScopeUGen_next() called when all the channels in the bus are ready to read?

elgiano commented 3 years ago

Thank you!

BelaScopeUGen_next is called once per block, and bus channels are always ready to read. The fact that BelaScope puts a BelaScopeUGen after the node tree should guarantee in most situations that the bus is read after all other active ugens have completed processing (for every block). Does this answer your question? :)

As far as I understand, Scope is supposed to plot at audio rate, i.e. to call log() for every frame of every block, am I right?

giuliomoro commented 3 years ago

yes and yes! Is there any reason why frameData is allocated wit RTAlloc() ? There is no need for it to take up space in the RT memory pool, as it's allocated in the constructor. Last time I worked on a UGen, I got fed up with the fact that you cannot have objects in the struct, and initialisation values are ignored, so I thought this could be a nicer approach:

class BelaScopeObj 
{
    Scope belaScope;
    std::vector<float> frameData;
    unsigned int noScopeChannels = 0;
    Unit* unit; // this is so that the Sc macros keep working inside the class
public:
    BelaScopeObj(Unit* unit) : unit(unit) {...}
// add more methods here
//....
};
struct BelaScopeUGen : public Unit
{
    BelaScopeObj* ptr;
};

so that you can write a more C++-style constructor/destructor for class BelaScopeObj and then simply call new/delete once from the UGen's Ctor and Dtor, which I think is a nicer approach, especially considering possible future expansions. What do you think?

If you are resubmitting a patch, please do not include the whitespace changes that your text editor added and also fix the indentation where appropriate. It seems that you are mixing tabs and spaces somewhere, e.g.:

    for(unsigned int ch = 0; ch < numChannels; ++ch)
    inputPointers[ch] = ZIN(ch+1);
giuliomoro commented 3 years ago

thanks, is there anything that could be added to prevent instantiating more than one instance of the class? There can be only one Scope object in use at a given time.

elgiano commented 3 years ago

For a start, what do you think about giving a single Scope object to the server itself? I think we could add it to SC_World.h. Then BelaScopeUGen would just get a handle to it, and call log on it, while setup and cleanup could also be done by the server, like:

s.setupBelaScope(maxChannels: 4); -> internally calls stop, cleanup, setup, and start on the scope object

I think safety concerns about calling setup() multiple times can be mitigated (actually eliminated IMO) by wrapping Scope.setup() in a function that always calls stop() and cleanup() first if scope is running.

What do you think?

giuliomoro commented 3 years ago

For a start, what do you think about giving a single Scope object to the server itself?

I'd rather avoid that. We are aiming to merge upstream, so the more we can keep our changes out of the way of regular Sc code, the better, so the approach of using UGens seems fine to me. What I was thinking is more having a static flag that can be used to check the number of instantiated objects. This is easy to do in C++, but I was wondering how to communicate the failure back to the language.

I think safety concerns about calling setup() multiple times can be mitigated (actually eliminated IMO) by wrapping Scope.setup() in a function that always calls stop() and cleanup() first if scope is running.

One could just delete the Scope object and recreate it. I assume that there is some guarantee in place that while the Dtor and Ctor are running no audio or control callback is being called, right?

Another interesting feature would be to have a trigger input to the scope for custom triggering.

elgiano commented 3 years ago

Ok, I see your point, but wouldn't the addition of a Scope object inside the BELA ifdef in SC_World still be compatible with the idea of merging? Then maybe we skip creating new primitives and server methods, and handle everything else in the UGen. My problem is that I don't really see a way to limit the creation of a UGen to a single instance. The best that comes to my mind is that BelaScopeUGen would check that static flag and turn into noop if there's more than one instance.

On the other hand it looks relatively easy to put the limit on sclang-side, but this would be unreliable if you have multiple clients (e.g. one on Bela and the other on host)

giuliomoro commented 3 years ago

My problem is that I don't really see a way to limit the creation of a UGen to a single instance.

Is there a way for the Ctor to fail? Given how it returns void, I suspect not but maybe there is a magic macro that communicates a failure?

but wouldn't the addition of a Scope object inside the BELA ifdef in SC_World still be compatible with the idea of merging?

You are right, I didn't realise we already have an #ifdef BELA in there *. And having one single Scope object as part of the server options is what makes the most sense anyhow.

Then maybe we skip creating new primitives and server methods, and handle everything else in the UGen.

Sounds good! Would you be able to do the above?

* I guess it's a sensible idea to have it there then. I am a bit concerned about having anything at all in SC_World, because it's a publicly visible structure, so in principle all plugins would have to be rebuilt for Bela (which is what we already do, but it'd be nice if we didn't have to). Anyhow, that's a consideration for another moment.

elgiano commented 3 years ago

I'm on it... but I'm a bit confused about linking. If SC_World requires Scope, does it mean we have to link libbelaextra to scsynth, and add Bela "root folder" to include_directories?

giuliomoro commented 3 years ago

That seems right yes, and undo your earlier change to CMakeLists.

elgiano commented 3 years ago

I added a commit where we create and store a Scope() object in SC_World, before the AudioDriver starts, and we call the scope's .setup() in belaSetup, on the AudioDriver. This way setup is not called in the audio thread, which used to give some initial frame drop.

Now we have a single Scope object per server, but I haven't yet figured out a way to call setup on it, other than at startup, without doing it on the audio thread, and without writing a primitive for it. One way I thought about is to init the max number of scope buffers only at startup, maybe through a server option?

It seems to me that everything a UGen does is done on the audio thread (that's why RTAlloc for frameData in the constructor), so it's ok to call scope->log(), but apparently not setup(), maybe because of WSServer or AuxTask allocations?

giuliomoro commented 3 years ago

Thanks for your new commit. I seem to understand that now we have a fixed number of channels, but it would be a better option to have them set up in the server options as you previously suggested (or better s.options.belaScope(maxChannels: 4); (the rationale behind this is that scope channels are expensive (CPU-wise), and one wouldn't want to have 8 when they need 2).

One way I thought about is to init the max number of scope buffers only at startup, maybe through a server option?

yes!

If I understand correctly, one can still instantiate multiple copies of the ugen, which is going to cause trouble (because calling scope.log() more than once per frame is going to send multiple frames to the scope per each audio frames). Maybe yet another approach would be to have the scope associated with some of the Out.ar() channels (how many would be decided by the s.options. field)? Then one can send audio to those channels, and we can call scope.log() from within the BelaAudioCallback().

One other alternative is to hook into Sc's own .scope. That seems more complicate by the looks of it.

elgiano commented 3 years ago

An alternative is to use the bus system as it is, reserve a bus language-side, and put in some guards for not doing that twice. The bus is read by a BelaScopeUGen, which calls .log, and we could also figure out a way for not having more than one such UGen running. Otherwise, if we want to call .log from BelaAudioCallback it looks like we either need to allocate our own bus out of the audio bus range (see above), or to have some kind of osc message to tell the server which bus we are using for scope.

elgiano commented 3 years ago

Extra question: sometimes I get this warning in the IDE (now it happened after I left the tab in the background, with a test project running for a long time, like more than one hour). And of course scoping doesn't work anymore afterwards. 

Error while sending to pipe from WSClient_scope_data: (12) Cannot allocate memory (size: 36864)

I got it sporadically since the first version of this PR... can you give me any clue about what is it?

elgiano commented 3 years ago

Update: I implemented a ServerOption ( -O belaMaxScopeChannels ), and single-instance-guards for sclang and BelaScopeUGen. Apart from that sporadic WSClient memory error I reported above, I feel good about this implementation :)

Let me know what you think, when you have time of course.

giuliomoro commented 3 years ago

Error while sending to pipe from WSClient_scope_data: (12) Cannot allocate memory (size: 36864)

this essentially means that you are sending too much data to the audio-to-websocket pipe, that is not sent via the websocket to the in-browser Scope fast enough. In practice, it often boils down to using too many Scope channels with a too high refresh rate for the CPU available. A common solution is increase the holdoff in the Scope GUI.

Could you please provide a .scd file that I can run on Bela to test the scope? I am really a noob with Sc, so something that I could just drop in the IDE and launch would be great, thanks.

Is there a way to add another "trigger" input channel to the scope? The server should be notified at setup time that a trigger channel is active, so that it can switch the Scope object to custom triggering mode (calling setTrigger() [this is improved with an enum in the Bela dev branch, which I shall soon merge into master]. It would then call scope->trigger() every time a 0 to 1 transition is encountered in the trigger input

A couple more issues:

Here's a patch that should fix these two issues:

diff --git a/SCClassLibrary/Common/Control/Server.sc b/SCClassLibrary/Common/Control/Server.sc
index 396c944a7..87f432f78 100644
--- a/SCClassLibrary/Common/Control/Server.sc
+++ b/SCClassLibrary/Common/Control/Server.sc
@@ -60,7 +60,7 @@ ServerOptions {
    var <>adcLevel;
    var <>numMultiplexChannels;
    var <>belaPRU;
-   var <>belaMaxScopeChannels = 2;
+   var <>belaMaxScopeChannels = 0;

    var <>recHeaderFormat="aiff";
    var <>recSampleFormat="float";
diff --git a/server/plugins/BELAUGens.cpp b/server/plugins/BELAUGens.cpp
index b9e72384d..8fd7b4f62 100644
--- a/server/plugins/BELAUGens.cpp
+++ b/server/plugins/BELAUGens.cpp
@@ -1379,11 +1379,11 @@ void BelaScopeUGen_Ctor(BelaScopeUGen *unit)
     uint32 numInputs = unit->mNumInputs;
     uint32 maxScopeChannels = unit->mWorld->mBelaMaxScopeChannels;
     if(numInputs > maxScopeChannels) {
-       rt_printf("BelaScopeUGen warning: can't scope %i channels, maxBelaScopeChannels is set to %i\n", numInputs, maxScopeChannels);
+       rt_fprintf(stderr, "BelaScopeUGen warning: can't initialise scope %i channels, maxBelaScopeChannels is set to %i\n", numInputs, maxScopeChannels);
     }
     BelaScopeUGen::instanceCount++;
     if(BelaScopeUGen::instanceCount > 1) {
-        rt_printf("BelaScopeUGen warning: creating a new instance when one is already active. This one will do nothing.\n");
+        rt_fprintf(stderr, "BelaScopeUGen warning: creating a new instance when one is already active. This one will do nothing.\n");
         SETCALC(BelaScopeUGen_noop);
         return;
     };
diff --git a/server/scsynth/SC_Bela.cpp b/server/scsynth/SC_Bela.cpp
index 4730530fd..9355f75bd 100644
--- a/server/scsynth/SC_Bela.cpp
+++ b/server/scsynth/SC_Bela.cpp
@@ -148,7 +148,7 @@ bool sc_belaSetup(BelaContext* belaContext, void* userData)
     // cast void pointer
    SC_BelaDriver *belaDriver = (SC_BelaDriver*) userData;
    gBelaSampleRate = belaContext->audioSampleRate;
-   belaDriver->mBelaScope->setup(8, gBelaSampleRate);
+   belaDriver->mBelaScope->setup(mBelaMaxScopeChannels, belaContext->audioSampleRate);
    return true;
 }

diff --git a/server/scsynth/scsynth_main.cpp b/server/scsynth/scsynth_main.cpp
index 451933847..600d7eefe 100644
--- a/server/scsynth/scsynth_main.cpp
+++ b/server/scsynth/scsynth_main.cpp
@@ -184,7 +184,7 @@ int main(int argc, char* argv[])
    options.mBelaDACLevel = 0;
    options.mBelaNumMuxChannels = 0;
    options.mBelaPRU = 1;
-    options.mBelaMaxScopeChannels = 2;
+   options.mBelaMaxScopeChannels = 0;
 #endif

    for (int i=1; i<argc;) {

There are still a few whitespace errors (which you can see with git diff, but those I can fix myself as there are already too many of those around).

giuliomoro commented 3 years ago

also, I'd be interested in hearing if @sensestage has any suggestion on this

elgiano commented 3 years ago

Thanks for looking at it!

Here is a simple sketch that scopes stereo inputs + 6 random sine waves:

s = Server.default;

s.options.numAnalogInChannels = 8;
s.options.numAnalogOutChannels = 8;
s.options.numDigitalChannels = 16;

s.options.blockSize = 64;
s.options.numInputBusChannels = 2;
s.options.numOutputBusChannels = 2;
s.options.maxLogins = 4;

s.options.postln;

s.options.belaMaxScopeChannels = 8;

s.waitForBoot({
  SynthDef("help-scope",{ arg out=0;
        var in = SoundIn.ar([0,1]).belaScope(0);
        var sin = SinOsc.ar(TExpRand.kr(50,1000,Dust.kr(1!5)).lag2(1)) * LFNoise2.ar(1!5).exprange(0.01,1);
        sin.belaScope(2);
        Out.ar(out, Pan2.ar(sin,[-1,1])+in);

  }).play;
});
giuliomoro commented 3 years ago

Error in WSClient: got it, so there's nothing we should do on this PR's side, or is it?

correct

about triggering, I have to use a bit of time to think about it :) I'm thinking we can give one more channel to the scope bus, and reserve it for triggering. I would say the last one. Would it be possible then to switch to customTrigger mode lazily, only when the first trigger is received? I guess this question boils down to: is it possible (safe) to change triggering mode on the audio thread?

I am actually thinking: if one wants to use "custom" triggering, they shall just send a trigger audio-rate signal into one scope channel, and select that as the triggering channel from the Scope frontend. No need to add any extra support for it. I think we are good to go (or as good as we can get with the current design). I will wait for a couple of days to see if @sensestage has any comments. In the meantime I will merge this on top of my mainlining attempt and build a beta release.

elgiano commented 3 years ago

mainlining attempt and build a beta release.

I know this is not the right place to talk about it, but I was at the supercollider dev meeting yesterday and we talked about that it would be great to merge Bela for version 3.12, which is expected before the end of the year. I would be happy to help through the process if needed and another contributor offered to help with testing! I see you are rebasing on develop, which is perfect because on 3.12 develop will be merged into main. Please let me know if I can help in any way!

In the meanwhile I'll write an Help file for BelaScope, and should I finally squash all my commits into one?

giuliomoro commented 3 years ago

In the meanwhile I'll write an Help file for BelaScope, and should I finally squash all my commits into one?

great, yes please

Please let me know if I can help in any way!

Test after I build the release. I will continue the conversation in #74

giuliomoro commented 3 years ago

Great I squashed and reformatted this here.

Here's a build, please test it (I enjoyed the scope example, thanks!):

https://github.com/BelaPlatform/supercollider/releases/tag/v3.11-bela2

Note: if you want to build this yourself, you need the very latest Bela master, as I changed a couple of things in bela-config

EDIT: updated link to bela2

giuliomoro commented 3 years ago

Updated the release to include Ableton Link support and Xenomai IPC:

https://github.com/BelaPlatform/supercollider/releases/tag/v3.11-bela3

elgiano commented 3 years ago

Just FYI: I will be unable to test this properly until at least next thursday. Sorry, I'm drowning in other works

sensestage commented 3 years ago

Hi there!

Yes, I do have suggestions. (Admittedly, I haven't read through the whole thread, so just sharing my initial thoughts)

My initial go at this ugen was to have one ugen BelaScopeChannel, to scope just one channel, and one UGen that can just be instantiated once that refers to the BelaScope, and that sets the amount of channels to use. So it does not have to be a startup parameter.

Back in the day I did not get around to making it all work out, but this was my initial start. It avoids having to add things to the World struct.

/*
int noScopeChannels = 0;
Scope * belaScope;

void BelaScopeChannel_next( BelaScope *unit )
{
    int scopeChannel = unit->mScopeChannel;
    float *in = IN(1);

    for(unsigned int n = 0; n < inNumSamples; n++) {
        belaScope->logChannel( scopeChannel, in[n] );
    }
}

void BelaScopeChannel_Ctor(BelaScope *unit)
{
    BelaContext *context = unit->mWorld->mBelaContext;

//     belaScope = Scope();
    // which channel is an input variable
//     belaScope->setup(3, context->audioSampleRate);
    float fChan = ZIN0(0); // number of channels
    mScopeChannel = (int ) fChan;
    // check whether channel is within number of channels of scope
    if ( mScopeChannel > noScopeChannels ){
            // error
    }
    // initiate first sample

    BelaScopeChannel_next( unit, 1);  
    // set calculation method
    SETCALC(BelaScopeChannel_next);
}

void BelaScope_next(BelaScope *unit)
{
}

void BelaScope_Ctor(BelaScope *unit)
{
    BelaContext *context = unit->mWorld->mBelaContext;

    float fChan = ZIN0(0); // number of channels
    noScopeChannels = (int) fChan;

    belaScope = Scope();
    // number of channels is a variable
    belaScope->setup(noScopeChannels, context->audioSampleRate);

    // initiate first sample
    BelaScope_next( unit, 1);  
    // set calculation method
    SETCALC(BelaScope_next);
}

void BelaScope_Dtor(BelaScope *unit)
{
    belaScope->stop();
    delete belaScope;
    noScopeChannels = 0;
}
*/
giuliomoro commented 3 years ago

@sensestage I see a couple of issues with your implementation :

Overall, I am not sure the added complexity is worth the trouble, if the only gain you get from this is to have a variable number of scope channels, but I'd be happy to hear your thoughts on this. Remember that I know very little about the inner workings of scsynth and even less about how to use sclang.

Admittedly, I haven't read through the whole thread, so just sharing my initial thoughts)

A good summary of the discussion is in the current implementation, which is all contained in 2fc8ce657613a24b5cecc5abe1776b5377af4bc6 and can be tested with the code in https://github.com/BelaPlatform/supercollider/pull/75#issuecomment-711890973 .

giuliomoro commented 3 years ago

@elgiano Just FYI: I will be unable to test this properly until at least next thursday. Sorry, I'm drowning in other works

No worries. Make sure you always test the latest release.

giuliomoro commented 3 years ago

@sensestage another thing about your implementation: would there be need for a s.sync between instantiating the BelaScope ugen and instantiating one of the BelaScopeChannel ugens?

elgiano commented 3 years ago

@sensestage I've seen your implementation, it is actually still there, commented out in BELAUGens.cpp :) The two problems I found:

So we ended up giving a Scope to the Server object, which initializes it and destructs it outside the audio thread. The max number of scope channels is fixed as a startup option, but it is still possible to scope less channels by writing to less Bus channels. Of course, Scope is still logging all the other (zero) channels anyway, and this can't be changed without re-initing the Scope object with a different number of channels, which would introduce a lot of complexity if it has to be done outside the audio thread (or better, I couldn't figure out a simple way to do this). Overall, the Bus implementation feels to me light and easy to use enough, with no significant overhead (or code duplication) other than reserving that Bus.

What do you think?

giuliomoro commented 3 years ago

@elgiano any luck in testing this?

elgiano commented 3 years ago

@giuliomoro I'm giving it a go now! Tried the oscilloscope test script above and it all works, I did some back and forth between the latest release and my version from git, and I can't see any noticeable difference. Except some more warnings when starting supercollider from the web ide:

giuliomoro commented 3 years ago

thanks, the first few lines are expected. The others not so much. Have you updated your Bela to the latest master and run make -C ~/Bela lib to update libbela which is used by Sc ?

elgiano commented 3 years ago

Got me, I had forgot that. With the latest Bela master I only get these "warnings" indeed

-- cold init from program
-- cobalt->init()
-- connected to Cobalt
Initialize_xenomai
-- memory locked
-- memory heaps mapped
-- boilerplate->init()
-- program bootstrap done

And everything still works :) Meaning that I get sound out and Oscilloscope is plotting. I've tested only with my Oscilloscope test patch and SuperCollider's default project.

On Sat, Nov 7, 2020 at 1:00 PM giuliomoro notifications@github.com wrote:

thanks, the first few lines are expected. The others not so much. Have you updated your Bela to the latest master and run make -C ~/Bela lib to update libbela which is used by Sc ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BelaPlatform/supercollider/pull/75#issuecomment-723437358, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACNMWWRPNDGED7GVCSSJP3SOUZFHANCNFSM4SPEML3Q .

giuliomoro commented 3 years ago

Great, I think I will submit for upstream merge soon

giuliomoro commented 3 years ago

although first I'd like to fix a few more outstanding issues:

elgiano commented 3 years ago

Great, although I still have to submit documentation for this! I'm almost done with it, shall I make another PR or shall we reopen this?

Also, if you have time, please consider this (https://github.com/BelaPlatform/Bela/pull/645) contribution to the main Bela repo, where I added syntax highlighting for SC code :)

I'll look into the other issues as well!

giuliomoro commented 3 years ago

Great, although I still have to submit documentation for this! I'm almost done with it, shall I make another PR or shall we reopen this?

Open a new one against the bela-merge-formatted-rebase branch

Also, if you have time, please consider this (BelaPlatform/Bela#645) contribution to the main Bela repo, where I added syntax highlighting for SC code :)

Thanks very much, I shall do that soon. I will also need an example for the scope to add to the main repo, so you can submit a separate PR there for that if you want. On that note, I wanted to add the ServerQuit.add({ 0.exit }) line to all Sc examples so that when the server is stopped (e.g.: by pressing the button on the Bela cape) (see here), so maybe you can start by adding it to your example (or even the existing ones if you want :) )