Added configuration needed to choose between Modern, OPL and Mark I engines

[donluca]

It compiles without errors but it's untested.

A code review is needed.

[probonopd]

Thank you very much @donluca. From glancing at Synth_Dexed I figured that:

# Engine Type
EngineType=MSFA

EngineType sets the synthesizer engine being used. Possible values are MSFA ("Modern (24-bit)", default), MKI ("Mark I"), and OPL ("OPL Series").

And here is the compiled version for testing:
MiniDexed_2023-02-26-4bb45cc

I'd be happy to hear from those who had suggested this feature whether this works as intended.

[donluca]

Right, apologies, I should have written that in the comments.

As discussed, I've left MSFA (the "Modern" engine) as the default as that's what people using Minidexed up until now were familiar with.

The other options are MKI ("Mark I" engine, the more accurate one towards the DX7 Mk1 and current standard engine in Dexed) and OPL ("OPL" engine which is the predecessor of the "Mark I" engine).

When I get my Rpi2 I'll test this myself.

[donluca]

I don't understand why the build is now failing here, on my repo it is correctly compiling...
I've double checked the changes and they all match what I have on my part.

Should I delete the PR and redo it from scratch?

EDIT: ok, wow, if I compile it for arm there are no errors, but if I try to compile for aarch64 this error pops up:

