The epic digital bus thread

[antisvin]

I think that in order to expose digital bus to user patches, the following would be needed:

• callbacks on receiving of commands, data and messages
• service calls for sending the same entities plus sending parameters, getting number of discovered peers and bus state
• in order to fully utilize digital bus with more than one device, some way to assign device names is required

Most of this stuff is a part of bus.h, but doesn’t have implementation other than debug printing.

For now I will concentrate on first 2 points and I mostly understand how they can be implemented. Besides that, I think an extra service call for switching patches should be added - it’s not required for digital bus, but something that could run as callback for commands in some cases.

[antisvin]

Noticed another missing thing - sending buttons over bus. It’s mentioned in bus.h, but was missing in standalone DigitalBus repo and not implemented in firmware. It should not be hard to add. This would also require adding an entry to OwlProtocol enum , I was thinking about using 0xb1 as it’s similar to parameter.

Also thinking about converting code from DigitalBus repo to a standalone library (free of Juce that I don’t like using). It should be possible to use it as is from something like Teensy or use FFI to access from Python (a desktop client made with PyQt could be interesting project)

[antisvin]

Some of the above ended up incorrect, but I’ve pushed a commit that adds necessary callbacks and missing pieces for digital bus. Direct connection between Magus and Owl works after buffer fixes on both sides, reconnect also works somehow. Next I would be adding some code to OwlProgram to handle this in patches and trying to make something useful out of it.

[mars]

great work!

The UART DMA transfer is probably intended to work with an IDLE interrupt, which calls the DMA callback with a variable amount of data transferred.

[antisvin]

Ok, I’ve understood the general idea about triggering DMA receive on idle interrupt. I will check if the issue is still happening with large buffer size - I was previously disabling idle interrupts once when looking for source of that hanging issue. So maybe I didn’t enable them initially and that caused this regression.

[mars]

I think otherwise transfer size of 4 is fine, too. IDLE IRQ is a bit tricky to get working right.

[mars]

Apart from enabling the interrupt, you will need something like this in the IRQ:

/**
  * @brief This function handles UART4 global interrupt.
  */
void UART4_IRQHandler(void)
{
  /* USER CODE BEGIN UART4_IRQn 0 */

  /* USER CODE END UART4_IRQn 0 */
  HAL_UART_IRQHandler(&huart4);
  /* USER CODE BEGIN UART4_IRQn 1 */
  /* Check for IDLE flag */
  UART_HandleTypeDef *huart = &huart4;
  if(huart->Instance->SR & UART_FLAG_IDLE){
    /* This part is important */
    /* Clear IDLE flag by reading status and data registers */
    volatile uint32_t tmp;                  /* Must be volatile to prevent optimizations */
    tmp = huart->Instance->SR;              /* Read status register */
    tmp = huart->Instance->DR;              /* Read data register */
    (void)tmp;                              /* Prevent compiler warnings */
    if(huart->hdmarx != NULL)
      huart->hdmarx->Instance->CR &= ~DMA_SxCR_EN;       /* Disabling DMA will force transfer complete interrupt if enabled */
    /* DMA1_Stream2->CR &= ~DMA_SxCR_EN;       /\* Disabling DMA will force transfer complete interrupt if enabled *\/ */
  }

  /* USER CODE END UART4_IRQn 1 */
}

[antisvin]

Guess you’ve missed the part where I’ve mentioned that I’ve copied this code from Prism . So it’s already there, but I’m not sure that idle interrupts were not disabled when I’ve added it.

I think that using 4 byte buffer could reduce performance if we send larger chunks of data or use a lot of devices. In other words, when there are a lot of successive transmissions. Otherwise it wouldn’t matter as we’ll get to idle state after first frame is sent.

[antisvin]

Well, it still wasn’t working. And then I’ve found a very silly mistake - code that I had was triggering wrong DMA channel (used by TX rather than RX). Sample you’ve just posted is using handler structure, which is certainly less error-prone. This fixed the issue at last.

[antisvin]

Here’s more or less what I’m trying to make in OwlProgram repo:

1. Most of functionality visible to end user will be in a separate class (DigitalBus) with an instance assigned to patch as .bus attribute if USE_DIGITALBUS flag is set

2. bus.registerParameter() or bus.registerButton() would be used with pid/bid argument (FloatParameter or IntegeraParameter instances could also be used instead of PID). Callback functions would send those parameters/buttons to bus once for every buffer. We will probably skip making those calls if outgoing buffer is full.

3. patch.handleMessage and bus.handleData would be user-defined callbacks for receiving objects of those types

4. initially I was thinking about making some sort of command registry in DigitalBus and only sending only registered commands to callback function. But now I’m not sure if that’s really necessary, as we can just pass command ID to patch.handleCommand and let the user write something like switch(command_id) … in there

5. bus.sendMessage, bus.sendData and bus.sendCommand probably don’t need explanation

6. Incoming data for messages is in bytes, so I’ve converted IntArray to a template class that would be used with int32_t and uint8_t types. It should be backwards compatible with existing code (ARM optimizations would also be used via type specialized methods). And there would be ByteArray class for passing bus data.

7. There would be a few functions like bus.isEnabled(), bus.enable(), bus.getPeers(), bus.getStatus() that would use service calls for accessing relevant parts of firmware.

[antisvin]

Made some progress with OwlProgram code. Some things may still change (haven’t event tried compiling this part yet), but I think DigitalBus.h will look pretty much like this:

#ifndef __DigitalBus_h__
#define __DigitalBus_h__

#include "main.h"
#include "IntArray.h"
#include "OpenWareMidiControl.h"
#include "PatchParameter.h"
#include "PatchProcessor.h"

static constexpr uint8_t MAX_BUS_PARAMETERS = 16;
static constexpr uint8_t MAX_BUS_BUTTONS = 16;
static constexpr uint8_t MAX_BUS_COMMANDS = 16;

// We don't really need 4 bytes here, but that's what we get from service call
enum DigitalBusStatus : int32_t {
    BUS_STATUS_IDLE      = 0x00,
    BUS_STATUS_DISCOVER  = 0x01,
    BUS_STATUS_CONNECTED = 0x02,
    BUS_STATUS_ERROR     = 0xff,
    BUS_STATUS_UNKNOWN   = -1    // This is used when service call fails, which should not ever happen
};

class DigitalBus {
public:
    DigitalBus();
    ~DigitalBus();
    // Parameter/button registration
    void registerParameter(const PatchParameterId& pid) {
        if (num_parameters < MAX_BUS_PARAMETERS - 1)
            registered_parameters[num_parameters++] = pid;
    }
    void registerParameter(const FloatParameter& param) {
        if (num_parameters < MAX_BUS_PARAMETERS - 1)
            registered_parameters[num_parameters++] = param.pid;
    }
    void registerParameter(const IntParameter& param) {
        if (num_parameters < MAX_BUS_PARAMETERS - 1)
            registered_parameters[num_parameters++] = param.pid;
    }
    void registerButton(const PatchButtonId& bid) {
        if (num_buttons < MAX_BUS_BUTTONS - 1) 
            registered_buttons[num_buttons++] = bid;
    }
    // Protocol object sending
    bool sendCommand(uint8_t cmd_id, uint16_t arg);
    bool sendData(const ByteArray& data);
    bool sendMessage(const char* msg);
    // Various settings
    bool isEnabled();
    void enable(bool state);
    DigitalBusStatus getStatus();
    int32_t getPeers();
private:
    void clear();
    PatchParameterId registered_parameters[MAX_BUS_PARAMETERS];
    uint8_t num_parameters;
    PatchButtonId registered_buttons[MAX_BUS_BUTTONS];
    uint8_t num_buttons;

    /*
    * I think parameters should be sent by this friend function.
    * Another option would be to have a callback and let firmware call it,
    * but this seems like an unnecessary service call with little benefit
    */
    friend void PatchProcessor::setParameterValues(int16_t *params);
    void sendParameters() {
        if (bus_parameter_callback != NULL && num_parameters > 0) {
            bus_parameter_callback(num_parameters, (uint8_t*)registered_parameters);
        }
    }
    bool sendButtons() {
        if (bus_button_callback != NULL && num_buttons > 0) {
            bus_button_callback(num_buttons, (uint8_t*)registered_buttons);
        }
    };

    /* Setup is made friend to avoid exposing callbacks or writing setters */
    friend void setup(ProgramVector *pv);
    void (*bus_parameter_callback)(uint8_t, uint8_t*) = NULL;
    void (*bus_button_callback)(uint8_t, uint8_t*) = NULL;
};

#endif

This is a request for comments, since I’m hoping that this could be merged to upstream eventually

[antisvin]

Martin, when I upload patch on one device with MIDI forwarding enabled, SySex data is also being sent over the digital bus. Is that by design or it’s better to try to omit such data transfers? Result is a harmless “Invalid SysEx” error and thousands received frames. Not sure if such patch distribution would work for the same devices.

Or maybe we could allow SysEx forwarding except patch uploads if someone would have such obscure need.

[antisvin]

This behavior with patch sending seems to happen even with MIDI forwarding disabled, so this is likely a bug. I’ll try to investigate it myself.

First test patch that I’m using as trivial test:

#ifndef __BusParamPatch_hpp__
#define __BusParamPatch_hpp__

#include "Patch.h"


class BusParamPatch : public Patch {
public:
    BusParamPatch(){
        registerParameter(PARAMETER_A, "A");
        registerParameter(PARAMETER_B, "B");
        registerParameter(PARAMETER_C, "C");
        registerParameter(PARAMETER_D, "D");
#ifdef OWL_MAGUS
        // Magus is parameter slave
#else
        // OWL is parameter master
        bus.registerParameter(PARAMETER_A);
        bus.registerParameter(PARAMETER_B);
        bus.registerParameter(PARAMETER_C);
        bus.registerParameter(PARAMETER_D);
#endif
    }

