christofmuc / KnobKraft-orm

The KnobKraft Orm - The free modern cross-platform MIDI Sysex Librarian
GNU Affero General Public License v3.0
207 stars 27 forks source link

Adaptations need to be able to produce an initial name #360

Open christofmuc opened 3 weeks ago

christofmuc commented 3 weeks ago
          @Andy2No I think I know understand the confusion happening with nameFromDump(): nameFromDump() should be implemented if the name is stored in the sysex, and then renamePatch() is required as well. I think this is not clearly stated in the programming guide.

There is a friendlyProgramName() function, but that is somewhat a misnomer, as it is meant to provide a better name for the program number, not the patch name. it is used to e.g. produce something like "B1-099" instead of the default 199.

There seems to be a "createDefaultName()" function missing which could be used to calculate a nice name until the user has given a better name manually. This could also be used together with isDefaultName() to create a better name in case the patch still has the synth's default name like "BasicProgram".

I will open a new issue for this topic!

Originally posted by @christofmuc in https://github.com/christofmuc/KnobKraft-orm/issues/346#issuecomment-2453069471

Andy2No commented 3 weeks ago

What's the syntax for createDefaultName() is it just passed message?

Was it added more recently than the v2.2.3 I'm looking at? Oddly, the github search doesn't find any reference to it, which baffles me because the word is here and in the other issue thread... which I've now lost and can't seem to find again.

I tried resurrecting nameFromDump, in the Reface CS adaptation I was working on, based on @milnak's Reface CP adaptation, and renamed it as createDefaultName. As far as I can tell, that function is never called, and it's not listed in the functions on the Adaptation tab. I also defined isDefaultName() in case that was needed for createDefaultName() to be called:

# def nameFromDump(message):
def createDefaultName(message):
# Build a "name" based on values from "MIDI PARAMETER CHANGE TABLE (Tone Generator)"
# as Reface CS doesn't have patch names, not even in SysEx.
    print("createDefaultName(..)  ")
    try:
        data = parseSysEx(message)
    except Exception as e:
        print("[CP] nameFromDump Exception:", e)
        return "(Invalid)"

    if data.get("Address", []) == DUMP_ADDR_PART_COMMON:
        return str(hash(getParameterChangeTable(data["Data"]).values()))

    return "(Invalid)"

def isDefaultName(patchName):
    return patchName == '00-00'

The log doesn't show the debug message and the imported patch name is still 00-00, which is also shown fro the patch number (friendlyProgramName isn't currently implemented).

15:19:41: info Adaptation: channelIfValidDeviceResponse {'Channel': 127, 'ManufacturerID': 67, 'Family': [0, 65], 'Model': [81, 6], 'Version': [6, 3, 0, 0]}
15:19:42: info Detected Yamaha Reface CStemp on channel 1 of device reface CS
15:19:58: info Current edit buffer from synth is patch '00-00'
15:19:58: info Retrieved 1 new or changed patches from the synth, uploaded to database

I am still able to rename the imported edit buffer patch but that's because nameFromDump() isn't defined. I'm not able to use createDefaultName() to define the sort of default name which @milnak intended for the CP, at least not so far. Defining nameFromDump() stops the imported edit buffers being able to be renamed, so my current approach is just to not define it. Since createDefaultName() doesn't appear to be called, at least not in this version, defining it has no effect as far as I can tell.

Andy2No commented 3 weeks ago

Okay. I think I get it, @christofmuc. When you said "There seems to be a "createDefaultName()" function missing" I thought you meant missing from the adaptation. I now realise you probably meant there ought to be such a thing as an option to use, but there isn't, yet.

Fair enough. I give up for now. I could post the Yamaha Reface CS adaptation as is, in a discussion, because it's working well enough for me. I probably ought to start again from the current release version of the Reface CP one, and add my changes again, but maybe that will change again anyway. I based this one on an earlier version, and I'm not really clear what the changes were, other than to the automatic naming function (nameFromDump()) which seems counterproductive to have, at this stage - not having one allows renaming by the user. Having one doesn't.

christofmuc commented 2 weeks ago

@Andy2No Yes, nameFromDump() is required to have renamePatch() as well. nameFromDump() is meant to extract the name from the sysex info, and renamePatch) to put a new name into the sysex.

What is missing is the nameForDump, which would be the function you are looking for to calculate a nice name for a patch which has none given. We can call it createDefaultName() maybe, because the user will want to change the name potentially afterwards.

We can release the CP adaptation, we just have to comment out the nameFromDump() until we have a new release that allows for createDefaultName()?

christofmuc commented 2 weeks ago

Sorry for confusing a proposed function name for something that actually exists!

Andy2No commented 2 weeks ago

My mistake. I completely misunderstood what you were saying, there.

Yes, personally, I think it's better for the nameFromDump() function to be commented out of the CP variant, for now. Being able to set the patch name manually seems more useful than having a cryptic automatically generated one that can't be overridden, to me.