Inconsistences in assigning memory for voice data

[diyelectromusic]

There are a number of places where voice data is processed that uses hardcoded sizes and there is one place where I think the size is wrong.

Voice data is defined in the following places: https://github.com/probonopd/MiniDexed/blob/main/src/sysexfileloader.h#L35 https://github.com/probonopd/MiniDexed/blob/main/src/performanceconfig.h#L29

But it is also hard-coded in the following places: https://github.com/probonopd/MiniDexed/blob/main/src/voices.c#L6 https://github.com/probonopd/MiniDexed/blob/main/src/minidexed.cpp#L459 https://github.com/probonopd/MiniDexed/blob/main/src/minidexed.cpp#L940 (possibly) https://github.com/probonopd/MiniDexed/blob/main/src/minidexed.cpp#L962 (possibly) https://github.com/probonopd/MiniDexed/blob/main/src/sysexfileloader.cpp#L376 https://github.com/probonopd/MiniDexed/blob/main/src/sysexfileloader.cpp#L434 and I'm pretty sure its tied to this too: https://github.com/probonopd/MiniDexed/blob/main/src/sysexfileloader.cpp#L414

And it is hard-coded here but I think it is wrong - this looks like it misses the last byte of the voice data: https://github.com/probonopd/MiniDexed/blob/main/src/minidexed.cpp#L1448 https://github.com/probonopd/MiniDexed/blob/main/src/minidexed.cpp#L1461

For what its worth there are similar issues in Dexed itself, but for our purposes the getVoiceData doesn't check the input buffer we provide, but instead goes on the size of its internal data structure it copies from, which is defined as follows: uint8_t data[NUM_VOICE_PARAMETERS]; where NUM_VOICE_PARAMETERS is 156.

These should all be using the same definition of course and using the value of 156 to match that used in Dexed.

Kevin

[probonopd]

Having to size things upfront is one of my least favorite aspects of C. Maybe the cleanest solution would be to define the NUM_VOICE_PARAMETERS in just one file and then include that file in all the other files that are currently defining the same variable under different names. I think it is good practice not to define multiple variables that describe the same value.

[diyelectromusic]

But also remember that interfaces and protocols /do/ need things defining up front (sizes, types, endianness, formats). That is kind of the point.

But yes, in an ideal world there would be a file that defines everything about the DX7, possibly one that then goes on to define everything about Dexed, and so on.

But yes, everything ought to use the definition in performanceconfig.h really (including the one in sysexfileloader.h).

To be honest, that value (156) hasn't changed in around 40 years... but I do think the 155 value is a bug - I haven't looked into the detail of how it works yet. I suspect the SysEx message it generates must be a byte short, but presumably the checksum still checks out... the concern is that the getVoiceData call writes 156 bytes into a 155 byte buffer, which obvs isn't great, but it is entirely possible there is some padding/etc on the stack which means we get away with it (155 being an odd number for a 32 bit/64 bit native cpu). But I've not investigated it.

Kevin

[BobanSpasic]

Operators On/Off parameter is part of VCED (as byte 156), but not of the VMEM format ("decompressed" gives 155 bytes). DX7 has 32 places for voice storage, plus one edit buffer. At receiving VCED SysEx, this goes to the edit buffer. Receiving VMEM SysEx goes to the 32 voices. CC messages can also transmit the Operators On/Off parameter. At receiving VMEM, all the operators will be set to On.

[diyelectromusic]

Ah interesting. Synth_Dexed's getVoiceData() will still memcpy 156 bytes into this 155 byte buffer mind as it uses sizeof(data) in the call :)

[BobanSpasic]

Got to correct myself here. Byte 156 isn't sent in VCED. It is a stored parameter, but it isn't sent in MIDI dumps at all (VCED or VMEM). It can be set by CC or from the keyboard's editing options.

So, here we have a difference with Korg Volca FM - Volca uses a modified VCED, where byte 156 is sent instead of the Checksum byte. Checksum isn't sent or calculated at all. It means, one can send a patch from DX7 to Volca FM, but DX7 does not accept patches for Volca FM because of the "wrong checksum".

[diyelectromusic]

Right glad that's nice and clear then!

:)