Attempt to fix #646

[probonopd]

https://github.com/probonopd/MiniDexed/issues/648

[github-actions[bot]]

Download the artifacts for this pull request:

• MiniDexed_2024-05-30-76aefa7.zip

[probonopd]

Thanks @diyelectromusic. It looks like my PR fixes one instance of this issue (at least the build log doesn't show one of the errors anymore); the other would need to be fixed by a similar change in Synth_Dexed. Please do review my change and, if you have a chance, test the build. Thanks!

[diyelectromusic]

I think the issue is that the fixed length 10 might be correct - it is setting the voice string within the fixed DX voice structure I think...

... but I'll need to have a proper look as it is also tied up with the mess that is get/setName in Dexed (more here: https://github.com/probonopd/MiniDexed/issues/501)

Kevin

[diyelectromusic]

Ok, yes, looking again, we do need the fixed 10 in the strncpy as that is the size of the voice name in the voice structure for the DX7.

So I think the fix will be the following:

void CMiniDexed::SetVoiceName (std::string VoiceName, unsigned nTG)
{
    assert (nTG < CConfig::ToneGenerators);
    assert (m_pTG[nTG]);
    char Name[11];
    strncpy(Name, VoiceName.c_str(),10);
    Name[10] = '\0';
    m_pTG[nTG]->getName (Name);
}

A similar thing is probably required in Synth_Dexed/dexed.cpp but as I say that is mixed up with the problems described in #501 so that would be a breaking change in the API to fix... but it really ought to be fixed!?

It does address my comment in #501 about SetVoiceName only have a buffer of 10 though - that is wrong and does need to be 11 as shown above.

Kevin

[probonopd]

Why not use VoiceName.size() as suggested in my PR? That way, we don't need to hardcode any sizes. I try to never hardcode sizes, as it is a frequent source of bugs.

[diyelectromusic]

Why not use VoiceName.size() as suggested in my PR? That way, we don't need to hardcode any sizes. I try to never hardcode sizes, as it is a frequent source of bugs.

Because this function is about truncating whatever voice name is passed in (which could be any size) to the explicit 10 characters defined in the DX7 SysEx voice structure, so as I say, the 10 is actually correct in this case, just not really accounted for properly in how its used.

We could make the argument that GetName should do all that - and yes, I agree it ought to - but GetName/SetName is a mess as per https://github.com/probonopd/MiniDexed/issues/501 and can't be fixed without breaking the API so I doubt that will happen.

So best just to do the robust thing here. In an ideal world I agree we wouldn't have hardcoded sizes like this, but the DX7 voice structure has been fixed for over 40 years, so I don't think we need to worry about that in this case :)

Kevin

[probonopd]

the DX7 voice structure has been fixed for over 40 years, so I don't think we need to worry about that in this case :)

Good point 👍