-
Notifications
You must be signed in to change notification settings - Fork 92
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
base: main
Are you sure you want to change the base?
Sys ex control #287
Conversation
Update build.yml
Minor update to build yaml
Hi @BenZonneveld, thank you very much. |
Replace my build.sh with the original. For testing I removed the extracting of the arm compiler and rebuilds of circle |
Can you please change that in https://github.com/BenZonneveld/MiniDexed/tree/SysExControl? Then the build in this Pull Request should "magically" start working. |
Update
Revert changes unrelated to the PR
Revert changes unrelated to the PR
Something weird is going on with the git submodules:
|
@BenZonneveld would this fix |
No...actually I removed the sending of sysex voices and only send the sysex on a patch request sysex, as it should be. |
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. |
Yes, we should definitely not remove this feature! |
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 |
It could be made configurable. |
And there we have a building branch with my patches. |
It builds! 👍 Can you describe what exactly this PR fixes/improves? I figured out:
|
Would #311 do the same job instead of having a private copy of Circle's |
...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. |
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. |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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).
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)); |
There was a problem hiding this comment.
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).
Iterator->second->Send (banksysex, sizeof(banksysex)*sizeof(uint8_t)); | |
Iterator->second->Send (banksysex, sizeof(banksysex)); |
Copilot uses AI. Check for mistakes.
The description I wish this PR would have had to begin with: Add support for
1. Changes to
|
@diyelectromusic @soyersoyer what do you think about the proposed changes? |
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 |
These have since been implemented. Is there any documentation on SysEX somewhere? |
You mean, like, in the DX7 manual? |
Several improvement in sysex control.
All instances get a fixed sysex channel as not to cause any conflicts.