Skip to content

Sys ex control #287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Conversation

BenZonneveld
Copy link

Several improvement in sysex control.
All instances get a fixed sysex channel as not to cause any conflicts.

@probonopd
Copy link
Owner

probonopd commented Jun 20, 2022

Hi @BenZonneveld, thank you very much.
I am a bit puzzled by the build error.
Can you build it locally for the Raspberry Pi 4?

@BenZonneveld
Copy link
Author

Replace my build.sh with the original. For testing I removed the extracting of the arm compiler and rebuilds of circle

@probonopd
Copy link
Owner

Can you please change that in https://github.com/BenZonneveld/MiniDexed/tree/SysExControl? Then the build in this Pull Request should "magically" start working.
Thanks!

BenZonneveld and others added 6 commits June 20, 2022 22:40
@probonopd
Copy link
Owner

Something weird is going on with the git submodules:

Cloning into '/home/runner/work/MiniDexed/MiniDexed/CMSIS_5'...
Cloning into '/home/runner/work/MiniDexed/MiniDexed/Synth_Dexed'...
Cloning into '/home/runner/work/MiniDexed/MiniDexed/circle-stdlib'...
Submodule path 'CMSIS_5': checked out '8a64562247485159a090ec4a01588f44f8d10311'
fatal: remote error: upload-pack: not our ref a3a696b003ef76f79dbd91ff8188a[12](https://github.com/probonopd/MiniDexed/runs/6975070899?check_suite_focus=true#step:3:13)3b74e5e17
fatal: Fetched in submodule path 'Synth_Dexed', but it did not contain a3a696b003ef76f79dbd91ff8188a123b74e5e17. Direct fetching of that commit failed.
Error: Process completed with exit code 128.

@probonopd
Copy link
Owner

probonopd commented Jun 20, 2022

@BenZonneveld would this fix
asb2m10/dexed#352?

@BenZonneveld
Copy link
Author

No...actually I removed the sending of sysex voices and only send the sysex on a patch request sysex, as it should be.
A program change shouldn't send a sysex with a voice dump.

@dcoredump
Copy link
Contributor

No...actually I removed the sending of sysex voices and only send the sysex on a patch request sysex, as it should be.
A program change shouldn't send a sysex with a voice dump.

I had added this because then the editors immediately display the correct data. Maybe this should be made configurable via minidexed.ini, because I found it a great help so far.

@probonopd
Copy link
Owner

Yes, we should definitely not remove this feature!

@BenZonneveld
Copy link
Author

No...actually I removed the sending of sysex voices and only send the sysex on a patch request sysex, as it should be.
A program change shouldn't send a sysex with a voice dump.

I had added this because then the editors immediately display the correct data. Maybe this should be made configurable via minidexed.ini, because I found it a great help so far.

Still, the normal way for an editor would be to do a patch request. It is what my MiniDexed master does: receive pgm change, request voice data.

If you always send the sysex voicedump it would mean I can never use MiniDexed with for example my DX7

@probonopd
Copy link
Owner

It could be made configurable.

@BenZonneveld
Copy link
Author

And there we have a building branch with my patches.

@probonopd
Copy link
Owner

probonopd commented Jul 6, 2022

It builds! 👍

Can you describe what exactly this PR fixes/improves?

I figured out:

  • More MIDI commands received via serial are interpreted
  • More MIDI control change messages are sent via serial
  • Convert NL+CR to NL on serial output

@probonopd
Copy link
Owner

probonopd commented Jul 17, 2022

Would #311 do the same job instead of having a private copy of Circle's serial.h? If possible I would like to avoid a private copy of that file, in order not to have the burden of maintenance. It looks to me like #311 also converts NL+CR to NL on serial output, but without the need for a patched serial.h.

@probonopd
Copy link
Owner

I had added this because then the editors immediately display the correct data. Maybe this should be made configurable via minidexed.ini, because I found it a great help so far.

...or we ask editors (yes I am thinking of you, Dexed) to send a patch request, like @BenZonneveld is saying? This is probably the way it should be done in MIDI.

@psethwick
Copy link

I tracked down the reason that my M-Audio Oxygen49 quits working after a program change to: receiving the sysex data afterwards, so +1 for configuration parameter, at least!

My keyboard probably shouldn't crash, but, well, it does. Probably something to do with its own memory dump feature.

@probonopd probonopd requested a review from Copilot April 14, 2025 16:58
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 8 out of 12 changed files in this pull request and generated 2 comments.

Files not reviewed (4)
  • Synth_Dexed: Language not supported
  • build.sh: Language not supported
  • circle-stdlib: Language not supported
  • rebuild.sh: Language not supported
Comments suppressed due to low confidence (1)

src/minidexed.cpp:449

  • Correct the typo in the log message by changing 'Resonanece' to 'Resonance'.
LOGDBG("Set Resonanece for TG %i", instanceID);

void CMIDIDevice::SendCtrlChange14Bit(uint8_t ctrl, int16_t val, uint8_t nTG)
{
uint8_t lsb = (val & 0x7f);
uint8_t msb = (val >> 9)&0x7f;
Copy link
Preview

Copilot AI Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shift amount in SendCtrlChange14Bit appears incorrect; for a 14-bit value the MSB should be extracted using (val >> 7) instead of (val >> 9).

Suggested change
uint8_t msb = (val >> 9)&0x7f;
uint8_t msb = (val >> 7)&0x7f;

Copilot uses AI. Check for mistakes.

// send voice dump to all MIDI interfaces
for(Iterator = s_DeviceMap.begin(); Iterator != s_DeviceMap.end(); ++Iterator)
{
Iterator->second->Send (banksysex, sizeof(banksysex)*sizeof(uint8_t));
Copy link
Preview

Copilot AI Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The size argument in SendBankName multiplies sizeof(banksysex) by sizeof(uint8_t), which is redundant since sizeof(uint8_t) is always 1; it should be simplified to sizeof(banksysex).

Suggested change
Iterator->second->Send (banksysex, sizeof(banksysex)*sizeof(uint8_t));
Iterator->second->Send (banksysex, sizeof(banksysex));

Copilot uses AI. Check for mistakes.

@probonopd
Copy link
Owner

The description I wish this PR would have had to begin with:

Add support for

  • Configuration dumps
  • Bank name requests
  • Improved alignment between SysEx messages and tone generators
  • Enhanced debugging and logging
  • Streamlined SysEx voice dumping and parameter updates

1. Changes to src/mididevice.cpp

a. Updated SysEx Handling

The HandleSystemExclusive method in CMIDIDevice now includes:

  • Instance ID Validation: Ensures that the SysEx messages align with the correct tone generator (TG) by comparing the SysEx instance ID (pMessage[2] & 0x0F) with the TG (nTG). A warning is logged if they do not match:
    uint8_t instanceID = pMessage[2] & 0xF;
    if (instanceID != nTG) {
        printf("WARNING instanceID and nTG do not match!!!!!\n");
    }
  • Support for New SysEx Requests:
    • Config Request: When SysEx ID 0x30 is received, it triggers a configuration dump.
    • Bank Name Request: When SysEx ID 0x40 is received, it sends the name of the current bank.

b. New SysEx Responses

  • Configuration Request (SendSystemExclusiveConfig):

    • Assembles a detailed configuration dump containing various tone generator parameters, including:
      • Compressor enable/disable.
      • Reverb settings (size, damp, low-pass, diffusion, level).
      • Volume, pan, master tune, cutoff, resonance, reverb send, pitch bend, portamento mode, time, etc.
    • Sends this configuration as a SysEx message to all MIDI devices.
  • Bank Name Request (SendBankName):

    • Constructs a SysEx message containing the bank name for a specific tone generator.
    • Extracts the bank name using:
      m_pSynthesizer->GetSysExFileLoader()->GetBankName(...)
    • Sends the message to all MIDI interfaces.
  • Voice Dump (SendSystemExclusiveVoice):

    • Refactored to use the instance ID instead of the TG directly, ensuring better alignment with SysEx messages.

2. Changes to src/mididevice.h

a. New SysEx-Related Methods

The following methods were added to the CMIDIDevice class:

  • SendSystemExclusiveConfig(): Sends a SysEx message containing the configuration of the synthesizer and tone generators.
  • SendBankName(uint8_t nTG): Sends the name of the current bank via SysEx.
  • Updated SendSystemExclusiveVoice to take only nVoice and nTG as parameters (removes nCable).

3. Changes to src/minidexed.cpp

a. SysEx Enhancements

  • Updated SysEx-related methods to ensure consistency with the new instance ID approach:
    • The getSysExVoiceDump method now sets the instance ID (nTG) in the SysEx message instead of the MIDI channel:
      dest[2] = nTG; // Sub-status and MIDI channel/Instance ID
  • Improved parameter change handling to trigger corresponding SysEx updates:
    • For example, when the volume is updated, a MIDI Control Change (CC) message is sent:
      m_SerialMIDI.SendCtrlChange(MIDI_CC_VOLUME, m_nVolume[nTG], nTG);

4. New MIDI Control Codes

Defined in src/mididevice.h:

  • Added new constants for MIDI Control Change (CC) messages:
    #define MIDI_CC_BANK_SELECT_MSB    0
    #define MIDI_CC_MODULATION         1
    #define MIDI_CC_VOLUME             7
    #define MIDI_CC_PAN_POSITION       10
    #define MIDI_CC_BANK_SELECT_LSB    32
    #define MIDI_CC_REVERB_LEVEL       91
    #define MIDI_CC_DETUNE_LEVEL       94

5. Logging and Debugging

  • Enhanced debugging information for SysEx handling:
    • Logs the return value from the SysEx handler:
      LOGDBG("SYSEX handler return value: %d", sysex_return);
    • Logs the details of the SysEx messages being processed, including the TG and instance ID.

@probonopd
Copy link
Owner

@diyelectromusic @soyersoyer what do you think about the proposed changes?

@diyelectromusic
Copy link
Collaborator

I think we need to isolate the key changes in this PR and show them applied to the latest code base - I'm struggling to see what depends on what... I mean what is with that serial.h for example? And all those build changes.

Also, I don't think we should be changing how the SysEx device number is interpreted without some discussion. We don't really have the concept in MiniDexed atm. I think we map it over to TG MIDI channel (rightly or wrongly).

Kevin

@soyersoyer
Copy link
Contributor

soyersoyer commented Apr 22, 2025

Added new constants for MIDI Control Change (CC) messages:

These have since been implemented.

Is there any documentation on SysEX somewhere?

@probonopd
Copy link
Owner

Is there any documentation on SysEX somewhere?

You mean, like, in the DX7 manual?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants