Rack: Module expander proposal

[AndrewBelt]

It would be useful to create expander modules to the left and right of other modules, and have them talk to each other directly.
This would allow modules like Permutation and Variant, or multiple instances of Stages, or any other future Eurorack ports to behave like real hardware by simply placing modules together in the rack.
To connect an expander with the rack UI, simply drag it to the (usually) right of its "mother" module, and they can communicate directly.

Proposed changes:

1. Add Model *model to Module.
2. Add Module *leftModule and Module *rightModule to Module.
3. Add methods to Engine to allow management of the above fields.
4. Make UI call the above Engine methods when modules are moved.
5. Serialize the left and right module IDs so a future "headless" mode of Rack does not need to rely on the width of ModuleWidgets to determine whether two modules are connected.

Usage example:

void Variant::process(const ProcessArgs &args) {
    if (leftModule && leftModule->model == modelPermutation) {
        Permutation *permutation = reinterpret_cast<Permutation*>(leftModule);
        // Grab state from Permutation and set its own state
    }
}

Issues with the above:

• Not thread-safe, i.e. incompatible with engine multithreading. Need to use message-passing through the engine instead with a one-sample delay, just like cables.
• The Permutation class in the above example needs to be declared in a header file so Variant can access its fields. Or, they need to be defined in the same file. I don't like deviating from the "one .cpp file per module" standard.
• Moving modules around the UI would require locking the engine threads. This should be improved by making the "connection" between adjacent modules upon DragEnd, rather than every time the module is moved during the drag gesture. This is made more complicated with the upcoming drag "forcing" mode.
• The patch format would have leftModuleId and rightModuleId for every adjacent module, which is mostly unnecessary and less readable. Additionally, while loading a patch, the IDs might not be resolvable to actual Module pointers yet until all Modules are loaded.

[Rcomian]

could we think of an expander module as ui only, without a process method of its own? so the mother module has access to the extra params and ports but has to drive them. this would avoid threading issues.

adding an expansion could be an explicit action on the UI of the mother module. a module could be defined as an expansion of another module. can be serialised and rendered specially. wouldn't have to be next to each other.

downsides would be that a module couldn't expand a mother module that the mother module doesn't know about. they'd have to be written and released together.

[AndrewBelt]

@Rcomian Hmm, that's an interesting idea. However, 3 of the 4 expander Eurorack modules that gave inspiration to this proposal should be able to function on their own. In fact, even when plugged in via a bus cable on the back, only a small subset of the module states become shared. So treating an expander as an additional panel for another Module is too limiting.

[david-c14]

Having to pass messages 1 frame behind, like cables, renders this rather like having a great wide (polyphonic) data bus between the devices. I really like the idea of the UI aesthetic here of just having the devices next to each other for them to automatically connect.

I'm wondering if it would be a step too far to have to explicitly link compatible devices with a context menu...