  void processAudio(AudioBuffer &buffer){
  }
};

#endif

I’ve actually got this to work (client side is mostly ready now) and saw parameters from Owl synced to Magus! But there are some things that need to be fixed. I.e. I’m sending parameters state on every audio frame and that creates too much packages - will make it to happen incrementally on parameter change.

Also, I’ve seen the same issue that happened previously after one of them was rebooted- devices get stuck in reconnection loop. This doesn’t normally happen when bus is idle, but if I send patch (generating lots of data in bus), discovery stops working. I suspect some kind of issue with frames alignment. Maybe bus reset is not taking something into consideration.

[antisvin]

I think I may have run into some bugs in OpenWare, because the behavior I’m seeing from the patch above doesn’t make sense as a bug from code I’ve added. Specifically this:

1. If at least one of parameters B-D on Owl are > 0, they are sent to bus
2. If all of them are == 0, param A value does not get sent to bus - no matter what’s its value

I suspect this may have to do something parameters comparison to NULL that I’ve seen in parameter updater code. Maybe 0 is equal to it and setting params exactly to 0 doesn’t work.

Also I’ve added incremental update sending, but looks like it’s not very effective due to potentiometer noise (will try comparing param change with some threshold as a workaround). However, if I set any pot to 0, its state is not being transferred. If I set one of B-D very close to zero and others set to 0, data is transferred infrequently - I suppose it happens when noise level gets above some threshold (either 1 or some value is set somewhere). In such case pot A state is update only when those updates arrive.

So parameter behavior is very strange in some regards. Also, I’ve disabled code that was inverting params on OWL in OwlProgram - I guess it was needed for legacy FW only. I’ll check if current behavior is not a side effect of that.

[antisvin]

Did some more troubleshooting, looks like I had wrong leftover callback - used initially for transferring all parameters before I’ve added incremental update sending. This fixed weird param A behavior. But other issues are still present. I’ve made some measurements to see what exactly is being transferred for one of parameters:

• min value is 22, which stops data from being transferred
• max value is 3979
• everything above min value has noise, usually going ±1 from current value

Is this something worth solving? I’m considering running params through moving average filter before sending. This is very cheap computationally (one addition, one substraction and one bit shift per param + array of something like 8/16/32 values). I’m not sure if I should just do it in digital bus code in client side or add this smoothing to FW itself. I think I’ve noticed that problem previously when using Owl as a VCO with one of parameters set to tuning control.

And it looks like we need to have ADC calibration tables for params to match full pot range correctly.

[antisvin]

Found out that there’s optional parameter smoothing in patch processor code from OpenWare. It should probably be done earlier (i.e. somewhere after reading ADC values) in order to be available for dynamic patches too.

Ok, code should be rewritten to use LinearParameterUpdater/SmoothValue instead of just sending raw values. That would probably solve issue with sending noisy params.

[antisvin]

So currently data from ADC is stored in parameters as is, all smoothing is performed later on scaled values. I’ve changed digital bus code to create separate instances that uses the same values range (0-4095) as parameter values, but supports smoothing. This lowers amount of updates that are made by 5-10 times.

Also found the place in Owl.cpp where MIDI data is forwarded to digital bus unconditionally, which caused it to send SYSEX patches via bus. I will add a check that settings.bus_forward_midi is set and confirm it solves this issue.

I think that MIDI forwarding should be exposed to user patches. This way a patch can toggle midi sending as necessary. It would be very interesting to use Magus as MIDI host for multiple devices.

Next, started looking into button state sending, initial code doesn’t work as expected yet.

[antisvin]

Just noticed that forces of darkness have been increasing in numbers in development branch lately. And at least one of them has UART enabled.

[antisvin]

Brief update:

• sending commands/data/messages sort of works in user patches, but there are issues with reconnects that needs to be addressed to make the latter 2 more reliable. Data and messages from disconnected devices are ignored by protocol handler, so patches would have to check if there’s an established connection before sending them.
• will add reset callback to handle disconnects (i.e. will be used to track connection state without constantly polling for state updates)
• will add parameter remapping as a workaround for multiple devices that send values from ADCs as parameter inputs. I’ve noticed that there was unused “parameterOffset” attribute in DigitalBusHandler code that likely was meant for something similar. But we can’t get away with a static offset, because it’s not needed for every parameter and exact number depends on parameters that will be mapped to bus on other devices.

[antisvin]

Some more findings:

1. Noticed that USB cable connected to Magus devices port causes idle detection to stop working. Possibly a ground loop through Owl connected to programmer.

2. OWL’s gate input didn’t work. Found out that it uses interrupt EXTI_9_5 which was disabled in Cube project, re-enabling it got it working (triggers LEDs).

Thinking about a PR for cube rollback (STM32F4 I2S code is broken in cube for a year or so) and this fix. Maybe patch switching could be added to it.