config.cpp:125:17: error: cast from 'const char*' to 'unsigned int' loses precision [-fpermissive] 125 | m_EngineType = (unsigned)m_Properties.GetString ("EngineType", "MSFA"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

As I said... I hope someone can check my code and see if I messed up somewhere because I know next to nothing about C.
Thanks.

[donluca]

I have managed to "fix" the issue, but I have a feeling this is not going to work...

[donluca]

This is the one.

Sorry about the commit spam @probonopd , the previous build you shared won't work because I forgot an important part of the code (to actually read from the config file) and further attempts had me fighting over type casting (who knew that uint8_t could actually store a string instead of just integers...).

Now it should be ok and good for testing.

[probonopd]

Build from your latest commit: MiniDexed_2023-02-27-7eed325

[diyelectromusic]

Erm... do you know this works? It looks like Dexed.h's setEngineType() is supposed to take one of the values from the enum ENGINE as a parameter (which with no initialisation, one would take to be 0,1,2), but it looks like you're passing in the first character of the string in the config file...? By default, luckily it looks like if Dexed doesn't recognise the passed in engine it defaults to MSFA...

I might be missing something (its a little late and I'm a bit tired), but thought I'd check just in case!

(btw - uint8_t can't store a string without some kind of buffer overrun... some might regard that as a feature ;))

Kevin

[donluca]

I think I've stated multiple times that I'm no C coder and that this really needs a code review.
To reply to your question: of course I do not know if this works as I'm still waiting for my RPi2 to arrive to test this myself, which is why we encouraged people to check the code/try it out on their setups to see if it actually works.

I've asked dcoredump about the right way to set the engine here and he acknowledged that m_pTG[i]->setEngineType(MKI); would, for example, set the engine to Mark I.

The function pConfig->GetEngineType () normally returns a const char* while setEngineType() expects a uint8_t so I had to cast m_Properties.GetString ("EngineType", "MSFA"); (which, if I understood correctly, reads the config file and, if the value is not set, uses in this case "MSFA" as default) to unsigned char * otherwise I'd get an error.

At this point, you probably understand that due to not knowing how to properly code in C, I'm pulling this most out of my arse, so your contribution is extremely appreciated, especially if you could take some time to go through the changes in this PR to see what/where I screwed up.

[diyelectromusic]

Ah, gotcha. Well the key issue here is that in @dcoredump 's comment, it is using the C enumerated types (so basically labels that are assigned to numbers as if they are a new type of their own) from dexed.h in the call, but you can't go from text in a config file to an enumerated type (which is basically a compiled-in processed thing) at run time - the final code knows nothing about the types - they are just for the programmer's convenience.

So I think it will just see the ASCII value for the first character of whatever string was in the config file, which Dexed won't recognise, so will always end up selecting MSFA.

What you need to do would be mirror what happens with m_SoundDevice in config.cpp which is also a config string setting, and then later on whatever calls getEngineType would have to check it in a similar way to how getSoundDevice is checked in minidexed.cpp, then when the appropriate string is matched, setEngineType can be called with the right enum parameter from Dexed.h...

But actually all that checking could happen directly in Config.cpp upon reading the config setting, which means that m_EngineType could then stay the uint8_t value we need to use and therefore your line setEngineType(getEngineType) would still work fine.

So I think you'd need code a bit like the following in Config.cpp:

const char *pEngineType = m_Properties.GetString ("EngineType", "MSFA");
if (strcmp (pEngineType, "MKI") == 0) {
  m_EngineType = MKI;
} else if (strcmp (pEngineType, "OPL") == 0) {
  m_EngineType = OPL;
} else {
  m_EngineType = MSFA
}

Something like that anyway - use C to check for the right config string, then store the Dexed.h value in m_EngineType for using later. This assumes that Dexed.h is included somewhere useful already, which I guess it must be as a key interface to Dexed!

Kevin

[diyelectromusic]

Thinking some more, an alternative, but more brittle solution (i.e. if Dexed.h changes, it will break) would be to set EngineType in minidexed.ini to one of 0, 1, 2 where they mean MSFA, MKI, and OPL respectively - then use m_Properties.GetNumber instead. That would work but as I say isn't robust and assumes the enum will always have the same value in Dexed.h. It also isn't as user friendly of course!

If we go for strings, should we also support lower case msfa, mki, opl too? Or just keep with upper case only? Technically we could force a toupper() call in there somewhere to allow for all possibilities I guess (if the circle/C library supports it...)

Kevin

[donluca]

Thank you so much for your through explanation, although I'd be a liar if I told you that I understood every single concept (and I won't voice my opinions on things such as "enumerated labels"), but at least I grasped enough of your post to make some more informed observations.

As far as I can tell, Minidexed plays a very safe approach in developing due to being tied to external projects by targeting specific commits so that no matter what happens upstream, Minidexed will keep on working without any kind of intervention.
That's fine and I followed this philosophy by targeting a specific commit of Synth_Dexed which added the aforementioned engines.

With this in mind, I think that your approach of just using numerals to choose the engine would make perfect sense because even if Synth_Dexed changes we won't need to do any adjustment because that part of the code is "frozen" in Minidexed.

From a user standpoint, most users probably don't even know/care about engines and won't touch that setting at all and those who actually care will probably read the documentation and learn the differences between the engines to make an educated choice on what best fits their own needs.

Hence, I propose we change the engine selection part in config.ini to this:

# Engine Type ( 0=Modern ; 1=Mark I ; 2=OPL )
EngineType=0

and call it a day.

Speaking of code, if I go down this path I guess I'll just have to change this:

m_EngineType = m_Properties.GetNumber ("EngineType", 0);

and we should be good, right?

[diyelectromusic]

With this in mind, I think that your approach of just using numerals to choose the engine would make perfect sense because even if Synth_Dexed changes we won't need to do any adjustment because that part of the code is "frozen" in Minidexed.

The problem there is that although the specific version downstream is chosen, that doesn't mean it never has to be updated and we've had problems in the past with not tracking updates properly, so I wouldn't want to rely on that.

In general I'd strongly recommend avoiding building in known brittleness and future maintenance problems, so if you don't want to do the string compares, but are happy with numbers then I'd recommend a similar translation in config.cpp from numbers in the config file to the MFSA/OPL/MKI definitions from Dexed.h. That way we are also robust against anything changing the underlying numbers (or a different compilation process and different architecture choosing different numbers).

So I'd say do something like:

uint8_t newEngineType = m_Properties.GetNumber ("EngineType", 0);
if (newEngineType == 1) {
  m_EngineType = MKI;
} else if (newEnginerType = 2) {
  m_EngineType = OPL;
} else {
  m_EngineType = MSFA
}

That kind of thing. Keeps the knowledge of the configuration setting values local to config.cpp and from that point onwards uses the definitions used by dexed.h whatever they may be.

In fact if we're separating out the two, then it might be more user-friendly to use 1,2,3 rather than 0,1,2... but either would be fine.

Kevin

[donluca]

I see, in that case then I agree that this approach is more desirable and I agree on choosing 1,2,3 in the minidexed.ini file.

I'll make the necessary changes and push a new commit.

[diyelectromusic]

} else if (newEnginerType = 2) {

Oops! Sorry - just noticed a type - that second test should be a double == too!

Kevin

[probonopd]

Here is the build:
MiniDexed_2023-03-21-4379b6f

[probonopd]

Bad news... even if getEngineType() is reporting other engines being used, they are not actually getting used, it's always the Modern engine.

@donluca is this still the case in the latest build?

Everyone: How does the latest build perform using the different engines on the various RPi models?
I don't want to ship something that is halfway-broken, so we should make sure that it works everywhere as expected.

[donluca]

The discussion #358 sums it up pretty much.

In short:

• Engines are working
• Banana71 reported that on RPi2 the Mark I (and the OPL even more so) engine is more resource hungry and he can't use all the 8 TGs together, which is very strange because MkI and OPL are more lightweight than Modern, so there is something "wrong" which needs to be investigated further.

That's pretty much it, it's up to you to decide whether, for now, put a disclaimer that in order to use all 8 TGs with MkI/OPL you'll need a RPi3 or better or wait for @dcoredump to look into this.

EDIT: to expand on this a bit: I did my own testing focused only on using a single TG and I didn't notice any issues with Mark I or OPL engine, but I didn't try using more than 1 TG at the same time.

[donluca]

Wait, I never closed this, what happened? 🤔

EDIT: oh I see, I changed the name of the branch on my repo and it messed up this PR... should I change the name of the branch back to what it was?
Or is there a way to change this PR so that it points to the correct branch which is setEngineType? @probonopd

[donluca]

I decided to restore the name of the branch for now, so at least this PR is still open.

[probonopd]

MiniDexed_2023-05-04-a680e19

[probonopd]

Note to self: Merge once CPU usage is better understood.