• A model could register which other models it is compatible with. (mitigating @Rcomian 's concern about releasing modules together.
• The context menu could offer to backplane connect adjacent devices only where the pair are registered as compatible.
• Linked modules could always drag together, until they are unlinked
• Links, thus defined in the patch format, would not need to be reforged until after all the modules in the patch have been loaded, in the same way that cables are not plugged in until all the modules are present.

[Skrylar]

I'm wondering if it would be a step too far to have to explicitly link compatible devices with a context menu...

Starting to sound like the "back panel" view of Reason :wink:

[AndrewBelt]

Updated proposal implementation:

• Add Model *Module::model.
• Add int Module::leftModuleId (resp right), which is set by the patch loader or RackWidget. The ID does not have to exist yet. No locking is required to set this variable.
• Add Module *Module::leftModule, which is managed by the Engine. Every "engine mutex locking" which currently is 64 samples, the engine will check all modules and make sure that leftModule and leftModuleId are synced.

Still thinking about how messages can be read and written to expander modules in a way that guarantees one sample latency in multi-threaded mode. Not sure of the details, but I'd like that functionality to be added to Module rather than Engine, for isolation and customization reasons.

[AndrewBelt]

Okay, here's the final piece I was looking for.

• Add void *Module::leftProducerMessage and void *Module::leftConsumerMessage (resp right).

If a Module wants to allow messages to be received, it can allocate two memory blocks (primitive, array, struct, etc) in its constructor (or simply use its own member fields) and set the producer and consumer message pointers. The module can read from the consumer message during its process() method. Other modules can write to its producer message during their process(). The engine will swap both pointers at the end of each sample step.

The only performance cost of this feature will be iterating through all modules every sample on the main engine thread. Probably less than 0.1% CPU.

[MarcBoule]

Will there be an example where we can see this in action, or at first you are only going to implement it in a closed-source plugin?

[AndrewBelt]

It will first be tested with Grayscale Permutation / Variant, which is closed-source.

So here's an example of an expander which displays 8 lights if placed to the right of its mother module. Let me know if this example is incorrect or insufficient.

struct Mother : Module {
    void process(const ProcessArgs &args) override {
        if (rightExpander.module && rightExpander.module->model == modelExpander) {
            // Get message from right expander
            float *message = (float*) rightExpander.module->leftExpander.producerMessage;
            // Write message
            for (int i = 0; i < 8; i++) {
                message[i] = inputs[i].getVoltage() / 10.f;
            }
            // Flip messages at the end of the timestep
            rightExpander.module->leftExpander.messageFlipRequested = true;
        }
    }
};

struct Expander : Module {
    float leftMessages[2][8] = {};

    Expander() {
        leftExpander.producerMessage = leftMessages[0];
        leftExpander.consumerMessage = leftMessages[1];
    }

    void process(const ProcessArgs &args) override {
        if (leftExpander.module && leftExpander.module->model == modelMother) {
            // Get consumer message
            float *message = (float*) leftExpander.consumerMessage;
            for (int i = 0; i < 8; i++) {
                lights[i].setBrightness(message[i]);
            }
        }
        else {
            // No mother module is connected.
            // TODO Clear the lights.
        }
    }
};

[MarcBoule]

Thanks for the code sample, it's much clearer now. The last thing I'm not 100% sure about concerns directionality; if we want the opposite direction, that is the expander to send data to the mother module, I assume we can use the mechanism in reverse: in the example, expander would write into producerLights and then the mother would read consumerLights, correct?

[AndrewBelt]

@MarcBoule Yeah, just switch the names Mother <-> Expander, and that's how you'd send in the opposite direction.

[MarcBoule]

With small expanders such as I use in Clocked and my Phrase/Gate Sequences (4HP), in your opinion is it better to utilise this new way of doing things or would it be acceptable to just dynamically widen/shorten the width of the module as I currently do?

I guess a related question is if Rack 1.0 will automatically move all down-row modules to the right if a given module gets widened in real time.

No problem for me to do either method, just wondering what is best in terms of UI/UX.

• Advantage of my current method: no separate modules in the module browser for the expanders.
• Disadvantage of my current method: expander overlaps with neighboring module if sufficient space is not made in the Rack before adding the expander.

[AndrewBelt]

Dynamic resizing will never be supported, so you'd better use expanders.

[MarcBoule]

All good. I'll be using "_Expander_" tag for those modules then, and if ever that is not the proper tag, please let me know. Thanks Andrew!

[AndrewBelt]

Completed proposal implementation in 9943d7b

[MarcBoule]

I just used this in Clocked to make its (separate) expansion panel, and it worked perfectly. The only tweak I would suggest to the code sample given 7 comments up from this one would be to rename the floats lights[8] array, since its name conflicts with the actual lights of the module.

[AndrewBelt]

Fixed. Good to hear.

A gotcha is that when disconnecting an expander, the buffer is not automatically cleared (because the engine doesn't know or care what data it contains, if any), so you should check that the expander's left module is its "mother" module. I've updated the example above.

[MarcBoule]

Indeed, it looks like I hadn't tested enough, and I see what you mean :-)
Thanks for the new info, I added that check and now it properly disconnects when we have even a single HP in between mother and expander, which seems much cleaner. Thanks Andrew!

[MarcBoule]

I've been doing some more testing with expanders, and I'm still seeing a slightly inconsistent behavior. The expander sees that it is still next to the mother module even after we separate them (the mother correctly sees that the expander is no longer connected). In my description, the expander is on the right side of the mother module.

Although I saw that you have a TODO in the code in RackWidget_updateAdjacent(), in app/RackWidget.cpp, I'm wondering if there could be an explanation for this in there. I notice in the code of that method, there is this:

        if (!found) {
            m->module->rightModuleId = -1;
        }

but at no point do we have code that will set leftModuleId = -1 in that method. Perhaps the code would need to do both left and right side checks for this reason? Or perhaps before starting the O(n^2) loops, it should reset the leftModuleId of all modules to -1 in a separate O(n) loop. Just tossing ideas around, if this is indeed a problem, you surely know how to fix it better than me! :-)

[squinkylabs]

I'm a little unsure of the threading model here. I get that the shared buffer is shared by different engine threads, and therefore you need to think of the communication as message passing. But to have two thread reading and writing to the same float simultaneously - are we assuming that on all CPU architectures that VCV runs on (Intel x64 only, I guess?) that reads and writes to 32-bit floats are atomic? Because standard c++ could have you declare these as std::atomic, right?

[AndrewBelt]

@squinkylabs

I get that the shared buffer is shared by different engine threads

It's not. See the above post https://github.com/VCVRack/Rack/issues/1204#issuecomment-469792020

[AndrewBelt]

@MarcBoule Thanks, fixed in 35a951690dbe7f632334ac25974184f5a368b08e. Does this solve your issue?

[MarcBoule]

All good now, thanks Andrew 👍

[squinkylabs]

Ah, ok. tx! Is there a "contract" around the order that module's process() gets called? Will left always get consistently called either before or after me? I want to pass audio over this pipe, and of course don't want to incur jitter by having a random plus or minus one sample delay over the pipe.

[AndrewBelt]

@squinkylabs Order of process() calls is undefined. It shouldn't matter. Message passing is always 1-sample latency because the producer and consumer buffers are flipped at the end of each engine timestep.

[squinkylabs]

Oh, of course! so as far as sample "frames" everyone is marching in-lock step, even though in "wall time" the processing is in arbitrary order. Obvious in retrospect, but a relief none-the-less.

[MarcBoule]

Just to update those following this thread here: expanders now require message-flip requests (see commit 7bd98943db33a972c8c1d6956558789083ff3058).

[AndrewBelt]

Changed API in 54544bb. Use

leftModule -> leftExpander.module
leftProducerMessage -> leftExpander.producerMessage
leftConsumerMessage -> leftExpander.consumerMessage
leftMessageFlipRequested -> leftExpander.messageFlipRequested
rightModule -> rightExpander.module
rightProducerMessage -> rightExpander.producerMessage
rightConsumerMessage -> rightExpander.consumerMessage
rightMessageFlipRequested -> rightExpander.messageFlipRequested

Pushing a message to a left expander module now looks like

float *buffer = (float*) rightExpander.module->leftExpander.producerMessage;
buffer[0] = ...